[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Fangrui Song via Phabricator via cfe-commits]

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