[IR] make -warn-frame-size into a module attr

[nickdesaulniers]

Summary

-Wframe-larger-than= is an interesting warning; we can't know the frame
size until PrologueEpilogueInsertion (PEI); very late in the compilation
pipeline.

-Wframe-larger-than= was propagated through CC1 as an -mllvm flag, then
was a cl::opt in LLVM's PEI pass; this meant it was dropped during LTO
and needed to be re-specified via -plugin-opt.

Instead, make it part of the IR proper as a module level attribute,
similar to D103048. Introduce -fwarn-stack-size CC1 option.

[nickdesaulniers]

clang/include/clang/Driver/Options.td

2578–2583

@rsmith I wasn't super sure about this hunk of the diff. Should I not be reusing the same flag between the Frontend and the Driver? (The Driver comes after the Frontend, IIUC? Is that right?)

[nickdesaulniers]

• add CHECK to llvm/test/Linker/warn-stack-frame.ll

[qcolombet]

Hi Nick,

From the backend prospective, this looks fine but you'll want someone to look at the front end part before landing that change.

Cheers,
-Quentin

[rsmith]

clang/include/clang/Driver/Options.td

2578–2583

(The driver comes before the frontend: the driver is the user-facing GCC-compatible clang binary, and the frontend is the -cc1 interface.)

What we do in other cases is to translate the driver-level (user-facing) warning flag into multiple frontend flags:

• The driver-level flag -Wwrite-strings (in C) gets mapped into the frontend-level flags -Wwrite-strings -fconst-strings, where -Wwrite-strings only controls whether the warning is shown, and -fconst-strings only controls whether string literals have type char[N] or const char[N].
• The driver-level flag -Wdeprecated (in C++) gets mapped into the frontend-level flags -Wdeprecated -fdeprecated-macro, where -Wdeprecated only controls whether the warning is shown, and -fdeprecated-macro only controls whether __DEPRECATED is implicitly defined by the preprocessor.

So I think the expected strategy here would probably be that the driver-level flag -Wframe-larger-than=N would get mapped into the frontend-level flags -Wframe-larger-than -fsomething=N, where -Wframe-larger-than only controls whether the warning is shown, and -fsomething=N sets the level of stack usage at which the backend triggers the warning.

[nickdesaulniers]

• introduce -fwarn-stack-size

[nickdesaulniers]

• update comment in clang/include/clang/Basic/CodeGenOptions.def

[nickdesaulniers]

• add < to same comment

[rsmith]

clang/test/Frontend/Wframe-larger-than.c

2 ↗

(On Diff #351008)

Comment needs updating.

[nickdesaulniers]

• move driver test from clang/test/Frontend/ to clang/test/Driver/, update comment, clarify TODOs

[nickdesaulniers]

FWIW, it was pointed out to me that the commit message doesn't precisely match the actual attribute name. The actual attribute name is warn-stack-size, but the commit message uses -warn-frame-size. Oh well.

[dblaikie]

llvm/test/Linker/warn-stack-frame.ll

7

If you like, you could make this a function attribute instead, that way it'd be compatible with LTO even when the value varies between compilations?

[nickdesaulniers]

llvm/test/Linker/warn-stack-frame.ll

7

Yep, this is already causing problems for us, so I will fix that ASAP: https://github.com/ClangBuiltLinux/linux/issues/1395.

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(StackRealignment  , 1, 0) ///< Control whether to force stack
                                      ///< realignment.
 CODEGENOPT(UseInitArray      , 1, 0) ///< Control whether to use .init_array or
                                      ///< .ctors.
 VALUE_CODEGENOPT(StackAlignment    , 32, 0) ///< Overrides default stack
                                             ///< alignment, if not 0.
 VALUE_CODEGENOPT(StackProbeSize    , 32, 4096) ///< Overrides default stack
                                                ///< probe size, even if 0.
+VALUE_CODEGENOPT(WarnStackSize     , 32, UINT_MAX) ///< Set via -fwarn-stack-size.
 CODEGENOPT(NoStackArgProbe, 1, 0) ///< Set when -mno-stack-arg-probe is used
 CODEGENOPT(DebugStrictDwarf, 1, 1) ///< Whether or not to use strict DWARF info.
 CODEGENOPT(DebugColumnInfo, 1, 0) ///< Whether or not to use column information
                                   ///< in debug info.
 
 CODEGENOPT(DebugTypeExtRefs, 1, 0) ///< Whether or not debug info should contain
                                    ///< external references to a PCH or module.
 

clang/include/clang/Driver/Options.td

defm fixed_point : BoolFOption<"fixed-point",
   PosFlag<SetTrue, [CC1Option], "Enable">, NegFlag<SetFalse, [], "Disable">,
   BothFlags<[], " fixed point types">>, ShouldParseIf<!strconcat("!", cplusplus.KeyPath)>;
 defm cxx_static_destructors : BoolFOption<"c++-static-destructors",
   LangOpts<"RegisterStaticDestructors">, DefaultTrue,
   NegFlag<SetFalse, [CC1Option], "Disable C++ static destructor registration">,
   PosFlag<SetTrue>>;
 def fsymbol_partition_EQ : Joined<["-"], "fsymbol-partition=">, Group<f_Group>,
   Flags<[CC1Option]>, MarshallingInfoString<CodeGenOpts<"SymbolPartition">>;
+def fwarn_stack_size_EQ : Joined<["-"], "fwarn-stack-size=">, Group<f_Group>,
+  Flags<[CC1Option]>,
+  MarshallingInfoInt<CodeGenOpts<"WarnStackSize">, "UINT_MAX">;
 
 defm memory_profile : OptInFFlag<"memory-profile", "Enable", "Disable", " heap memory profiling">;
 def fmemory_profile_EQ : Joined<["-"], "fmemory-profile=">,
     Group<f_Group>, Flags<[CC1Option]>, MetaVarName<"<directory>">,
     HelpText<"Enable heap memory profiling and dump results into <directory>">;
 
 // Begin sanitizer flags. These should all be core options exposed in all driver
 // modes.

 def : Flag<["-"], "ftree-slp-vectorize">, Alias<fslp_vectorize>;
 def : Flag<["-"], "fno-tree-slp-vectorize">, Alias<fno_slp_vectorize>;
 def Wlarge_by_value_copy_def : Flag<["-"], "Wlarge-by-value-copy">,
   HelpText<"Warn if a function definition returns or accepts an object larger "
            "in bytes than a given value">, Flags<[HelpHidden]>;
 def Wlarge_by_value_copy_EQ : Joined<["-"], "Wlarge-by-value-copy=">, Flags<[CC1Option]>,
   MarshallingInfoInt<LangOpts<"NumLargeByValueCopy">>;
 
 // These "special" warning flags are effectively processed as f_Group flags by the driver:
 // Just silence warnings about -Wlarger-than for now.
 def Wlarger_than_EQ : Joined<["-"], "Wlarger-than=">, Group<clang_ignored_f_Group>;
 def Wlarger_than_ : Joined<["-"], "Wlarger-than-">, Alias<Wlarger_than_EQ>;
 def Wframe_larger_than_EQ : Joined<["-"], "Wframe-larger-than=">, Group<f_Group>, Flags<[NoXarchOption]>;
 
 def : Flag<["-"], "fterminated-vtables">, Alias<fapple_kext>;
 defm threadsafe_statics : BoolFOption<"threadsafe-statics",
   LangOpts<"ThreadsafeStatics">, DefaultTrue,
   NegFlag<SetFalse, [CC1Option], "Do not emit code to make initialization of local statics thread safe">,
   PosFlag<SetTrue>>;
 def ftime_report : Flag<["-"], "ftime-report">, Group<f_Group>, Flags<[CC1Option]>,
   MarshallingInfoFlag<CodeGenOpts<"TimePasses">>;
 def ftime_report_EQ: Joined<["-"], "ftime-report=">, Group<f_Group>,

clang/lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::Release() {
   if (!getCodeGenOpts().StackProtectorGuardReg.empty())
     getModule().setStackProtectorGuardReg(
         getCodeGenOpts().StackProtectorGuardReg);
   if (getCodeGenOpts().StackProtectorGuardOffset != INT_MAX)
     getModule().setStackProtectorGuardOffset(
         getCodeGenOpts().StackProtectorGuardOffset);
   if (getCodeGenOpts().StackAlignment)
     getModule().setOverrideStackAlignment(getCodeGenOpts().StackAlignment);
+  if (getCodeGenOpts().WarnStackSize != UINT_MAX)
+    getModule().setWarnStackSize(getCodeGenOpts().WarnStackSize);
 
   getTargetCodeGenInfo().emitTargetMetadata(*this, MangledDeclNames);
 
   EmitBackendOptionsMetadata(getCodeGenOpts());
 
   // Set visibility from DLL storage class
   // We do this at the end of LLVM IR generation; after any operation
   // that might affect the DLL storage class or the visibility, and

clang/lib/Driver/ToolChains/Clang.cpp

if (Arg *A = Args.getLastArg(options::OPT_mabi_EQ_vec_extabi,
     if (A->getOption().getID() == options::OPT_mabi_EQ_vec_extabi)
       CmdArgs.push_back("-mabi=vec-extabi");
     else
       D.Diag(diag::err_aix_default_altivec_abi);
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_Wframe_larger_than_EQ)) {
     StringRef v = A->getValue();
-    CmdArgs.push_back("-mllvm");
-    CmdArgs.push_back(Args.MakeArgString("-warn-stack-size=" + v));
+    CmdArgs.push_back(Args.MakeArgString("-fwarn-stack-size=" + v));
     A->claim();
   }
 
   if (!Args.hasFlag(options::OPT_fjump_tables, options::OPT_fno_jump_tables,
                     true))
     CmdArgs.push_back("-fno-jump-tables");
 
   if (Args.hasFlag(options::OPT_fprofile_sample_accurate,

clang/test/Driver/Wframe-larger-than.c

+// RUN: %clang -Wframe-larger-than=42 \
+// RUN:   -v -E - < /dev/null 2>&1 | FileCheck %s --check-prefix=ENABLE
+// RUN: %clang -Wframe-larger-than=42 -Wno-frame-larger-than= \
+// RUN:   -v -E - < /dev/null 2>&1 | FileCheck %s --check-prefix=DISABLE
+// RUN: %clang -Wframe-larger-than=42 -Wno-frame-larger-than= -Wframe-larger-than=43 \
+// RUN:   -v -E - < /dev/null 2>&1 | FileCheck %s --check-prefix=REENABLE
+//
+// TODO: we might want to look into being able to disable, then re-enable this
+// warning properly. We could have the driver turn -Wframe-larger-than=X into
+// -Wframe-larger-than -fwarn-stack-size=X.  Also, we should support
+// -Wno-frame-larger-than (no = suffix) like GCC.
+
+// ENABLE: cc1 {{.*}} -fwarn-stack-size=42
+// DISABLE: cc1 {{.*}} -fwarn-stack-size=42 {{.*}} -Wno-frame-larger-than=
+// REENABLE: cc1 {{.*}} -fwarn-stack-size=43 {{.*}} -Wno-frame-larger-than=

clang/test/Frontend/backend-diagnostic.c

 // REQUIRES: x86-registered-target
 // Play around with backend reporting:
 // _REGULAR_: Regular behavior, no warning switch enabled.
 // _PROMOTE_: Promote warning to error.
 // _IGNORE_: Drop backend warning.
 //
-// RUN: not %clang_cc1 %s -mllvm -warn-stack-size=0 -no-integrated-as -S -o - -triple=i386-apple-darwin 2> %t.err
+// RUN: not %clang_cc1 %s -fwarn-stack-size=0 -no-integrated-as -S -o - -triple=i386-apple-darwin 2> %t.err
 // RUN: FileCheck < %t.err %s --check-prefix=REGULAR --check-prefix=ASM
-// RUN: not %clang_cc1 %s -mllvm -warn-stack-size=0 -no-integrated-as -S -o - -triple=i386-apple-darwin -Werror=frame-larger-than= 2> %t.err
+// RUN: not %clang_cc1 %s -fwarn-stack-size=0 -no-integrated-as -S -o - -triple=i386-apple-darwin -Werror=frame-larger-than= 2> %t.err
 // RUN: FileCheck < %t.err %s --check-prefix=PROMOTE --check-prefix=ASM
-// RUN: not %clang_cc1 %s -mllvm -warn-stack-size=0 -no-integrated-as -S -o - -triple=i386-apple-darwin -Wno-frame-larger-than= 2> %t.err
+// RUN: not %clang_cc1 %s -fwarn-stack-size=0 -no-integrated-as -S -o - -triple=i386-apple-darwin -Wno-frame-larger-than= 2> %t.err
 // RUN: FileCheck < %t.err %s --check-prefix=IGNORE --check-prefix=ASM
 //
 // RUN: %clang_cc1 %s -S -o - -triple=i386-apple-darwin -verify -no-integrated-as
 
 extern void doIt(char *);
 
 // REGULAR: warning: stack frame size of {{[0-9]+}} bytes in function 'stackSizeWarning'
 // PROMOTE: error: stack frame size of {{[0-9]+}} bytes in function 'stackSizeWarning'

clang/test/Misc/backend-stack-frame-diagnostics-fallback.cpp

 // REQUIRES: x86-registered-target
-// RUN: %clang_cc1 %s -mllvm -warn-stack-size=0 -emit-codegen-only -triple=i386-apple-darwin 2>&1 | FileCheck %s
+// RUN: %clang_cc1 %s -fwarn-stack-size=0 -emit-codegen-only -triple=i386-apple-darwin 2>&1 | FileCheck %s
 
 // TODO: Emit rich diagnostics for thunks and move this into the appropriate test file.
 // Until then, test that we fall back and display the LLVM backend diagnostic.
 namespace frameSizeThunkWarning {
   struct A {
     virtual void f();
   };
 

llvm/include/llvm/IR/Module.h

/// @}
   /// Get/set what offset from the stack protector to use.
   int getStackProtectorGuardOffset() const;
   void setStackProtectorGuardOffset(int Offset);
 
   /// Get/set the stack alignment overridden from the default.
   unsigned getOverrideStackAlignment() const;
   void setOverrideStackAlignment(unsigned Align);
 
+  /// Get/set the stack frame size threshold to warn on.
+  unsigned getWarnStackSize() const;
+  void setWarnStackSize(unsigned Threshold);
+
   /// @name Utility functions for querying and setting the build SDK version
   /// @{
 
   /// Attach a build SDK version metadata to this module.
   void setSDKVersion(const VersionTuple &V);
 
   /// Get the build SDK version metadata.
   ///

llvm/lib/CodeGen/PrologEpilogInserter.cpp

 };
 
 } // end anonymous namespace
 
 char PEI::ID = 0;
 
 char &llvm::PrologEpilogCodeInserterID = PEI::ID;
 
-static cl::opt<unsigned>
-WarnStackSize("warn-stack-size", cl::Hidden, cl::init((unsigned)-1),
-              cl::desc("Warn for stack size bigger than the given"
-                       " number"));
-
 INITIALIZE_PASS_BEGIN(PEI, DEBUG_TYPE, "Prologue/Epilogue Insertion", false,
                       false)
 INITIALIZE_PASS_DEPENDENCY(MachineLoopInfo)
 INITIALIZE_PASS_DEPENDENCY(MachineDominatorTree)
 INITIALIZE_PASS_DEPENDENCY(MachineOptimizationRemarkEmitterPass)
 INITIALIZE_PASS_END(PEI, DEBUG_TYPE,
                     "Prologue/Epilogue Insertion & Frame Finalization", false,
                     false)
bool PEI::runOnMachineFunction(MachineFunction &MF) {
   // post-pass, scavenge the virtual registers that frame index elimination
   // inserted.
   if (TRI->requiresRegisterScavenging(MF) && FrameIndexVirtualScavenging)
     scavengeFrameVirtualRegs(MF, *RS);
 
   // Warn on stack size when we exceeds the given limit.
   MachineFrameInfo &MFI = MF.getFrameInfo();
   uint64_t StackSize = MFI.getStackSize();
-  if (WarnStackSize.getNumOccurrences() > 0 && WarnStackSize < StackSize) {
+
+  unsigned Threshold = MF.getFunction().getParent()->getWarnStackSize();
+  if (StackSize > Threshold) {
     DiagnosticInfoStackSize DiagStackSize(F, StackSize);
     F.getContext().diagnose(DiagStackSize);
   }
   ORE->emit([&]() {
     return MachineOptimizationRemarkAnalysis(DEBUG_TYPE, "StackSize",
                                              MF.getFunction().getSubprogram(),
                                              &MF.front())
            << ore::NV("NumStackBytes", StackSize) << " stack bytes in function";

llvm/lib/IR/Module.cpp

if (auto *CI = mdconst::dyn_extract_or_null<ConstantInt>(MD))
     return CI->getZExtValue();
   return 0;
 }
 
 void Module::setOverrideStackAlignment(unsigned Align) {
   addModuleFlag(ModFlagBehavior::Error, "override-stack-alignment", Align);
 }
 
+unsigned Module::getWarnStackSize() const {
+  Metadata *MD = getModuleFlag("warn-stack-size");
+  if (auto *CI = mdconst::dyn_extract_or_null<ConstantInt>(MD))
+    return CI->getZExtValue();
+  return UINT_MAX;
+}
+
+void Module::setWarnStackSize(unsigned Threshold) {
+  addModuleFlag(ModFlagBehavior::Error, "warn-stack-size", Threshold);
+}
+
 void Module::setSDKVersion(const VersionTuple &V) {
   SmallVector<unsigned, 3> Entries;
   Entries.push_back(V.getMajor());
   if (auto Minor = V.getMinor()) {
     Entries.push_back(*Minor);
     if (auto Subminor = V.getSubminor())
       Entries.push_back(*Subminor);
     // Ignore the 'build' component as it can't be represented in the object

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

-; RUN: llc -mtriple thumbv7-apple-ios3.0.0 -warn-stack-size=80 < %s 2>&1 >/dev/null | FileCheck %s
+; RUN: llc -mtriple thumbv7-apple-ios3.0.0 < %s 2>&1 >/dev/null | FileCheck %s
 ; Check the internal option that warns when the stack size exceeds the
 ; given amount.
 ; <rdar://13987214>
 
 ; CHECK-NOT: nowarn
 define void @nowarn() nounwind ssp "frame-pointer"="all" {
 entry:
   %buffer = alloca [12 x i8], align 1
   %arraydecay = getelementptr inbounds [12 x i8], [12 x i8]* %buffer, i64 0, i64 0
   call void @doit(i8* %arraydecay) nounwind
   ret void
 }
 
 ; CHECK: warning: stack size limit exceeded (92) in warn
 define void @warn() nounwind ssp "frame-pointer"="all" {
 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.module.flags = !{!0}
+!0 = !{i32 1, !"warn-stack-size", i32 80}

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

-; RUN: llc -mtriple x86_64-apple-macosx10.8.0 -warn-stack-size=80 < %s 2>&1 >/dev/null | FileCheck %s
+; RUN: llc -mtriple x86_64-apple-macosx10.8.0 < %s 2>&1 >/dev/null | FileCheck %s
 ; Check the internal option that warns when the stack size exceeds the
 ; given amount.
 ; <rdar://13987214>
 
 ; CHECK-NOT: nowarn
 define void @nowarn() nounwind ssp {
 entry:
   %buffer = alloca [12 x i8], align 1
   %arraydecay = getelementptr inbounds [12 x i8], [12 x i8]* %buffer, i64 0, i64 0
   call void @doit(i8* %arraydecay) nounwind
   ret void
 }
 
 ; CHECK: warning: stack size limit exceeded (88) in warn
 define void @warn() nounwind ssp {
 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.module.flags = !{!0}
+!0 = !{i32 1, !"warn-stack-size", i32 80}

llvm/test/Linker/warn-stack-frame.ll

+; RUN: split-file %s %t
+; RUN: llvm-link %t/main.ll %t/match.ll
+; RUN: not llvm-link %t/main.ll %t/mismatch.ll 2>&1 | \
+; RUN:   FileCheck --check-prefix=CHECK-MISMATCH %s
+
+; CHECK-MISMATCH: error: linking module flags 'warn-stack-size': IDs have conflicting values
+
+;--- main.ll
+!llvm.module.flags = !{!0}
+!0 = !{i32 1, !"warn-stack-size", i32 80}
+;--- match.ll
+!llvm.module.flags = !{!0}
+!0 = !{i32 1, !"warn-stack-size", i32 80}
+;--- mismatch.ll
+!llvm.module.flags = !{!0}
+!0 = !{i32 1, !"warn-stack-size", i32 81}