[clang] Fixing the perf regression in the captured context preanalyse…

…s 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

@@ -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

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"
 
 

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

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 )

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`]

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]

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]