[Clang][AArch64] Limit arm_locally_streaming to function definitions only.

[sdesmalen]

Summary

This requires forcibly toggling the 'willHaveBody' value of the Decl that's
being built up before/after handling the attributes in order to query
this property on the Decl.

This also updates a WASM test that seemed incorrect (the attribute is
only allowed on function declarations, not definitions).

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

9835–9839

This seems like a hack to work around what feels like a bug -- if the declarator knows the function is a definition, then why does the FunctionDecl AST node claim the function won't have a body? It seems strange to me that we don't set that bit when acting on the function declarator but instead wait until we're acting on the start of the function definition to set it; does anything break if you start setting that flag earlier?

[sdesmalen]

clang/lib/Sema/SemaDecl.cpp

9835–9839

This seems like a hack to work around what feels like a bug -- if the declarator knows the function is a definition, then why does the FunctionDecl AST node claim the function won't have a body?

I agree it is a bit of a workaround, I wasn't sure if it was a bug or by design, but there are several places in the codebase that are invoked after this point that seem to depend on the fact that willHaveBody has not been set to determine if something is a redeclaration.

For example, setting NewFD->setWillHaveBody() to true if D.isFunctionDefinition(), causes the following test to fail:

template <class T1, class T2>
struct pair {
  T1 first;
  T2 second;

  pair() : first(), second() {}
  pair(const T1 &a, const T2 &b) : first(a), second(b) {}

  template<class U1, class U2>
  pair(const pair<U1, U2> &other) : first(other.first),
                                    second(other.second) {}
};

I did some digging to see if I could fix this some other way, but didn't spot any easy ways to do this.

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

9835–9839

I did some digging to see if I could fix this some other way, but didn't spot any easy ways to do this.

Yeah, it may require more involved changes, but I'm not super comfortable extending our technical debt with this bit unless there's really no other reasonable way to do it.

We don't have any attributes that behave the way you're proposing this one behaves. Most often, attributes can be written on any declaration and are inherited by subsequent declarations. Every once in a while we have an attribute that can be written only on a declaration, but none that require a function definition (some require a record definition though). So I'm not too surprised this is an edge case we've not hit before.

Why is this limited to just the definition and not allowed on a declaration?

clang/test/AST/ast-dump-wasm-attr-export.c

13–15

This is either hiding a bug or materially changing the test.

1. It's hiding a bug because those functions are supposed to be diagnosed and that's not happening: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDeclAttr.cpp#L7313

2. If the checking code in SemaDeclAttr.cpp is actually unintentional for some reason, then this materially changes the test coverage to demonstrate that the attribute is not lost by a subsequent redeclaration without the attribute (so when later code looks up a decl, it still sees the attribute).

I think it's most likely hiding a bug though.

[sdesmalen]

This patch can be abandoned, since this is now properly implemented in D127762 as a declaration attribute.

clang/include/clang/Basic/Attr.td

 // attribute subject which accepts a Decl. Normally, this is not a problem,
 // because the attribute can have no Subjects clause to accomplish this. But in
 // the case of a SubsetSubject, there's no way to express it without this hack.
 def DeclBase : AttrSubject;
 def FunctionLike : SubsetSubject<DeclBase,
                                  [{S->getFunctionType(false) != nullptr}],
                                  "functions, function pointers">;
 
+def FunctionDef : SubsetSubject<Function, [{S->isThisDeclarationADefinition()}],
+                                "function definitions">;
+
 def OpenCLKernelFunction
     : SubsetSubject<Function, [{S->hasAttr<OpenCLKernelAttr>()}],
                     "kernel functions">;
 
 // HasFunctionProto is a more strict version of FunctionLike, so it should
 // never be specified in a Subjects list along with FunctionLike (due to the
 // inclusive nature of subject testing).
 def HasFunctionProto : SubsetSubject<DeclBase,

 def ArmStreaming : DeclOrTypeAttr, TargetSpecificAttr<TargetAArch64SME> {
   let Spellings = [Clang<"arm_streaming">];
   let Subjects = SubjectList<[HasFunctionProto], ErrorDiag>;
   let Documentation = [ArmSmeStreamingDocs];
 }
 
 def ArmLocallyStreaming : DeclOrTypeAttr, TargetSpecificAttr<TargetAArch64SME> {
   let Spellings = [Clang<"arm_locally_streaming">];
-  let Subjects = SubjectList<[Function], ErrorDiag>;
+  let Subjects = SubjectList<[FunctionDef], ErrorDiag>;
   let Documentation = [ArmSmeLocallyStreamingDocs];
 }
 
 def ArmSharedZA : DeclOrTypeAttr, TargetSpecificAttr<TargetAArch64SME> {
   let Spellings = [Clang<"arm_shared_za">];
   let Subjects = SubjectList<[HasFunctionProto], ErrorDiag>;
   let Documentation = [ArmSmeSharedZADocs];
 }

clang/lib/Sema/SemaDecl.cpp

Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
   // apply an implicit SectionAttr from #pragma code_seg if active.
   if (!NewFD->hasAttr<CodeSegAttr>()) {
     if (Attr *SAttr = getImplicitCodeSegOrSectionAttrForFunction(NewFD,
                                                                  D.isFunctionDefinition())) {
       NewFD->addAttr(SAttr);
     }
   }
 
-  // Handle attributes.
+  // Because not all of the declaration may have been parsed at the point of
+  // calling this function, we forcably toggle 'WillHaveBody' if we're parsing
+  // a function definition so that the code handling the attributes can check
+  // whether the attribute is applied on the correct construct. We also reset
+  // the 'WillHaveBody' value, because other places in Clang assume this hasn't
+  // already been set.
+  if (D.isFunctionDefinition()) {
+    NewFD->setWillHaveBody();
+    ProcessDeclAttributes(S, NewFD, D);
+    NewFD->setWillHaveBody(false);
+  } else
     ProcessDeclAttributes(S, NewFD, D);
 
   if (getLangOpts().OpenCL) {
     // OpenCL v1.1 s6.5: Using an address space qualifier in a function return
     // type declaration will generate a compilation error.
     LangAS AddressSpace = NewFD->getReturnType().getAddressSpace();
     if (AddressSpace != LangAS::Default) {
       Diag(NewFD->getLocation(),
            diag::err_opencl_return_value_with_address_space);

clang/test/AST/ast-dump-wasm-attr-export.c

 // Test without serialization:
 // RUN: %clang_cc1 -triple wasm32-unknown-unknown -ast-dump %s \
 // RUN: | FileCheck --strict-whitespace %s
 //
 // Test with serialization:
 // RUN: %clang_cc1 -triple wasm32-unknown-unknown -emit-pch -o %t %s
 // RUN: %clang_cc1 -triple wasm32-unknown-unknown -x c -include-pch %t -ast-dump-all /dev/null \
 // RUN: | sed -e "s/ <undeserialized declarations>//" -e "s/ imported//" \
 // RUN: | FileCheck --strict-whitespace %s
 
 // Test that functions can be redeclared and they retain their attributes.
 
-__attribute__((export_name("export_red"))) void red(void) {}
-__attribute__((export_name("export_orange"))) void orange(void) {}
-__attribute__((export_name("export_yellow"))) void yellow(void) {}
+__attribute__((export_name("export_red"))) void red(void);
+__attribute__((export_name("export_orange"))) void orange(void);
+__attribute__((export_name("export_yellow"))) void yellow(void);
 
 void red(void);
 void orange(void);
 void yellow(void);
 
 // CHECK: |-FunctionDecl {{.+}} used red 'void (void)'
-// CHECK: | |-CompoundStmt {{.+}}
 // CHECK: | |-WebAssemblyExportNameAttr {{.+}} "export_red"
 // CHECK: | `-UsedAttr {{.+}} Implicit
 // CHECK: |-FunctionDecl {{.+}} used orange 'void (void)'
-// CHECK: | |-CompoundStmt {{.+}}
 // CHECK: | |-WebAssemblyExportNameAttr {{.+}} "export_orange"
 // CHECK: | `-UsedAttr {{.+}} Implicit
 // CHECK: |-FunctionDecl {{.+}} used yellow 'void (void)'
-// CHECK: | |-CompoundStmt {{.+}}
 // CHECK: | |-WebAssemblyExportNameAttr {{.+}} "export_yellow"
 // CHECK: | `-UsedAttr {{.+}} Implicit
 // CHECK: |-FunctionDecl {{.+}} used red 'void (void)'
 // CHECK: | |-UsedAttr {{.+}} Inherited Implicit
 // CHECK: | `-WebAssemblyExportNameAttr {{.+}} Inherited "export_red"
 // CHECK: |-FunctionDecl {{.+}} used orange 'void (void)'
 // CHECK: | |-UsedAttr {{.+}} Inherited Implicit
 // CHECK: | `-WebAssemblyExportNameAttr {{.+}} Inherited "export_orange"
 // CHECK: `-FunctionDecl {{.+}} used yellow 'void (void)'
 // CHECK:   |-UsedAttr {{.+}} Inherited Implicit
 // CHECK:     `-WebAssemblyExportNameAttr {{.+}} Inherited "export_yellow"

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: Alias (SubjectMatchRule_function, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: AlignValue (SubjectMatchRule_variable, SubjectMatchRule_type_alias)
 // CHECK-NEXT: AlwaysDestroy (SubjectMatchRule_variable)
 // CHECK-NEXT: AlwaysInline (SubjectMatchRule_function)
 // CHECK-NEXT: Annotate ()
 // CHECK-NEXT: AnyX86NoCfCheck (SubjectMatchRule_hasType_functionType)
 // CHECK-NEXT: ArcWeakrefUnavailable (SubjectMatchRule_objc_interface)
 // CHECK-NEXT: ArmBuiltinAlias (SubjectMatchRule_function)
-// CHECK-NEXT: ArmLocallyStreaming (SubjectMatchRule_function)
 // CHECK-NEXT: AssumeAligned (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: Assumption (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: Availability ((SubjectMatchRule_record, SubjectMatchRule_enum, SubjectMatchRule_enum_constant, SubjectMatchRule_field, SubjectMatchRule_function, SubjectMatchRule_namespace, SubjectMatchRule_objc_category, SubjectMatchRule_objc_implementation, SubjectMatchRule_objc_interface, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_objc_protocol, SubjectMatchRule_record, SubjectMatchRule_type_alias, SubjectMatchRule_variable))
 // CHECK-NEXT: BPFPreserveAccessIndex (SubjectMatchRule_record)
 // CHECK-NEXT: BTFDeclTag (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record, SubjectMatchRule_field, SubjectMatchRule_type_alias)
 // CHECK-NEXT: BuiltinAlias (SubjectMatchRule_function)
 // CHECK-NEXT: CFAuditedTransfer (SubjectMatchRule_function)
 // CHECK-NEXT: CFConsumed (SubjectMatchRule_variable_is_parameter)

clang/test/Sema/aarch64-sme-func-attrs.c

 __attribute__((arm_new_za, arm_preserves_za)) void new_preserves_za(void);
 
 // expected-cpp-error@+4 {{'arm_new_za' and 'arm_preserves_za' attributes are not compatible}}
 // expected-cpp-note@+3 {{conflicting attribute is here}}
 // expected-error@+2 {{'arm_new_za' and 'arm_preserves_za' attributes are not compatible}}
 // expected-note@+1 {{conflicting attribute is here}}
 __attribute__((arm_preserves_za, arm_new_za)) void preserves_new_za(void);
 
+// expected-cpp-error@+2 {{'arm_locally_streaming' attribute only applies to function definitions}}
+// expected-error@+1 {{'arm_locally_streaming' attribute only applies to function definitions}}
+__attribute__((arm_locally_streaming)) void sme_arm_locally_streaming_on_declaration(void);
+
 // Invalid attributes on function pointers
 
 // expected-cpp-error@+4 {{'arm_streaming_compatible' and 'arm_streaming' attributes are not compatible}}
 // expected-cpp-note@+3 {{conflicting attribute is here}}
 // expected-error@+2 {{'arm_streaming_compatible' and 'arm_streaming' attributes are not compatible}}
 // expected-note@+1 {{conflicting attribute is here}}
 void __attribute__((arm_streaming, arm_streaming_compatible)) streaming_ptr_invalid(void);
 // expected-cpp-error@+4 {{'arm_streaming_compatible' and 'arm_streaming' attributes are not compatible}}

 void __attribute__((arm_new_za, arm_preserves_za)) preserves_za_ptr_invalid(void);
 // expected-cpp-error@+4 {{'arm_preserves_za' and 'arm_new_za' attributes are not compatible}}
 // expected-cpp-note@+3 {{conflicting attribute is here}}
 // expected-error@+2 {{'arm_preserves_za' and 'arm_new_za' attributes are not compatible}}
 // expected-note@+1 {{conflicting attribute is here}}
 typedef __attribute__((arm_new_za, arm_preserves_za)) void (*fptrty10) (void);
 fptrty10 invalid_preserve_za_func() { return preserves_za_ptr_invalid; }
 
-// expected-cpp-error@+2 {{'arm_locally_streaming' attribute only applies to functions}}
-// expected-error@+1 {{'arm_locally_streaming' attribute only applies to functions}}
+// expected-cpp-error@+2 {{'arm_locally_streaming' attribute only applies to function definitions}}
+// expected-error@+1 {{'arm_locally_streaming' attribute only applies to function definitions}}
 typedef __attribute__((arm_locally_streaming)) void (*fptrty11) (void);
 
 // expected-warning@+2 {{'arm_streaming' attribute ignored}}
 // expected-warning@+1 {{'arm_streaming' only applies to function types; type here is 'void ()'}}
 __attribute__((arm_streaming)) void function_no_prototype();
 
 //
 // Check for incorrect conversions of function pointers with the attributes