[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Roy Sundahl via Phabricator via cfe-commits]

rsundahl marked 22 inline comments as done.
rsundahl added a comment.

Thank you for your review and thoughtful input @eugenis, @MaskRay and @vitalybuka. I think we're close to having everything incorporated. (@MaskRay, the doc files went from .md to .rst and I implemented all of your suggestions there.



================
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>,
----------------
MaskRay wrote:
> 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.
(Not sure if this is exactly what you meant @MaskRay but I think it's close.)


================
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");
----------------
MaskRay wrote:
> 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.
I couldn't get this one to work. Did I do it wrong? (Couldn't find example in the code to go from.)

Tried:
```
  if (StableABI) {
    CmdArgs.push_back("-mllvm=-asan-instrumentation-with-call-threshold=0");
    CmdArgs.push_back("-mllvm=-asan-max-inline-poisoning-size=0");
  }
```
Got:
```
error: unknown argument: '-mllvm=-asan-instrumentation-with-call-threshold=0'
error: unknown argument: '-mllvm=-asan-max-inline-poisoning-size=0'
```


================
Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:41
+
+void __asan_after_dynamic_init(void) {
+    __asan_abi_after_dynamic_init();
----------------
MaskRay wrote:
> C++ prefers `()` instead of `(void)`.
These are actually "C'
Added:
```
extern "C" {
...
}
```


================
Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:486
+    // TBD: Fail here
+    return (void *) 0;
+}
----------------
MaskRay wrote:
> You may apply `clang-format`, which may turn this into `(void *)0`, but `nullptr` likely works better.
clang-format left this as-is. I suspect this is because I also added the extern "C" brackets.


================
Comment at: compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:16
+//
+// RUN:  sed -e ':a' -e 'N' -e '$!ba'                                             \
+// RUN:      -e 's/ //g'                                                          \
----------------
MaskRay wrote:
> Does Darwin sed accept `;` to combine multiple `-e` into one `-e` with `;`?
I couldn't get the same behavior out of the intersection of GNU an BSD set. Tried hard in https://reviews.llvm.org/D138824 and landed with the -e's. iirc exactly what the problem was with semicolons, just that I was relieved when I found a format that worked for all the platforms.


================
Comment at: compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:26
+//
+// RUN: cat %t.imports | sort | uniq > %t.imports-sorted
+// RUN: cat %t.exports | sort | uniq > %t.exports-sorted
----------------
MaskRay wrote:
> `sort %t.imports`. See `Useless use of cat`
Good point!


================
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.
----------------
MaskRay wrote:
> This workaround is unneeded. I sent D150410 to clean up other `lit.cfg.py` files.
Wasn't actually used anyway but good to know!


================
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
----------------
thetruestblue wrote:
> MaskRay wrote:
> > `!=`
> The thought here was to leave basic lit patterns in-tact to expand to other OSs if others want to in the future. But if there's no desire for that, it doesn't make a big difference to me. 
I landed on just an else clause. Let me know if that's ok @thetruestblue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143675/new/

https://reviews.llvm.org/D143675