[PATCH] D143108: [Asan] Add "funclet" OpBundle to Asan calls that are generated inside a funclet

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

rnk explained the background for this to me but I'm still somewhat unfamiliar with the details so you may need to explain some things to me

it'd be nice to add an end-to-end test in compiler-rt for whatever case you were seeing

the test coverage doesn't seem sufficient, can you add some more tests?



================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:649
+  Function *OwnerFn = nullptr;
+  std::optional<std::vector<CallInst *>> InsertedCalls;
+
----------------
I'd separate out a bool instead of using std::optional, that's a bit clearer


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:702
+      ColorVector &Colors = BlockColors[BB];
+       // A BB could be colorless when it is unreachable, in which case it will
+      // be removed by a later pass anyway.
----------------
should run clang-format before uploading


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:706
+        continue;
+     assert(Colors.size() == 1 && "Expected monochromatic BB!");
+
----------------
I'm unfamiliar with this code, why is this true?


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2740-2743
+  if (AsanCtorFunction)
+    AsanCtorCallInserter.finalize();
+  if (AsanDtorFunction)
+    AsanDtorCallInserter.finalize();
----------------
is EH relevant to the ctor/dtor functions?


================
Comment at: llvm/test/Instrumentation/AddressSanitizer/asan-funclet.ll:3
+
+; RUN: opt -S -passes=asan -asan-instrumentation-with-call-threshold=0 < %s | FileCheck %s
+
----------------
please use `llvm/utils/update_test_checks.py` instead of manual CHECK lines


================
Comment at: llvm/test/Instrumentation/AddressSanitizer/asan-funclet.ll:6
+
+target triple = "x86_64-pc-windows-msvc"
+
----------------
test needs `REQUIRES: x86-registered-target`


================
Comment at: llvm/test/Instrumentation/AddressSanitizer/asan-funclet.ll:21
+
+define dso_local void @FuncletPersonality() sanitize_address personality ptr @__CxxFrameHandler3 {
+entry:
----------------
please remove irrelevant things in the IR like `dso_local`/`noundef`/etc and also make the function names a little nicer


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

https://reviews.llvm.org/D143108