[PATCH] D125038: [Instrumentation] Share InstrumentationIRBuilder between TSan and SanCov

[Marco Elver via Phabricator via llvm-commits]

melver added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Instrumentation.h:202-211
+  explicit InstrumentationIRBuilder(BasicBlock *TheBB, Args &&...args)
+      : IRBuilder<>(TheBB, std::forward<Args>(args)...) {
+    ensureDebugInfo(*this, *TheBB->getParent());
+  }
+
+  template <typename... Args>
+  explicit InstrumentationIRBuilder(Instruction *IP, Args &&...args)
----------------
nickdesaulniers wrote:
> melver wrote:
> > nickdesaulniers wrote:
> > > Do we construct any IRBuilders with more than 1 arg?
> > > 
> > > Do we still need `explicit` if there's more than 1 arg?
> > Yeah, I wanted to try this on AddressSanitizer.cpp, where there are some. It's not used right now, but for completeness I felt I should add it. If you think we should only add the support when it's actually used, I can remove.
> > 
> > WDYT?
> > 
> > Yes, the explicit should stay for vararg constructors where there can only be 1 arg.
> Unless you're actively planning to do that port for asan soon (why not do it in this patch, too), I feel like YAGNI, but I also don't care enough either way, hence the approval.  I bring it up only because I was wondering what was being used from <utility> (which is `std::forward`).
Fair point.

I tried ASan just now, but requires a bit more care. I will try to get to it eventually. (FYI, in the kernel we have LTO depends on !KASAN, which is why nobody noticed - but if I remove that restriction, things break as expected.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125038