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

[Sylvain Audi via Phabricator via llvm-commits]

saudi marked 3 inline comments as done.
saudi added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:706
+        continue;
+     assert(Colors.size() == 1 && "Expected monochromatic BB!");
+
----------------
aeubanks wrote:
> I'm unfamiliar with this code, why is this true?
This is a constraint on the funclets in the Windows EH model.

An illustration in code is in WinEHPrepare pass `verifyPreparedFunclets` method here : https://github.com/llvm/llvm-project/blob/llvmorg-18-init/llvm/lib/CodeGen/WinEHPrepare.cpp#L1167
 (from which I copied the assert)

In the documentation I found this: https://llvm.org/docs/ExceptionHandling.html#funclet-parent-tokens
It explains that the funclet structure must represent a tree.

I'm not an expert in this, but my understanding is that in WinEH model, the funclets need to be structured as a tree; the coloring allows to identify which "node" each EH-related BasicBlock belongs to, having the constraint that the BB must belong to one and only one node.

@rnk can you confirm?


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:2740-2743
+  if (AsanCtorFunction)
+    AsanCtorCallInserter.finalize();
+  if (AsanDtorFunction)
+    AsanDtorCallInserter.finalize();
----------------
aeubanks wrote:
> is EH relevant to the ctor/dtor functions?
My approach was to play it safe by using the `RuntimeCallInserter` for any runtime calls to `__asan_*()` introduced by the AddressSanitizer pass; the `RuntimeCallInserter` being responsible for adding funclet bundles as a postprocess.

I agree it's confusing. I updated the code where I removed the modifications for unneeded cases, where the `__asan_xxx` calls can't be in a EH pad, like this example of the module ctor/dtor


================
Comment at: llvm/test/Instrumentation/AddressSanitizer/asan-funclet.ll:3
+
+; RUN: opt -S -passes=asan -asan-instrumentation-with-call-threshold=0 < %s | FileCheck %s
+
----------------
aeubanks wrote:
> please use `llvm/utils/update_test_checks.py` instead of manual CHECK lines
The pass introduces a lot of instructions and a few basic blocks. The output of update_test_checks.py gets large and too specific for testing this particular feature. It would easily break when asan gets modified in the future.

This is my first time using that script. Is the usual approach to apply `update_test_checks.py` and only keep the `CHECK` lines necessary with potential minor modifications?




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

https://reviews.llvm.org/D143108