[PATCH] D143675: Discussion: Darwin Sanitizers Stable ABI

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:14
+extern "C" {
+// Globals
+void __asan_register_image_globals(uptr *flag) {
----------------
Comments are usually a complete sentence with a period. There are exceptions, but a "Globals" needs elaboration to make it better understandable by a reader.


================
Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:16
+void __asan_register_image_globals(uptr *flag) {
+    __asan_abi_register_image_globals();
+}
----------------
2-space indentation, here and throughout.


================
Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:41
+
+void __asan_after_dynamic_init(void) {
+    __asan_abi_after_dynamic_init();
----------------
C++ prefers `()` instead of `(void)`.


================
Comment at: compiler-rt/lib/asan_abi/asan_abi_shim.cpp:486
+    // TBD: Fail here
+    return (void *) 0;
+}
----------------
You may apply `clang-format`, which may turn this into `(void *)0`, but `nullptr` likely works better.


================
Comment at: compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:1
+// RUN: %clang_asan_abi  -O2 -c -fsanitize-stable-abi -fsanitize=address -O0 %s -o %t.o
+// RUN: %clangxx -c %p/../../../../lib/asan_abi/asan_abi.cpp -o asan_abi.o
----------------
excess spaces

`-O2 ... -O0`?


================
Comment at: compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:4
+// RUN: %clangxx -dead_strip -o %t %t.o %libasan_abi asan_abi.o && %run %t 2>&1
+
+
----------------
One blank line suffices.


================
Comment at: compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:9
+
+// RUN: nm -g %libasan_abi \
+// RUN:  | grep " [TU] "   \
----------------
We generally prefer llvm counterparts to the system binary utilities. Use `llvm-nm`


================
Comment at: compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:15
+// RUN:  > %t.exports
+//
+// RUN:  sed -e ':a' -e 'N' -e '$!ba'                                             \
----------------
unneeded `^//$` lines, here and throughout.


================
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'                                                          \
----------------
Does Darwin sed accept `;` to combine multiple `-e` into one `-e` with `;`?


================
Comment at: compiler-rt/test/asan_abi/TestCases/Darwin/llvm_interface_symbols.cpp:21
+// RUN:  %t.asan_interface.inc                                                    \
+// RUN:  | grep  -v -f %p/../../../../lib/asan_abi/darwin_exclude_symbols.inc     \
+// RUN:  | grep -e "INTERFACE_\(WEAK_\)\?FUNCTION"                                \
----------------
2-space indentation for `|` continuation lines as well


================
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
----------------
`sort %t.imports`. See `Useless use of cat`


================
Comment at: compiler-rt/test/asan_abi/TestCases/linkstaticlibrary.cpp:1
+// RUN: %clang_asan_abi  -O2 -c -fsanitize-stable-abi -fsanitize=address -O0 %s -o %t.o
+// RUN: %clangxx -c %p/../../../lib/asan_abi/asan_abi.cpp -o asan_abi.o
----------------
`-O2 ... -O0`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143675