[PATCH] Implement the no_split_stack attribute.

[Aaron Ballman]

On Sat, May 17, 2014 at 11:16 AM, Alp Toker <alp at nuanti.com> wrote:
>
> On 17/05/2014 16:40, Aaron Ballman wrote:
>>
>> On Sat, May 17, 2014 at 2:23 AM, Peter Collingbourne <peter at pcc.me.uk>
>> wrote:
>>>
>>> This is a GNU attribute that allows split stacks to be turned off on a
>>> per-function basis.
>>>
>>> http://reviews.llvm.org/D3817
>>>
>>> Files:
>>>    include/clang/Basic/Attr.td
>>>    lib/CodeGen/CGCall.cpp
>>>    lib/Sema/SemaDeclAttr.cpp
>>>    test/CodeGen/split-stacks.c
>>>
>>> Index: include/clang/Basic/Attr.td
>>> ===================================================================
>>> --- include/clang/Basic/Attr.td
>>> +++ include/clang/Basic/Attr.td
>>> @@ -823,6 +823,12 @@
>>>     let Documentation = [Undocumented];
>>>   }
>>>
>>> +def NoSplitStack : InheritableAttr {
>>> +  let Spellings = [GNU<"no_split_stack">];
>>
>> Should this be GCC instead of GNU? (so it can be spelled
>> [[gnu::no_split_stack]] as well.)
>>
>>> +  let Subjects = SubjectList<[Function]>;
>>
>> Should it also apply to templated functions? ObjC methods?
>>
>>> +  let Documentation = [Undocumented];
>>
>> Please add documentation for this attribute.
>>
>>> +}
>>> +
>>>   def NonNull : InheritableAttr {
>>>     let Spellings = [GCC<"nonnull">];
>>>     let Subjects = SubjectList<[ObjCMethod, HasFunctionProto, ParmVar],
>>> WarnDiag,
>>> Index: lib/CodeGen/CGCall.cpp
>>> ===================================================================
>>> --- lib/CodeGen/CGCall.cpp
>>> +++ lib/CodeGen/CGCall.cpp
>>> @@ -1117,7 +1117,8 @@
>>>       FuncAttrs.addAttribute(llvm::Attribute::NoRedZone);
>>>     if (CodeGenOpts.NoImplicitFloat)
>>>       FuncAttrs.addAttribute(llvm::Attribute::NoImplicitFloat);
>>> -  if (CodeGenOpts.EnableSegmentedStacks)
>>> +  if (CodeGenOpts.EnableSegmentedStacks &&
>>> +      !(TargetDecl && TargetDecl->hasAttr<NoSplitStackAttr>()))
>>>       FuncAttrs.addAttribute("split-stack");
>>>
>>>     if (AttrOnCallSite) {
>>> Index: lib/Sema/SemaDeclAttr.cpp
>>> ===================================================================
>>> --- lib/Sema/SemaDeclAttr.cpp
>>> +++ lib/Sema/SemaDeclAttr.cpp
>>> @@ -4173,6 +4173,9 @@
>>>     case AttributeList::AT_NoCommon:
>>>       handleSimpleAttribute<NoCommonAttr>(S, D, Attr);
>>>       break;
>>> +  case AttributeList::AT_NoSplitStack:
>>> +    handleSimpleAttribute<NoSplitStackAttr>(S, D, Attr);
>>> +    break;
>>>     case AttributeList::AT_NonNull:
>>>       if (ParmVarDecl *PVD = dyn_cast<ParmVarDecl>(D))
>>>         handleNonNullAttrParameter(S, PVD, Attr);
>>> Index: test/CodeGen/split-stacks.c
>>> ===================================================================
>>> --- test/CodeGen/split-stacks.c
>>> +++ test/CodeGen/split-stacks.c
>>> @@ -5,13 +5,21 @@
>>>     return 0;
>>>   }
>>>
>>> +__attribute__((no_split_stack))
>>> +int nosplit() {
>>> +  return 0;
>>> +}
>>> +
>>>   int main() {
>>>     return foo();
>>>   }
>>>
>>> -// CHECK-SEGSTK: define i32 @foo() #0 {
>>> -// CHECK-SEGSTK: define i32 @main() #0 {
>>> -// CHECK-SEGSTK: #0 = { {{.*}} "split-stack" {{.*}} }
>>> +// CHECK-SEGSTK: define i32 @foo() [[SS:#[0-9]+]] {
>>> +// CHECK-SEGSTK: define i32 @nosplit() [[NSS:#[0-9]+]] {
>>> +// CHECK-SEGSTK: define i32 @main() [[SS]] {
>>> +// CHECK-SEGSTK-NOT: [[NSS]] = { {{.*}} "split-stack" {{.*}} }
>>> +// CHECK-SEGSTK: [[SS]] = { {{.*}} "split-stack" {{.*}} }
>>> +// CHECK-SEGSTK-NOT: [[NSS]] = { {{.*}} "split-stack" {{.*}} }
>>>
>>>   // CHECK-NOSEGSTK: define i32 @foo() #0 {
>>>   // CHECK-NOSEGSTK: define i32 @main() #0 {
>>
>> Missing sema tests for the attribute (that it applies only to
>> functions, takes no arguments, etc).
>
>
> Actually I like how Peter's kept the tests concise. It seems reasonable to
> trust the attribute machinery for subject/range checking, similar to the way
> we only test the semantics of type trait expressions, compiler builtins etc.
> where there's existing coverage for standard handling.

It's insufficient and we've had several regressions in the past from
that sort of thing. If the attribute accepts no arguments, you need a
test ensuring that. If the attribute appertains to a particular
subject, you need a test ensuring that. (And so on.)

~Aaron