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

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

In D116589#3220829 <https://reviews.llvm.org/D116589#3220829>, @MaskRay wrote:

> The refined behavior (`__attribute__((no_stack_protector)) ` caller with an always_inline callee) looks good to me

Note that the patch doesn't differentiate between always_inline and regular inline.



================
Comment at: llvm/docs/LangRef.rst:1994
 
-    A function with the ``ssp`` attribute but without the ``alwaysinline``
-    attribute cannot be inlined into a function without a
----------------
MaskRay wrote:
> I think the comments are still useful. It is less about semantic requirement, but about the behavior which users may want to know.
> 
> Upgrading ssp/sspstrong/sspreq level is like an implementation compromise in the absence of instruction-level (fine-grained) ssp as we only have function attributes.
But the old comment is no longer true: an ssp function *can* now be inlined into a non-ssp function, just like any other function, and alwaysinline doesn't make any difference.

Or am I missing something?


================
Comment at: llvm/test/Transforms/Inline/inline_ssp.ll:2
 ; RUN: opt -inline %s -S | FileCheck %s
 ; RUN: opt -passes='cgscc(inline)' %s -S | FileCheck %s
 ; Ensure SSP attributes are propagated correctly when inlining.
----------------
nickdesaulniers wrote:
> I think there's a pass called `always-inline`, too. Should we add that as a run line?
I don't think we'd get any new coverage from that really. The interesting thing to test is the inline case. It's good to have a test case with autoinline too, to make sure they don't diverge in the future, but the inline pass will handle that too.


================
Comment at: llvm/test/Transforms/Inline/inline_ssp.ll:50
 entry:
-; CHECK: @inline_req_req() #0
+; CHECK: @inline_req_req() #[[SSPREQ:[0-9]]]
   call void @fun_sspreq()
----------------
nickdesaulniers wrote:
> converting all of these attribute checks to regexes can also probably be pre-committed.
Okay, will do.


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

https://reviews.llvm.org/D116589