[PATCH] D74361: [Clang] Undef attribute for global variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7436
+  case ParsedAttr::AT_LoaderUninitialized:
+    handleLoaderUninitializedAttr(S, D, AL);
+    break;
----------------
rjmccall wrote:
> JonChesterfield wrote:
> > aaron.ballman wrote:
> > > If you don't need any custom semantic checking, you can remove that function and instead call `handleSimpleAttribute<LoaderUninitializedAttr>(S, D, AL);`
> > I think this patch does need some custom semantic checking, I just haven't been able to work out how to implement it. Specifically, the attribute is an initializer, so
> > 
> > `int foo __attribute__((loader_uninitialised)) = some_value;`
> > 
> > should be a warning, as the = some_value is going to be ignored.
> This should be an error, not a warning, unless there's a specific need to be lenient.
Agreed that this should be an error rather than a warning.

Not 100% certain, but I suspect you'll need to add that checking to `Sema::AddInitializerToDecl()` because I'm guessing that the initializer has not been processed by the time the attributes are being applied to the variable declaration. You can check `VarDecl::hasInit()` within `handleLoaderUninitializedAttr()` to see if that specific declaration has an initializer, but I'm betting that gives you the wrong answer.


================
Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:14
+// CHECK: @tentative = global i32 undef
+int tentative  [[clang::loader_uninitialized]];
+
----------------
JonChesterfield wrote:
> aaron.ballman wrote:
> > What should happen with redeclarations? e.g., in C:
> > ```
> > int a;
> > 
> > int foo() { return a; }
> > 
> > int a __attribute__((loader_uninitialized));
> > ```
> > (This would be a useful test case to add.)
> > 
> > Also, I'd like to see a test case where the attributed global is an array.
> Ah, yes. I was thinking about tentative definitions before changing this test to C++. Fixed the name to be less misleading.
> 
> C++ just rejects it. Multiple definitions => error. Added test to sema.
> 
> C accepts it in either order. Which I believe it should. Either one is a tentative definition, and the other provides an actual definition (of undef), or the definition (of undef) is followed by a redeclaration.
> 
> This leaves the hole that while the following is rightly rejected in C for having multiple definitions:
> ```int a __attr__(...);
> int a = 10;```
> 
> This is erroneously accepted, with the attribute ignored:
> ```int a = 10;
> int a __attr__(...);```
> 
> 
> 
> 
> This is erroneously accepted, with the attribute ignored:

I think you will probably want to add another case to `mergeDeclAttribute()` following the similar patterns there so that you can reject when you need to merge declarations that should not be merged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74361