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

[Maksim Panchenko via Phabricator via llvm-commits]

maksfb added inline comments.


================
Comment at: bolt/include/bolt/Core/MCPlusBuilder.h:174
+
+    DstInst.addOperand(MCOperand::createInst(AnnotationInst));
+    SrcInst.erase(std::prev(SrcInst.end()));
----------------
yota9 wrote:
> Maybe add assert(!getAnnotationInst(DstInst) just for extra safety
We have it at line 168.


================
Comment at: bolt/include/bolt/Core/MCPlusBuilder.h:175
+    DstInst.addOperand(MCOperand::createInst(AnnotationInst));
+    SrcInst.erase(std::prev(SrcInst.end()));
+  }
----------------
yota9 wrote:
> How about to make separate function smth like "getAnnotationOpIndex" and use it also in getAnnotationInst and stripAnnotations? Just to ensure everything is in sync 
Operand handling should be happening in MCPlusBuilder annotation interface (after this change). I can add `removeAnnotationInst()`, but it still will be a part of the internal interface. The direct handling of operands in `createInstrumentedIndirectCall()` was really unfortunate. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144806