[IR] convert warn-stack-size from module flag to fn attr

[nickdesaulniers]

Summary

Otherwise, this causes issues when building with LTO for object files
that use different values.

Link: https://github.com/ClangBuiltLinux/linux/issues/1395

[dblaikie]

what're the rules about module level attributes like this? For instance: Does this affect/hinder inlining (if the attributes don't match between the caller and callee)? Other situations that might matter?

llvm/lib/CodeGen/PrologEpilogInserter.cpp

282

I guess the 0 value here is the default value if the value can't be parsed as an integer? Is that desirable? I guess maybe we should ignore it (use UINT_MAX here instead, maybe) and fail in the verifier.

But I guess if we fail in the verifier, then it doesn't really matter/shouldn't be tested what the behavior is here when presented with invalid IR.

(but this is a divergence from the module flag handling, which looks like it does silently ignore non-numeric values, by using UINT_MAX)

[nickdesaulniers]

In D104342#2820811, @dblaikie wrote:

what're the rules about module level attributes like this? For instance: Does this affect/hinder inlining (if the attributes don't match between the caller and callee)? Other situations that might matter?

I think you meant s/module level/function level/? That's a good question, one I had to think about a little bit. Here's my thoughts on the behavior this should exhibit, please let me know if you agree.

When using -Wframe-larger-than=<threshold> per TU, the developer wants to be alerted if any stack frame exceeds a threshold. The Linux kernel's use case is that the kernel's stack is limited to (usually) two pages (ulimit -s; typically 8KiB, but different architectures do support non-4KiB page sizes), so functions using more than 1KiB of stack are usually indicative of large objects being stack allocated that should have been heap allocated.

Currently, in C (and C with GNU extensions), there is no way to describe to the compiler function-grain specific values for -Wframe-larger-than=; rather than fine grain per function control, we only have coarse grain TU control.

So in the general case (non-LTO), we can only perform inline substitution at call sites visible from callers' definitions. Because there's no GNU C function attribute to change the current value of -Wframe-larger-than=, it's not possible for that value to differ between caller and callee. But with LTO; shit gets weird.

Suddenly now with LTO, we have cross TU (cross Module) visibility into call sites, so we can inline across TU/Module boundaries. Thus we can have an IR intermediary object with a call site where the caller's value of -Wframe-larger-than= differs from the callees! So the question is what should happen in such a case?

The extremely conservative approach which we have done in the past for certain mismatched function attributes is to simply not perform inline substitution, if we have no other options. This adds overhead to the inliner to check the XOR of attribute lists of the caller and callee for each call site.

But I *think* (and am open to sugguestions) that we should:

1. permit inline substitution
2. the caller's value of "warn-stack-size"= IR Fn Attr wins

I think this is ok because: if caller is defined in TU1 with -Wframe-larger-than= distinct from callee defined in TU2 with a different value of -Wframe-larger-than=, then we don't care what callee's value was. callee may even be DCE'd if it's inlined into a lone call site. I'd expect in such cases that callee's value was larger than caller's, in which case callee should be attributed no_inline for LTO if the tighter threshold for caller now warns. If callee's value was smaller than callers and we performed inline substitution, I think that's also perfectly fine, caller should not become "more strict."

Generally in the Linux kernel, we see a common value of -Wframe-larger-than= throughout most of the TUs, with only a few generally having a larger value to relax constraints a little. (Also, those relaxations are questionable, given the intent of -Wframe-larger-than= use in the kernel in the first place).

Let me add such a test to encode that intention; though I don't know yet what's involved/possible to implement. Let's see.

llvm/lib/CodeGen/PrologEpilogInserter.cpp

282

IIUC, the first parameter to getAsInteger is the Radix, not the default value on failure to parse. But it does return true on error, so I should check that here.

I also should add a verifier check for this new function attribute. While the "string key equals string value" attributes are quite flexible, it would be good to have some rigidity in requiring the string value to be parseable as an unsigned int.

[nickdesaulniers]

llvm/lib/CodeGen/PrologEpilogInserter.cpp

282

Oh, I should use base 10 as the radix, otherwise it will try to parse hex and binary literals.

[dblaikie]

In D104342#2825357, @nickdesaulniers wrote:

In D104342#2820811, @dblaikie wrote:

what're the rules about module level attributes like this? For instance: Does this affect/hinder inlining (if the attributes don't match between the caller and callee)? Other situations that might matter?

I think you meant s/module level/function level/? That's a good question, one I had to think about a little bit. Here's my thoughts on the behavior this should exhibit, please let me know if you agree.

When using -Wframe-larger-than=<threshold> per TU, the developer wants to be alerted if any stack frame exceeds a threshold. The Linux kernel's use case is that the kernel's stack is limited to (usually) two pages (ulimit -s; typically 8KiB, but different architectures do support non-4KiB page sizes), so functions using more than 1KiB of stack are usually indicative of large objects being stack allocated that should have been heap allocated.

Currently, in C (and C with GNU extensions), there is no way to describe to the compiler function-grain specific values for -Wframe-larger-than=; rather than fine grain per function control, we only have coarse grain TU control.

So in the general case (non-LTO), we can only perform inline substitution at call sites visible from callers' definitions. Because there's no GNU C function attribute to change the current value of -Wframe-larger-than=, it's not possible for that value to differ between caller and callee. But with LTO; shit gets weird.

Suddenly now with LTO, we have cross TU (cross Module) visibility into call sites, so we can inline across TU/Module boundaries. Thus we can have an IR intermediary object with a call site where the caller's value of -Wframe-larger-than= differs from the callees! So the question is what should happen in such a case?

The extremely conservative approach which we have done in the past for certain mismatched function attributes is to simply not perform inline substitution, if we have no other options. This adds overhead to the inliner to check the XOR of attribute lists of the caller and callee for each call site.

But I *think* (and am open to sugguestions) that we should:

1. permit inline substitution
2. the caller's value of "warn-stack-size"= IR Fn Attr wins

I think this is ok because: if caller is defined in TU1 with -Wframe-larger-than= distinct from callee defined in TU2 with a different value of -Wframe-larger-than=, then we don't care what callee's value was. callee may even be DCE'd if it's inlined into a lone call site. I'd expect in such cases that callee's value was larger than caller's, in which case callee should be attributed no_inline for LTO if the tighter threshold for caller now warns. If callee's value was smaller than callers and we performed inline substitution, I think that's also perfectly fine, caller should not become "more strict."

Generally in the Linux kernel, we see a common value of -Wframe-larger-than= throughout most of the TUs, with only a few generally having a larger value to relax constraints a little. (Also, those relaxations are questionable, given the intent of -Wframe-larger-than= use in the kernel in the first place).

Let me add such a test to encode that intention; though I don't know yet what's involved/possible to implement. Let's see.

Sure, that all sounds pretty reasonable to me - mostly I was curious what the existing/default behavior is (if we do nothing other than what's already in this patch, how does the inliner handle different values/mismatched presence of warn-stack-size attributes, for instance) - to check that whatever it does seems reasonable/acceptable.

[nickdesaulniers]

• verifier checks, inliner test, use base 10 radix

[dblaikie]

Sounds OK to me.

Another thing you might want to check is linkonce_odr functions - I guess you'll get an arbitrary choice between two linkonce_odr functions under LTO where they have different warn-stack-size? Maybe there's a way/place to merge and always pick the lower or upper value if there's one you think would be more right?

llvm/test/Transforms/Inline/warn-stack-size.ll

1 ↗

(On Diff #352841)

Nice to see the test - though I probably wouldn't bother adding this test if this behavior already falls out of more general support in the inliner and the way it already handles attributes - the general behavior is likely already tested elsewhere? (though it'd be good to confirm that either in tests and/or the inliner code itself)

my original question was to confirm that the inliner already had accounted for this situation in a way that was desirable & it looks like/sounds like it is.

[nickdesaulniers]

• fix lint, add linker test

[nickdesaulniers]

In D104342#2827847, @dblaikie wrote:

Another thing you might want to check is linkonce_odr functions - I guess you'll get an arbitrary choice between two linkonce_odr functions under LTO where they have different warn-stack-size? Maybe there's a way/place to merge and always pick the lower or upper value if there's one you think would be more right?

I've added an llvm-link test for this. I'm not sure it adds any signal though here; I think the answer to such a question is "don't do that."

llvm/test/Transforms/Inline/warn-stack-size.ll

1 ↗

(On Diff #352841)

AttributeFuncs::areInlineCompatible seems to define the disallow-list for mismatched function attributes.

AttributeFuncs::mergeAttributesForInlining() seems to be the merging strategy for certain function attributes.

I agree that this test just confirms that the implicit default merge strategy is used. I guess it would fail if someone unintentionally changed that, but I don't mind removing this test either. WDYT?

[nickdesaulniers]

• git add the Linker test!

[dblaikie]

In D104342#2828193, @nickdesaulniers wrote:

In D104342#2827847, @dblaikie wrote:

Another thing you might want to check is linkonce_odr functions - I guess you'll get an arbitrary choice between two linkonce_odr functions under LTO where they have different warn-stack-size? Maybe there's a way/place to merge and always pick the lower or upper value if there's one you think would be more right?

I've added an llvm-link test for this. I'm not sure it adds any signal though here; I think the answer to such a question is "don't do that."

I don't think it's as easy as "don't do that". Unless someone passes exactly the same flags to every compilation (which they won't, that's why this is being implemented as a function attribute) then it'll be really easy for an inline function in a header (say, std::vector<int>::size - something easy for two unrelated translation units to use) in two different translation units each with a different warn-stack-size flag and so to get somewhat arbitrary behavior about how that function is warned on.

For instance: maybe the function doesn't get a warning because a copy with a higher warn-stack-size value is picked, until the translation unit using that higher value is refactored and starts using std::list instead of std::vector... and now some other TU's std::vector is picked, with a lower warn-stack-size value and breaks the build (assuming -Werror)...

llvm/test/Transforms/Inline/warn-stack-size.ll

1 ↗

(On Diff #352841)

generally I wouldn't add a test like this, or the LTO one - I'd just confirm that their features are separately tested and the behavior that's desired for how I want to use the feature (eg: I wouldn't test that the add instruction lowers to some machine code in my optimization - I'd confirm the add instruction has the desired semantics for whatever transformation I want to perform)

[MaskRay]

Agree that a function attribute is more appropriate considering the LTO behavior.

llvm/docs/LangRef.rst

2052

In for once, is for redundant?

2054

Abbreviated as a non-negative integer

[MaskRay]

module attr

module flag metadata (or module flag) is more appropriate.

[nickdesaulniers]

• fix langref, delete inline and linker tests

[nickdesaulniers]

In D104342#2828388, @dblaikie wrote:

In D104342#2828193, @nickdesaulniers wrote:

In D104342#2827847, @dblaikie wrote:

Another thing you might want to check is linkonce_odr functions - I guess you'll get an arbitrary choice between two linkonce_odr functions under LTO where they have different warn-stack-size? Maybe there's a way/place to merge and always pick the lower or upper value if there's one you think would be more right?

I've added an llvm-link test for this. I'm not sure it adds any signal though here; I think the answer to such a question is "don't do that."

I don't think it's as easy as "don't do that". Unless someone passes exactly the same flags to every compilation (which they won't, that's why this is being implemented as a function attribute) then it'll be really easy for an inline function in a header (say, std::vector<int>::size - something easy for two unrelated translation units to use) in two different translation units each with a different warn-stack-size flag and so to get somewhat arbitrary behavior about how that function is warned on.

Ah, ok that's which language construct can produce linkonce_odr. Fair point.

For instance: maybe the function doesn't get a warning because a copy with a higher warn-stack-size value is picked, until the translation unit using that higher value is refactored and starts using std::list instead of std::vector... and now some other TU's std::vector is picked, with a lower warn-stack-size value and breaks the build (assuming -Werror)...

This seems like a general problem perhaps with linking IR? Perhaps IRLinker::copyFunctionProto or IRLinker::mapAttributeTypes should try to do something here, though I'm not sure yet which policy would be preferred?

[nickdesaulniers]

As a heads up, this will conflict with D104667.

[dblaikie]

In D104342#2831703, @nickdesaulniers wrote:

In D104342#2828388, @dblaikie wrote:

In D104342#2828193, @nickdesaulniers wrote:

In D104342#2827847, @dblaikie wrote:

Another thing you might want to check is linkonce_odr functions - I guess you'll get an arbitrary choice between two linkonce_odr functions under LTO where they have different warn-stack-size? Maybe there's a way/place to merge and always pick the lower or upper value if there's one you think would be more right?

I've added an llvm-link test for this. I'm not sure it adds any signal though here; I think the answer to such a question is "don't do that."

I don't think it's as easy as "don't do that". Unless someone passes exactly the same flags to every compilation (which they won't, that's why this is being implemented as a function attribute) then it'll be really easy for an inline function in a header (say, std::vector<int>::size - something easy for two unrelated translation units to use) in two different translation units each with a different warn-stack-size flag and so to get somewhat arbitrary behavior about how that function is warned on.

Ah, ok that's which language construct can produce linkonce_odr. Fair point.

For instance: maybe the function doesn't get a warning because a copy with a higher warn-stack-size value is picked, until the translation unit using that higher value is refactored and starts using std::list instead of std::vector... and now some other TU's std::vector is picked, with a lower warn-stack-size value and breaks the build (assuming -Werror)...

This seems like a general problem perhaps with linking IR? Perhaps IRLinker::copyFunctionProto or IRLinker::mapAttributeTypes should try to do something here, though I'm not sure yet which policy would be preferred?

I don't know that there's a good answer (in more extreme cases - like different optimization levels or CPU features, at least at Google our answer has ended up being "compile the whole program with the right CPU features, because there's no great way to support good optimizations while respecting CPU features on a per-function basis"), basically - so this was more a "heads up, this is going to be possibly unavoidably messy/unreliable on the edges".

Probably worth at least writing up the risk/instability in the docs for the warning (in clang) and attribute (in llvm). (don't mind if that's in this patch or a follow-up).

(why I think there's no solution to this: any rule (highest wins, lowest wins, mismatch fails to compile) will create surprising/problematic effects, eg: you have one TU with the function in it and some value for the attribute - a new TU could introduce a copy of the function with a higher or lower value - and whichever choice of policy would then cause problems for one case or the other case. (either enforcing a stronger warning level on code that didn't ask for it, or slackening the warning level for code that thought it was protected by the warning))

[nickdesaulniers]

In D104342#2831717, @dblaikie wrote:

I don't know that there's a good answer (in more extreme cases - like different optimization levels or CPU features, at least at Google our answer has ended up being "compile the whole program with the right CPU features, because there's no great way to support good optimizations while respecting CPU features on a per-function basis"), basically - so this was more a "heads up, this is going to be possibly unavoidably messy/unreliable on the edges".

Probably worth at least writing up the risk/instability in the docs for the warning (in clang) and attribute (in llvm). (don't mind if that's in this patch or a follow-up).

Sure. Let me land this, since we (Google & ClangBuiltLinux) have some tests and builds failing due to https://reviews.llvm.org/D103928. I will then send a follow up for us to iterate on in regards to documenting this more.

(why I think there's no solution to this: any rule (highest wins, lowest wins, mismatch fails to compile) will create surprising/problematic effects, eg: you have one TU with the function in it and some value for the attribute - a new TU could introduce a copy of the function with a higher or lower value - and whichever choice of policy would then cause problems for one case or the other case. (either enforcing a stronger warning level on code that didn't ask for it, or slackening the warning level for code that thought it was protected by the warning))

I agree. I don't think even having a function attribute in C for -Wframe-larger-than would resolve such policy issues either.

[dblaikie]

In D104342#2831733, @nickdesaulniers wrote:

In D104342#2831717, @dblaikie wrote:

I don't know that there's a good answer (in more extreme cases - like different optimization levels or CPU features, at least at Google our answer has ended up being "compile the whole program with the right CPU features, because there's no great way to support good optimizations while respecting CPU features on a per-function basis"), basically - so this was more a "heads up, this is going to be possibly unavoidably messy/unreliable on the edges".

Probably worth at least writing up the risk/instability in the docs for the warning (in clang) and attribute (in llvm). (don't mind if that's in this patch or a follow-up).

Sure. Let me land this, since we (Google & ClangBuiltLinux) have some tests and builds failing due to https://reviews.llvm.org/D103928. I will then send a follow up for us to iterate on in regards to documenting this more.

(why I think there's no solution to this: any rule (highest wins, lowest wins, mismatch fails to compile) will create surprising/problematic effects, eg: you have one TU with the function in it and some value for the attribute - a new TU could introduce a copy of the function with a higher or lower value - and whichever choice of policy would then cause problems for one case or the other case. (either enforcing a stronger warning level on code that didn't ask for it, or slackening the warning level for code that thought it was protected by the warning))

I agree. I don't think even having a function attribute in C for -Wframe-larger-than would resolve such policy issues either.

At least then we could probably say it's an ODR violation (the two function definitions would be not the same if the user wrote the attribute differently for two definitions of the inline function in two different translation units) to have the function declared with different values for the attribute within the same program (so you could still compile two different files (that include a common header with a common function with the attribute specified there) with different values for the command line flag - because the function would get a consistent attribute value for the warning) - and then the linker could actually reject it on mismatch. But with the attribute currently coming from the command line, that's not feasible.

[nickdesaulniers]

In D104342#2831738, @dblaikie wrote:

In D104342#2831717, @dblaikie wrote:

Probably worth at least writing up the risk/instability in the docs for the warning (in clang) and attribute (in llvm). (don't mind if that's in this patch or a follow-up).

I would think https://clang.llvm.org/docs/DiagnosticsReference.html#wframe-larger-than would be an appropriate place to document this for -Wframe-larger-than=, but it seems this whole page is generated via TableGen. It's not clear to me how we could insert such a note.

Langref changes: https://reviews.llvm.org/D104736.

At least then we could probably say it's an ODR violation (the two function definitions would be not the same if the user wrote the attribute differently for two definitions of the inline function in two different translation units) to have the function declared with different values for the attribute within the same program (so you could still compile two different files (that include a common header with a common function with the attribute specified there) with different values for the command line flag - because the function would get a consistent attribute value for the warning) - and then the linker could actually reject it on mismatch. But with the attribute currently coming from the command line, that's not feasible.

[dblaikie]

In D104342#2834119, @nickdesaulniers wrote:

In D104342#2831738, @dblaikie wrote:

In D104342#2831717, @dblaikie wrote:

Probably worth at least writing up the risk/instability in the docs for the warning (in clang) and attribute (in llvm). (don't mind if that's in this patch or a follow-up).

I would think https://clang.llvm.org/docs/DiagnosticsReference.html#wframe-larger-than would be an appropriate place to document this for -Wframe-larger-than=, but it seems this whole page is generated via TableGen. It's not clear to me how we could insert such a note.

Yeah, I don't think we have a way to add more verbose/custom documentation for diagnostics. (@aaron.ballman might have some ideas)

Langref changes: https://reviews.llvm.org/D104736.

Thanks!

[aaron.ballman]

In D104342#2834240, @dblaikie wrote:

In D104342#2834119, @nickdesaulniers wrote:

I would think https://clang.llvm.org/docs/DiagnosticsReference.html#wframe-larger-than would be an appropriate place to document this for -Wframe-larger-than=, but it seems this whole page is generated via TableGen. It's not clear to me how we could insert such a note.

Yeah, I don't think we have a way to add more verbose/custom documentation for diagnostics. (@aaron.ballman might have some ideas)

I'm not aware of any way to do that today -- it would require more machinery for generating the diagnostic documentation, which is made harder by this particular diagnostic being a backend one that's not written out from tablegen.

[dblaikie]

In D104342#2836222, @aaron.ballman wrote:

In D104342#2834240, @dblaikie wrote:

In D104342#2834119, @nickdesaulniers wrote:

I would think https://clang.llvm.org/docs/DiagnosticsReference.html#wframe-larger-than would be an appropriate place to document this for -Wframe-larger-than=, but it seems this whole page is generated via TableGen. It's not clear to me how we could insert such a note.

Yeah, I don't think we have a way to add more verbose/custom documentation for diagnostics. (@aaron.ballman might have some ideas)

I'm not aware of any way to do that today -- it would require more machinery for generating the diagnostic documentation, which is made harder by this particular diagnostic being a backend one that's not written out from tablegen.

Fair enough - thanks for the context!

clang/lib/CodeGen/CodeGenFunction.cpp

#undef SANITIZER
   if (CGM.getCodeGenOpts().PackedStack) {
     if (getContext().getTargetInfo().getTriple().getArch() !=
         llvm::Triple::systemz)
       CGM.getDiags().Report(diag::err_opt_not_valid_on_target)
         << "-mpacked-stack";
     Fn->addFnAttr("packed-stack");
   }
 
+  if (CGM.getCodeGenOpts().WarnStackSize != UINT_MAX)
+    Fn->addFnAttr("warn-stack-size",
+                  std::to_string(CGM.getCodeGenOpts().WarnStackSize));
+
   if (RetTy->isVoidType()) {
     // Void type; nothing to return.
     ReturnValue = Address::invalid();
 
     // Count the implicit return.
     if (!endsWithReturn(D))
       ++NumReturnExprs;
   } else if (CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::Indirect) {

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/test/Frontend/fwarn-stack-size.c

+// RUN: %clang_cc1 -fwarn-stack-size=42 -emit-llvm -o - %s | FileCheck %s
+void foo(void) {}
+// CHECK: define {{.*}} @foo() [[ATTR:#[0-9]+]] {
+// CHECK: attributes [[ATTR]] = {{.*}} "warn-stack-size"="42"

llvm/docs/LangRef.rst

``mustprogress``
     attribute is intended to model the requirements of the first section of
     [intro.progress] of the C++ Standard. As a consequence, a loop in a
     function with the `mustprogress` attribute can be assumed to terminate if
     it does not interact with the environment in an observable way, and
     terminating loops without side-effects can be removed. If a `mustprogress`
     function does not satisfy this contract, the behavior is undefined.  This
     attribute does not apply transitively to callees, but does apply to call
     sites within the function. Note that `willreturn` implies `mustprogress`.
+``"warn-stack-size"="<threshold>"``
+    This attribute sets a threshold to emit diagnostics once the frame size is
+    known should the frame size exceed the specified value.  It takes one
+    required integer value, which should be a non-negative integer, and less
+    than `UINT_MAX`.
 ``vscale_range(<min>[, <max>])``
     This attribute indicates the minimum and maximum vscale value for the given
     function. A value of 0 means unbounded. If the optional max value is omitted
     then max is set to the value of min. If the attribute is not present, no
     assumptions are made about the range of vscale.
 
 Call Site Attributes
 ----------------------

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

bool PEI::runOnMachineFunction(MachineFunction &MF) {
   // 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();
 
-  unsigned Threshold = MF.getFunction().getParent()->getWarnStackSize();
+  unsigned Threshold = UINT_MAX;
+  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 (StackSize > Threshold) {
     DiagnosticInfoStackSize DiagStackSize(F, StackSize);
     F.getContext().diagnose(DiagStackSize);
   }
   ORE->emit([&]() {
     return MachineOptimizationRemarkAnalysis(DEBUG_TYPE, "StackSize",
                                              MF.getFunction().getSubprogram(),
                                              &MF.front())

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/lib/IR/Verifier.cpp

#include "llvm/IR/Metadata.def"
   void verifySwiftErrorCall(CallBase &Call, const Value *SwiftErrorVal);
   void verifySwiftErrorValue(const Value *SwiftErrorVal);
   void verifyTailCCMustTailAttrs(AttrBuilder Attrs, StringRef Context);
   void verifyMustTailCall(CallInst &CI);
   bool verifyAttributeCount(AttributeList Attrs, unsigned Params);
   void verifyAttributeTypes(AttributeSet Attrs, bool IsFunction,
                             const Value *V);
   void verifyParameterAttrs(AttributeSet Attrs, Type *Ty, const Value *V);
+  void checkUnsignedBaseTenFuncAttr(AttributeList Attrs, StringRef Attr,
+                                    const Value *V);
   void verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
                            const Value *V, bool IsIntrinsic);
   void verifyFunctionMetadata(ArrayRef<std::pair<unsigned, MDNode *>> MDs);
 
   void visitConstantExprsRecursively(const Constant *EntryC);
   void visitConstantExpr(const ConstantExpr *CE);
   void verifyStatepoint(const CallBase &Call);
   void verifyFrameRecoverIndices();
Assert(!Attrs.hasAttribute(Attribute::ByRef),
            V);
     Assert(!Attrs.hasAttribute(Attribute::SwiftError),
            "Attribute 'swifterror' only applies to parameters "
            "with pointer type!",
            V);
   }
 }
 
+void Verifier::checkUnsignedBaseTenFuncAttr(AttributeList Attrs, StringRef Attr,
+                                            const Value *V) {
+  if (Attrs.hasFnAttribute(Attr)) {
+    StringRef S = Attrs.getAttribute(AttributeList::FunctionIndex, Attr)
+                      .getValueAsString();
+    unsigned N;
+    if (S.getAsInteger(10, N))
+      CheckFailed("\"" + Attr + "\" takes an unsigned integer: " + S, V);
+  }
+}
+
 // Check parameter attributes against a function type.
 // The value V is printed in error messages.
 void Verifier::verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
                                    const Value *V, bool IsIntrinsic) {
   if (Attrs.isEmpty())
     return;
 
   if (AttributeListsVisited.insert(Attrs.getRawPointer()).second) {
void Verifier::verifyFunctionAttrs(FunctionType *FT, AttributeList Attrs,
 
   if (Attrs.hasFnAttribute("frame-pointer")) {
     StringRef FP = Attrs.getAttribute(AttributeList::FunctionIndex,
                                       "frame-pointer").getValueAsString();
     if (FP != "all" && FP != "non-leaf" && FP != "none")
       CheckFailed("invalid value for 'frame-pointer' attribute: " + FP, V);
   }
 
-  if (Attrs.hasFnAttribute("patchable-function-prefix")) {
-    StringRef S = Attrs
-                      .getAttribute(AttributeList::FunctionIndex,
-                                    "patchable-function-prefix")
-                      .getValueAsString();
-    unsigned N;
-    if (S.getAsInteger(10, N))
-      CheckFailed(
-          "\"patchable-function-prefix\" takes an unsigned integer: " + S, V);
-  }
-  if (Attrs.hasFnAttribute("patchable-function-entry")) {
-    StringRef S = Attrs
-                      .getAttribute(AttributeList::FunctionIndex,
-                                    "patchable-function-entry")
-                      .getValueAsString();
-    unsigned N;
-    if (S.getAsInteger(10, N))
-      CheckFailed(
-          "\"patchable-function-entry\" takes an unsigned integer: " + S, V);
-  }
+  checkUnsignedBaseTenFuncAttr(Attrs, "patchable-function-prefix", V);
+  checkUnsignedBaseTenFuncAttr(Attrs, "patchable-function-entry", V);
+  checkUnsignedBaseTenFuncAttr(Attrs, "warn-stack-size", V);
 }
 
 void Verifier::verifyFunctionMetadata(
     ArrayRef<std::pair<unsigned, MDNode *>> MDs) {
   for (const auto &Pair : MDs) {
     if (Pair.first == LLVMContext::MD_prof) {
       MDNode *MD = Pair.second;
       Assert(MD->getNumOperands() >= 2,

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

 ; 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" {
+define void @nowarn() nounwind ssp "frame-pointer"="all" "warn-stack-size"="80" {
 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" {
+define void @warn() nounwind ssp "frame-pointer"="all" "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.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 < %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 {
+define void @nowarn() nounwind ssp "warn-stack-size"="80" {
 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 {
+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
 }
 
 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}

llvm/test/Verifier/invalid-warn-stack-size.ll

+; RUN: not opt -passes=verify %s -disable-output 2>&1 | FileCheck %s
+define void @foo() "warn-stack-size"="42" { ret void }
+define void @bar() "warn-stack-size"="-1" { ret void }
+define void @baz() "warn-stack-size"="999999999999999999999" { ret void }
+define void @qux() "warn-stack-size"="a lot lol" { ret void }
+
+; CHECK-NOT: "warn-stack-size" takes an unsigned integer: 42
+; CHECK: "warn-stack-size" takes an unsigned integer: -1
+; CHECK: "warn-stack-size" takes an unsigned integer: 999999999999999999999
+; CHECK: "warn-stack-size" takes an unsigned integer: a lot lol