subject:"\[PATCH\] D119996\: \[safestack\] Support safestack in stack size diagnostics"

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-04-21 Thread Paul Kirth via Phabricator via cfe-commits

paulkirth added a comment.

@vvereschaka I have a fix out in D124203 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119996

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-04-21 Thread Paul Kirth via Phabricator via cfe-commits

paulkirth added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119996

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-04-21 Thread Vlad Vereschaka via Phabricator via cfe-commits

vvereschaka added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119996

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-04-20 Thread Paul Kirth via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
paulkirth marked an inline comment as done.
Closed by commit rG61e36e87df1a: [safestack] Support safestack in stack size 
diagnostics (authored by paulkirth).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119996

Files:
  clang/test/Frontend/stack-usage-safestack.c
  llvm/include/llvm/CodeGen/MachineFrameInfo.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/PrologEpilogInserter.cpp
  llvm/lib/CodeGen/SafeStack.cpp
  llvm/test/CodeGen/X86/warn-stack.ll
  llvm/test/Transforms/SafeStack/ARM/debug.ll

Index: llvm/test/Transforms/SafeStack/ARM/debug.ll
===
--- llvm/test/Transforms/SafeStack/ARM/debug.ll
+++ llvm/test/Transforms/SafeStack/ARM/debug.ll
@@ -10,8 +10,8 @@
 ; 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
 
Index: llvm/test/CodeGen/X86/warn-stack.ll
===
--- llvm/test/CodeGen/X86/warn-stack.ll
+++ llvm/test/CodeGen/X86/warn-stack.ll
@@ -21,4 +21,17 @@
   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*)
Index: llvm/lib/CodeGen/SafeStack.cpp
===
--- llvm/lib/CodeGen/SafeStack.cpp
+++ llvm/lib/CodeGen/SafeStack.cpp
@@ -48,6 +48,7 @@
 #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"
@@ -633,6 +634,13 @@
   // FIXME: no need to update BasePointer in leaf functions.
   unsigned FrameSize = alignTo(SSL.getFrameSize(), StackAlignment);
 
+  MDBuilder MDB(F.getContext());
+  SmallVector 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());
 
Index: llvm/lib/CodeGen/PrologEpilogInserter.cpp
===
--- llvm/lib/CodeGen/PrologEpilogInserter.cpp
+++ llvm/lib/CodeGen/PrologEpilogInserter.cpp
@@ -283,6 +283,9 @@
 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);
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -107,6 +107,27 @@
   llvm_unreachable("Invalid machine function property");
 }
 
+void setUnsafeStackSize(const Function , MachineFrameInfo ) {
+  if (!F.hasFnAttribute(Attribute::SafeStack))
+return;
+
+  auto *Existing =
+  dyn_cast_or_null(F.getMetadata(LLVMContext::MD_annotation));
+
+  if (!Existing || Existing->getNumOperands() != 2)
+return;
+
+  auto *MetadataName = "unsafe-stack-size";
+  if (auto  = Existing->getOperand(0)) {
+if (cast(N.get())->getString() == MetadataName) {
+  if (auto  = Existing->getOperand(1)) {
+auto Val = mdconst::extract(Op)->getZExtValue();
+FrameInfo.setUnsafeStackSize(Val);
+  }
+}
+  }
+}
+
 // Pin the vtable to this file.
 void MachineFunction::Delegate::anchor() {}
 
@@ -175,6 +196,8 @@
   /*ForcedRealign=*/CanRealignSP &&
   F.hasFnAttribute(Attribute::StackAlignment));
 
+  setUnsafeStackSize(F, *FrameInfo);
+
   if

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-04-19 Thread Petr Hosek via Phabricator via cfe-commits

phosek accepted this revision.
phosek added a comment.

LGTM




Comment at: clang/test/Frontend/stack-usage-safestack.c:3
+
+// 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

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119996

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-03-31 Thread Paul Kirth via Phabricator via cfe-commits

paulkirth updated this revision to Diff 419493.
paulkirth added a comment.
Herald added a subscriber: pengfei.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119996

Files:
  clang/test/Frontend/stack-usage-safestack.c
  llvm/include/llvm/CodeGen/MachineFrameInfo.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/PrologEpilogInserter.cpp
  llvm/lib/CodeGen/SafeStack.cpp
  llvm/test/CodeGen/X86/warn-stack.ll
  llvm/test/Transforms/SafeStack/ARM/debug.ll

Index: llvm/test/Transforms/SafeStack/ARM/debug.ll
===
--- llvm/test/Transforms/SafeStack/ARM/debug.ll
+++ llvm/test/Transforms/SafeStack/ARM/debug.ll
@@ -10,8 +10,8 @@
 ; 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
 
Index: llvm/test/CodeGen/X86/warn-stack.ll
===
--- llvm/test/CodeGen/X86/warn-stack.ll
+++ llvm/test/CodeGen/X86/warn-stack.ll
@@ -21,4 +21,17 @@
   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*)
Index: llvm/lib/CodeGen/SafeStack.cpp
===
--- llvm/lib/CodeGen/SafeStack.cpp
+++ llvm/lib/CodeGen/SafeStack.cpp
@@ -48,6 +48,7 @@
 #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"
@@ -642,6 +643,13 @@
   // FIXME: no need to update BasePointer in leaf functions.
   unsigned FrameSize = alignTo(SSL.getFrameSize(), StackAlignment);
 
+  MDBuilder MDB(F.getContext());
+  SmallVector 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());
 
Index: llvm/lib/CodeGen/PrologEpilogInserter.cpp
===
--- llvm/lib/CodeGen/PrologEpilogInserter.cpp
+++ llvm/lib/CodeGen/PrologEpilogInserter.cpp
@@ -283,6 +283,9 @@
 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);
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -107,6 +107,27 @@
   llvm_unreachable("Invalid machine function property");
 }
 
+void setUnsafeStackSize(const Function , MachineFrameInfo ) {
+  if (!F.hasFnAttribute(Attribute::SafeStack))
+return;
+
+  auto *Existing =
+  dyn_cast_or_null(F.getMetadata(LLVMContext::MD_annotation));
+
+  if (!Existing || Existing->getNumOperands() != 2)
+return;
+
+  auto *MetadataName = "unsafe-stack-size";
+  if (auto  = Existing->getOperand(0)) {
+if (cast(N.get())->getString() == MetadataName) {
+  if (auto  = Existing->getOperand(1)) {
+auto Val = mdconst::extract(Op)->getZExtValue();
+FrameInfo.setUnsafeStackSize(Val);
+  }
+}
+  }
+}
+
 // Pin the vtable to this file.
 void MachineFunction::Delegate::anchor() {}
 
@@ -175,6 +196,8 @@
   /*ForcedRealign=*/CanRealignSP &&
   F.hasFnAttribute(Attribute::StackAlignment));
 
+  setUnsafeStackSize(F, *FrameInfo);
+
   if

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-03-31 Thread serge via Phabricator via cfe-commits

serge-sans-paille added inline comments.



Comment at: clang/test/Frontend/stack-usage-safestack.c:1
+/// Check that stack frame size warnings behave the same when safe stack is 
enabled
+

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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119996

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-03-30 Thread Paul Kirth via Phabricator via cfe-commits

paulkirth updated this revision to Diff 419222.
paulkirth added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119996

Files:
  clang/test/Frontend/stack-usage-safestack.c
  llvm/include/llvm/CodeGen/MachineFrameInfo.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/PrologEpilogInserter.cpp
  llvm/lib/CodeGen/SafeStack.cpp
  llvm/test/Transforms/SafeStack/ARM/debug.ll

Index: llvm/test/Transforms/SafeStack/ARM/debug.ll
===
--- llvm/test/Transforms/SafeStack/ARM/debug.ll
+++ llvm/test/Transforms/SafeStack/ARM/debug.ll
@@ -10,8 +10,8 @@
 ; 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
 
Index: llvm/lib/CodeGen/SafeStack.cpp
===
--- llvm/lib/CodeGen/SafeStack.cpp
+++ llvm/lib/CodeGen/SafeStack.cpp
@@ -48,6 +48,7 @@
 #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"
@@ -642,6 +643,13 @@
   // FIXME: no need to update BasePointer in leaf functions.
   unsigned FrameSize = alignTo(SSL.getFrameSize(), StackAlignment);
 
+  MDBuilder MDB(F.getContext());
+  SmallVector 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());
 
Index: llvm/lib/CodeGen/PrologEpilogInserter.cpp
===
--- llvm/lib/CodeGen/PrologEpilogInserter.cpp
+++ llvm/lib/CodeGen/PrologEpilogInserter.cpp
@@ -283,6 +283,9 @@
 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);
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -107,6 +107,27 @@
   llvm_unreachable("Invalid machine function property");
 }
 
+void setUnsafeStackSize(const Function , MachineFrameInfo ) {
+  if (!F.hasFnAttribute(Attribute::SafeStack))
+return;
+
+  auto *Existing =
+  dyn_cast_or_null(F.getMetadata(LLVMContext::MD_annotation));
+
+  if (!Existing || Existing->getNumOperands() != 2)
+return;
+
+  auto *MetadataName = "unsafe-stack-size";
+  if (auto  = Existing->getOperand(0)) {
+if (cast(N.get())->getString() == MetadataName) {
+  if (auto  = Existing->getOperand(1)) {
+auto Val = mdconst::extract(Op)->getZExtValue();
+FrameInfo.setUnsafeStackSize(Val);
+  }
+}
+  }
+}
+
 // Pin the vtable to this file.
 void MachineFunction::Delegate::anchor() {}
 
@@ -175,6 +196,8 @@
   /*ForcedRealign=*/CanRealignSP &&
   F.hasFnAttribute(Attribute::StackAlignment));
 
+  setUnsafeStackSize(F, *FrameInfo);
+
   if (F.hasFnAttribute(Attribute::StackAlignment))
 FrameInfo->ensureMaxAlignment(*F.getFnStackAlign());
 
Index: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -1186,7 +1186,8 @@
   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);
 
@@ -1201,7 +1202,8 @@
 return;
 
   const MachineFrameInfo  = MF.getFrameInfo();
-  uint64_t StackSize = FrameInfo.getStackSize();
+  uint64_t StackSize =
+  FrameInfo.getStackSize() + FrameInfo.getUnsafeStackSize();
 
   if (StackUsageStream == nullptr) {
 std::error_code EC;
Index: llvm/include/llvm/CodeGen/MachineFrameInfo.h
===
---

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-03-07 Thread Paul Kirth via Phabricator via cfe-commits

paulkirth added subscribers: seaneveson, MatzeB.
paulkirth added a comment.

@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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119996

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-03-07 Thread Paul Kirth via Phabricator via cfe-commits

paulkirth updated this revision to Diff 413556.
paulkirth added a comment.

Update emitStackSection to emit the size of the combined stack


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119996

Files:
  clang/test/Frontend/stack-usage-safestack.c
  llvm/include/llvm/CodeGen/MachineFrameInfo.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/PrologEpilogInserter.cpp
  llvm/lib/CodeGen/SafeStack.cpp
  llvm/test/Transforms/SafeStack/ARM/debug.ll

Index: llvm/test/Transforms/SafeStack/ARM/debug.ll
===
--- llvm/test/Transforms/SafeStack/ARM/debug.ll
+++ llvm/test/Transforms/SafeStack/ARM/debug.ll
@@ -10,8 +10,8 @@
 ; 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
 
Index: llvm/lib/CodeGen/SafeStack.cpp
===
--- llvm/lib/CodeGen/SafeStack.cpp
+++ llvm/lib/CodeGen/SafeStack.cpp
@@ -49,6 +49,7 @@
 #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"
@@ -645,6 +646,13 @@
   // FIXME: no need to update BasePointer in leaf functions.
   unsigned FrameSize = alignTo(SSL.getFrameSize(), StackAlignment);
 
+  MDBuilder MDB(F.getContext());
+  SmallVector 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());
 
Index: llvm/lib/CodeGen/PrologEpilogInserter.cpp
===
--- llvm/lib/CodeGen/PrologEpilogInserter.cpp
+++ llvm/lib/CodeGen/PrologEpilogInserter.cpp
@@ -285,6 +285,9 @@
 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);
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -109,6 +109,27 @@
   llvm_unreachable("Invalid machine function property");
 }
 
+void setUnsafeStackSize(const Function , MachineFrameInfo ) {
+  if (!F.hasFnAttribute(Attribute::SafeStack))
+return;
+
+  auto *Existing =
+  dyn_cast_or_null(F.getMetadata(LLVMContext::MD_annotation));
+
+  if (!Existing || Existing->getNumOperands() != 2)
+return;
+
+  auto *MetadataName = "unsafe-stack-size";
+  if (auto  = Existing->getOperand(0)) {
+if (cast(N.get())->getString() == MetadataName) {
+  if (auto  = Existing->getOperand(1)) {
+auto Val = mdconst::extract(Op)->getZExtValue();
+FrameInfo.setUnsafeStackSize(Val);
+  }
+}
+  }
+}
+
 // Pin the vtable to this file.
 void MachineFunction::Delegate::anchor() {}
 
@@ -177,6 +198,8 @@
   /*ForcedRealign=*/CanRealignSP &&
   F.hasFnAttribute(Attribute::StackAlignment));
 
+  setUnsafeStackSize(F, *FrameInfo);
+
   if (F.hasFnAttribute(Attribute::StackAlignment))
 FrameInfo->ensureMaxAlignment(*F.getFnStackAlign());
 
Index: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -1193,7 +1193,8 @@
   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);
 
@@ -1208,7 +1209,8 @@
 return;
 
   const MachineFrameInfo  = MF.getFrameInfo();
-  uint64_t StackSize = FrameInfo.getStackSize();
+  uint64_t StackSize =
+  FrameInfo.getStackSize() + FrameInfo.getUnsafeStackSize();
 
   if (StackUsageStream == nullptr) {
 std::error_code EC;
Index: llvm/include/llvm/CodeGen/MachineFrameInfo.h

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-03-04 Thread Paul Kirth via Phabricator via cfe-commits

paulkirth added inline comments.



Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1211
   const MachineFrameInfo  = MF.getFrameInfo();
-  uint64_t StackSize = FrameInfo.getStackSize();
+  uint64_t StackSize =
+  FrameInfo.getStackSize() + FrameInfo.getUnsafeStackSize();

mcgrathr wrote:
> 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.
> 
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119996

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-03-04 Thread Roland McGrath via Phabricator via cfe-commits

mcgrathr added inline comments.



Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1211
   const MachineFrameInfo  = MF.getFrameInfo();
-  uint64_t StackSize = FrameInfo.getStackSize();
+  uint64_t StackSize =
+  FrameInfo.getStackSize() + FrameInfo.getUnsafeStackSize();

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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119996

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

2022-03-04 Thread Paul Kirth via Phabricator via cfe-commits

paulkirth created this revision.
Herald added a subscriber: hiraditya.
paulkirth updated this revision to Diff 409705.
paulkirth added a comment.
paulkirth updated this revision to Diff 409797.
tstellar added a subscriber: serge-sans-paille.
paulkirth edited the summary of this revision.
paulkirth added reviewers: phosek, mcgrathr, tstellar.
Herald added a project: All.
paulkirth published this revision for review.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Fix ARM test breakage


paulkirth added a comment.

Actually fix the ARM test


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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119996

Files:
  clang/test/Frontend/stack-usage-safestack.c
  llvm/include/llvm/CodeGen/MachineFrameInfo.h
  llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  llvm/lib/CodeGen/MachineFunction.cpp
  llvm/lib/CodeGen/PrologEpilogInserter.cpp
  llvm/lib/CodeGen/SafeStack.cpp
  llvm/test/Transforms/SafeStack/ARM/debug.ll

Index: llvm/test/Transforms/SafeStack/ARM/debug.ll
===
--- llvm/test/Transforms/SafeStack/ARM/debug.ll
+++ llvm/test/Transforms/SafeStack/ARM/debug.ll
@@ -10,8 +10,8 @@
 ; 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
 
Index: llvm/lib/CodeGen/SafeStack.cpp
===
--- llvm/lib/CodeGen/SafeStack.cpp
+++ llvm/lib/CodeGen/SafeStack.cpp
@@ -49,6 +49,7 @@
 #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"
@@ -645,6 +646,13 @@
   // FIXME: no need to update BasePointer in leaf functions.
   unsigned FrameSize = alignTo(SSL.getFrameSize(), StackAlignment);
 
+  MDBuilder MDB(F.getContext());
+  SmallVector 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());
 
Index: llvm/lib/CodeGen/PrologEpilogInserter.cpp
===
--- llvm/lib/CodeGen/PrologEpilogInserter.cpp
+++ llvm/lib/CodeGen/PrologEpilogInserter.cpp
@@ -285,6 +285,9 @@
 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);
Index: llvm/lib/CodeGen/MachineFunction.cpp
===
--- llvm/lib/CodeGen/MachineFunction.cpp
+++ llvm/lib/CodeGen/MachineFunction.cpp
@@ -109,6 +109,27 @@
   llvm_unreachable("Invalid machine function property");
 }
 
+void setUnsafeStackSize(const Function , MachineFrameInfo ) {
+  if (!F.hasFnAttribute(Attribute::SafeStack))
+return;
+
+  auto *Existing =
+  dyn_cast_or_null(F.getMetadata(LLVMContext::MD_annotation));
+
+  if (!Existing || Existing->getNumOperands() != 2)
+return;
+
+  auto *MetadataName = "unsafe-stack-size";
+  if (auto  = Existing->getOperand(0)) {
+if (cast(N.get())->getString() == MetadataName) {
+  if (auto  = Existing->getOperand(1)) {
+auto Val = mdconst::extract(Op)->getZExtValue();
+FrameInfo.setUnsafeStackSize(Val);
+  }
+}
+  }
+}
+
 // Pin the vtable to this file.
 void MachineFunction::Delegate::anchor() {}
 
@@ -177,6 +198,8 @@
   /*ForcedRealign=*/CanRealignSP &&
   F.hasFnAttribute(Attribute::StackAlignment));
 
+  setUnsafeStackSize(F, *FrameInfo);
+
   if (F.hasFnAttribute(Attribute::StackAlignment))
 FrameInfo->ensureMaxAlignment(*F.getFnStackAlign());
 
Index: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
===
--- llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -1208,7 +1208,8 @@
 return;
 
   const MachineFrameInfo  = MF.getFrameInfo();
-  uint64_t StackSize = FrameInfo.getStackSize();
+

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

[PATCH] D119996: [safestack] Support safestack in stack size diagnostics

13 matches

Site Navigation

Mail list logo

Footer information