[PATCH] D14972: [safestack] Protect byval function arguments.

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4466-4471
@@ -4465,17 +4465,8 @@
       bool isParameter = Variable->isParameter() || isa<Argument>(Address);
-
-      const AllocaInst *AI = dyn_cast<AllocaInst>(Address);
-
-      if (isParameter && !AI) {
-        FrameIndexSDNode *FINode = dyn_cast<FrameIndexSDNode>(N.getNode());
-        if (FINode)
-          // Byval parameter.  We have a frame index at this point.
-          SDV = DAG.getFrameIndexDbgValue(
-              Variable, Expression, FINode->getIndex(), 0, dl, SDNodeOrder);
-        else {
-          // Address is an argument, so try to emit its dbg value using
-          // virtual register info from the FuncInfo.ValueMap.
-          EmitFuncArgumentDbgValue(Address, Variable, Expression, dl, 0, false,
-                                   N);
-          return nullptr;
-        }
+      auto FINode = dyn_cast<FrameIndexSDNode>(N.getNode());
+      if (isParameter && FINode) {
+        // Byval parameter. We have a frame index at this point.
+        SDV = DAG.getFrameIndexDbgValue(Variable, Expression,
+                                        FINode->getIndex(), 0, dl, SDNodeOrder);
+      } else if (isa<Argument>(Address)) {
+        // Address is an argument, so try to emit its dbg value using
----------------
Can you please split this out into a separate change with its own test case?

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:60
@@ -59,2 +59,3 @@
 STATISTIC(NumUnsafeDynamicAllocas, "Number of unsafe dynamic allocas");
+STATISTIC(NumUnsafeByValArgumengs, "Number of unsafe byval arguments");
 STATISTIC(NumUnsafeStackRestorePoints, "Number of setjmps and landingpads");
----------------
*Arguments

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:228-233
@@ +227,8 @@
+  uint64_t AllocaSize;
+  if (const auto *AI = dyn_cast<AllocaInst>(AllocaPtr)) {
+    AllocaSize = DL->getTypeStoreSize(AI->getAllocatedType());
+  } else {
+    const auto *Arg = dyn_cast<Argument>(AllocaPtr);
+    AllocaSize = DL->getTypeStoreSize(Arg->getType()->getPointerElementType());
+  }
+
----------------
Seems simpler to have the caller pass in the size (or just the type) instead of computing it here.

================
Comment at: lib/Transforms/Instrumentation/SafeStack.cpp:492
@@ -462,1 +491,3 @@
   int64_t StaticOffset = 0; // Current stack top.
+  IRB.SetInsertPoint(cast<Instruction>(BasePointer->getNextNode()));
+
----------------
Is it necessary to cast the result of `getNextNode` (here and elsewhere)? Looks like it already returns an instruction (see e.g. line 516).

================
Comment at: test/Transforms/SafeStack/debug-loc.ll:8-12
@@ +7,7 @@
+; dbg.declare for %zzz and %xxx are gone; replaced with dbg.declare based off the unsafe stack pointer
+; CHECK-NOT: call void @llvm.dbg.declare
+; CHECK: call void @llvm.dbg.declare(metadata i8* %[[USP]], metadata ![[VAR_ARG:.*]], metadata ![[EXPR_ARG:.*]])
+; CHECK-NOT: call void @llvm.dbg.declare
+; CHECK: call void @llvm.dbg.declare(metadata i8* %[[USP]], metadata ![[VAR_LOCAL:.*]], metadata ![[EXPR_LOCAL:.*]])
+; CHECK-NOT: call void @llvm.dbg.declare
+; dbg.declare appears before the first use
----------------
This should all live next to the code so that it is clear what corresponds to what.

================
Comment at: test/Transforms/SafeStack/debug-loc.ll:18
@@ +17,3 @@
+; CHECK-DAG: ![[VAR_ARG]] = !DILocalVariable(name: "zzz"
+; CHECK-DAG: ![[EXPR_ARG]] = !DIExpression(DW_OP_deref, DW_OP_minus,
+
----------------
Should this be checking for specific constants? (same below)


Repository:
  rL LLVM

http://reviews.llvm.org/D14972