From 7615963bf43b0ea0017b1503c2e584f69c01a91d Mon Sep 17 00:00:00 2001 From: Jules Villard Date: Fri, 5 Oct 2018 09:35:13 -0700 Subject: [PATCH] [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 | 1 + infer/src/IR/SourceFiles.ml | 4 +- infer/src/backend/callbacks.ml | 33 ++++++------ infer/tests/base.make | 9 +++- .../build_systems/codetoanalyze/hello_dup.c | 13 +++++ .../build_systems/duplicate_symbols/Makefile | 19 +++++++ .../duplicate_symbols/issues.exp | 1 + infer/tests/clang.make | 1 - .../codetoanalyze/c/bufferoverrun/for_loop.c | 6 +-- .../codetoanalyze/c/performance/instantiate.c | 4 +- .../cpp/ownership/reference_wrapper.cpp | 12 ++--- .../cpp/ownership/use_after_delete.cpp | 50 +++++++++---------- .../codetoanalyze/cpp/uninit/members.cpp | 18 +++---- .../tests/codetoanalyze/objc/errors/Makefile | 4 +- .../initialization/struct_initlistexpr.c | 4 +- infer/tests/infer.make | 1 + 16 files changed, 111 insertions(+), 69 deletions(-) create mode 100644 infer/tests/build_systems/codetoanalyze/hello_dup.c create mode 100644 infer/tests/build_systems/duplicate_symbols/Makefile create mode 100644 infer/tests/build_systems/duplicate_symbols/issues.exp diff --git a/Makefile b/Makefile index f50c0e5ed..3c3a0bb5d 100644 --- a/Makefile +++ b/Makefile @@ -35,6 +35,7 @@ BUILD_SYSTEMS_TESTS += \ delete_results_dir \ diff \ diff_gen_build_script \ + duplicate_symbols \ fail_on_issue \ j1 \ linters \ diff --git a/infer/src/IR/SourceFiles.ml b/infer/src/IR/SourceFiles.ml index fd97ba4df..d12b99467 100644 --- a/infer/src/IR/SourceFiles.ml +++ b/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 "@[%a@]" pp_result stmt ) diff --git a/infer/src/backend/callbacks.ml b/infer/src/backend/callbacks.ml index 69f4f1449..8d2b41d8e 100644 --- a/infer/src/backend/callbacks.ml +++ b/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. *) diff --git a/infer/tests/base.make b/infer/tests/base.make index 848886941..a487d9295 100644 --- a/infer/tests/base.make +++ b/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 diff --git a/infer/tests/build_systems/codetoanalyze/hello_dup.c b/infer/tests/build_systems/codetoanalyze/hello_dup.c new file mode 100644 index 000000000..d1dd780a4 --- /dev/null +++ b/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 + +void test() { + int* s = NULL; + *s = 42; +} diff --git a/infer/tests/build_systems/duplicate_symbols/Makefile b/infer/tests/build_systems/duplicate_symbols/Makefile new file mode 100644 index 000000000..fd06e119a --- /dev/null +++ b/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 diff --git a/infer/tests/build_systems/duplicate_symbols/issues.exp b/infer/tests/build_systems/duplicate_symbols/issues.exp new file mode 100644 index 000000000..126a0a90f --- /dev/null +++ b/infer/tests/build_systems/duplicate_symbols/issues.exp @@ -0,0 +1 @@ +DUPLICATE_SYMBOLS source:hello.c source_captured:hello_dup.c pname:test diff --git a/infer/tests/clang.make b/infer/tests/clang.make index e19cdae63..77fe38552 100644 --- a/infer/tests/clang.make +++ b/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) diff --git a/infer/tests/codetoanalyze/c/bufferoverrun/for_loop.c b/infer/tests/codetoanalyze/c/bufferoverrun/for_loop.c index 2ed619e18..211b32501 100644 --- a/infer/tests/codetoanalyze/c/bufferoverrun/for_loop.c +++ b/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; } diff --git a/infer/tests/codetoanalyze/c/performance/instantiate.c b/infer/tests/codetoanalyze/c/performance/instantiate.c index 29094e14e..5b2ae06f8 100644 --- a/infer/tests/codetoanalyze/c/performance/instantiate.c +++ b/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(); } } diff --git a/infer/tests/codetoanalyze/cpp/ownership/reference_wrapper.cpp b/infer/tests/codetoanalyze/cpp/ownership/reference_wrapper.cpp index 2561a128e..9b1ff26f3 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/reference_wrapper.cpp +++ b/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() { diff --git a/infer/tests/codetoanalyze/cpp/ownership/use_after_delete.cpp b/infer/tests/codetoanalyze/cpp/ownership/use_after_delete.cpp index b2274cafa..5f867130b 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/use_after_delete.cpp +++ b/infer/tests/codetoanalyze/cpp/ownership/use_after_delete.cpp @@ -9,50 +9,50 @@ #include #include -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 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; diff --git a/infer/tests/codetoanalyze/cpp/uninit/members.cpp b/infer/tests/codetoanalyze/cpp/uninit/members.cpp index 6a7b5156e..bcd833e08 100644 --- a/infer/tests/codetoanalyze/cpp/uninit/members.cpp +++ b/infer/tests/codetoanalyze/cpp/uninit/members.cpp @@ -6,7 +6,7 @@ */ #include -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; diff --git a/infer/tests/codetoanalyze/objc/errors/Makefile b/infer/tests/codetoanalyze/objc/errors/Makefile index ef2b5c672..9100f05b5 100644 --- a/infer/tests/codetoanalyze/objc/errors/Makefile +++ b/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 diff --git a/infer/tests/codetoanalyze/objc/errors/initialization/struct_initlistexpr.c b/infer/tests/codetoanalyze/objc/errors/initialization/struct_initlistexpr.c index abdc13852..de32be608 100644 --- a/infer/tests/codetoanalyze/objc/errors/initialization/struct_initlistexpr.c +++ b/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}; diff --git a/infer/tests/infer.make b/infer/tests/infer.make index 7f7ecfd88..7c98a7b49 100644 --- a/infer/tests/infer.make +++ b/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)