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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
