MaskRay added a comment. In D143675#4336365 <https://reviews.llvm.org/D143675#4336365>, @rsundahl wrote:
> In D143675#4310903 <https://reviews.llvm.org/D143675#4310903>, @eugenis wrote: > >> I'm fine with it in general. Is asan_abi.cpp meant as a temporary stub? It's >> not even link anywhere in the current version. > > We now use it during testing to close the loop on the question of whether the > file is "complete" in the sense that it satisfies the minimal "no-op" > implementation of the abi. We also moved from having a hand-curated include > file to using the actual interface file which should be the root truth for > what needs to be in there. We discovered a few additional functions that were > in asan_interface.h but aren't strictly part of the interface between the > instrumentation and the runtime but rather are used intra-runtime. Some other > routines living in asan_interface.h are really documented "helper" functions. > Maybe these should be aggregated somewhere else and/or under a different > namespace. For now we ignore those entrypoints by listing them in > compiler-rt/lib/asan_abi/darwin_exclude_symbols.inc How is `compiler-rt/lib/asan_abi/darwin_exclude_symbols.inc` used in the upstream and downstream build system? In this patch this file is only used by one test? ================ Comment at: clang/include/clang/Driver/Options.td:1785 HelpText<"Use default code inlining logic for the address sanitizer">; +def fsanitize_address_stable_abi : Flag<["-"], "fsanitize-address-stable-abi">, + Group<f_clang_Group>, ---------------- rsundahl wrote: > vitalybuka wrote: > > how likely you will need thus for other sanitizers in future > > should this be rather -fsanitize-stable-abi which is ignore for now for > > other sanitizers? > Thanks @vitalybuka. I agree and made this change. For now I still only > consume the flag if sanitizer=address so that we continue to get the unused > option warning in the other cases and left the test for that warning intact. See `BooleanFFlag`. Some existing sanitizer options are not written with the best practice. If `-fno-sanitize-stable-abi` is the default, there is usually no need to have a duplicate help message `Disable ...`. Documenting the non-default boolean option suffices. ================ Comment at: clang/lib/Driver/SanitizerArgs.cpp:918 + options::OPT_fno_sanitize_stable_abi, + StableABI); + ---------------- Existing code unnecessarily reads the previous value (false) of the variable. No need to copy that for new code. ================ Comment at: clang/lib/Driver/SanitizerArgs.cpp:1292 + CmdArgs.push_back("-mllvm"); + CmdArgs.push_back("-asan-instrumentation-with-call-threshold=0"); + CmdArgs.push_back("-mllvm"); ---------------- Optional nit: I added `-mllvm=` as an alias in `D143325`. You can use `-mllvm=-asan-instrumentation-with-call-threshold=0` to decrease the number/length of cc1 options. Add some comments before `if (StableABI) {` why the two `cl::opt` options are specified. ================ Comment at: clang/test/Driver/fsanitize.c:266 +// RUN: %clang --target=x86_64-linux-gnu -fsanitize-stable-abi %s -### 2>&1 | \ +// RUN: FileCheck %s --check-prefix=CHECK-ASAN-STABLE-WARN ---------------- I think the tests should go to a new file `fsanitize-stable-abi.c`. The checks are different enough from the rest of `fsanitize.c` (which can be split). ================ Comment at: clang/test/Driver/fsanitize.c:269 +// CHECK-ASAN-STABLE-WARN: warning: argument unused during compilation: '-fsanitize-stable-abi' +// RUN: %clang --target=x86_64-linux-gnu -fsanitize=address -fsanitize-stable-abi %s -### 2>&1 | \ +// RUN: FileCheck %s --check-prefix=CHECK-ASAN-STABLE-OK ---------------- I presume that you want to test the positive forms with Darwin triples like `arm64-apple-darwin`? We can even argue that the option should lead to an error for non-Darwin triples. ================ Comment at: compiler-rt/docs/asan_abi.md:1 +# Darwin Sanitizers Stable ABI + ---------------- The existing compiler-rt/docs docs use `.rst`. Better to use `.rst` to not introduce more than one format for one subproject. `.rst` is used much more than `.md` in llvm-project anyway. ================ Comment at: compiler-rt/lib/asan_abi/asan_abi.h:13 +#include <stdbool.h> +#include <sys/types.h> +#include <stddef.h> ---------------- use clang-format to sort the headers. I'd expect that stdbool and stddef are placed together for any sorting behavior. ================ Comment at: compiler-rt/lib/asan_abi/asan_abi.h:15 +#include <stddef.h> +extern "C" { +void __asan_abi_register_image_globals(void); ---------------- add a blank line before `extern "C" {` ================ Comment at: compiler-rt/test/asan_abi/lit.cfg.py:9 + +# Get shlex.quote if available (added in 3.3), and fall back to pipes.quote if +# it's not available. ---------------- This workaround is unneeded. I sent D150410 to clean up other `lit.cfg.py` files. ================ Comment at: compiler-rt/test/asan_abi/lit.cfg.py:20 + attr_value = getattr(config, attr_name, None) + if attr_value == None: + lit_config.fatal( ---------------- `is None` is better. ================ Comment at: compiler-rt/test/asan_abi/lit.cfg.py:83 +# Only run the tests on supported OSs. +if config.host_os not in ['Darwin']: + config.unsupported = True ---------------- `!=` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143675/new/ https://reviews.llvm.org/D143675 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
