From 165b77bac6e94909e3fec3e3e757db95390bd7ce Mon Sep 17 00:00:00 2001 From: Dulma Churchill Date: Mon, 18 Nov 2024 05:22:14 -0800 Subject: [PATCH] [clang] Fixing the perf regression in the captured context preanalyses and reenable them Summary: So we have two analyses with their domains. We want to process those domains to add some data to the attributes of some blocks. I believe that the way it was done before was inefficient. Now we pre-process the domains to only have a simple data structure with the data that we actually need, and we preprocess the procedures to keep only the blocks. Then we process one domain after the other. This should be ok now. After enabling the preanalyses again, we are able to report BLOCK_PARAMETER_NOT_NULL_CHECKED on captured variables again, as well as CXX_STRING_CAPTURED_IN_BLOCK and NSSTRING_INTERNAL_PTR_CAPTURED_IN_BLOCK. Reviewed By: ngorogiannis Differential Revision: D65942978 fbshipit-source-id: 3424fc73841fc4a3dcf8ff85eaa5818e440c69c8 --- infer/man/man1/infer-full.txt | 6 +- infer/src/base/Config.ml | 2 +- infer/src/checkers/ComputeCapturedInfo.ml | 236 +++++++++++++----- infer/src/checkers/ParameterNotNullChecked.ml | 11 +- .../parameter-not-null-checked/issues.exp | 1 + .../objc/self-in-block/issues.exp | 2 + .../objcpp/self-in-block/issues.exp | 1 + 7 files changed, 187 insertions(+), 72 deletions(-) diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index a7a97a0e8b..1dca827dd2 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -2664,9 +2664,9 @@ INTERNAL OPTIONS --complete-capture-from-reset Cancel the effect of --complete-capture-from. - --compute-captured-context - Activates: Compute context information for captured variables in - Objective-C blocks (Conversely: --no-compute-captured-context) + --no-compute-captured-context + Deactivates: Compute context information for captured variables in + Objective-C blocks (Conversely: --compute-captured-context) --config-impact-config-field-patterns +regex Register known config fields that have a config value. The matched diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml index 0873e643e1..8e3c537315 100644 --- a/infer/src/base/Config.ml +++ b/infer/src/base/Config.ml @@ -1333,7 +1333,7 @@ and complete_capture_from = and compute_captured_context = - CLOpt.mk_bool ~long:"compute-captured-context" ~default:false + CLOpt.mk_bool ~long:"compute-captured-context" ~default:true "Compute context information for captured variables in Objective-C blocks" diff --git a/infer/src/checkers/ComputeCapturedInfo.ml b/infer/src/checkers/ComputeCapturedInfo.ml index a7bd8098cb..fc5e41cd6a 100644 --- a/infer/src/checkers/ComputeCapturedInfo.ml +++ b/infer/src/checkers/ComputeCapturedInfo.ml @@ -118,11 +118,41 @@ module CheckedForNull = struct end module Analyzer = AbstractInterpreter.MakeWTO (TransferFunctions) + + let flatten_checked_for_null_domain domain = + let process_mem mem domain_elements = + let process_pvar_domain pvar (domain : DomainData.t) domain_elements = + match domain.captured_in_closure with + | Some procname when domain.checked -> + Procname.Map.update procname + (fun pvars -> + match pvars with Some pvars -> Some (pvar :: pvars) | None -> Some [pvar] ) + domain_elements + | _ -> + domain_elements + in + PVars.fold process_pvar_domain mem.Mem.pvars domain_elements + in + Domain.fold process_mem domain Procname.Map.empty + + + let process_checked_for_null_pvar checked_for_null_data captured_pvar = + let pvar_name = Pvar.get_name captured_pvar.CapturedVar.pvar in + let checked = List.exists ~f:(fun pvar -> Mangled.equal pvar pvar_name) checked_for_null_data in + let context_info = + match captured_pvar.CapturedVar.context_info with + | Some context_info -> + {context_info with CapturedVar.is_checked_for_null= checked} + | None -> + {CapturedVar.is_checked_for_null= checked; is_internal_pointer_of= None} + in + {captured_pvar with CapturedVar.context_info= Some context_info} end module InternalStringPointer = struct module VarsDomainData = struct - type t = {internal_pointer_of: Typ.t} [@@deriving compare] + type t = {internal_pointer_of: Typ.t; captured_in_closure: Procname.t option} + [@@deriving compare] let pp fmt {internal_pointer_of} = F.fprintf fmt "internal_pointer_of=%a" (Typ.pp Pp.text) internal_pointer_of @@ -134,10 +164,18 @@ module InternalStringPointer = struct let compare = compare VarsDomainData.compare end + module PVarsDomainData = struct + type t = {id: Ident.t; captured_in_closure: Procname.t option} [@@deriving compare] + + let pp fmt {id; captured_in_closure} = + F.fprintf fmt "id=%a, captured_in_closure=%a" Ident.pp id (Pp.option Procname.pp) + captured_in_closure + end + module PVars = struct - include PrettyPrintable.MakePPMonoMap (Mangled) (Ident) + include PrettyPrintable.MakePPMonoMap (Mangled) (PVarsDomainData) - let compare = compare Ident.compare + let compare = compare PVarsDomainData.compare end module Mem = struct @@ -146,24 +184,42 @@ module InternalStringPointer = struct let pp fmt {vars; pvars} = F.fprintf fmt "Vars= %a@\nPVars= %a@\n" Vars.pp vars PVars.pp pvars let set_internal_pointer var typ astate = - let vars = Vars.add var {VarsDomainData.internal_pointer_of= typ} astate.vars in + let vars = + Vars.add var + {VarsDomainData.internal_pointer_of= typ; captured_in_closure= None} + astate.vars + in {astate with vars} - let is_internal_pointer_of pvar astate = - match PVars.find_opt pvar astate.pvars with - | Some var -> ( - match Vars.find_opt var astate.vars with - | Some domainData -> - Some domainData.internal_pointer_of - | None -> - None ) - | None -> - None + let call exps astate = + let update_captured_in_closure_in_arg astate (exp, _) = + let update_captured_in_closure closure_pname astate (_, captured_var) = + Logging.d_printfln "captured var is %a" CapturedVar.pp captured_var ; + let pvars = + PVars.update + (Pvar.get_name captured_var.CapturedVar.pvar) + (fun data_opt -> + Option.value_map + ~f:(fun data -> + Some {data with PVarsDomainData.captured_in_closure= Some closure_pname} ) + data_opt ~default:None ) + astate.pvars + in + {astate with pvars} + in + match exp with + | Exp.Closure {name; captured_vars} -> + Logging.d_printfln "closure name is %a" Procname.pp name ; + List.fold ~f:(update_captured_in_closure name) captured_vars ~init:astate + | _ -> + astate + in + List.fold ~f:update_captured_in_closure_in_arg exps ~init:astate let store pvar id astate = - let pvars = PVars.add (Pvar.get_name pvar) id astate.pvars in + let pvars = PVars.add (Pvar.get_name pvar) {id; captured_in_closure= None} astate.pvars in {astate with pvars} end @@ -172,14 +228,9 @@ module InternalStringPointer = struct let set_internal_pointer var typ astate : t = map (Mem.set_internal_pointer var typ) astate - let is_internal_pointer_of pvar astate = - fold - (fun state typ_opt -> - match typ_opt with Some _ -> typ_opt | None -> Mem.is_internal_pointer_of pvar state ) - astate None - - let store pvar id astate = map (Mem.store pvar id) astate + + let call exps astate = map (Mem.call exps) astate end module TransferFunctions = struct @@ -226,59 +277,93 @@ module InternalStringPointer = struct else astate | _ -> astate ) - | Call ((var, _), Exp.Const (Const.Cfun procname), (_, typ) :: _args, _, _) -> - let procname_qualifiers = Procname.get_qualifiers procname in - if - QualifiedCppName.Match.match_qualifiers cstring_using_encoding procname_qualifiers - || QualifiedCppName.Match.match_qualifiers utf8string procname_qualifiers - then - match typ.desc with - | Tptr (inside_typ, _) -> - Domain.set_internal_pointer var inside_typ astate - | _ -> - astate - else astate + | Call ((var, _), Exp.Const (Const.Cfun procname), (_, typ) :: _args, _, _) + when QualifiedCppName.Match.match_qualifiers cstring_using_encoding + (Procname.get_qualifiers procname) + || QualifiedCppName.Match.match_qualifiers utf8string + (Procname.get_qualifiers procname) -> ( + match typ.desc with + | Tptr (inside_typ, _) -> + Domain.set_internal_pointer var inside_typ astate + | _ -> + astate ) + | Call (_, _, args, _loc, _call_flags) -> + Domain.call args astate | _ -> astate end module Analyzer = AbstractInterpreter.MakeWTO (TransferFunctions) + + let flatten_is_internal_pointer_domain domain = + let process_mem mem domain_elements = + let process_var_domain pvar {PVarsDomainData.id; captured_in_closure} domain_elements = + match captured_in_closure with + | Some procname -> ( + match Vars.find_opt id mem.Mem.vars with + | Some (vars_domain_data : VarsDomainData.t) -> + Procname.Map.update procname + (fun pvars -> + let tuple = (pvar, vars_domain_data.internal_pointer_of) in + match pvars with Some pvars -> Some (tuple :: pvars) | None -> Some [tuple] ) + domain_elements + | None -> + domain_elements ) + | _ -> + domain_elements + in + PVars.fold process_var_domain mem.Mem.pvars domain_elements + in + Domain.fold process_mem domain Procname.Map.empty + + + let process_is_internal_pointer_pvar internal_string_pointer_data captured_pvar = + let pvar_name = Pvar.get_name captured_pvar.CapturedVar.pvar in + match + List.find ~f:(fun (pvar, _) -> Mangled.equal pvar pvar_name) internal_string_pointer_data + with + | Some (_, is_internal_pointer_of) -> + let context_info = + match captured_pvar.CapturedVar.context_info with + | Some context_info -> + {context_info with CapturedVar.is_internal_pointer_of= Some is_internal_pointer_of} + | None -> + { CapturedVar.is_internal_pointer_of= Some is_internal_pointer_of + ; CapturedVar.is_checked_for_null= false } + in + {captured_pvar with CapturedVar.context_info= Some context_info} + | None -> + captured_pvar end -let update_captured_in_closures cfg checked_for_null_domain_opt internal_string_pointer_domain_opt = - let process_mem {CheckedForNull.Mem.pvars} = - let process_pvar pvar {CheckedForNull.DomainData.checked; captured_in_closure} = - match captured_in_closure with - | Some procname -> - let proc_desc = Procname.Hash.find cfg procname in - let attributes = Procdesc.get_attributes proc_desc in - let update_captured captured_var = - let pvar_name = Pvar.get_name captured_var.CapturedVar.pvar in - if Mangled.equal pvar pvar_name then - let is_internal_pointer_of = - Option.bind - ~f:(fun domain -> InternalStringPointer.Domain.is_internal_pointer_of pvar domain) - internal_string_pointer_domain_opt - in - let context_info = - {CapturedVar.is_checked_for_null= checked; is_internal_pointer_of} - in - {captured_var with CapturedVar.context_info= Some context_info} - else captured_var +let update_captured_context_closures blocks checked_for_null_domain_opt + internal_string_pointer_domain_opt = + let update_context_in_closure block attributes procname domain_opt process_pvar = + match domain_opt with + | Some domain -> ( + match Procname.Map.find_opt procname domain with + | Some internal_string_pointer_data -> + let updated_captured = + List.map + ~f:(process_pvar internal_string_pointer_data) + attributes.ProcAttributes.captured in - let captured = List.map ~f:update_captured attributes.ProcAttributes.captured in - let attributes = {attributes with captured} in - Procdesc.set_attributes proc_desc attributes + let attributes = {attributes with ProcAttributes.captured= updated_captured} in + Procdesc.set_attributes block attributes | None -> - () - in - CheckedForNull.PVars.iter process_pvar pvars + () ) + | None -> + () in - match checked_for_null_domain_opt with - | Some checked_for_null_domain -> - CheckedForNull.Domain.iter process_mem checked_for_null_domain - | None -> - () + let process_block block = + let attributes = Procdesc.get_attributes block in + let procname = Procdesc.get_proc_name block in + update_context_in_closure block attributes procname checked_for_null_domain_opt + CheckedForNull.process_checked_for_null_pvar ; + update_context_in_closure block attributes procname internal_string_pointer_domain_opt + InternalStringPointer.process_is_internal_pointer_pvar + in + List.iter ~f:process_block blocks let process cfg = @@ -290,6 +375,11 @@ let process cfg = let checked_for_null_domain = CheckedForNull.Analyzer.compute_post proc_desc ~initial proc_desc in + let flat_checked_for_null_domain = + Option.map + ~f:(fun map -> CheckedForNull.flatten_checked_for_null_domain map) + checked_for_null_domain + in let initial = InternalStringPointer.Domain.singleton { InternalStringPointer.Mem.vars= InternalStringPointer.Vars.empty @@ -298,6 +388,20 @@ let process cfg = let internal_string_pointer_domain = InternalStringPointer.Analyzer.compute_post proc_desc ~initial proc_desc in - update_captured_in_closures cfg checked_for_null_domain internal_string_pointer_domain + let flat_internal_string_pointer_domain = + Option.map + ~f:(fun map -> InternalStringPointer.flatten_is_internal_pointer_domain map) + internal_string_pointer_domain + in + let get_blocks cfg = + Cfg.fold_sorted cfg + ~f:(fun blocks proc_desc -> + let procname = Procdesc.get_proc_name proc_desc in + if Procname.is_objc_block procname then proc_desc :: blocks else blocks ) + ~init:[] + in + let blocks = get_blocks cfg in + update_captured_context_closures blocks flat_checked_for_null_domain + flat_internal_string_pointer_domain in Procname.Hash.iter (process cfg) cfg diff --git a/infer/src/checkers/ParameterNotNullChecked.ml b/infer/src/checkers/ParameterNotNullChecked.ml index cd133188cc..2bfc251569 100644 --- a/infer/src/checkers/ParameterNotNullChecked.ml +++ b/infer/src/checkers/ParameterNotNullChecked.ml @@ -241,13 +241,20 @@ let is_block_param formals name = let get_captured_formals attributes = let captured = attributes.ProcAttributes.captured in let formal_of_captured (captured : CapturedVar.t) = - match (captured.captured_from, captured.context_info) with - | Some {is_formal= Some proc}, Some {CapturedVar.is_checked_for_null} -> ( + match captured.captured_from with + | Some {is_formal= Some proc} -> ( match IRAttributes.load proc with | Some proc_attributes -> let formals = find_block_param proc_attributes.ProcAttributes.formals (Pvar.get_name captured.pvar) in + let is_checked_for_null = + match captured.CapturedVar.context_info with + | Some {CapturedVar.is_checked_for_null} -> + is_checked_for_null + | None -> + false + in Option.map formals ~f:(fun formals -> (formals, proc_attributes, is_checked_for_null)) | None -> None ) diff --git a/infer/tests/codetoanalyze/objc/parameter-not-null-checked/issues.exp b/infer/tests/codetoanalyze/objc/parameter-not-null-checked/issues.exp index bcf80dec23..ad2c6cd455 100644 --- a/infer/tests/codetoanalyze/objc/parameter-not-null-checked/issues.exp +++ b/infer/tests/codetoanalyze/objc/parameter-not-null-checked/issues.exp @@ -4,4 +4,5 @@ codetoanalyze/objc/parameter-not-null-checked/Blocks_as_parameters.m, Blocks_as_ codetoanalyze/objc/parameter-not-null-checked/Blocks_as_parameters.m, Blocks_as_parameters.twoBlocksNotCheckedBad:and:and:, 3, BLOCK_PARAMETER_NOT_NULL_CHECKED, no_bucket, WARNING, [Parameter `block2` of Blocks_as_parameters.twoBlocksNotCheckedBad:and:and:,Executing `block2`], "block2("=>"BLOCK_CALL_SAFE(block2"@54:5 codetoanalyze/objc/parameter-not-null-checked/Blocks_as_parameters.m, Blocks_as_parameters.nonnullBlockTwoBlocksBad:and:and:, 4, BLOCK_PARAMETER_NOT_NULL_CHECKED, no_bucket, WARNING, [Parameter `block2` of Blocks_as_parameters.nonnullBlockTwoBlocksBad:and:and:,Executing `block2`], "block2("=>"BLOCK_CALL_SAFE(block2"@82:3 codetoanalyze/objc/parameter-not-null-checked/Blocks_as_parameters.m, Blocks_as_parameters.blockCheckedAssignNULLBad:and:, 3, BLOCK_PARAMETER_NOT_NULL_CHECKED, no_bucket, WARNING, [Parameter `block` of Blocks_as_parameters.blockCheckedAssignNULLBad:and:,Checking `block` for nil,Assigned,Executing `block`], "block("=>"BLOCK_CALL_SAFE(block"@88:5 +codetoanalyze/objc/parameter-not-null-checked/Blocks_as_parameters.m, objc_block_Blocks_as_parameters.m:121, 1, BLOCK_PARAMETER_NOT_NULL_CHECKED, no_bucket, WARNING, [Parameter `completion` of Blocks_as_parameters.uploadTaskWithRequestBad:fromFile:delegate:delegateQueue:completion:,Executing `completion`], "completion("=>"BLOCK_CALL_SAFE(completion"@122:5 codetoanalyze/objc/parameter-not-null-checked/Blocks_as_parameters.m, blockNotCheckedBadNoAutofix, 0, BLOCK_PARAMETER_NOT_NULL_CHECKED, no_bucket, WARNING, [Parameter `block` of blockNotCheckedBadNoAutofix,Executing `block`] diff --git a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp index 76937faa19..e07f1a70d3 100644 --- a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp +++ b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp @@ -1,3 +1,5 @@ +codetoanalyze/objc/self-in-block/CStringUsingEncodingTest.m, objc_block_CStringUsingEncodingTest.m:18, 1, NSSTRING_INTERNAL_PTR_CAPTURED_IN_BLOCK, no_bucket, ERROR, [Captured variable &encoding defined here,Using captured &encoding] +codetoanalyze/objc/self-in-block/CStringUsingEncodingTest.m, objc_block_CStringUsingEncodingTest.m:34, 1, NSSTRING_INTERNAL_PTR_CAPTURED_IN_BLOCK, no_bucket, ERROR, [Captured variable &utf8string defined here,Using captured &utf8string] codetoanalyze/objc/self-in-block/NoescapeBlock.m, objc_block_NoescapeBlock.m:35, 1, MULTIPLE_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &weakSelf] codetoanalyze/objc/self-in-block/NoescapeBlock.m, objc_block_NoescapeBlock.m:35, 1, WEAK_SELF_IN_NO_ESCAPE_BLOCK, no_bucket, ERROR, [Using &weakSelf,Using &weakSelf] codetoanalyze/objc/self-in-block/NoescapeBlock.m, objc_block_NoescapeBlock.m:35, 2, WEAK_SELF_IN_NO_ESCAPE_BLOCK, no_bucket, ERROR, [Using &weakSelf,Using &weakSelf] diff --git a/infer/tests/codetoanalyze/objcpp/self-in-block/issues.exp b/infer/tests/codetoanalyze/objcpp/self-in-block/issues.exp index b879e64682..d5ee1a3630 100644 --- a/infer/tests/codetoanalyze/objcpp/self-in-block/issues.exp +++ b/infer/tests/codetoanalyze/objcpp/self-in-block/issues.exp @@ -2,3 +2,4 @@ codetoanalyze/objcpp/self-in-block/CxxRefInBlock.mm, objc_block_CxxRefInBlock.mm codetoanalyze/objcpp/self-in-block/CxxRefInBlock.mm, objc_block_CxxRefInBlock.mm:52, 1, CXX_REF_CAPTURED_IN_BLOCK, no_bucket, ERROR, [Captured variable &y defined here,Using captured &y] codetoanalyze/objcpp/self-in-block/CxxRefInBlock.mm, objc_block_CxxRefInBlock.mm:114, 1, CXX_REF_CAPTURED_IN_BLOCK, no_bucket, ERROR, [Captured variable &y defined here,Using captured &y] codetoanalyze/objcpp/self-in-block/CxxRefInBlock.mm, objc_block_CxxRefInBlock.mm:114, 2, CXX_REF_CAPTURED_IN_BLOCK, no_bucket, ERROR, [Captured variable &z defined here,Using captured &z] +codetoanalyze/objcpp/self-in-block/CxxStringInBlock.mm, objc_block_CxxStringInBlock.mm:27, 1, CXX_STRING_CAPTURED_IN_BLOCK, no_bucket, ERROR, [Captured variable &c defined here,Using captured &c]