[PATCH] D20547: [safestack] Sink unsafe address computation to each use.

[Evgeniy Stepanov via llvm-commits]

eugenis added inline comments.

================
Comment at: lib/CodeGen/SafeStack.cpp:614
@@ +613,3 @@
+    std::string Name = std::string(AI->getName()) + ".unsafe";
+    AI->setName("");
+    Value *Replacement = nullptr;
----------------
pcc wrote:
> eugenis wrote:
> > pcc wrote:
> > > Why bother setting the name if we're about to remove the instruction?
> > Because otherwise CreateBitCast(..., Name) would de-duplicate the name and the new instruction would end up with something like Name.unsafe.12345.
> Aren't you achieving that by appending the ".unsafe" suffix?
Right. Will remove.

================
Comment at: lib/CodeGen/SafeStack.cpp:637
@@ +636,3 @@
+    // isUsedByMetadata().
+    if ((AI->hasValueHandle() || AI->isUsedByMetadata()) && Replacement) {
+      AI->replaceAllUsesWith(Replacement);
----------------
pcc wrote:
> eugenis wrote:
> > pcc wrote:
> > > This seems a little weird to me. I wouldn't expect anything to be holding a value handle here (and if it did, it probably wouldn't want the replacement for whichever instruction happened to be at the end of the use list) and the only important metadata is debug info, which is handled by `replaceDbgDeclareForAlloca`, I believe. Can we just remove this part?
> > The new debug metadata generated in replaceDbgDeclareForAlloca is still tied to the alloca that is being replaced. Without this line, it gets replaced with an empty metadata.
> > 
> > The condition (AI->hasValueHandle() || AI->isUsedByMetadata()) is taken from Value::replaceAllUsesWith, and it describes all cases where that function would do any work that is not yet done at this point (i.e. replacing the non-IR uses).
> > 
> > The new debug metadata generated in replaceDbgDeclareForAlloca is still tied to the alloca that is being replaced. Without this line, it gets replaced with an empty metadata.
> 
> That's surprising.
> http://llvm-cs.pcc.me.uk/lib/Transforms/Utils/Local.cpp#1223
> http://llvm-cs.pcc.me.uk/lib/Transforms/Utils/Local.cpp#1185
> It certainly looks like the intent is to replace the address. Is there some case where we aren't replacing it correctly?
> 
> > The condition (AI->hasValueHandle() || AI->isUsedByMetadata()) is taken from Value::replaceAllUsesWith, and it describes all cases where that function would do any work that is not yet done at this point (i.e. replacing the non-IR uses).
> 
> We shouldn't cargo cult that logic from Value::replaceAllUsesWith, as it only makes sense when the replacement is uniform.
That case is llvm.dbg.value.

replaceDbgDeclareForAlloca does not touch dbg.value. They are updated by RAUW, but that does not give them the same "good" debug location as replaceDbgDeclareForAlloca does for dbg.declare.

I'll fix it in replaceDbgDeclareForAlloca


Repository:
  rL LLVM

http://reviews.llvm.org/D20547