[siof] better error reports using `Errlog.loc_trace`s

Summary:
This adds generic support for reporting error traces as usual infer issues
traces (instead of putting them in the textual description of the error) to
Trace.ml and SinkTrace.ml.

The siof checker is made to use these new traces, and gets an improved error
message mentioning the name of the problematic global as well, which requires a
slight API change in Pvar.re.

The support in Trace.ml is incomplete: passthroughs are ignored. This missing
feature will be needed by Quandary to migrate its error messages.

Reviewed By: sblackshear

Differential Revision: D4159542

fbshipit-source-id: 8c1101d

infer/src/IR/Procname.re

@@ -500,7 +500,13 @@ let is_infer_undefined pn =>
     false
   };
 
-let is_globals_initializer (name, _) => string_is_prefix Config.clang_initializer_prefix name;
+let get_global_name_of_initializer =
+  fun
+  | C (name, _) when string_is_prefix Config.clang_initializer_prefix name => {
+      let prefix_len = String.length Config.clang_initializer_prefix;
+      Some (String.sub name prefix_len (String.length name - prefix_len))
+    }
+  | _ => None;
 
 
 /** to_string for C_function type */

infer/src/IR/Procname.rei

@@ -240,9 +240,9 @@ let is_class_initializer: t => bool;
 let is_infer_undefined: t => bool;
 
 
-/** Is this procedure one inserted by Infer to initialize the global variables of this translation
+/** Return the name of the global for which this procedure is the initializer if this is an
-    unit? */
+    initializer, None otherwise. */
-let is_globals_initializer: c => bool;
+let get_global_name_of_initializer: t => option string;
 
 
 /** Pretty print a proc name. */

infer/src/checkers/SinkTrace.ml

@@ -24,6 +24,10 @@ module type S = sig
 
   (** update sink with the given call site *)
   val with_callsite : t -> CallSite.t -> t
+
+  val to_sink_loc_trace :
+    ?desc_of_sink:(Sink.t -> string) -> ?sink_should_nest:(Sink.t -> bool) ->
+    sink_path -> Errlog.loc_trace_elem list
 end
 
 module MakeSink(TraceElem : TraceElem.S) = struct
@@ -49,6 +53,9 @@ module Make (TraceElem : TraceElem.S) = struct
       (fun (passthroughs, _, sinks) -> passthroughs, sinks)
       (get_reportable_paths t ~trace_of_pname)
 
+  let to_sink_loc_trace ?desc_of_sink ?sink_should_nest (passthroughs, sinks) =
+    to_loc_trace ?desc_of_sink ?sink_should_nest (passthroughs, [], sinks)
+
   let with_callsite t call_site =
     IList.fold_left
       (fun t_acc sink ->

infer/src/checkers/SinkTrace.mli

@@ -20,6 +20,10 @@ module type S = sig
 
   (** update sink with the given call site *)
   val with_callsite : t -> CallSite.t -> t
+
+  val to_sink_loc_trace :
+    ?desc_of_sink:(Sink.t -> string) -> ?sink_should_nest:(Sink.t -> bool) ->
+    sink_path -> Errlog.loc_trace_elem list
 end
 
 module MakeSink (TraceElem : TraceElem.S) :

infer/src/checkers/Siof.ml

@@ -45,16 +45,12 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
     if globals = [] then
       Domain.Bottom
     else
-      let sink_of_global global pname loc =
-        let site = CallSite.make pname loc in
-        SiofTrace.Sink.make global site in
-      let pname = Procdesc.get_proc_name pdesc in
       let trace = match astate with
         | Domain.Bottom -> SiofTrace.initial
         | Domain.NonBottom t -> t in
       let globals_trace =
         IList.fold_left (fun trace_acc global ->
-            SiofTrace.add_sink (sink_of_global global pname loc) trace_acc)
+            SiofTrace.add_sink (SiofTrace.make_access global loc) trace_acc)
           trace
           globals in
       Domain.NonBottom globals_trace
@@ -102,7 +98,7 @@ module Analyzer =
 
 module Interprocedural = Analyzer.Interprocedural (Summary)
 
-let report_siof trace pdesc tenv loc =
+let report_siof tenv trace pdesc gname loc =
   let trace_of_pname pname =
     match Summary.read_summary tenv pdesc pname with
     | Some (SiofDomain.NonBottom summary) -> summary
@@ -110,40 +106,43 @@ let report_siof trace pdesc tenv loc =
 
   let pp_sink f sink =
     let pp_source f v = match Pvar.get_source_file v with
-      | Some source_file when DB.source_file_equal DB.source_file_empty source_file ->
+      | Some source_file when not (DB.source_file_equal DB.source_file_empty source_file) ->
-          F.fprintf f ""
+          F.fprintf f " from file %s" (DB.source_file_to_string source_file)
-      | None ->
+      | _ ->
-          F.fprintf f ""
+          () in
-      | Some source_file ->
-          F.fprintf f " from file %s" (DB.source_file_to_string source_file) in
     let v = SiofTrace.Sink.kind sink in
     F.fprintf f "%s%a" (Pvar.get_simplified_name v) pp_source v in
 
-  let pp_path_part f path =
+  let trace_of_error path =
-    let pp_path f path =
+    let desc_of_sink sink =
-      let pp_sep f () = F.fprintf f " => " in
+      let callsite = SiofTrace.Sink.call_site sink in
-      let pp_elem f (sink, _) =
+      if SiofTrace.is_intraprocedural_access sink then
-        CallSite.pp f (SiofTrace.Sink.call_site sink) in
+        Format.asprintf "access to %s" (Pvar.get_simplified_name (SiofTrace.Sink.kind sink))
-      (F.pp_print_list ~pp_sep) pp_elem f path in
+      else
-    if (IList.length path > 1)
+        Format.asprintf "call to %a" Procname.pp (CallSite.pname callsite) in
-    then F.fprintf f "Full path: %a" pp_path path in
+    let sink_should_nest sink = not (SiofTrace.is_intraprocedural_access sink) in
-
+    let trace_elem_of_global =
-  let report_one_path (_, path) =
+      Errlog.make_trace_element 0 loc
+        (Format.asprintf "initialization of %s" gname)
+        [] in
+    trace_elem_of_global::(SiofTrace.to_sink_loc_trace ~desc_of_sink ~sink_should_nest path) in
+
+  let report_one_path ((_, path) as sink_path) =
     let final_sink = fst (IList.hd path) in
     let description =
       F.asprintf
-        "This global variable initializer accesses the following globals in another translation \
+        "The initializer of %s accesses global variables in another translation unit: %a"
-         unit: %a. %a"
+        gname
-        pp_sink final_sink
+        pp_sink final_sink in
-        pp_path_part (IList.rev path) in
+    let ltr = trace_of_error sink_path in
     let caller_pname = Procdesc.get_proc_name pdesc in
-    let exn = Exceptions.Checkers
+    let msg = Localise.to_string Localise.static_initialization_order_fiasco in
-        ("STATIC_INITIALIZATION_ORDER_FIASCO", Localise.verbatim_desc description) in
+    let exn = Exceptions.Checkers (msg, Localise.verbatim_desc description) in
-    Reporting.log_error caller_pname ~loc exn in
+    Reporting.log_error caller_pname ~loc ~ltr exn in
 
   IList.iter report_one_path (SiofTrace.get_reportable_sink_paths trace ~trace_of_pname)
 
-let siof_check pdesc tenv = function
+let siof_check tenv pdesc gname = function
   | Some (SiofDomain.NonBottom post) ->
       let attrs = Procdesc.get_attributes pdesc in
       let is_orig_file f = match attrs.ProcAttributes.translation_unit with
@@ -158,15 +157,15 @@ let siof_check pdesc tenv = function
           (fun sink -> is_foreign (SiofTrace.Sink.kind sink))
           (SiofTrace.sinks post) in
       if not (SiofTrace.Sinks.is_empty foreign_global_sinks)
-      then report_siof post pdesc tenv attrs.ProcAttributes.loc;
+      then report_siof tenv post pdesc gname attrs.ProcAttributes.loc;
   | Some SiofDomain.Bottom | None ->
       ()
 
 let checker ({ Callbacks.tenv; proc_desc } as callback) =
   let post = Interprocedural.checker callback ProcData.empty_extras in
   let pname = Procdesc.get_proc_name proc_desc in
-  match pname with
+  match Procname.get_global_name_of_initializer pname with
-  | Procname.C c when Procname.is_globals_initializer c ->
+  | Some gname ->
-      siof_check proc_desc tenv post
+      siof_check tenv proc_desc gname post
-  | _ ->
+  | None ->
       ()

infer/src/checkers/SiofTrace.ml

@@ -12,34 +12,38 @@ open! Utils
 module F = Format
 module L = Logging
 
-module Globals = struct
+module Global = struct
   type t = Pvar.t
   let compare = Pvar.compare
   let pp fmt pvar = (Pvar.pp pe_text) fmt pvar
 end
 
-include SinkTrace.Make(struct
+module TraceElem = struct
-    module Kind = Globals
+  module Kind = Global
 
   type t = {
     site : CallSite.t;
-      kind: Kind.t;
+    kind: [`Call | `Access] * Kind.t;
   }
 
   let call_site { site; } = site
 
-    let kind { kind; } = kind
+  let kind { kind; } = snd kind
 
-    let make kind site = { kind; site; }
+  let make kind site = { kind = (`Call, kind); site; }
 
-    let with_callsite t site = { t with site; }
+  let with_callsite { kind=(_, kind); } site = { kind=(`Call, kind); site; }
 
   let compare t1 t2 =
-      CallSite.compare t1.site t2.site
+    let n = CallSite.compare t1.site t2.site in
-      |> next Kind.compare t1.kind t2.kind
+    if n <> 0 then n
+    else
+      let n = tags_compare (fst t1.kind) (fst t2.kind) in
+      if n <> 0 then n
+      else Kind.compare (snd t1.kind) (snd t2.kind)
 
   let pp fmt { site; kind; } =
-      F.fprintf fmt "Access to %a at %a" Kind.pp kind CallSite.pp site
+    F.fprintf fmt "Access to %a at %a" Kind.pp (snd kind) CallSite.pp site
 
   module Set = PrettyPrintable.MakePPSet (struct
       type nonrec t = t
@@ -47,4 +51,12 @@ include SinkTrace.Make(struct
       let pp_element = pp
     end)
 
-  end)
+end
+
+include SinkTrace.Make(TraceElem)
+
+let make_access kind loc =
+  let site = CallSite.make Procname.empty_block loc in
+  { TraceElem.kind = (`Access, kind); site; }
+
+let is_intraprocedural_access { TraceElem.kind=(kind, _); } = kind = `Access

infer/src/checkers/SiofTrace.mli

@@ -9,11 +9,15 @@
 
 open! Utils
 
-module Globals :
+module Global :
 sig
   type t = Pvar.t
   val compare : t -> t -> int
   val pp : Format.formatter -> t -> unit
 end
 
-include SinkTrace.S with module Sink.Kind = Globals
+include SinkTrace.S with module Sink.Kind = Global
+
+val make_access : Global.t -> Location.t -> Sink.t
+
+val is_intraprocedural_access : Sink.t -> bool

infer/src/checkers/Trace.ml

@@ -50,6 +50,14 @@ module type S = sig
   (** get a path for each of the reportable source -> sink flows in this trace *)
   val get_reportable_paths : t -> trace_of_pname:(Procname.t -> t) -> path list
 
+  (** create a loc_trace from a path; [source_should_nest s] should be true when we are going one
+      deeper into a call-chain, ie when lt_level should be bumper in the next loc_trace_elem, and
+      similarly for [sink_should_nest] *)
+  val to_loc_trace :
+    ?desc_of_source:(Source.t -> string) -> ?source_should_nest:(Source.t -> bool) ->
+    ?desc_of_sink:(Sink.t -> string) -> ?sink_should_nest:(Sink.t -> bool) ->
+    path -> Errlog.loc_trace
+
   (** create a trace from a source *)
   val of_source : Source.t -> t
 
@@ -214,6 +222,33 @@ module Make (Spec : Spec) = struct
          passthroughs, sources_passthroughs, sinks_passthroughs)
       (get_reports t)
 
+  let to_loc_trace
+      ?(desc_of_source=fun source ->
+          let callsite = Source.call_site source in
+          Format.asprintf "call to %a" Procname.pp (CallSite.pname callsite))
+      ?(source_should_nest=(fun _ -> true))
+      ?(desc_of_sink=fun sink ->
+          let callsite = Sink.call_site sink in
+          Format.asprintf "call to %a" Procname.pp (CallSite.pname callsite))
+      ?(sink_should_nest=(fun _ -> true))
+      (_, sources, sinks) =
+    let trace_elem_of_path_elem call_site desc should_nest =
+      let level = ref 0 in
+      fun (elem, _) ->
+        let lt_level = !level in
+        let desc = desc elem in
+        let callsite = call_site elem in
+        if should_nest elem then incr level;
+        Errlog.make_trace_element lt_level (CallSite.loc callsite) desc [] in
+    let trace_elem_of_source =
+      trace_elem_of_path_elem Source.call_site desc_of_source source_should_nest in
+    let trace_elem_of_sink =
+      trace_elem_of_path_elem Sink.call_site desc_of_sink sink_should_nest in
+    (* reverse sinks intentionally, do not reverse sources(?) *)
+    IList.rev_append
+      (IList.rev_map trace_elem_of_source sources)
+      (IList.map trace_elem_of_sink (IList.rev sinks))
+
   let of_source source =
     let sources = Sources.singleton source in
     let passthroughs = Passthroughs.empty in

infer/src/checkers/Trace.mli

@@ -50,6 +50,14 @@ module type S = sig
   (** get a path for each of the reportable source -> sink flows in this trace *)
   val get_reportable_paths : t -> trace_of_pname:(Procname.t -> t) -> path list
 
+  (** create a loc_trace from a path; [source_should_nest s] should be true when we are going one
+      deeper into a call-chain, ie when lt_level should be bumper in the next loc_trace_elem, and
+      similarly for [sink_should_nest] *)
+  val to_loc_trace :
+    ?desc_of_source:(Source.t -> string) -> ?source_should_nest:(Source.t -> bool) ->
+    ?desc_of_sink:(Sink.t -> string) -> ?sink_should_nest:(Sink.t -> bool) ->
+    path -> Errlog.loc_trace
+
   (** create a trace from a source *)
   val of_source : Source.t -> t
 

infer/tests/codetoanalyze/cpp/checkers/Makefile

@@ -12,7 +12,7 @@ ANALYZER = checkers
 # see explanations in cpp/errors/Makefile for the custom isystem
 CLANG_OPTIONS = -x c++ -std=c++11 -nostdinc++ -isystem$(MODELS_DIR)/cpp/include -isystem$(CLANG_INCLUDES)/c++/v1/ -c
 INFER_OPTIONS = --cxx --ml-buckets cpp --no-filtering --debug-exceptions
-INFERPRINT_OPTIONS = --issues-txt
+INFERPRINT_OPTIONS = --issues-tests
 
 SOURCES = \
   siof/const.cpp \

infer/tests/codetoanalyze/cpp/checkers/issues.exp

@@ -1,4 +1,4 @@
-siof/const_use.cpp:17: ERROR: STATIC_INITIALIZATION_ORDER_FIASCO This global variable initializer accesses the following globals in another translation unit: u. 
+siof/const_use.cpp, __infer_globals_initializer_use_u, 0, STATIC_INITIALIZATION_ORDER_FIASCO
-siof/pod_across_translation_units-1.cpp:13: ERROR: STATIC_INITIALIZATION_ORDER_FIASCO This global variable initializer accesses the following globals in another translation unit: y from file siof/pod_across_translation_units-2.cpp. Full path: baz at [line 13] => bar at [line 12] => foo at [line 11]
+siof/pod_across_translation_units-1.cpp, __infer_globals_initializer_x, 0, STATIC_INITIALIZATION_ORDER_FIASCO
-siof/siof_across_translation_units-1.cpp:21: ERROR: STATIC_INITIALIZATION_ORDER_FIASCO This global variable initializer accesses the following globals in another translation unit: another_global_object from file siof/siof_across_translation_units-1.cpp. 
+siof/siof_across_translation_units-1.cpp, __infer_globals_initializer_another_global_object, 0, STATIC_INITIALIZATION_ORDER_FIASCO
-siof/siof_across_translation_units-1.cpp:21: ERROR: STATIC_INITIALIZATION_ORDER_FIASCO This global variable initializer accesses the following globals in another translation unit: global_object. 
+siof/siof_across_translation_units-1.cpp, __infer_globals_initializer_another_global_object, 0, STATIC_INITIALIZATION_ORDER_FIASCO