[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.

mostly lgtm, but one major suggestion



================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:663
+
+  void initialize(Function &Fn) {
+    assert(!isInitialized() && !TrackInsertedCalls);
----------------
these `initialize`/`reset` aren't great in terms of keeping C++ lifetimes straight, I think we should go with a more RAII approach where we construct a `RuntimeCallInserter` per function we instrument and pass that around. this will make sure we automatically handle all the corner cases like the `LooksLikeCodeInBug11395` codepath, and also make sure we never reuse `RuntimeCallInserter` for multiple functions. then we can get rid of all the manual calls to `initialize`/`finalize`.


================
Comment at: llvm/test/Instrumentation/AddressSanitizer/asan-funclet.ll:3
+
+; RUN: opt -S -passes=asan -asan-instrumentation-with-call-threshold=0 < %s | FileCheck %s
+
----------------
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


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

https://reviews.llvm.org/D143108