[safestack] Support safestack in stack size diagnostics

[paulkirth]

Summary

Current stack size diagnostics ignore the size of the unsafe stack.
This patch attaches the size of the static portion of the unsafe stack
to the function as metadata, which can be used by the backend to emit
diagnostics regarding stack usage.

[paulkirth]

Fix ARM test breakage

[paulkirth]

Actually fix the ARM test

[mcgrathr]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1205–1206

Should we also update emitStackSizeSection to match?
I'm not sure whether that's meant to be used for diagnostic-like cases or for more concrete backend uses where the distinction between the two stacks still matters.

[paulkirth]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1205–1206

I am not exactly sure, but https://reviews.llvm.org/D39788, which I believe adds the feature seems like it is only used for diagnostic information according to the RFC they mention: http://lists.llvm.org/pipermail/llvm-dev/2017-August/117028.html.

Changing the behavior to be aware of the safe-stack, however, does seem consistent with the original goal of the feature, so I'm happy to update the patch to reflect that.

[paulkirth]

Update emitStackSection to emit the size of the combined stack

[paulkirth]

@MatzeB @seaneveson Can either of you confirm that the logic we're inserting here is not in conflict with the stack size section? Based on D39788 and the original RFC, I would say accounting for safe stack usage is the correct thing to do, but I am not certain.

[paulkirth]

Rebase

[serge-sans-paille]

clang/test/Frontend/stack-usage-safestack.c

2

I think this should be a bitcode-level test à la test/CodeGen/X86/warn-stack.ll
Or at least it should use fixed-size integers so that the checked values are more explicit (a.k.a the test here works because of the triple that implies sizeof(int) == 4).

[paulkirth]

Update tests.

• Dont rely on size of int based on platform
• Add checking to backend tests for warn-stack-size to ensure the behavior is consistant when safestack is enabled

[phosek]

LGTM

clang/test/Frontend/stack-usage-safestack.c

3

Is there any particular reason for using the i386-apple-darwin triple here and below? I'm not even sure if SafeStack is officially supported and tested on Darwin, x86_64-linux may be a safer choice.

[vvereschaka]

Hi @paulkirth,

using of specific triple within stack-usage-safestack.c test causes a failure for the compilers, which don't support these triples (arm/aarch64 in my case).
Such as:

error: unable to create target: 'No available targets are compatible with triple "i386-apple-darwin"'

see more details in https://lab.llvm.org/buildbot/#/builders/119/builds/8169/steps/9/logs/FAIL__Clang__stack-usage-safestack_c result for the failed build.

would you fix the test by removing these triples from the command line or by isolating this test for specific target with // REQUIRES: directive?

[paulkirth]

In D119996#3465735, @vvereschaka wrote:

Hi @paulkirth,

using of specific triple within stack-usage-safestack.c test causes a failure for the compilers, which don't support these triples (arm/aarch64 in my case).
Such as:

error: unable to create target: 'No available targets are compatible with triple "i386-apple-darwin"'

see more details in https://lab.llvm.org/buildbot/#/builders/119/builds/8169/steps/9/logs/FAIL__Clang__stack-usage-safestack_c result for the failed build.

would you fix the test by removing these triples from the command line or by isolating this test for specific target with // REQUIRES: directive?

Oh, that's surprising. I followed the procedure from another test, so I'm surprised that this is failing when that one is not, but maybe I missed the REQUIRES directive. I can probably get a change out fairly quickly, but if its blocking you, feel free to revert this and we can re-land later.

[paulkirth]

@vvereschaka I have a fix out in D124203

clang/test/Frontend/stack-usage-safestack.c

+/// Check that stack frame size warnings behave the same when safe stack is enabled
+
+// RUN: %clang_cc1 %s -fwarn-stack-size=48 -S -o - -triple=i386-apple-darwin 2>&1 | FileCheck --check-prefix=REGULAR %s
+// RUN: %clang_cc1 %s -fwarn-stack-size=1060 -S -o - -triple=i386-apple-darwin 2>&1 | FileCheck --check-prefix=IGNORE %s
+
+// RUN: %clang_cc1 %s -fsanitize=safe-stack -fwarn-stack-size=48 -S -o - -triple=i386-apple-darwin 2>&1 | FileCheck --check-prefix=SAFESTACK %s
+// RUN: %clang_cc1 %s -fsanitize=safe-stack -fwarn-stack-size=1060 -S -o - -triple=i386-apple-darwin 2>&1 | FileCheck --check-prefix=IGNORE %s
+
+extern void init(char *buf, int size);
+extern int use_buf(char *buf, int size);
+
+// REGULAR: warning: stack frame size ([[#]]) exceeds limit ([[#]]) in 'stackSizeWarning'
+// SAFESTACK: warning: stack frame size ([[#]]) exceeds limit ([[#]]) in 'stackSizeWarning'
+// IGNORE-NOT: stack frame size ([[#]]) exceeds limit ([[#]]) in 'stackSizeWarning'
+void stackSizeWarning() {
+  char buf[1024];
+  init(buf, sizeof(buf));
+  if (buf[0] < 42)
+    use_buf(buf, sizeof(buf));
+}

llvm/include/llvm/CodeGen/MachineFrameInfo.h

private:
   /// stack objects like arguments so we can't treat them as immutable.
   bool HasTailCall = false;
 
   /// Not null, if shrink-wrapping found a better place for the prologue.
   MachineBasicBlock *Save = nullptr;
   /// Not null, if shrink-wrapping found a better place for the epilogue.
   MachineBasicBlock *Restore = nullptr;
 
+  /// Size of the UnsafeStack Frame
+  uint64_t UnsafeStackSize = 0;
+
 public:
   explicit MachineFrameInfo(unsigned StackAlignment, bool StackRealignable,
                             bool ForcedRealign)
       : StackAlignment(assumeAligned(StackAlignment)),
         StackRealignable(StackRealignable), ForcedRealign(ForcedRealign) {}
 
   MachineFrameInfo(const MachineFrameInfo &) = delete;
 
public:
 
   void setCalleeSavedInfoValid(bool v) { CSIValid = v; }
 
   MachineBasicBlock *getSavePoint() const { return Save; }
   void setSavePoint(MachineBasicBlock *NewSave) { Save = NewSave; }
   MachineBasicBlock *getRestorePoint() const { return Restore; }
   void setRestorePoint(MachineBasicBlock *NewRestore) { Restore = NewRestore; }
 
+  uint64_t getUnsafeStackSize() const { return UnsafeStackSize; }
+  void setUnsafeStackSize(uint64_t Size) { UnsafeStackSize = Size; }
+
   /// Return a set of physical registers that are pristine.
   ///
   /// Pristine registers hold a value that is useless to the current function,
   /// but that must be preserved - they are callee saved registers that are not
   /// saved.
   ///
   /// Before the PrologueEpilogueInserter has placed the CSR spill code, this
   /// method always returns an empty set.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::emitStackSizeSection(const MachineFunction &MF) {
   // Don't emit functions with dynamic stack allocations.
   if (FrameInfo.hasVarSizedObjects())
     return;
 
   OutStreamer->PushSection();
   OutStreamer->SwitchSection(StackSizeSection);
 
   const MCSymbol *FunctionSymbol = getFunctionBegin();
-  uint64_t StackSize = FrameInfo.getStackSize();
+  uint64_t StackSize =
+      FrameInfo.getStackSize() + FrameInfo.getUnsafeStackSize();
   OutStreamer->emitSymbolValue(FunctionSymbol, TM.getProgramPointerSize());
   OutStreamer->emitULEB128IntValue(StackSize);
 
   OutStreamer->PopSection();
 }
 
 void AsmPrinter::emitStackUsage(const MachineFunction &MF) {
   const std::string &OutputFilename = MF.getTarget().Options.StackUsageOutput;
 
   // OutputFilename empty implies -fstack-usage is not passed.
   if (OutputFilename.empty())
     return;
 
   const MachineFrameInfo &FrameInfo = MF.getFrameInfo();
-  uint64_t StackSize = FrameInfo.getStackSize();
+  uint64_t StackSize =
+      FrameInfo.getStackSize() + FrameInfo.getUnsafeStackSize();
 
   if (StackUsageStream == nullptr) {
     std::error_code EC;
     StackUsageStream =
         std::make_unique<raw_fd_ostream>(OutputFilename, EC, sys::fs::OF_Text);
     if (EC) {
       errs() << "Could not open file: " << EC.message();
       return;

llvm/lib/CodeGen/MachineFunction.cpp

static const char *getPropertyName(MachineFunctionProperties::Property Prop) {
   case P::TiedOpsRewritten: return "TiedOpsRewritten";
   case P::FailsVerification: return "FailsVerification";
   case P::TracksDebugUserValues: return "TracksDebugUserValues";
   }
   // clang-format on
   llvm_unreachable("Invalid machine function property");
 }
 
+void setUnsafeStackSize(const Function &F, MachineFrameInfo &FrameInfo) {
+  if (!F.hasFnAttribute(Attribute::SafeStack))
+    return;
+
+  auto *Existing =
+      dyn_cast_or_null<MDTuple>(F.getMetadata(LLVMContext::MD_annotation));
+
+  if (!Existing || Existing->getNumOperands() != 2)
+    return;
+
+  auto *MetadataName = "unsafe-stack-size";
+  if (auto &N = Existing->getOperand(0)) {
+    if (cast<MDString>(N.get())->getString() == MetadataName) {
+      if (auto &Op = Existing->getOperand(1)) {
+        auto Val = mdconst::extract<ConstantInt>(Op)->getZExtValue();
+        FrameInfo.setUnsafeStackSize(Val);
+      }
+    }
+  }
+}
+
 // Pin the vtable to this file.
 void MachineFunction::Delegate::anchor() {}
 
 void MachineFunctionProperties::print(raw_ostream &OS) const {
   const char *Separator = "";
   for (BitVector::size_type I = 0; I < Properties.size(); ++I) {
     if (!Properties[I])
       continue;
void MachineFunction::init() {
   // explicitly asked us not to.
   bool CanRealignSP = STI->getFrameLowering()->isStackRealignable() &&
                       !F.hasFnAttribute("no-realign-stack");
   FrameInfo = new (Allocator) MachineFrameInfo(
       getFnStackAlignment(STI, F), /*StackRealignable=*/CanRealignSP,
       /*ForcedRealign=*/CanRealignSP &&
           F.hasFnAttribute(Attribute::StackAlignment));
 
+  setUnsafeStackSize(F, *FrameInfo);
+
   if (F.hasFnAttribute(Attribute::StackAlignment))
     FrameInfo->ensureMaxAlignment(*F.getFnStackAlign());
 
   ConstantPool = new (Allocator) MachineConstantPool(getDataLayout());
   Alignment = STI->getTargetLowering()->getMinFunctionAlignment();
 
   // FIXME: Shouldn't use pref alignment if explicit alignment is set on F.
   // FIXME: Use Function::hasOptSize().

llvm/lib/CodeGen/PrologEpilogInserter.cpp

if (MF.getFunction().hasFnAttribute("warn-stack-size")) {
     bool Failed = MF.getFunction()
                       .getFnAttribute("warn-stack-size")
                       .getValueAsString()
                       .getAsInteger(10, Threshold);
     // Verifier should have caught this.
     assert(!Failed && "Invalid warn-stack-size fn attr value");
     (void)Failed;
   }
+  if (MF.getFunction().hasFnAttribute(Attribute::SafeStack)) {
+    StackSize += MFI.getUnsafeStackSize();
+  }
   if (StackSize > Threshold) {
     DiagnosticInfoStackSize DiagStackSize(F, StackSize, Threshold, DS_Warning);
     F.getContext().diagnose(DiagStackSize);
   }
   ORE->emit([&]() {
     return MachineOptimizationRemarkAnalysis(DEBUG_TYPE, "StackSize",
                                              MF.getFunction().getSubprogram(),
                                              &MF.front())

llvm/lib/CodeGen/SafeStack.cpp

 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/MDBuilder.h"
+#include "llvm/IR/Metadata.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IR/Use.h"
 #include "llvm/IR/Value.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Debug.h"
for (AllocaInst *AI : StaticAllocas) {
     AI->eraseFromParent();
   }
 
   // Re-align BasePointer so that our callees would see it aligned as
   // expected.
   // FIXME: no need to update BasePointer in leaf functions.
   unsigned FrameSize = alignTo(SSL.getFrameSize(), StackAlignment);
 
+  MDBuilder MDB(F.getContext());
+  SmallVector<Metadata *, 2> Data;
+  Data.push_back(MDB.createString("unsafe-stack-size"));
+  Data.push_back(MDB.createConstant(ConstantInt::get(Int32Ty, FrameSize)));
+  MDNode *MD = MDTuple::get(F.getContext(), Data);
+  F.setMetadata(LLVMContext::MD_annotation, MD);
+
   // Update shadow stack pointer in the function epilogue.
   IRB.SetInsertPoint(BasePointer->getNextNode());
 
   Value *StaticTop =
       IRB.CreateGEP(Int8Ty, BasePointer, ConstantInt::get(Int32Ty, -FrameSize),
                     "unsafe_stack_static_top");
   IRB.CreateStore(StaticTop, UnsafeStackPtr);
   return StaticTop;

llvm/test/CodeGen/X86/warn-stack.ll

 define void @warn() nounwind ssp "warn-stack-size"="80" {
 entry:
   %buffer = alloca [80 x i8], align 1
   %arraydecay = getelementptr inbounds [80 x i8], [80 x i8]* %buffer, i64 0, i64 0
   call void @doit(i8* %arraydecay) nounwind
   ret void
 }
 
+; Ensure that warn-stack-size also considers the size of the unsafe stack.
+; With safestack enabled the machine stack size is well below 80, but the
+; combined stack size of the machine stack and unsafe stack will exceed the
+; warning threshold
+
+; CHECK: warning: stack frame size (120) exceeds limit (80) in function 'warn_safestack'
+define void @warn_safestack() nounwind ssp safestack "warn-stack-size"="80" {
+entry:
+  %buffer = alloca [80 x i8], align 1
+  %arraydecay = getelementptr inbounds [80 x i8], [80 x i8]* %buffer, i64 0, i64 0
+  call void @doit(i8* %arraydecay) nounwind
+  ret void
+}
 declare void @doit(i8*)

llvm/test/Transforms/SafeStack/ARM/debug.ll

 ; RUN: opt -safe-stack -safestack-use-pointer-address < %s -S | FileCheck %s
 target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
 target triple = "armv7-pc-linux-android"
 
 ; Original C used to generate debug info:
 ; char*** addr;
 ; char** __safestack_pointer_address() {
 ;   return *addr;
 ; }
 ; void Capture(char*x);
 ; void f() { char c[16]; Capture(c); }
 
-; CHECK: !35 = !DILocation(line: 3, column: 11, scope: !17, inlinedAt: !36)
-; CHECK: !36 = distinct !DILocation(line: 6, scope: !27)
+; CHECK: !36 = !DILocation(line: 3, column: 11, scope: !17, inlinedAt: !37)
+; CHECK: !37 = distinct !DILocation(line: 6, scope: !27)
 
 @addr = common local_unnamed_addr global i8*** null, align 4, !dbg !0
 
 ; Function Attrs: norecurse nounwind readonly safestack
 define i8** @__safestack_pointer_address() local_unnamed_addr #0 !dbg !17 {
 entry:
   %0 = load i8***, i8**** @addr, align 4, !dbg !20, !tbaa !21
   %1 = load i8**, i8*** %0, align 4, !dbg !25, !tbaa !21