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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
