[PATCH] D116589: Don't override __attribute__((no_stack_protector)) by inlining

[Hans Wennborg via Phabricator via llvm-commits]

hans created this revision.
hans added reviewers: nickdesaulniers, manojgupta, MaskRay, rnk, void.
Herald added subscribers: ormris, ChuanqiXu, dexonsmith, jdoerfert, steven_wu, haicheng, hiraditya, eraman.
hans requested review of this revision.
Herald added a project: LLVM.

Since 26c6a3e736d3, LLVM's inliner will "upgrade" the caller's stack protector attribute based on the callee. This leads to surprising results with Clang's no_stack_protector attribute added in 4fbf84c1732f (D46300 <https://reviews.llvm.org/D46300>). Consider the following code compiled with clang -fstack-protector-strong -Os (https://godbolt.org/z/7s3rW7a1q).

  extern void h(int* p);
  
  inline __attribute__((always_inline)) int g() {
    return 0;
  } 
  
  int __attribute__((__no_stack_protector__)) f() {
    int a[1];
    h(a);
    return g();
  }

LLVM will inline g() into f(), and f() will get a stack protector, against the users explicit wishes, potentially breaking the program e.g. if h() changes the value of the stack cookie. That's a miscompile.

More recently, bc044a88ee3c <https://reviews.llvm.org/rGbc044a88ee3c16e4164740732a7adc91a29b24f5> (D91816 <https://reviews.llvm.org/D91816>) addressed this problem by preventing inlining when the stack protector is disabled in the caller and enabled in the callee or vice versa. However, the problem remains if the callee is marked always_inline as in the example above. This is affecting users, see e.g. http://crbug.com/1274129 and http://llvm.org/pr52886.

One way to fix this would be to prevent inlining also in the always_inline case. Despite the name, always_inline does not guarantee inlining, so this would be legal but potentially surprising to users.

However, I think the better fix is to not enable the stack protector in a caller based on the callee. The motivation for the old behaviour is unclear, it seems counter-intuitive, and causes real problems as we've seen.

This patch implements that fix, which means in the example above, g() gets inlined into f() (also without always_inline), and f() is emitted without stack protector. I think that matches most developers' expectations, and that's also what GCC does.

Another effect of this change is that a no_stack_protector function can now be inlined into a stack protected function, e.g. (https://godbolt.org/z/hafP6W856)

  extern void h(int* p);
  
  inline int __attribute__((__no_stack_protector__)) __attribute__((always_inline)) g() {
    return 0;
  }
  
  int f() {
    int a[1];
    h(a);
    return g();
  }

I think that's fine. Such code would be unusual since no_stack_protector is normally applied to a program entry point which sets up the stack canary. And even if such code exists, inlining doesn't change the semantics: there is still no stack cookie setup/check around entry/exit of the g() code region, but there may be in the surrounding context, as there was before inlining. This also matches GCC.

Please let me know what you think.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116589

Files:
  llvm/docs/LangRef.rst
  llvm/lib/Analysis/InlineCost.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/test/ThinLTO/X86/nossp.ll
  llvm/test/Transforms/Inline/inline_nossp.ll
  llvm/test/Transforms/Inline/inline_ssp.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D116589.397246.patch
Type: text/x-patch
Size: 12861 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220104/1f88ee50/attachment.bin>