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

[Sylvain Audi via Phabricator via llvm-commits]

saudi added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:706
+        continue;
+     assert(Colors.size() == 1 && "Expected monochromatic BB!");
+
----------------
rnk wrote:
> saudi wrote:
> > 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?
> I think it is possible to construct cases where valid transforms will lead to multi-color blocks. Consider patches like https://reviews.llvm.org/D29428
> 
> I would actually strengthen the assert here to report an error via the LLVMContext. That seems to be the behavior that folks want.
If I understand correctly:
 - `WinEHPrepare` pass is run later, and ensures the blocks are monochromatic
 - In `AddressSanitizer` pass, this constraint is not guaranteed, we could be trying to introduce a runtime call inside a multi-color block
And you are suggesting detecting and reporting that case as a remark.

Could another approach be to tag the new calls somehow, in a way that would let `WinHEPrepare` know that instead of discarding it, it should just add the missing OpBundle? 

And if so, could you point me in the right direction? (I was thinking of adding a new opbundle "autofunclet", but there might be much better ways?)


================
Comment at: llvm/test/Instrumentation/AddressSanitizer/asan-funclet.ll:3
+
+; RUN: opt -S -passes=asan -asan-instrumentation-with-call-threshold=0 < %s | FileCheck %s
+
----------------
vitalybuka wrote:
> aeubanks wrote:
> > saudi wrote:
> > > 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?
> > > 
> > > 
> > typically yeah we still recommend people to use update_test_checks.py since updating them is easy if other patches change the tests, and manual checks are error-prone and can often become obsolete. but most asan tests aren't using it so this is fine
> Can you please precommit the  test generated with update_test_checks without your patch
> so in this patch we can see a difference in generated code.
> Assuming compiler does not crash without the patch.
Just to be sure, are you suggesting that I do the following?
 1) prepare the file `asan-funclet.ll` before the fix is applied, apply `update_test_checks.py` and do a NFC commit with this single file `asan-funclet.ll`
 2) apply my code change, and update asan-funclet.ll using `update_test_checks.py` 
   -> update this patch,  so that we can see more easily what happens?

Note that the test file will become much larger: applying `update_test_checks.py` generates 500+ lines of checks. It's why I went with the more manual checks, but I can surely do that.


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

https://reviews.llvm.org/D143108