pghs975uc
/
infer_clone
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

[liveness] ignore most dead stores when the variable has been passed by reference

Browse Source 
Summary:
See added tests. Passing a variable by reference to a function `foo` can
cause the variable to be added to the global state so any stores after
that might be live as long as there is another function call after the
store (since the global state shouldn't outlive the scope of the
function). Currently we don't check that the latter is true; to report
these we would need to extend the abstract domain to remember which
stores have been made without a subsequent call.

Also change Blacklisted -> Dangerous everywhere since the corresponding
option is called "liveness_dangerous_classes".

Reviewed By: skcho

Differential Revision: D26606151

fbshipit-source-id: e869e5df1
master
Jules Villard 4 years ago committed by Facebook GitHub Bot
parent 671727be53
commit 1320e79201
2 changed files with 115 additions and 51 deletions
Show Stats Download Patch File Download Diff File

128
infer/src/checkers/liveness.ml
Unescape View File

@ -39,20 +39,20 @@ let string_list_of_json ~option_name ~init = function
module type LivenessConfig = sig
val is_blacklisted_destructor : Procname.t -> bool
val is_dangerous_destructor : Procname.t -> bool
end
(** Use this config to get a reliable liveness pre-analysis that tells you which variables are live
at which program point *)
module PreAnalysisMode : LivenessConfig = struct
(** do not do any funky stuff *)
let is_blacklisted_destructor _proc_name = false
let is_dangerous_destructor _proc_name = false
end
(** Use this config to get a dead store checker that can take some liberties wrt a strict liveness
analysis *)
module CheckerMode : LivenessConfig = struct
let blacklisted_destructor_matcher =
module CheckerMode = struct
let dangerous_destructor_matcher =
QualifiedCppName.Match.of_fuzzy_qual_names
(string_list_of_json ~option_name:"liveness-dangerous-classes" ~init:[]
Config.liveness_dangerous_classes)
@ -63,29 +63,41 @@ module CheckerMode : LivenessConfig = struct
QualifiedCppName.Match.of_fuzzy_qual_names ["std::unique_ptr"; "std::shared_ptr"]
let is_blacklisted_class_name class_name =
let is_dangerous_class_name class_name =
Typ.Name.unqualified_name class_name
|> QualifiedCppName.Match.match_qualifiers blacklisted_destructor_matcher
|> QualifiedCppName.Match.match_qualifiers dangerous_destructor_matcher
let is_wrapper_of_blacklisted_class_name class_name =
let is_wrapper_of_dangerous_class_name class_name =
Typ.Name.unqualified_name class_name
|> QualifiedCppName.Match.match_qualifiers standard_wrappers_matcher
&&
match Typ.Name.get_template_spec_info class_name with
| Some (Template {args= TType {desc= Tstruct name} :: _; _}) ->
is_blacklisted_class_name name
is_dangerous_class_name name
| _ ->
false
let is_blacklisted_destructor (callee_pname : Procname.t) =
match callee_pname with
| ObjC_Cpp cpp_pname when Procname.ObjC_Cpp.is_destructor cpp_pname ->
is_blacklisted_class_name cpp_pname.class_name
|| is_wrapper_of_blacklisted_class_name cpp_pname.class_name
let is_dangerous_proc_name (proc_name : Procname.t) =
match proc_name with
| ObjC_Cpp cpp_pname ->
is_dangerous_class_name cpp_pname.class_name
|| is_wrapper_of_dangerous_class_name cpp_pname.class_name
| _ ->
false
let is_destructor (proc_name : Procname.t) =
match proc_name with
| ObjC_Cpp cpp_pname ->
Procname.ObjC_Cpp.is_destructor cpp_pname
| _ ->
false
let is_dangerous_destructor (proc_name : Procname.t) =
is_destructor proc_name && is_dangerous_proc_name proc_name
end
(** compilers 101-style backward transfer functions for liveness analysis. gen a variable when it is
@ -129,8 +141,8 @@ module TransferFunctions (LConfig : LivenessConfig) (CFG : ProcCfg.S) = struct
| Sil.Prune (exp, _, _, _) ->
exp_add_live exp astate
| Sil.Call ((ret_id, _), Const (Cfun callee_pname), _, _, _)
when LConfig.is_blacklisted_destructor callee_pname ->
Logging.d_printfln_escaped "Blacklisted destructor %a, ignoring reads@\n" Procname.pp
when LConfig.is_dangerous_destructor callee_pname ->
Logging.d_printfln_escaped "Dangerous destructor %a, ignoring reads@\n" Procname.pp
callee_pname ;
Domain.remove (Var.of_id ret_id) astate
| Sil.Call ((ret_id, _), call_exp, actuals, _, {CallFlags.cf_assign_last_arg}) ->
@ -166,37 +178,62 @@ let matcher_scope_guard =
|> QualifiedCppName.Match.of_fuzzy_qual_names
module CapturedByRefTransferFunctions (CFG : ProcCfg.S) = struct
module PassedByRefTransferFunctions (CFG : ProcCfg.S) = struct
module CFG = CFG
module Domain = VarSet
type analysis_data = unit
let exec_instr astate () _ instr =
List.fold (Sil.exps_of_instr instr)
~f:(fun acc exp ->
Exp.fold_captured exp
~f:(fun acc exp ->
match Exp.ignore_cast exp with
| Exp.Lvar pvar ->
(* captured by reference, add *)
Domain.add (Var.of_pvar pvar) acc
| _ ->
(* captured by value or init-capture, skip *)
acc )
acc )
~init:astate
let add_if_lvar expr astate =
match Exp.ignore_cast expr with
| Exp.Lvar pvar ->
(* passed or captured by reference, add *)
Domain.add (Var.of_pvar pvar) astate
| _ ->
(* passed or captured by value or init-capture, skip *)
astate
let proc_name_of_expr expr =
match (expr : Exp.t) with Const (Cfun proc_name) -> Some proc_name | _ -> None
let is_dangerous expr =
(* ignore all captures from "dangerous" classes *)
proc_name_of_expr expr |> Option.exists ~f:CheckerMode.is_dangerous_proc_name
let exec_instr astate () _ (instr : Sil.instr) =
let astate =
match instr with
| Call (_ret, f, actuals, _loc, _flags) when not (is_dangerous f) ->
let actuals =
if Option.exists (proc_name_of_expr f) ~f:Procname.is_constructor then
(* Skip "this" in constructors, assuming constructors are less likely to have global
side effects that store "this" in the global state. We could also skip "this" in
all (non-static) methods but this becomes less clear: constructing an object then
calling methods on it can have side-effects even if the object is used for nothing
else. *)
List.tl actuals |> Option.value ~default:[]
else actuals
in
List.fold actuals ~init:astate ~f:(fun astate (actual, _typ) -> add_if_lvar actual astate)
| _ ->
astate
in
List.fold (Sil.exps_of_instr instr) ~init:astate ~f:(fun astate exp ->
Exp.fold_captured exp ~f:(fun astate exp -> add_if_lvar exp astate) astate )
let pp_session_name _node fmt = F.pp_print_string fmt "captured by ref"
let pp_session_name _node fmt = F.pp_print_string fmt "passed by reference"
end
module CapturedByRefAnalyzer =
AbstractInterpreter.MakeRPO (CapturedByRefTransferFunctions (ProcCfg.Exceptional))
module PassedByRefAnalyzer =
AbstractInterpreter.MakeRPO (PassedByRefTransferFunctions (ProcCfg.Exceptional))
let get_captured_by_ref_invariant_map proc_desc =
let get_passed_by_ref_invariant_map proc_desc =
let cfg = ProcCfg.Exceptional.from_pdesc proc_desc in
CapturedByRefAnalyzer.exec_cfg cfg () ~initial:VarSet.empty
PassedByRefAnalyzer.exec_cfg cfg () ~initial:VarSet.empty
module IntLitSet = Caml.Set.Make (IntLit)
@ -215,7 +252,7 @@ let ignored_constants =
let checker {IntraproceduralAnalysis.proc_desc; err_log} =
let captured_by_ref_invariant_map = get_captured_by_ref_invariant_map proc_desc in
let passed_by_ref_invariant_map = get_passed_by_ref_invariant_map proc_desc in
let cfg = CFG.from_pdesc proc_desc in
let invariant_map = CheckerAnalyzer.exec_cfg cfg proc_desc ~initial:Domain.empty in
(* we don't want to report in harmless cases like int i = 0; if (...) { i = ... } else { i = ... }
@ -250,11 +287,11 @@ let checker {IntraproceduralAnalysis.proc_desc; err_log} =
|> Option.exists ~f:(fun local ->
local.ProcAttributes.is_constexpr || local.ProcAttributes.is_declared_unused )
in
let should_report pvar typ live_vars captured_by_ref_vars =
let should_report pvar typ live_vars passed_by_ref_vars =
not
( Pvar.is_frontend_tmp pvar || Pvar.is_return pvar || Pvar.is_global pvar
|| is_constexpr_or_unused pvar
|| VarSet.mem (Var.of_pvar pvar) captured_by_ref_vars
|| VarSet.mem (Var.of_pvar pvar) passed_by_ref_vars
|| Domain.mem (Var.of_pvar pvar) live_vars
|| Procdesc.is_captured_pvar proc_desc pvar
|| is_scope_guard typ
@ -268,15 +305,14 @@ let checker {IntraproceduralAnalysis.proc_desc; err_log} =
let ltr = [Errlog.make_trace_element 0 loc "Write of unused value" []] in
Reporting.log_issue proc_desc err_log ~loc ~ltr Liveness IssueType.dead_store message
in
let report_dead_store live_vars captured_by_ref_vars = function
let report_dead_store live_vars passed_by_ref_vars = function
| Sil.Store {e1= Lvar pvar; typ; e2= rhs_exp; loc}
when should_report pvar typ live_vars captured_by_ref_vars && not (is_sentinel_exp rhs_exp) ->
when should_report pvar typ live_vars passed_by_ref_vars && not (is_sentinel_exp rhs_exp) ->
log_report pvar typ loc
| Sil.Call (_, e_fun, (arg, typ) :: _, loc, _) -> (
match (Exp.ignore_cast e_fun, Exp.ignore_cast arg) with
| Exp.Const (Cfun (Procname.ObjC_Cpp _ as pname)), Exp.Lvar pvar
when Procname.is_constructor pname && should_report pvar typ live_vars captured_by_ref_vars
->
when Procname.is_constructor pname && should_report pvar typ live_vars passed_by_ref_vars ->
log_report pvar typ loc
| _, _ ->
() )
@ -284,11 +320,11 @@ let checker {IntraproceduralAnalysis.proc_desc; err_log} =
()
in
let report_on_node node =
let captured_by_ref_vars =
let passed_by_ref_vars =
match
CapturedByRefAnalyzer.extract_post
PassedByRefAnalyzer.extract_post
(ProcCfg.Exceptional.Node.id (CFG.Node.underlying_node node))
captured_by_ref_invariant_map
passed_by_ref_invariant_map
with
| Some post ->
post
@ -299,7 +335,7 @@ let checker {IntraproceduralAnalysis.proc_desc; err_log} =
Instrs.iter (CFG.instrs node) ~f:(fun instr ->
match CheckerAnalyzer.extract_pre node_id invariant_map with
| Some live_vars ->
report_dead_store live_vars captured_by_ref_vars instr
report_dead_store live_vars passed_by_ref_vars instr
| None ->
() )
in

38
infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp
Unescape View File

@ -64,13 +64,13 @@ void FN_capture_no_read_bad() {
[x]() { return; }();
}
void init_capture_reassign_bad() {
int init_capture_reassign_bad() {
int i = 1; // this is a dead store
return [i = 1]() { return i; }();
}
void FN_init_capture_no_call_bad() {
[i = 1]() { return i; };
auto FN_init_capture_no_call_bad() {
return [i = 1]() { return i; };
}
void FN_init_capture_call_bad2() {
@ -150,7 +150,7 @@ int plus_plus_loop_ok(int n) {
return i;
}
void lambda_bad() {
auto lambda_bad() {
int x = []() {
int y = 1;
y = 2;
@ -486,7 +486,7 @@ class Exceptions {
return 0;
}
void read_in_goto_ok(bool b) {
int read_in_goto_ok(bool b) {
int i = 1;
try {
if (b) {
@ -506,6 +506,7 @@ class Exceptions {
catch (...) {
return i;
}
return 0;
}
int return_in_try1_ok() {
bool b;
@ -628,4 +629,31 @@ void ignored_constants_ok() {
int z = 44;
}
void store_total_in_global_state(int& total);
void reads_global_state_with_total_in_it();
void passed_by_ref_ok() {
int total;
store_total_in_global_state(total);
total = 42;
reads_global_state_with_total_in_it();
}
void FN_passed_by_ref_not_used_bad() {
int total;
store_total_in_global_state(total);
total = 42;
// any use of [total] would have to occur before function exit as accesses
// after function exit are invalid since total goes out of scope
}
void passed_by_ref_in_loop_ok(int n) {
int total;
for (int i = 0; i < n; i++) {
store_total_in_global_state(total);
}
total = 42;
reads_global_state_with_total_in_it();
}
} // namespace dead_stores

Write Preview
Loading…
Cancel
Save