From ae9a184108e72f082d2f21fc9f66abd8309d2ea7 Mon Sep 17 00:00:00 2001 From: Martin Medler Date: Sun, 27 Aug 2023 17:47:53 +0200 Subject: [PATCH] wip --- src/analyze_includes/main.py | 10 +++- src/analyze_includes/parse_source.py | 57 +++++++++++++------ src/analyze_includes/test/BUILD | 2 + src/analyze_includes/test/data/some_defines.h | 2 + src/analyze_includes/test/data/use_defines.h | 13 +++++ .../test/parse_source_test.py | 24 +++++--- src/aspect/dwyu.bzl | 4 +- test/aspect/defines/BUILD | 15 ++++- test/aspect/defines/README.md | 11 +--- .../defines/defines_from_bazel_target.h | 12 ++-- test/aspect/defines/in_file_defines.h | 10 ++-- test/aspect/defines/lib/BUILD | 11 ---- test/aspect/defines/lib/a.h | 0 test/aspect/defines/support/BUILD | 29 +++++++++- test/aspect/defines/support/a.h | 1 + test/aspect/defines/{lib => support}/b.h | 0 test/aspect/defines/support/c.h | 1 + .../defines/support/conditional_defines.h | 13 +++++ test/aspect/defines/support/some_defines.h | 3 + .../transitive_defines_from_bazel_target.h | 11 ++-- .../use_defines_from_dependency_header.h | 16 ++++++ test/aspect/execute_tests.py | 20 +------ 22 files changed, 179 insertions(+), 86 deletions(-) create mode 100644 src/analyze_includes/test/data/some_defines.h create mode 100644 src/analyze_includes/test/data/use_defines.h delete mode 100644 test/aspect/defines/lib/BUILD delete mode 100644 test/aspect/defines/lib/a.h create mode 100644 test/aspect/defines/support/a.h rename test/aspect/defines/{lib => support}/b.h (100%) create mode 100644 test/aspect/defines/support/c.h create mode 100644 test/aspect/defines/support/conditional_defines.h create mode 100644 test/aspect/defines/support/some_defines.h create mode 100644 test/aspect/defines/use_defines_from_dependency_header.h diff --git a/src/analyze_includes/main.py b/src/analyze_includes/main.py index 105e0212..b19503c5 100644 --- a/src/analyze_includes/main.py +++ b/src/analyze_includes/main.py @@ -76,10 +76,16 @@ def main(args: Namespace) -> int: implementation_deps=args.implementation_deps, ) all_includes_from_public = get_relevant_includes_from_files( - files=args.public_files, ignored_includes=ignored_includes, defines=system_under_inspection.defines + files=args.public_files, + ignored_includes=ignored_includes, + defines=system_under_inspection.defines, + include_paths=system_under_inspection.include_paths, ) all_includes_from_private = get_relevant_includes_from_files( - files=args.private_files, ignored_includes=ignored_includes, defines=system_under_inspection.defines + files=args.private_files, + ignored_includes=ignored_includes, + defines=system_under_inspection.defines, + include_paths=system_under_inspection.include_paths, ) result = evaluate_includes( diff --git a/src/analyze_includes/parse_source.py b/src/analyze_includes/parse_source.py index baa428b2..2d63a34a 100644 --- a/src/analyze_includes/parse_source.py +++ b/src/analyze_includes/parse_source.py @@ -4,27 +4,33 @@ from pathlib import Path from typing import List, Optional -from pcpp import Preprocessor +from pcpp.preprocessor import Action, OutputDirective, Preprocessor class SimpleParsingPreprocessor(Preprocessor): """ - This preprocessor configuration is used to prune commented code and to resolve preprocessor statements for defines - which are injected through Bazel. We do not resolve include statements. Meaning each file is analyzed only for - itself. + This preprocessor configuration is used to prune commented code and to resolve preprocessor statements. The main + points for us is resolving branching statements (e.g. '#ifdef') to analyze the correct code parts. """ - def on_file_open(self, _, __): + def on_include_not_found(self, is_malformed, is_system_include, curdir, includepath): """ - Raising here prevents include statements being resolved + We ignore missing include statements. Mostly they are system includes from the standard headers which are not + available for the DWYU analysis. Those don't matter to the DWYU analysis. Even if we would fail to other headers + this is not dramatic. The worst that can happen is that we miss defines from included headers. Given until + recently were not able to parse defines from included headers at all, we accept this shortcoming for now. """ - raise OSError("Do not open file") + raise OutputDirective(Action.IgnoreAndPassThrough) - def on_error(self, _, __, ___): - """ - Since unresolved include statements cause errors we silence error reporting - """ - pass + +def make_pre_processor() -> SimpleParsingPreprocessor: + """ + We can't overwrite member values from the base class in the ctor of our derived class and thus set them after + construction. + """ + processor = SimpleParsingPreprocessor() + processor.passthru_includes = re.compile(".*") + return processor @dataclass @@ -65,19 +71,34 @@ def is_ignored(self, include: str) -> bool: return is_ignored_path or is_ignored_pattern -def get_includes_from_file(file: Path, defines: List[str]) -> List[Include]: +def get_includes_from_file(file: Path, defines: List[str], include_paths: List[str]) -> List[Include]: """ Parse a C/C++ file and extract include statements which are neither commented nor disabled through a define. """ with open(file, encoding="utf-8") as fin: - pre_processor = SimpleParsingPreprocessor() + pre_processor = make_pre_processor() + for define in defines: - pre_processor.define(define) - pre_processor.parse(fin.read()) + # TODO investigate why 'FOO="bar"' is bad, but 'FOO ="bar' not + # TODO investigate did this cause some analysis failing and we simply never catched it by tests? + # pre_processor.define(define.replace("=", " = ")) + # TODO double check: It seems '=' is actually invalid inside C++ for defines and merely a syntax outside in the compiler CLI + pre_processor.define(define.replace("=", " ")) + for path in include_paths: + pre_processor.add_path(path) + + # print("###########") + # print(pre_processor.macros) + # print("###########") + pre_processor.parse(fin.read()) output_sink = StringIO() pre_processor.write(output_sink) + # print("-------------------") + # print(output_sink.getvalue()) + # print("-------------------") + return [ Include(file=file, include=include) for include in re.findall(r'^\s*#include\s*["<](.+)[">]', output_sink.getvalue(), re.MULTILINE) @@ -94,11 +115,11 @@ def filter_includes(includes: List[Include], ignored_includes: IgnoredIncludes) def get_relevant_includes_from_files( - files: Optional[List[str]], ignored_includes: IgnoredIncludes, defines: List[str] + files: Optional[List[str]], ignored_includes: IgnoredIncludes, defines: List[str], include_paths: List[str] ) -> List[Include]: all_includes = [] if files: for file in files: - includes = get_includes_from_file(file=Path(file), defines=defines) + includes = get_includes_from_file(file=Path(file), defines=defines, include_paths=include_paths) all_includes.extend(includes) return filter_includes(includes=all_includes, ignored_includes=ignored_includes) diff --git a/src/analyze_includes/test/BUILD b/src/analyze_includes/test/BUILD index 6ed7b6ac..3b1b81cc 100644 --- a/src/analyze_includes/test/BUILD +++ b/src/analyze_includes/test/BUILD @@ -28,7 +28,9 @@ py_test( "data/commented_includes/single_line_comments.h", "data/empty_header.h", "data/header_with_defines.h", + "data/some_defines.h", "data/some_header.h", + "data/use_defines.h", ], deps = ["//src/analyze_includes:lib"], ) diff --git a/src/analyze_includes/test/data/some_defines.h b/src/analyze_includes/test/data/some_defines.h new file mode 100644 index 00000000..21f70556 --- /dev/null +++ b/src/analyze_includes/test/data/some_defines.h @@ -0,0 +1,2 @@ +#define MY_DEFINE +#define THE_ANSWER 42 diff --git a/src/analyze_includes/test/data/use_defines.h b/src/analyze_includes/test/data/use_defines.h new file mode 100644 index 00000000..f31aeec8 --- /dev/null +++ b/src/analyze_includes/test/data/use_defines.h @@ -0,0 +1,13 @@ +#include "src/analyze_includes/test/data/some_defines.h" + +#ifdef MY_DEFINE +#include "expected/include_a.h" +#else +#include "bad/include_a.h" +#endif + +#if THE_ANSWER > 40 +#include "expected/include_b.h" +#else +#include "bad/include_b.h" +#endif diff --git a/src/analyze_includes/test/parse_source_test.py b/src/analyze_includes/test/parse_source_test.py index da7610ad..8584e594 100644 --- a/src/analyze_includes/test/parse_source_test.py +++ b/src/analyze_includes/test/parse_source_test.py @@ -100,19 +100,19 @@ def test_filter_includes_for_patterns(self): class TestGetIncludesFromFile(unittest.TestCase): def test_empty_header(self): test_file = Path("src/analyze_includes/test/data/empty_header.h") - result = get_includes_from_file(test_file, defines=[]) + result = get_includes_from_file(test_file, defines=[], include_paths=[]) self.assertEqual(result, []) def test_single_include(self): test_file = Path("src/analyze_includes/test/data/another_header.h") - result = get_includes_from_file(test_file, defines=[]) + result = get_includes_from_file(test_file, defines=[], include_paths=[]) self.assertEqual(result, [Include(file=test_file, include="foo/bar.h")]) def test_multiple_includes(self): test_file = Path("src/analyze_includes/test/data/some_header.h") - result = get_includes_from_file(test_file, defines=[]) + result = get_includes_from_file(test_file, defines=[], include_paths=[]) self.assertEqual(len(result), 4) self.assertTrue(Include(file=test_file, include="bar.h") in result) @@ -121,7 +121,7 @@ def test_multiple_includes(self): def test_commented_includes_single_line_comments(self): test_file = Path("src/analyze_includes/test/data/commented_includes/single_line_comments.h") - result = get_includes_from_file(test_file, defines=[]) + result = get_includes_from_file(test_file, defines=[], include_paths=[]) self.assertEqual(len(result), 2) self.assertTrue(Include(file=test_file, include="active_a.h") in result) @@ -129,7 +129,7 @@ def test_commented_includes_single_line_comments(self): def test_commented_includes_block_comments(self): test_file = Path("src/analyze_includes/test/data/commented_includes/block_comments.h") - result = get_includes_from_file(test_file, defines=[]) + result = get_includes_from_file(test_file, defines=[], include_paths=[]) self.assertEqual(len(result), 8) self.assertTrue(Include(file=test_file, include="active_a.h") in result) @@ -143,13 +143,13 @@ def test_commented_includes_block_comments(self): def test_commented_includes_mixed_style(self): test_file = Path("src/analyze_includes/test/data/commented_includes/mixed_style.h") - result = get_includes_from_file(test_file, defines=[]) + result = get_includes_from_file(test_file, defines=[], include_paths=[]) self.assertEqual(result, [Include(file=test_file, include="active.h")]) def test_includes_selected_through_defines(self): test_file = Path("src/analyze_includes/test/data/header_with_defines.h") - result = get_includes_from_file(test_file, defines=["FOO", "BAZ 42"]) + result = get_includes_from_file(test_file, defines=["FOO", "BAZ 42"], include_paths=[]) self.assertEqual(len(result), 4) self.assertTrue(Include(file=test_file, include="has_internal.h") in result) @@ -157,6 +157,15 @@ def test_includes_selected_through_defines(self): self.assertTrue(Include(file=test_file, include="no_bar.h") in result) self.assertTrue(Include(file=test_file, include="baz_greater_40.h") in result) + def test_includes_selected_through_defines_from_header(self): + test_file = Path("src/analyze_includes/test/data/use_defines.h") + result = get_includes_from_file(test_file, defines=[], include_paths=[]) + + self.assertEqual(len(result), 3) + self.assertTrue(Include(file=test_file, include="src/analyze_includes/test/data/some_defines.h") in result) + self.assertTrue(Include(file=test_file, include="expected/include_a.h") in result) + self.assertTrue(Include(file=test_file, include="expected/include_b.h") in result) + class TestGetRelevantIncludesFromFiles(unittest.TestCase): def test_get_relevant_includes_from_files(self): @@ -164,6 +173,7 @@ def test_get_relevant_includes_from_files(self): files=["src/analyze_includes/test/data/some_header.h", "src/analyze_includes/test/data/another_header.h"], ignored_includes=IgnoredIncludes(paths=["vector"], patterns=[]), defines=[], + include_paths=[], ) self.assertEqual(len(result), 4) diff --git a/src/aspect/dwyu.bzl b/src/aspect/dwyu.bzl index 4c19ea3f..32db0681 100644 --- a/src/aspect/dwyu.bzl +++ b/src/aspect/dwyu.bzl @@ -187,9 +187,11 @@ def dwyu_aspect_impl(target, ctx): if _do_ensure_private_deps(ctx): args.add("--implementation_deps_available") + all_hdrs = target[CcInfo].compilation_context.headers.to_list() + analysis_inputs = [ctx.file._config, processed_target] + processed_deps + processed_implementation_deps + private_files + all_hdrs ctx.actions.run( executable = ctx.executable._dwyu_binary, - inputs = [ctx.file._config, processed_target] + public_files + private_files + processed_deps + processed_implementation_deps, + inputs = analysis_inputs, outputs = [report_file], mnemonic = "CompareIncludesToDependencies", progress_message = "Analyze dependencies of {}".format(target.label), diff --git a/test/aspect/defines/BUILD b/test/aspect/defines/BUILD index 1bd96b77..23f09e99 100644 --- a/test/aspect/defines/BUILD +++ b/test/aspect/defines/BUILD @@ -1,7 +1,7 @@ cc_library( name = "in_file_defines", hdrs = ["in_file_defines.h"], - deps = ["//test/aspect/defines/lib:a"], + deps = ["//test/aspect/defines/support:lib_a"], ) cc_library( @@ -10,14 +10,23 @@ cc_library( copts = ["-DSOME_COPT 42"], defines = ["SOME_DEFINE"], local_defines = ["LOCAL_DEFINE"], - deps = ["//test/aspect/defines/lib:a"], + deps = ["//test/aspect/defines/support:lib_a"], ) cc_library( name = "transitive_defines_from_bazel_target", hdrs = ["transitive_defines_from_bazel_target.h"], deps = [ - "//test/aspect/defines/lib:a", + "//test/aspect/defines/support:lib_a", "//test/aspect/defines/support:transitive_define", ], ) + +cc_library( + name = "use_defines_from_dependency_header", + hdrs = ["use_defines_from_dependency_header.h"], + deps = [ + "//test/aspect/defines/support:conditional_defines", + "//test/aspect/defines/support:lib_b", + ], +) diff --git a/test/aspect/defines/README.md b/test/aspect/defines/README.md index 3f74709f..b375a26e 100644 --- a/test/aspect/defines/README.md +++ b/test/aspect/defines/README.md @@ -1,11 +1,2 @@ Defines can influence which include statements are relevant. - -These tests concentrate on parsing single files based on defines: - -- specified in the parsed file itself -- coming from the C/C++ toolchain -- defined by the Bazel target attributes `defines`, `local_defines` or `cops` - -Defines can also be imported into a file via an included header which specifies a define. -This use case is not yet supported. -We might add it at a later stage. +In these test we ensure our preprocessor behaves as expected and chooses the desired code parts for analysis. diff --git a/test/aspect/defines/defines_from_bazel_target.h b/test/aspect/defines/defines_from_bazel_target.h index a1094e66..6dff2916 100644 --- a/test/aspect/defines/defines_from_bazel_target.h +++ b/test/aspect/defines/defines_from_bazel_target.h @@ -1,17 +1,17 @@ #ifdef SOME_DEFINE -#include "test/aspect/defines/lib/a.h" +#include "test/aspect/defines/support/a.h" #else -#include "test/aspect/defines/lib/b.h" +#include "test/aspect/defines/support/b.h" #endif #ifdef LOCAL_DEFINE -#include "test/aspect/defines/lib/a.h" +#include "test/aspect/defines/support/a.h" #else -#include "test/aspect/defines/lib/b.h" +#include "test/aspect/defines/support/b.h" #endif #if SOME_COPT > 40 -#include "test/aspect/defines/lib/a.h" +#include "test/aspect/defines/support/a.h" #else -#include "test/aspect/defines/lib/b.h" +#include "test/aspect/defines/support/b.h" #endif diff --git a/test/aspect/defines/in_file_defines.h b/test/aspect/defines/in_file_defines.h index 78aecd24..eeb12281 100644 --- a/test/aspect/defines/in_file_defines.h +++ b/test/aspect/defines/in_file_defines.h @@ -1,19 +1,19 @@ #define USE_A #ifdef USE_A -#include "test/aspect/defines/lib/a.h" +#include "test/aspect/defines/support/a.h" #else -#include "test/aspect/defines/lib/b.h" +#include "test/aspect/defines/support/b.h" #endif #ifdef NON_EXISTING_DEFINE -#include "test/aspect/defines/lib/b.h" +#include "test/aspect/defines/support/b.h" #endif #define SOME_VALUE 42 #if SOME_VALUE > 40 -#include "test/aspect/defines/lib/a.h" +#include "test/aspect/defines/support/a.h" #else -#include "test/aspect/defines/lib/b.h" +#include "test/aspect/defines/support/b.h" #endif diff --git a/test/aspect/defines/lib/BUILD b/test/aspect/defines/lib/BUILD deleted file mode 100644 index 41506238..00000000 --- a/test/aspect/defines/lib/BUILD +++ /dev/null @@ -1,11 +0,0 @@ -cc_library( - name = "a", - hdrs = ["a.h"], - visibility = ["//test/aspect/defines:__pkg__"], -) - -cc_library( - name = "b", - hdrs = ["b.h"], - visibility = ["//test/aspect/defines:__pkg__"], -) diff --git a/test/aspect/defines/lib/a.h b/test/aspect/defines/lib/a.h deleted file mode 100644 index e69de29b..00000000 diff --git a/test/aspect/defines/support/BUILD b/test/aspect/defines/support/BUILD index 602bf3e3..9449eb5c 100644 --- a/test/aspect/defines/support/BUILD +++ b/test/aspect/defines/support/BUILD @@ -1,7 +1,34 @@ +package(default_visibility = ["//test/aspect/defines:__pkg__"]) + cc_library( name = "transitive_define", copts = ["-DLOCAL_COPT"], # should not influence other targets defines = ["TRANSITIVE_DEFINE"], local_defines = ["LOCAL_DEFINE"], # should not influence other targets - visibility = ["//test/aspect/defines:__pkg__"], +) + +cc_library( + name = "conditional_defines", + hdrs = ["conditional_defines.h"], + deps = [":some_defines"], +) + +cc_library( + name = "some_defines", + hdrs = ["some_defines.h"], +) + +cc_library( + name = "lib_a", + hdrs = ["a.h"], +) + +cc_library( + name = "lib_b", + hdrs = ["b.h"], +) + +cc_library( + name = "lib_c", + hdrs = ["c.h"], ) diff --git a/test/aspect/defines/support/a.h b/test/aspect/defines/support/a.h new file mode 100644 index 00000000..6b82c2b2 --- /dev/null +++ b/test/aspect/defines/support/a.h @@ -0,0 +1 @@ +// some content diff --git a/test/aspect/defines/lib/b.h b/test/aspect/defines/support/b.h similarity index 100% rename from test/aspect/defines/lib/b.h rename to test/aspect/defines/support/b.h diff --git a/test/aspect/defines/support/c.h b/test/aspect/defines/support/c.h new file mode 100644 index 00000000..6b82c2b2 --- /dev/null +++ b/test/aspect/defines/support/c.h @@ -0,0 +1 @@ +// some content diff --git a/test/aspect/defines/support/conditional_defines.h b/test/aspect/defines/support/conditional_defines.h new file mode 100644 index 00000000..d1f75b0c --- /dev/null +++ b/test/aspect/defines/support/conditional_defines.h @@ -0,0 +1,13 @@ +#include "test/aspect/defines/support/some_defines.h" + +// Set defines based on values from included header + +#ifdef SWITCH_USE_B +#define USE_B +#endif + +#if SOME_SWITCH_VALUE > 100 +#define SOME_VALUE 42 +#else +#define SOME_VALUE 0 +#endif diff --git a/test/aspect/defines/support/some_defines.h b/test/aspect/defines/support/some_defines.h new file mode 100644 index 00000000..e97fb7e4 --- /dev/null +++ b/test/aspect/defines/support/some_defines.h @@ -0,0 +1,3 @@ +// Those are used in examples including this header +#define SWITCH_USE_B +#define SOME_SWITCH_VALUE 1337 diff --git a/test/aspect/defines/transitive_defines_from_bazel_target.h b/test/aspect/defines/transitive_defines_from_bazel_target.h index 7a9467f7..67cea2ae 100644 --- a/test/aspect/defines/transitive_defines_from_bazel_target.h +++ b/test/aspect/defines/transitive_defines_from_bazel_target.h @@ -1,18 +1,21 @@ // Define introduced through target on which we depend. The target uses the attribute 'defines' which is propagated to // all users of the target. #ifdef TRANSITIVE_DEFINE -#include "test/aspect/defines/lib/a.h" +#include "test/aspect/defines/support/a.h" #else -#include "test/aspect/defines/lib/b.h" +#include "test/aspect/defines/support/b.h" #endif +#include "test/aspect/defines/support/a.h" +#include + // The following defines shall never be active as they are set though Bazel target attributes 'copts' and // 'local_defines' which should not leak to users of the target #ifdef LOCAL_DEFINE -#include "test/aspect/defines/lib/b.h" +#include "test/aspect/defines/support/b.h" #endif #ifdef LOCAL_COPT -#include "test/aspect/defines/lib/b.h" +#include "test/aspect/defines/support/b.h" #endif diff --git a/test/aspect/defines/use_defines_from_dependency_header.h b/test/aspect/defines/use_defines_from_dependency_header.h new file mode 100644 index 00000000..9234d716 --- /dev/null +++ b/test/aspect/defines/use_defines_from_dependency_header.h @@ -0,0 +1,16 @@ +#include "test/aspect/defines/support/conditional_defines.h" + +// Analyze include statements based on cod parts active due to defines from included header + +#ifdef USE_B +#include "test/aspect/defines/support/b.h" +#else +#include "test/aspect/defines/support/c.h" +#endif + +// Defined in included header +#if SOME_VALUE > 40 +#include "test/aspect/defines/support/b.h" +#else +#include "test/aspect/defines/support/c.h" +#endif diff --git a/test/aspect/execute_tests.py b/test/aspect/execute_tests.py index 539f601e..2fc86270 100755 --- a/test/aspect/execute_tests.py +++ b/test/aspect/execute_tests.py @@ -265,25 +265,9 @@ expected=ExpectedResult(success=True), ), TestCase( - name="in_file_defines", + name="processing_defines", cmd=TestCmd( - target="//test/aspect/defines:in_file_defines", - aspect=DEFAULT_ASPECT, - ), - expected=ExpectedResult(success=True), - ), - TestCase( - name="defines_from_bazel_target", - cmd=TestCmd( - target="//test/aspect/defines:defines_from_bazel_target", - aspect=DEFAULT_ASPECT, - ), - expected=ExpectedResult(success=True), - ), - TestCase( - name="transitive_defines_from_bazel_target", - cmd=TestCmd( - target="//test/aspect/defines:transitive_defines_from_bazel_target", + target="//test/aspect/defines:all", aspect=DEFAULT_ASPECT, ), expected=ExpectedResult(success=True),