[PATCH] D128256: [Clang][AArch64] Limit arm_locally_streaming to function definitions only.

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen abandoned this revision.
sdesmalen added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128256

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128256: [Clang][AArch64] Limit arm_locally_streaming to function definitions only.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:9835-9839
+  if (D.isFunctionDefinition()) {
+NewFD->setWillHaveBody();
+ProcessDeclAttributes(S, NewFD, D);
+NewFD->setWillHaveBody(false);
+  } else

sdesmalen wrote:
> aaron.ballman wrote:
> > 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?
> > 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 
> struct pair {
>   T1 first;
>   T2 second;
> 
>   pair() : first(), second() {}
>   pair(const T1 , const T2 ) : first(a), second(b) {}
> 
>   template
>   pair(const pair ) : 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.
> 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?



Comment at: clang/test/AST/ast-dump-wasm-attr-export.c:13-15
+__attribute__((export_name("export_red"))) void red(void);
+__attribute__((export_name("export_orange"))) void orange(void);
+__attribute__((export_name("export_yellow"))) void yellow(void);

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128256

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128256: [Clang][AArch64] Limit arm_locally_streaming to function definitions only.

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:9835-9839
+  if (D.isFunctionDefinition()) {
+NewFD->setWillHaveBody();
+ProcessDeclAttributes(S, NewFD, D);
+NewFD->setWillHaveBody(false);
+  } else

aaron.ballman wrote:
> 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?
> 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 
struct pair {
  T1 first;
  T2 second;

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

  template
  pair(const pair ) : 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128256

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128256: [Clang][AArch64] Limit arm_locally_streaming to function definitions only.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:9835-9839
+  if (D.isFunctionDefinition()) {
+NewFD->setWillHaveBody();
+ProcessDeclAttributes(S, NewFD, D);
+NewFD->setWillHaveBody(false);
+  } else

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128256

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128256: [Clang][AArch64] Limit arm_locally_streaming to function definitions only.

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen created this revision.
sdesmalen added reviewers: aaron.ballman, sunfish.
Herald added subscribers: pmatos, asb, jdoerfert, kristof.beyls, sbc100.
Herald added a project: All.
sdesmalen requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.
Herald added a project: clang.

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).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128256

Files:
  clang/include/clang/Basic/Attr.td
  clang/lib/Sema/SemaDecl.cpp
  clang/test/AST/ast-dump-wasm-attr-export.c
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/aarch64-sme-func-attrs.c

Index: clang/test/Sema/aarch64-sme-func-attrs.c
===
--- clang/test/Sema/aarch64-sme-func-attrs.c
+++ clang/test/Sema/aarch64-sme-func-attrs.c
@@ -100,6 +100,10 @@
 // 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}}
@@ -138,8 +142,8 @@
 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}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -18,7 +18,6 @@
 // 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))
Index: clang/test/AST/ast-dump-wasm-attr-export.c
===
--- clang/test/AST/ast-dump-wasm-attr-export.c
+++ clang/test/AST/ast-dump-wasm-attr-export.c
@@ -10,24 +10,21 @@
 
 // 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 {{.+}}