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

JonChesterfield added a comment. This appears to have missed a case for openmp. Credit to @jdoerfert for the repro: https://godbolt.org/z/xWTYbv struct DST { long X; long Y; }; #pragma omp declare target DST G2 [[clang::loader_uninitialized, clang::address_space(5)]];

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

JonChesterfield added a comment. Fix proposed at D85990 . Thanks for the detailed bug report! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361

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

JonChesterfield added a comment. In D74361#2218294 , @erichkeane wrote: > Yep! Declaring a global variable that isn't 'extern' with an incomplete type > is disallowed anyway, so if you call RequireCompleteType, you're likely just > diagnosing that

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

erichkeane added a comment. Yep! Declaring a global variable that isn't 'extern' with an incomplete type is disallowed anyway, so if you call RequireCompleteType, you're likely just diagnosing that early. You MIGHT have to do some work to allow: struct S; extern S foo

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

JonChesterfield added a comment. In D74361#2218258 , @erichkeane wrote: > I did a little debugging, and the problem is the template itself isn't a > complete type That's clear cut then, thanks. This patch was limited to trivially constructible types,

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

erichkeane added a comment. I did a little debugging, and the problem is the template itself isn't a complete type until you require it with RequireCompleteType. You have this same problem with incomplete types: https://godbolt.org/z/hvf3M4 I would suspect you either add a RequireCompleteType

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

JonChesterfield added a comment. In D74361#2218143 , @erichkeane wrote: > Also, see this bug: This crashes immediately when used on a template > instantiation: https://bugs.llvm.org/show_bug.cgi?id=47169 Thanks for the bug report! Template

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

erichkeane added a comment. Also, see this bug: This crashes immediately when used on a template instantiation: https://bugs.llvm.org/show_bug.cgi?id=47169 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361

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

erichkeane added a comment. In D74361#1927931 , @JonChesterfield wrote: > In D74361#1927863 , @thakis wrote: > >> This breaks tests on Windows: http://45.33.8.238/win/10664/step_7.txt >> >> Please take a look, and

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

JonChesterfield added a comment. In D74361#1927863 , @thakis wrote: > This breaks tests on Windows: http://45.33.8.238/win/10664/step_7.txt > > Please take a look, and if it takes some time please revert while you > investigate. Thanks! It seems

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

thakis added a comment. This breaks tests on Windows: http://45.33.8.238/win/10664/step_7.txt Please take a look, and if it takes some time please revert while you investigate. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/

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

JonChesterfield added a comment. Tests made assumptions about alignment which are invalid on arm, failed a buildbot. The tests don't actually care about alignment, so fixed by dropping the `, align #` part of the patterns. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGc45eaeabb77a: [Clang] Undef attribute for global variables (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/

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

JonChesterfield added a comment. Thanks guys. Patched the C codegen test before landing - minor change on trunk in the last week, orthogonal to this. Delighted to have one less reason to write assembly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

JonChesterfield updated this revision to Diff 250901. JonChesterfield added a comment. - Update C codegen test to match output of trunk Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 Files:

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

rjmccall accepted this revision. rjmccall added a comment. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 ___ cfe-commits mailing list

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

JonChesterfield added a comment. ping 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

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

JonChesterfield updated this revision to Diff 250264. JonChesterfield marked an inline comment as done. JonChesterfield added a comment. - undo reformat of existing def Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/

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

JonChesterfield updated this revision to Diff 250263. JonChesterfield added a comment. - Amend diagnostic as suggested, clang-format new lines in SemaKinds.td Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361

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

JonChesterfield added a comment. In D74361#1920329 , @aaron.ballman wrote: > Aside from the diagnostic wording, I think this LG to me. However, I'd > appreciate if @rjmccall would also sign off. Thanks! @rjmccall? Comment at:

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from the diagnostic wording, I think this LG to me. However, I'd appreciate if @rjmccall would also sign off. Comment at:

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

JonChesterfield added a comment. ping @aaron.ballman - does that look right to you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 ___ cfe-commits mailing

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

JonChesterfield added a comment. > I thought err_loader_uninitialized_extern says that the variable cannot have > external linkage? Embarrassing! This was a badly written error message, now fixed to: `external declaration of variable cannot have the 'loader_uninitialized' attribute` The

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

JonChesterfield updated this revision to Diff 248807. JonChesterfield marked 2 inline comments as done. JonChesterfield added a comment. - Review comments, add tests for private_extern Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/

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

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:2718 + // attribute then there are two different definitions. + // In C++, prefer the standard diagnostics + if (!S.getLangOpts().CPlusPlus) { Missing a full stop at the

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

JonChesterfield added a comment. ping 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

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

JonChesterfield added a comment. I'm finally happy with the semantic checks here. Thanks for the guidance on where to look for the hooks. - attributed variable must be at global scope - all initializers are rejected - default constructors must be trivial (to reduce the scope of this patch) -

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

JonChesterfield updated this revision to Diff 247776. JonChesterfield added a comment. - Error on redeclaration with loader_uninit in C Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 Files:

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

JonChesterfield updated this revision to Diff 247753. JonChesterfield added a comment. - error on extern variables, minor cleanup Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 Files:

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

JonChesterfield updated this revision to Diff 247745. JonChesterfield marked 2 inline comments as done. JonChesterfield added a comment. - Reduce scope to trivial default constructed types Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/

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

JonChesterfield added a comment. I've continued thinking about / experimenting with this. Curiously - `X x;` and `X x{};` are considered distinct by clang. The current patch will only accept the former. I'll add some tests for that. I think there's a reasonable chance that the developers who

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

Quuxplusone added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + rjmccall wrote: > JonChesterfield wrote: >

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

rjmccall added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + JonChesterfield wrote: > rjmccall wrote: > >

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

rjmccall added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + Quuxplusone wrote: > rjmccall wrote: > >

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

JonChesterfield marked 2 inline comments as done. JonChesterfield added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; +

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

Quuxplusone added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + rjmccall wrote: > JonChesterfield wrote: >

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

rjmccall added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + JonChesterfield wrote: > Quuxplusone wrote: >

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

JonChesterfield marked 4 inline comments as done. JonChesterfield added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4367 +This is useful for variables that are always written to before use where the +default zero initialization provided by the toolchain

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

JonChesterfield updated this revision to Diff 247696. JonChesterfield marked 2 inline comments as done. JonChesterfield added a comment. - Reject initialisers, update doc to recommended string Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

Quuxplusone added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + This test case is identical to line 36 of

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

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

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

rjmccall added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4367 +This is useful for variables that are always written to before use where the +default zero initialization provided by the toolchain loader is expensive. + }]; How about: >

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

JonChesterfield marked 2 inline comments as done. JonChesterfield added a comment. Fixed the spelling/formatting, added more tests. The C++ case would be improved by warning on `int x __attribute__((loader_uninitialised)) = 0` as there are two initializers. The semantics for C are not what I

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

JonChesterfield updated this revision to Diff 247652. JonChesterfield marked 2 inline comments as done. JonChesterfield added a comment. - Address review comments, more test coverage Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/

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

aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4365 +The ``loader_uninitialized`` attribute can be placed on global variables to +indicate that the variable does not need to be zero

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

JonChesterfield updated this revision to Diff 247617. JonChesterfield added a comment. - Rename attribute, propose some documentation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 Files:

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

JonChesterfield added a comment. The above patch composes sensibly with openmp, e.g. #include #pragma omp declare target int data __attribute__((no_zero_initializer)); #pragma omp allocate(data) allocator(omp_pteam_mem_alloc) #pragma omp end declare target @data = hidden

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

rjmccall added a comment. In D74361#1896982 , @JonChesterfield wrote: > In D74361#1879559 , @rjmccall wrote: > > > This will need to impact static analysis and the AST, I think. Local > > variables can't be

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

JonChesterfield marked an inline comment as done. JonChesterfield added inline comments. Comment at: clang/test/CodeGenCXX/attr-no-zero-initializer.cpp:40 +// CHECK: @unt = global %struct.nontrivial undef +nontrivial unt [[clang::no_zero_initializer]];

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

Quuxplusone added inline comments. Comment at: clang/test/CodeGenCXX/attr-no-zero-initializer.cpp:40 +// CHECK: @unt = global %struct.nontrivial undef +nontrivial unt [[clang::no_zero_initializer]]; Can you explain a bit about how this interacts with C++

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

JonChesterfield updated this revision to Diff 247156. JonChesterfield added a comment. - clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 Files: clang/include/clang/Basic/Attr.td

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

JonChesterfield added a comment. In D74361#1879559 , @rjmccall wrote: > This will need to impact static analysis and the AST, I think. Local > variables can't be redeclared, but global variables can, so disallowing > initializers syntactically when the

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

JonChesterfield marked an inline comment as done. JonChesterfield added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6509 +static void handleNoZeroInitializerAttr(Sema , Decl *D, const ParsedAttr ) { + D->addAttr(::new (S.Context)