[proc-cfg][2/5] fix duplicate symbols detection

Summary:
Fix the logic for computing duplicate symbols. It was broken at some point and some duplicate symbols creeped into our tests. Fix these, and add a test to avoid duplicate symbols detection to regress again.

Also, this removes one use of `Cfg.load`, on the way to removing file-wide CFGs from the database.

Reviewed By: ngorogiannis

Differential Revision: D10173349

fbshipit-source-id: a0d2365b3

Makefile

@@ -35,6 +35,7 @@ BUILD_SYSTEMS_TESTS += \
   delete_results_dir \
   diff \
   diff_gen_build_script \
+  duplicate_symbols \
   fail_on_issue \
   j1 \
   linters \

infer/src/IR/SourceFiles.ml

@@ -190,6 +190,8 @@ let pp_all ~filter ~cfgs ~type_environment ~procedure_names ~freshly_captured fm
       in
       let pp_result fmt stmt =
         Container.iter stmt ~f:(pp fmt)
-          ~fold:(SqliteUtils.result_fold_single_column_rows db ~log:"printing all source files")
+          ~fold:
+            (SqliteUtils.result_fold_single_column_rows ~finalize:false db
+               ~log:"printing all source files")
       in
       F.fprintf fmt "@[<v>%a@]" pp_result stmt )

infer/src/backend/callbacks.ml

@@ -98,20 +98,22 @@ let iterate_cluster_callbacks all_procs exe_env source_file =
       !cluster_callbacks
 
 
-let dump_duplicate_procs (exe_env : Exe_env.t) source_file procs =
+let dump_duplicate_procs source_file procs =
   let duplicate_procs =
     List.filter_map procs ~f:(fun pname ->
-        match Exe_env.get_proc_desc exe_env pname with
-        | Some pdesc when (* defined in the current file *) Procdesc.is_defined pdesc -> (
-          match Attributes.load pname with
-          | Some {translation_unit; loc}
-            when (* defined in another file *)
-                 (not (SourceFile.equal source_file translation_unit))
-                 && (* really defined in the current file and not in an include *)
-                    SourceFile.equal source_file loc.file ->
-              Some (pname, translation_unit)
-          | _ ->
-              None )
+        match Attributes.load pname with
+        | Some
+            { is_defined=
+                true
+                (* likely not needed: if [pname] is part of [procs] then it *is* defined, so we
+                   expect the attribute to be defined too *)
+            ; translation_unit
+            ; loc }
+          when (* defined in another file *)
+               (not (SourceFile.equal source_file translation_unit))
+               && (* really defined in that file and not in an include *)
+                  SourceFile.equal translation_unit loc.file ->
+            Some (pname, translation_unit)
         | _ ->
             None )
   in
@@ -120,8 +122,9 @@ let dump_duplicate_procs (exe_env : Exe_env.t) source_file procs =
       ~append:true ~perm:0o666 ~f:(fun outc ->
         let fmt = F.formatter_of_out_channel outc in
         List.iter duplicate_procs ~f:(fun (pname, source_captured) ->
-            F.fprintf fmt "@.DUPLICATE_SYMBOLS source:%a source_captured:%a pname:%a@."
-              SourceFile.pp source_file SourceFile.pp source_captured Typ.Procname.pp pname ) )
+            F.fprintf fmt "DUPLICATE_SYMBOLS source:%a source_captured:%a pname:%a@\n"
+              SourceFile.pp source_file SourceFile.pp source_captured Typ.Procname.pp pname ) ;
+        F.pp_print_flush fmt () )
   in
   if not (List.is_empty duplicate_procs) then output_to_file duplicate_procs
 
@@ -141,7 +144,7 @@ let analyze_file (exe_env : Exe_env.t) source_file =
     (* analyze all the currently defined procedures *)
     SourceFiles.proc_names_of_source source_file
   in
-  if Config.dump_duplicate_symbols then dump_duplicate_procs exe_env source_file procs_to_analyze ;
+  if Config.dump_duplicate_symbols then dump_duplicate_procs source_file procs_to_analyze ;
   let analyze_proc_name pname = ignore (Ondemand.analyze_proc_name pname : Summary.t option) in
   List.iter ~f:analyze_proc_name procs_to_analyze ;
   (* Invoke cluster callbacks. *)

infer/tests/base.make

@@ -13,8 +13,13 @@ TEST_REL_DIR = $(patsubst $(abspath $(TESTS_DIR))/%,%,$(abspath $(CURDIR)))
 
 define check_no_duplicates
   if grep -q "DUPLICATE_SYMBOLS" $(1); then \
-    printf '$(TERM_ERROR)Duplicate symbols found in $(CURDIR).$(TERM_RESET)\n' >&2; \
-    printf '$(TERM_ERROR)Please make sure all the function names in all the source test files are different.$(TERM_RESET)' >&2; \
+    printf '$(TERM_ERROR)Duplicate symbols found in $(CURDIR):$(TERM_RESET)\n' >&2; \
+    printf '$(TERM_ERROR)========$(TERM_RESET)\n' >&2; \
+    while read line; do \
+      printf '$(TERM_ERROR)%s$(TERM_RESET)\n' "$$line" >&2; \
+    done <$(1); \
+    printf '$(TERM_ERROR)========$(TERM_RESET)\n' >&2; \
+    printf '$(TERM_ERROR)Please make sure all the function names in all the source test files are different.$(TERM_RESET)\n' >&2; \
     exit 1; \
   fi
 endef

infer/tests/build_systems/codetoanalyze/hello_dup.c

@@ -0,0 +1,13 @@
+/*
+ * Copyright (c) 2015-present, Facebook, Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#include <stdlib.h>
+
+void test() {
+  int* s = NULL;
+  *s = 42;
+}

infer/tests/build_systems/duplicate_symbols/Makefile

@@ -0,0 +1,19 @@
+# Copyright (c) 2016-present, Facebook, Inc.
+#
+# This source code is licensed under the MIT license found in the
+# LICENSE file in the root directory of this source tree.
+
+TESTS_DIR = ../..
+
+CLANG_OPTIONS = -c
+INFER_OPTIONS = --project-root ../codetoanalyze
+INFERPRINT_OPTIONS = --issues-tests
+
+SOURCES = ../codetoanalyze/hello.c ../codetoanalyze/hello_dup.c
+
+include $(TESTS_DIR)/clang.make
+
+issues.exp.test:
+	$(QUIET)cat infer-out$(TEST_SUFFIX)/duplicates.txt > $@
+#	delete duplicates file to avoid throwing an error because duplicate symbols were found
+	$(QUIET)$(REMOVE) infer-out$(TEST_SUFFIX)/duplicates.txt

infer/tests/build_systems/duplicate_symbols/issues.exp

@@ -0,0 +1 @@
+DUPLICATE_SYMBOLS source:hello.c source_captured:hello_dup.c pname:test

infer/tests/clang.make

@@ -17,4 +17,3 @@ infer-out$(TEST_SUFFIX)/report.json: $(CLANG_DEPS) $(SOURCES) $(HEADERS) $(TESTS
 	  $(INFER_BIN) --results-dir $(@D) --dump-duplicate-symbols \
 	    $(INFER_OPTIONS) -- \
 	    clang $(CLANG_OPTIONS) $(SOURCES))
-	$(QUIET)$(call check_no_duplicates,infer-out$(TEST_SUFFIX)/duplicates.txt)

infer/tests/codetoanalyze/c/bufferoverrun/for_loop.c

@@ -37,14 +37,14 @@ void for_loop() {
   }
 }
 
-void nop() { int k = 0; }
+void no_op() { int k = 0; }
 
 int two_loops(int m) {
   for (int i = 0; i < m; i++) {
-    nop();
+    no_op();
   }
   for (int j = 0; j < m; j++) {
-    nop();
+    no_op();
   }
   return m;
 }

infer/tests/codetoanalyze/c/performance/instantiate.c

@@ -5,12 +5,12 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-void nop() { int x = 0; }
+void no_op() { int x = 0; }
 
 // Expected: Theta(n)
 void do_n_times(int n) {
   for (int i = 0; i < n; i++) {
-    nop();
+    no_op();
   }
 }
 

infer/tests/codetoanalyze/cpp/ownership/reference_wrapper.cpp

@@ -9,22 +9,22 @@ struct B {
   int f;
 };
 
-struct A {
-  A(int f) { b = new B(f); }
+struct WrapsB {
+  WrapsB(int f) { b = new B(f); }
   B* b;
   B* getb() { return b; };
-  ~A() { delete b; }
+  ~WrapsB() { delete b; }
 };
 
 struct ReferenceWrapperHeap {
-  ReferenceWrapperHeap(A& a) : b(a.getb()){};
+  ReferenceWrapperHeap(WrapsB& a) : b(a.getb()){};
   B* b;
 };
 
 ReferenceWrapperHeap getwrapperHeap() {
-  A a(1);
+  WrapsB a(1);
   return a; // We store a.b in ReferenceWrapper, but we delete a.b in the
-            // destructor of A
+            // destructor of WrapsB
 }
 
 int FN_reference_wrapper_heap_bad() {

infer/tests/codetoanalyze/cpp/ownership/use_after_delete.cpp

@@ -9,50 +9,50 @@
 #include <string>
 #include <vector>
 
-struct S {
+struct Simple {
   int f;
 };
 
 void deref_deleted_bad() {
-  auto s = new S{1};
+  auto s = new Simple{1};
   delete s;
-  S tmp = *s;
+  Simple tmp = *s;
 }
 
-S* return_deleted_bad() {
-  auto s = new S{1};
+Simple* return_deleted_bad() {
+  auto s = new Simple{1};
   delete s;
   return s;
 }
 
-S* reassign_deleted_ok() {
-  auto s = new S{1};
+Simple* reassign_deleted_ok() {
+  auto s = new Simple{1};
   delete s;
-  s = new S{2};
+  s = new Simple{2};
   return s;
 }
 
 void reassign_field_of_deleted_bad() {
-  auto s = new S{1};
+  auto s = new Simple{1};
   delete s;
   s->f = 7;
 }
 
-void reassign_field_of_reinitialized_ok(S* tmp) {
-  auto s = new S{1};
+void reassign_field_of_reinitialized_ok(Simple* tmp) {
+  auto s = new Simple{1};
   delete s;
   s = tmp;
   s->f = 7;
 }
 
 void double_delete_bad() {
-  auto s = new S{1};
+  auto s = new Simple{1};
   delete s;
   delete s;
 }
 
-S* delete_in_branch_bad(bool b) {
-  auto s = new S{1};
+Simple* delete_in_branch_bad(bool b) {
+  auto s = new Simple{1};
   if (b) {
     delete s;
   }
@@ -60,7 +60,7 @@ S* delete_in_branch_bad(bool b) {
 }
 
 void delete_in_branch_ok(bool b) {
-  auto s = new S{1};
+  auto s = new Simple{1};
   if (b) {
     delete s;
   } else {
@@ -69,7 +69,7 @@ void delete_in_branch_ok(bool b) {
 }
 
 void use_in_branch_bad(bool b) {
-  auto s = new S{1};
+  auto s = new Simple{1};
   delete s;
   if (b) {
     auto tmp = *s;
@@ -77,7 +77,7 @@ void use_in_branch_bad(bool b) {
 }
 
 void delete_in_loop_bad() {
-  auto s = new S{1};
+  auto s = new Simple{1};
   for (int i = 0; i < 10; i++) {
     delete s;
   }
@@ -85,7 +85,7 @@ void delete_in_loop_bad() {
 
 void delete_in_loop_ok() {
   for (int i = 0; i < 10; i++) {
-    auto s = new S{1};
+    auto s = new Simple{1};
     delete s;
   }
 }
@@ -99,15 +99,15 @@ void delete_ref_in_loop_ok(int j, std::vector<std::string> v) {
 }
 
 void use_in_loop_bad() {
-  auto s = new S{1};
+  auto s = new Simple{1};
   delete s;
   for (int i = 0; i < 10; i++) {
     s->f = i;
   }
 }
 
-S* gated_delete_abort_ok(bool b) {
-  auto s = new S{1};
+Simple* gated_delete_abort_ok(bool b) {
+  auto s = new Simple{1};
   if (b) {
     delete s;
     std::abort();
@@ -115,8 +115,8 @@ S* gated_delete_abort_ok(bool b) {
   return s;
 }
 
-S* gated_exit_abort_ok(bool b) {
-  auto s = new S{1};
+Simple* gated_exit_abort_ok(bool b) {
+  auto s = new Simple{1};
   if (b) {
     delete s;
     exit(1);
@@ -124,8 +124,8 @@ S* gated_exit_abort_ok(bool b) {
   return s;
 }
 
-S* gated_delete_throw_ok(bool b) {
-  auto s = new S{1};
+Simple* gated_delete_throw_ok(bool b) {
+  auto s = new Simple{1};
   if (b) {
     delete s;
     throw 5;

infer/tests/codetoanalyze/cpp/uninit/members.cpp

@@ -6,7 +6,7 @@
  */
 #include <string>
 
-class A {
+class SomeStructA {
 
  public:
   int* ptr;
@@ -14,11 +14,11 @@ class A {
   std::string* p_a_name;
   int i;
 
-  A() {} // user defined default constructor.
-         // Does not initialize i so, it gets random value
+  SomeStructA() {} // user defined default constructor.
+  // Does not initialize i so, it gets random value
 };
 
-class B {
+class SomeStructB {
 
  public:
   std::string a_name;
@@ -29,7 +29,7 @@ class B {
 };
 
 int access_members_bad() {
-  A a;
+  SomeStructA a;
 
   std::string n = a.a_name;
   int i = a.i; // error
@@ -38,7 +38,7 @@ int access_members_bad() {
 };
 
 int access_members_bad2() {
-  A a{};
+  SomeStructA a{};
 
   std::string n = a.a_name;
   int i = a.i; // error
@@ -48,7 +48,7 @@ int access_members_bad2() {
 
 int access_members_ok() {
 
-  B b{}; // call default implicit constructor which initialize i.
+  SomeStructB b{}; // call default implicit constructor which initialize i.
   std::string n = b.a_name;
   int i = b.i;
 
@@ -57,7 +57,7 @@ int access_members_ok() {
 
 int access_members_bad3() {
 
-  B b; // no constructor is called
+  SomeStructB b; // no constructor is called
   std::string n = b.a_name;
   int i = b.i;
 
@@ -65,7 +65,7 @@ int access_members_bad3() {
 };
 
 int access_pointer_members_bad() {
-  A a;
+  SomeStructA a;
 
   int* p = a.ptr;
   int i = a.i;

infer/tests/codetoanalyze/objc/errors/Makefile

@@ -126,19 +126,16 @@ infer-out-all/report.json: $(CLANG_DEPS) $(SOURCES_BUCKET_ALL)
 	$(QUIET)$(call silent_on_success,Testing infer/Objective-C with all memleak buckets,\
 	  $(INFER_BIN) $(INFER_OPTIONS) --ml-buckets all -o infer-out-all -- \
 	  clang $(CLANG_OPTIONS) $(SOURCES_BUCKET_ALL))
-	$(QUIET)$(call check_no_duplicates,infer-out-all/duplicates.txt)
 
 infer-out-arc/report.json: $(CLANG_DEPS) $(SOURCES_ARC)
 	$(QUIET)$(call silent_on_success,Testing infer/Objective-C with arc memleak buckets,\
 	  $(INFER_BIN) $(INFER_OPTIONS) --ml-buckets cf -o infer-out-arc -- \
 	  clang $(CLANG_OPTIONS) -fobjc-arc $(SOURCES_ARC))
-	$(QUIET)$(call check_no_duplicates,infer-out-arc/duplicates.txt)
 
 infer-out/report.json: $(CLANG_DEPS) $(SOURCES_DEFAULT)
 	$(QUIET)$(call silent_on_success,Testing infer/Objective-C with CF memleak buckets,\
 	  $(INFER_BIN) $(INFER_OPTIONS) --ml-buckets cf -o infer-out -- \
 	  clang $(CLANG_OPTIONS) $(SOURCES_DEFAULT))
-	$(QUIET)$(call check_no_duplicates,infer-out/duplicates.txt)
 
 issues.exp.test: infer-out-all/report.json infer-out-arc/report.json infer-out/report.json
 	$(QUIET)$(INFER_BIN) report -q $(INFERPRINT_OPTIONS) $@.all \
@@ -148,3 +145,4 @@ issues.exp.test: infer-out-all/report.json infer-out-arc/report.json infer-out/r
 	$(QUIET)$(INFER_BIN) report -q $(INFERPRINT_OPTIONS) $@.default \
 	  -o infer-out --from-json-report infer-out/report.json
 	$(QUIET)cat $@.all $@.arc $@.default > $@
+	$(QUIET)cat infer-out-all/duplicates.txt infer-out-arc/duplicates.txt >> infer-out/duplicates.txt

infer/tests/codetoanalyze/objc/errors/initialization/struct_initlistexpr.c

@@ -10,9 +10,9 @@ typedef struct Point {
   int y;
 } Point;
 
-int foo() { return 5; }
+int return_5() { return 5; }
 
-int main() { struct Point p = {1, foo() + 3}; }
+void init_Point() { struct Point p = {1, return_5() + 3}; }
 
 int point_coords_set_correctly(Point* p) {
   *p = (Point){4, 5};

infer/tests/infer.make

@@ -26,6 +26,7 @@ print: issues.exp.test$(TEST_SUFFIX)
 test: issues.exp.test$(TEST_SUFFIX)
 	$(QUIET)cd $(TESTS_DIR) && \
 	$(call check_no_diff,$(TEST_REL_DIR)/issues.exp$(TEST_RESULT_SUFFIX),$(TEST_REL_DIR)/issues.exp.test$(TEST_SUFFIX))
+	$(QUIET)$(call check_no_duplicates,$(INFER_OUT)/duplicates.txt)
 
 .PHONY: print
 replace: issues.exp.test$(TEST_SUFFIX)