[PATCH] D144806: [BOLT] Fix intermittent crash with instrumentation

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb created this revision.
maksfb added reviewers: yota9, Amir, ayermolo, rafauler, yavtuk, treapster.
Herald added a subscriber: pengfei.
Herald added a project: All.
maksfb requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

When createInstrumentedIndirectCall() was invoked for tail calls, we
attached annotation instruction twice to the new call instruction.
First in createDirectCall(), and then again while copying over the
metadata operands.

As a result, the annotations were not properly stripped for such calls
before the call to freeAnnotations() in LowerAnnotations pass. That lead
to use-after-free while restoring the offsets with setOffset() call.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D144806

Files:
  bolt/include/bolt/Core/MCPlusBuilder.h
  bolt/lib/Core/MCPlusBuilder.cpp
  bolt/lib/Passes/Instrumentation.cpp
  bolt/lib/Target/X86/X86MCPlusBuilder.cpp


Index: bolt/lib/Target/X86/X86MCPlusBuilder.cpp
===================================================================
--- bolt/lib/Target/X86/X86MCPlusBuilder.cpp
+++ bolt/lib/Target/X86/X86MCPlusBuilder.cpp
@@ -3085,8 +3085,7 @@
     Inst.addOperand(MCOperand::createReg(X86::NoRegister)); // AddrSegmentReg
   }
 
-  InstructionListType createInstrumentedIndirectCall(const MCInst &CallInst,
-                                                     bool TailCall,
+  InstructionListType createInstrumentedIndirectCall(MCInst &&CallInst,
                                                      MCSymbol *HandlerFuncAddr,
                                                      int CallSiteID,
                                                      MCContext *Ctx) override {
@@ -3137,14 +3136,13 @@
     createLoadImmediate(Insts.back(), TempReg, CallSiteID);
     Insts.emplace_back();
     createPushRegister(Insts.back(), TempReg, 8);
-    Insts.emplace_back();
-    createDirectCall(Insts.back(), HandlerFuncAddr, Ctx,
-                     /*TailCall=*/TailCall);
-    // Carry over metadata
-    for (int I = MCPlus::getNumPrimeOperands(CallInst),
-             E = CallInst.getNumOperands();
-         I != E; ++I)
-      Insts.back().addOperand(CallInst.getOperand(I));
+
+    MCInst &NewCallInst = Insts.emplace_back();
+    createDirectCall(NewCallInst, HandlerFuncAddr, Ctx, isTailCall(CallInst));
+
+    // Carry over metadata including tail call marker if present.
+    stripAnnotations(NewCallInst);
+    moveAnnotations(std::move(CallInst), NewCallInst);
 
     return Insts;
   }
Index: bolt/lib/Passes/Instrumentation.cpp
===================================================================
--- bolt/lib/Passes/Instrumentation.cpp
+++ bolt/lib/Passes/Instrumentation.cpp
@@ -215,7 +215,7 @@
   BinaryContext &BC = FromFunction.getBinaryContext();
   bool IsTailCall = BC.MIB->isTailCall(*Iter);
   InstructionListType CounterInstrs = BC.MIB->createInstrumentedIndirectCall(
-      *Iter, IsTailCall,
+      std::move(*Iter),
       IsTailCall ? IndTailCallHandlerExitBBFunction->getSymbol()
                  : IndCallHandlerExitBBFunction->getSymbol(),
       IndCallSiteID, &*BC.Ctx);
Index: bolt/lib/Core/MCPlusBuilder.cpp
===================================================================
--- bolt/lib/Core/MCPlusBuilder.cpp
+++ bolt/lib/Core/MCPlusBuilder.cpp
@@ -299,6 +299,9 @@
   auto IsTC = hasAnnotation(Inst, MCAnnotation::kTailCall);
 
   Inst.erase(std::prev(Inst.end()));
+  assert(!getAnnotationInst(Inst) &&
+         "More than one annotation instruction detected.");
+
   if (KeepTC && IsTC)
     setTailCall(Inst);
 }
Index: bolt/include/bolt/Core/MCPlusBuilder.h
===================================================================
--- bolt/include/bolt/Core/MCPlusBuilder.h
+++ bolt/include/bolt/Core/MCPlusBuilder.h
@@ -163,6 +163,18 @@
   /// MCPlusBuilder classes must use convert/lower/create* interfaces instead.
   void setTailCall(MCInst &Inst);
 
+  /// Transfer annotations from \p SrcInst to \p DstInst.
+  void moveAnnotations(MCInst &&SrcInst, MCInst &DstInst) const {
+    assert(!getAnnotationInst(DstInst) &&
+           "Destination instruction should not have annotations.");
+    const MCInst *AnnotationInst = getAnnotationInst(SrcInst);
+    if (!AnnotationInst)
+      return;
+
+    DstInst.addOperand(MCOperand::createInst(AnnotationInst));
+    SrcInst.erase(std::prev(SrcInst.end()));
+  }
+
 public:
   class InstructionIterator {
   public:
@@ -1864,9 +1876,8 @@
   void stripAnnotations(MCInst &Inst, bool KeepTC = false);
 
   virtual InstructionListType
-  createInstrumentedIndirectCall(const MCInst &CallInst, bool TailCall,
-                                 MCSymbol *HandlerFuncAddr, int CallSiteID,
-                                 MCContext *Ctx) {
+  createInstrumentedIndirectCall(MCInst &&CallInst, MCSymbol *HandlerFuncAddr,
+                                 int CallSiteID, MCContext *Ctx) {
     llvm_unreachable("not implemented");
     return InstructionListType();
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D144806.500495.patch
Type: text/x-patch
Size: 4044 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230226/4a2562a0/attachment.bin>