[Clang] Undef attribute for global variables

[JonChesterfield]

Summary

[Clang] Attribute to allow defining undef global variables

Initializing global variables is very cheap on hosted implementations. The
C semantics of zero initializing globals work very well there. It is not
necessarily cheap on freestanding implementations. Where there is no loader
available, code must be emitted near the start point to write the appropriate
values into memory.

At present, external variables can be declared in C++ and definitions provided
in assembly (or IR) to achive this effect. This patch provides an attribute in
order to remove this reason for writing assembly for performance sensitive
freestanding implementations.

A close analogue in tree is LDS memory for amdgcn, where the kernel is
responsible for initializing the memory after it starts executing on the gpu.
Uninitalized variables in LDS are observably cheaper than zero initialized.

Patch is loosely based on the cuda shared and opencl __local variable
implementation which also produces undef global variables.

[jdoerfert]

FWIW, this would really help us to create an OpenMP device (=GPU) runtime written almost entirely in OpenMP, C++, and very few clang builtins.

[rjmccall]

I'm not sure if it's a good idea to be re-using the existing attribute like this. It's not that unreasonable, I guess, but I'd like to hear JF's thoughts.

I think you do need to update the documentation to mention the impact on globals. For normal targets (i.e. ones with loaders), I assume this guarantees a zero pattern. Except maybe not for thread-locals? Do we have any targets where we can actually optimize thread-locals by suppressing initialization?

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 attribute is present doesn't necessarily stop other declarations from defining the variable. Also, you need to make the attribute mark something as a definition, the same way that e.g. the alias attribute does. Also, this probably ought to disable the default-initialization of non-trivial types in C++, in case that's not already done.

clang/test/Sema/attr-uninitialized.c

9 ↗

(On Diff #243681)

In general, you shouldn't remove the existing test lines; just modify them to not report a diagnostic.

12 ↗

(On Diff #243681)

This should still be diagnosed; it doesn't make any sense to apply this to parameters.

[jfb]

In D74361#1879559, @rjmccall wrote:

I'm not sure if it's a good idea to be re-using the existing attribute like this. It's not that unreasonable, I guess, but I'd like to hear JF's thoughts.

The current uninitialized attribute fits the model C and C++ follow for attributes: they have no semantic meaning for a valid program, in that a compiler can just ignore them and (barring undefined behavior) there's no observable effect to the program. This updated semantic changes the behavior of a valid C and C++ program, because the standards specify the value of uninitialized globals and TLS. I'd much rather have a separate attribute, say no_zero_init, to explicitly say what this does.

[JonChesterfield]

It zero initialises on x86, but perhaps by coincidence rather than guarantee.

Fair enough regarding reuse of the existing attribute, I'll create a new one.

Thanks for the pointers on additional cases to test for too. I'll return with an improved patch.

[JonChesterfield]

• Rename attribute, add to hasDefiningAttr

[JonChesterfield]

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 attribute is present doesn't necessarily stop other declarations from defining the variable. Also, you need to make the attribute mark something as a definition, the same way that e.g. the alias attribute does. Also, this probably ought to disable the default-initialization of non-trivial types in C++, in case that's not already done.

I would like this to be the case but am having a tough time understanding how sema works. VarDecl::hasInit() looked promising but doesn't appear to indicate whether there is a syntactic initialiser (e.g. = 10) present. I will continue to investigate. Codegen appears to be working fine.

In D74361#1880904, @jfb wrote:

The current uninitialized attribute fits the model C and C++ follow for attributes: they have no semantic meaning for a valid program, in that a compiler can just ignore them and (barring undefined behavior) there's no observable effect to the program. This updated semantic changes the behavior of a valid C and C++ program, because the standards specify the value of uninitialized globals and TLS. I'd much rather have a separate attribute, say no_zero_init, to explicitly say what this does.

This proposed attribute can similarly be ignored without observable semantic effect. Instead of an IR undef variable, we would have an IR zeroinitialized variable, which is a legitimate implementation choice for undef. Adding the attribute to an existing program will, in general, change its meaning - but that's also true of other attributes.

[JonChesterfield]

clang/lib/Sema/SemaDeclAttr.cpp

6547

cast<VarDecl>(D)->hasInit() seems to always return true here - presumably the error needs to be emitted sometime before this

[JonChesterfield]

• clang-format

[• Quuxplusone]

clang/test/CodeGenCXX/attr-no-zero-initializer.cpp

40 ↗

(On Diff #247156)

Can you explain a bit about how this interacts with C++ constructors? Will this object not have its constructor called at startup; or is it that the constructor will be called but the memory simply won't have been zeroed before calling the constructor?

For P1144 relocation (D50114, D61761, etc), Anton Zhilin and I have been discussing the possibility of a similar-sounding attribute that would skip the constructor of a local variable altogether, allowing someone to write e.g.

T relocate(T *source) {
    [[unconstructed]] T result;
    memcpy(result, source, sizeof(T));
    return result;
}

If your attribute does exactly that, then I'm interested. If your attribute doesn't do that, but is occupying real estate that implies that it does, then I'm also interested.

[JonChesterfield]

clang/test/CodeGenCXX/attr-no-zero-initializer.cpp

40 ↗

(On Diff #247156)

This change is orthogonal I think. No change to object lifetime. Without the attribute, a global is zero initialized and then the constructor is called before main. With it, the global is undef before the constructor call.

A different attribute that suppresses the con(de)structor call entirely is also of interest to me. A freestanding implementation is likely to accept global constructors without warning, and emit code for them, but not actually run them at startup. That's not a great UX.

[rjmccall]

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 redeclared, but global variables can, so disallowing initializers syntactically when the attribute is present doesn't necessarily stop other declarations from defining the variable. Also, you need to make the attribute mark something as a definition, the same way that e.g. the alias attribute does. Also, this probably ought to disable the default-initialization of non-trivial types in C++, in case that's not already done.

I would like this to be the case but am having a tough time understanding how sema works. VarDecl::hasInit() looked promising but doesn't appear to indicate whether there is a syntactic initialiser (e.g. = 10) present. I will continue to investigate. Codegen appears to be working fine.

Looks like you figured this out.

In D74361#1880904, @jfb wrote:

The current uninitialized attribute fits the model C and C++ follow for attributes: they have no semantic meaning for a valid program, in that a compiler can just ignore them and (barring undefined behavior) there's no observable effect to the program. This updated semantic changes the behavior of a valid C and C++ program, because the standards specify the value of uninitialized globals and TLS. I'd much rather have a separate attribute, say no_zero_init, to explicitly say what this does.

This proposed attribute can similarly be ignored without observable semantic effect. Instead of an IR undef variable, we would have an IR zeroinitialized variable, which is a legitimate implementation choice for undef. Adding the attribute to an existing program will, in general, change its meaning - but that's also true of other attributes.

I agree with this reasoning, but since you seem willing to change the attribute name, the point is now moot.

clang/include/clang/Basic/Attr.td

3319

We try to always add documentation for any new attribute.

I'm not sure I like the new name; it doesn't read right to me. Maybe loader_uninitialized makes the intent clear enough?

Thinking more about it, I agree with you that this is orthogonal to C++ initialization. Users on targets like yours probably ought to be able to disable C++ initialization without having to disable zero-initialization, or vice-versa.

[JonChesterfield]

The above patch composes sensibly with openmp, e.g.

#include <omp.h>
#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 addrspace(3) global i32 undef, align 4

I like loader_uninitialized. There's some prior art on using NOLOAD from a (gnu) linker script to achieve the same result, which is a similar name. I'll update the patch accordingly.

I found an arm toolchain which supports UNINIT in linker scripts. Asking around I've also heard that games dev is a potential user for this feature (perhaps analogous to mmap's MAP_UNINITIALIZED?) but haven't been able to confirm specifics.

I'll take a first shot at the documentation too.

[JonChesterfield]

• Rename attribute, propose some documentation

[aaron.ballman]

clang/include/clang/Basic/AttrDocs.td

4365

initialised -> initialized

clang/lib/Sema/SemaDeclAttr.cpp

7471

If you don't need any custom semantic checking, you can remove that function and instead call handleSimpleAttribute<LoaderUninitializedAttr>(S, D, AL);

[aaron.ballman]

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

15

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.

clang/test/Sema/attr-loader-uninitialized.cpp

20

I'd also like to see a test demonstrating it doesn't accept any arguments.

[JonChesterfield]

• Address review comments, more test coverage

[JonChesterfield]

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 hoped for where there are multiple definitions, one of which is via this attribute. Added a test for that. Recommendations for where to poke sema to raise an error on the second one are warmly invited. I'll try checkNewAttributesAfterDef, but will make a debug build first.

clang/lib/Sema/SemaDeclAttr.cpp

7471

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.

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

15

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__(...);

[rjmccall]

clang/include/clang/Basic/AttrDocs.td

4367

How about:

The `loader_uninitialized` attribute can be placed on global variables to
indicate that the variable does not need to be zero initialized by the loader.
On most targets, zero-initialization does not incur any additional cost.
For example, most general purpose operating systems deliberately ensure
that all memory is properly initialized in order to avoid leaking privileged
information from the kernel or other programs. However, some targets
do not make this guarantee, and on these targets, avoiding an unnecessary
zero-initialization can have a significant impact on load times and/or code
size.

A declaration with this attribute is a non-tentative definition just as if it
provided an initializer. Variables with this attribute are considered to be
uninitialized in the same sense as a local variable, and the programs must
write to them before reading from them. If the variable's type is a C++ class
type with a non-trivial default constructor, or an array thereof, this attribute
only suppresses the static zero-initialization of the variable, not the dynamic
initialization provided by executing the default constructor.

clang/lib/Sema/SemaDeclAttr.cpp

7471

This should be an error, not a warning, unless there's a specific need to be lenient.

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

7471

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.

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

15

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.

[JonChesterfield]

• Reject initialisers, update doc to recommended string

[• Quuxplusone]

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

24

This test case is identical to line 36 of clang/test/Sema/attr-loader-uninitialized.cpp, where you say you don't want it to compile at all.

I think you need a clearer idea of how this interacts with initializers. Is it merely supposed to eliminate the zero-initialization that happens before the user-specified construction/initialization, or is it supposed to compete with the user-specified construction/initialization?

That is, for

nontrivial unt [[clang::loader_uninitialized]];

is it merely supposed to call unt::unt() on a chunk of undef memory (instead of the usual chunk of zeroed memory), or is it supposed to skip the constructor entirely? And for

int x [[clang::loader_uninitialized]] = foo();

is it merely supposed to call foo() and assign the result to a chunk of undef memory (instead of the usual chunk of zeroed memory), or is it supposed to skip the initialization entirely?

[JonChesterfield]

clang/include/clang/Basic/AttrDocs.td

4367

That's a lot better! Thank you. Updated the patch to use your wording verbatim.

clang/lib/Sema/SemaDeclAttr.cpp

7471

Nice, Yes, that's much better - I should have searched for the opencl __local handling. As you suspected, hasInit() just returns true at that point.

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

24

I think you commented while the first working piece of sema landed. My thinking is relatively clear but my understanding of clang's semantic analysis is a work in progress!

Initializers (= foo()) are straightforward. Error on the basis that the attribute effectively means = undef, and one should not have two initializers. A test case is now added for that (and now passes).

The codegen I want for a default constructed global marked with this variable is:

• global variable allocated, with undef as the original value
• default constructor call synthesized
• said default constructor set up for invocation from crt, before main, writing over the undef value

Where the default constructor can be optimized as usual, e.g. if it always writes a constant, we can init with that constant instead of the undef and elide the constructor.

I don't have that actually working yet - the constructor call is not being emitted, so we just have the undef global.

I think it's important to distinguish between the values of the bits when the program is loaded and whether constructor/destructors are called, as one could want any combination of the two.

[rjmccall]

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

24

I think Arthur is suggesting that it would be useful to allow the attribute to be used in conjunction with an initializer in C++, since if the initializer has to be run dynamically, we can still meaningfully suppress the static zero-initialization. That is, we've accepted that it's useful to do this when *default-initializing* a global, but it's actually useful when doing *any* kind of dynamic initialization.

Maybe we can leave it as an error in C++ when the initializer is a constant expression. Although that might be unnecessarily brittle if e.g. the constructor is constexpr in one library version but not another.

[• Quuxplusone]

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

24

No, that's exctly what I mean. You seem to be holding two contradictory ideas simultaneously:

[[loader_uninitialized]] X x = X{};  // two initializers, therefore error

[[loader_uninitialized]] X x {}; // one initializer plus one constructor, therefore fine

In C++, these two declarations have identical semantics. It doesn't make sense to say that one of them "calls a constructor" and the other one "has an initializer." They're literally the same thing.

Similarly in both C99 and C++ with plain old ints:

[[loader_uninitialized]] int x = foo();

This means "call foo and dynamically initialize x with the result," just as surely as

[[loader_uninitialized]] X x = X();

means "call X::X and dynamically initialize x with the result." Having one rule for dynamic initializers of primitive types and a separate rule for dynamic initializers of class types doesn't work.

Furthermore, "dynamic initialization" can be promoted to compile-time:

[[loader_uninitialized]] int x = 42;
[[loader_uninitialized]] std::string_view x = "hello world";

It wouldn't make semantic sense to say that one of these has "two initializers" and the other has "one initializer," because both of the initializations end up happening at compile time and getting put into .data.

[JonChesterfield]

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

24

I'm under the impression that the constructs are:

X x = X{};  // default construct an X and then call the copy constructor at x
X x {}; // default construct an X at the location of x
X x; // default construct an X at the location of x

The C++ initialisation rules are very complicated so I won't be shocked to have got that wrong.

If your toolchain actually runs global constructors then there isn't much use to marking C++ objects with this attribute, unless said constructor does nothing and would thus leave the bytes still undef.

Accepting default constructors (or, perhaps only accepting trivial types?) would allow people to use this attribute with the approximation of C++ available on systems that don't actually run the global constructors.

Equally, if this seems too complicated, I'm happy to restrict this to C as that's still an improvement on the asm and/or linker scripts I have available today.

[rjmccall]

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

24

Similarly in both C99 and C++ with plain old ints:

C does not allow non-constant initialization of global variables.

Let me try to explain what we're thinking here. If the variable provides both an initializer and this attribute, and the compiler can successfully perform constant initialization (as it's required to do in C, and as it often does in C++), then the attribute has probably become meaningless because we're not going to suppress that constant initialization. The compiler strongly prefers to diagnose meaningless attributes in some way, because they often indirectly indicate a bug. For example, maybe the initializer isn't supposed to be there but it's hidden behind a huge mess of macros; or maybe somebody added a constant initializer because they changed the variable schema to make that possible, and so the programmer should now remove the attribute and the dynamic initialization code that went along with it. Compared to that, the C++ use case of "allow a dynamic initializer but suppress static zero-initialization" is pretty marginal; that doesn't mean we absolutely shouldn't support it, but it does suggest that we shouldn't prioritize it over other aspects of the feature, like catching that possible bug.

[rjmccall]

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

24

If your toolchain actually runs global constructors then there isn't much use to marking C++ objects with this attribute, unless said constructor does nothing and would thus leave the bytes still undef.

That's not totally true. C++ still requires globals to be zero-initialized before it runs dynamic initializers, and the dynamic initializer is very likely to make that zero-initialization totally redundant. Arthur is right that there's no inherent reason that that only applies to dynamic *default* initialization, though, as opposed to any non-constant initialization.

I'd prefer to avoid making this C-specific; Clang takes a lot of pride in trying to make feature cross-language as much as possible. We could just make the error about providing both the attribute and an initializer C-specific, though.

[• Quuxplusone]

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

24

Ah, I was wrong when I said "in both C99 and C++" int x = foo(); would be supported. You're right, that's a C++ism. Even C11 doesn't seem to support dynamic initializers, not even for function-local static variables.

I suppose I should also mention that I totally don't understand the domain here. :) To me, the way you avoid storing an explicit initializer in the ELF file is you allocate the variable into .bss instead of .data; and the way you avoid paying for zero-initialization at runtime would be that you allocate the variable into a hand-crafted section instead of .bss (example from Keil docs). In ordinary ELF-land, there's no way to say "I want this variable in .data but not initialized," nor "I want this variable in .bss but not zero-initialized" — the file format literally can't represent those concepts.

[JonChesterfield]

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 want to elide the runtime cost of zero initialising will usually also want to avoid dynamic initialisation. That suggests we could accept only trivially default constructible classes, where the undef initializer is correct in that one cannot determine whether a no-op constructor actually ran or not. If we go with that, then the more complicated question of exactly how this should interact with user-controlled disabling of dynamic initialization can be postponed until that feature is introduced. This patch is then reasonably self contained.

This patch is strongly related to the linker script approach, or equivalent asm. It's target-dependent how the uninitialised data gets represented, e.g. x64 will put it in bss anyway. Opencl's __local is similarly uninitialised, and gets annotated with an address space that maps onto magic in llc and/or the loader.

[JonChesterfield]

• Reduce scope to trivial default constructed types

[JonChesterfield]

• error on extern variables, minor cleanup

[JonChesterfield]

• Error on redeclaration with loader_uninit in C

[JonChesterfield]

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)
• extern variables rejected as they can't meaningfully have a definition
• attribute on a declaration following a normal definition in C rejected

Patch is a bit bigger than I hoped for but quite self contained. Everything is guarded by a test on the attribute.

@Quuxplusone does restricting this to trivial default construction resolve your concerns?

[JonChesterfield]

ping

[aaron.ballman]

clang/lib/Sema/SemaDecl.cpp

2718

Missing a full stop at the end of the sentence. The comment can probably be re-flowed with the preceding sentence too.

11930

Missing full stop.

12358

Should this either be calling VarDecl::hasExternalStorage() or looking at the linkage of the variable, rather than at the storage class written in the source?

clang/test/CodeGen/attr-loader-uninitialized.c

5

I thought err_loader_uninitialized_extern says that the variable cannot have external linkage?

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

5

I thought err_loader_uninitialized_extern says that the variable cannot have external linkage?

[JonChesterfield]

• Review comments, add tests for private_extern

[JonChesterfield]

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 premise behind this feature is to be semantically identical to:
type foo = undef;

The initializer value is orthogonal to the variable linkage and visibility. If = 4 was ok, so should this attribute be.

What is not meaningful is to declare that a variable is defined elsewhere that is uninitialized.
That is, extern int x = 42; // warning: 'extern' variable has an initializer
Therefore [[loader_uninitialized]] int x = 42; // also bad

This patch makes the latter an error, on the basis that it's definitely a mistake to provide two initializers for one variable (one 42, one undef).

C++ thinks const int x; is an error and C thinks const int x; is fine. This patch remains consistent with that.

clang/lib/Sema/SemaDecl.cpp

12358

Interesting question, thank you. SC_Extern is right.

hasExternalStorage is true if extern or private_extern. I hadn't seen private_extern before, but it appears to be a way to model hidden visibility. It accepts a normal initializer, e.g.
__private_extern__ int private_extern_can_be_initialised = 10;
therefore should also work with undef.

Added a test for this (which will fail if SC_Extern is replaced with hasExternalStorage).

Replying to linkage out of line as it comes up on a few inline comments.

[JonChesterfield]

ping @aaron.ballman - does that look right to you?

[aaron.ballman]

Aside from the diagnostic wording, I think this LG to me. However, I'd appreciate if @rjmccall would also sign off.

clang/include/clang/Basic/DiagnosticSemaKinds.td

5343

How would you feel about: "variable %0 cannot be declared both 'extern' and with the 'loader_uninitialized' attribute" (or something along those lines) to clarify "external declaration"?

[JonChesterfield]

• Amend diagnostic as suggested, clang-format new lines in SemaKinds.td

[JonChesterfield]

• undo reformat of existing def

[JonChesterfield]

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?

clang/include/clang/Basic/DiagnosticSemaKinds.td

5343

Better, thanks. Also discovered clang-format has learned to do sensible things with tablegen so applied it to the new defs.

[JonChesterfield]

ping

[rjmccall]

LGTM!

[JonChesterfield]

• Update C codegen test to match output of trunk

[JonChesterfield]

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.

[JonChesterfield]

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.

[thakis]

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.

[JonChesterfield]

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 Windows inserts 'dso_local' into the middle of the generated IR.

I can't test on Windows so the two that failed CI are now marked as "UNSUPPORTED: system-windows".

Do you know a usual work around for variation in symbol visibility? I'm happy to copy it from another test but am wary of guessing what might work on Windows.

[erichkeane]

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 if it takes some time please revert while you investigate.

Thanks! It seems Windows inserts 'dso_local' into the middle of the generated IR.

I can't test on Windows so the two that failed CI are now marked as "UNSUPPORTED: system-windows".

Do you know a usual work around for variation in symbol visibility? I'm happy to copy it from another test but am wary of guessing what might work on Windows.

Our windows builds always just add dso_local, so we typically just do a wildcard there to handle those cases (or, separate check lines for windows).

[erichkeane]

Also, see this bug: This crashes immediately when used on a template instantiation: https://bugs.llvm.org/show_bug.cgi?id=47169

[JonChesterfield]

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 instantiations are missing from the test cases, I will go debugging.

[erichkeane]

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 for your loader_uninitialized checks, or move the check somewhere where it is already complete.

[JonChesterfield]

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, and we don't know whether the type is trivially constructible if it's incomplete. Thus it does require complete types and we're missing a check in sema

[erichkeane]

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 __attribute__((loader_uninitialized));

[JonChesterfield]

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

You MIGHT have to do some work to allow:

struct S;
extern S foo __attribute__((loader_uninitialized));

I'll add that to the tests. Looking OK so far, added a check to SemaDecl and the crash is gone. No fix needed for C. Will have a patch up soon

edit: extern + loader_uninitialized was rejected in the first patch, will check it still works like that

[JonChesterfield]

Fix proposed at D85990. Thanks for the detailed bug report!

[JonChesterfield]

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)]];
#pragma omp end declare target

error: no matching constructor for initialization of '__attribute__((address_space(5))) DST'

There shouldn't be a constructor call there. Noting the limitation here so I can find it easily in the near future.

clang/include/clang/Basic/Attr.td

 }
 
 def Uninitialized : InheritableAttr {
   let Spellings = [Clang<"uninitialized", 0>];
   let Subjects = SubjectList<[LocalVar]>;
   let Documentation = [UninitializedDocs];
 }
 
+def LoaderUninitialized : Attr {
+  let Spellings = [Clang<"loader_uninitialized">];
+  let Subjects = SubjectList<[GlobalVar]>;
+  let Documentation = [LoaderUninitializedDocs];
+}
+
 def ObjCExternallyRetained : InheritableAttr {
   let LangOpts = [ObjCAutoRefCount];
   let Spellings = [Clang<"objc_externally_retained">];
   let Subjects = SubjectList<[NonParmVar, Function, Block, ObjCMethod]>;
   let Documentation = [ObjCExternallyRetainedDocs];
 }
 
 def NoBuiltin : Attr {

clang/include/clang/Basic/AttrDocs.td

 initialize trivial automatic stack variables. By default, trivial automatic
 stack variables are uninitialized. This attribute is used to override the
 command-line parameter, forcing variables to remain uninitialized. It has no
 semantic meaning in that using uninitialized values is undefined behavior,
 it rather documents the programmer's intent.
   }];
 }
 
+def LoaderUninitializedDocs : Documentation {
+  let Category = DocCatVariable;
+  let Content = [{
+The ``loader_uninitialized`` attribute can be placed on global variables to
+indicate that the variable does not need to be zero initialized by the loader.
+On most targets, zero-initialization does not incur any additional cost.
+For example, most general purpose operating systems deliberately ensure
+that all memory is properly initialized in order to avoid leaking privileged
+information from the kernel or other programs. However, some targets
+do not make this guarantee, and on these targets, avoiding an unnecessary
+zero-initialization can have a significant impact on load times and/or code
+size.
+
+A declaration with this attribute is a non-tentative definition just as if it
+provided an initializer. Variables with this attribute are considered to be
+uninitialized in the same sense as a local variable, and the programs must
+write to them before reading from them. If the variable's type is a C++ class
+type with a non-trivial default constructor, or an array thereof, this attribute
+only suppresses the static zero-initialization of the variable, not the dynamic
+initialization provided by executing the default constructor.
+  }];
+}
+
 def CallbackDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 The ``callback`` attribute specifies that the annotated function may invoke the
 specified callback zero or more times. The callback, as well as the passed
 arguments, are identified by their parameter name or position (starting with
 1!) in the annotated function. The first position in the attribute identifies
 the callback callee, the following positions declare describe its arguments.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_typecheck_zero_static_array_size : Warning<
   InGroup<ArrayBounds>;
 def err_array_size_non_int : Error<"size of array has non-integer type %0">;
 def err_init_element_not_constant : Error<
   "initializer element is not a compile-time constant">;
 def ext_aggregate_init_not_constant : Extension<
   "initializer for aggregate is not a compile-time constant">, InGroup<C99>;
 def err_local_cant_init : Error<
   "'__local' variable cannot have an initializer">;
+def err_loader_uninitialized_cant_init
+    : Error<"variable with 'loader_uninitialized' attribute cannot have an "
+            "initializer">;
+def err_loader_uninitialized_trivial_ctor
+    : Error<"variable with 'loader_uninitialized' attribute must have a "
+            "trivial default constructor">;
+def err_loader_uninitialized_redeclaration
+    : Error<"redeclaration cannot add 'loader_uninitialized' attribute">;
+def err_loader_uninitialized_extern_decl
+    : Error<"variable %0 cannot be declared both 'extern' and with the "
+            "'loader_uninitialized' attribute">;
 def err_block_extern_cant_init : Error<
   "'extern' variable cannot have an initializer">;
 def warn_extern_init : Warning<"'extern' variable has an initializer">,
   InGroup<DiagGroup<"extern-initializer">>;
 def err_variable_object_no_init : Error<
   "variable-sized object may not be initialized">;
 def err_excess_initializers : Error<
   "excess elements in %select{array|vector|scalar|union|struct}0 initializer">;

clang/lib/AST/DeclBase.cpp

ExternalSourceSymbolAttr *Decl::getExternalSourceSymbolAttr() const {
   if (auto *dcd = dyn_cast<Decl>(getDeclContext())) {
     return dcd->getAttr<ExternalSourceSymbolAttr>();
   }
 
   return nullptr;
 }
 
 bool Decl::hasDefiningAttr() const {
-  return hasAttr<AliasAttr>() || hasAttr<IFuncAttr>();
+  return hasAttr<AliasAttr>() || hasAttr<IFuncAttr>() ||
+         hasAttr<LoaderUninitializedAttr>();
 }
 
 const Attr *Decl::getDefiningAttr() const {
   if (auto *AA = getAttr<AliasAttr>())
     return AA;
   if (auto *IFA = getAttr<IFuncAttr>())
     return IFA;
+  if (auto *NZA = getAttr<LoaderUninitializedAttr>())
+    return NZA;
   return nullptr;
 }
 
 static StringRef getRealizedPlatform(const AvailabilityAttr *A,
                                      const ASTContext &Context) {
   // Check if this is an App Extension "platform", and if so chop off
   // the suffix for matching with the actual platform.
   StringRef RealizedPlatform = A->getPlatform()->getName();

clang/lib/CodeGen/CGDecl.cpp

llvm::Constant *CodeGenModule::getOrCreateStaticVarDecl(
   llvm::Type *LTy = getTypes().ConvertTypeForMem(Ty);
   LangAS AS = GetGlobalVarAddressSpace(&D);
   unsigned TargetAS = getContext().getTargetAddressSpace(AS);
 
   // OpenCL variables in local address space and CUDA shared
   // variables cannot have an initializer.
   llvm::Constant *Init = nullptr;
   if (Ty.getAddressSpace() == LangAS::opencl_local ||
-      D.hasAttr<CUDASharedAttr>())
+      D.hasAttr<CUDASharedAttr>() || D.hasAttr<LoaderUninitializedAttr>())
     Init = llvm::UndefValue::get(LTy);
   else
     Init = EmitNullConstant(Ty);
 
   llvm::GlobalVariable *GV = new llvm::GlobalVariable(
       getModule(), LTy, Ty.isConstant(getContext()), Linkage, Init, Name,
       nullptr, llvm::GlobalVariable::NotThreadLocal, TargetAS);
   GV->setAlignment(getContext().getDeclAlign(&D).getAsAlign());

clang/lib/CodeGen/CodeGenModule.cpp

bool IsCUDAShadowVar =
        D->hasAttr<CUDASharedAttr>());
   // HIP pinned shadow of initialized host-side global variables are also
   // left undefined.
   bool IsHIPPinnedShadowVar =
       getLangOpts().CUDAIsDevice && D->hasAttr<HIPPinnedShadowAttr>();
   if (getLangOpts().CUDA &&
       (IsCUDASharedVar || IsCUDAShadowVar || IsHIPPinnedShadowVar))
     Init = llvm::UndefValue::get(getTypes().ConvertType(ASTTy));
+  else if (D->hasAttr<LoaderUninitializedAttr>())
+    Init = llvm::UndefValue::get(getTypes().ConvertType(ASTTy));
   else if (!InitExpr) {
     // This is a tentative definition; tentative definitions are
     // implicitly initialized with { 0 }.
     //
     // Note that tentative definitions are only emitted at the end of
     // a translation unit, so they should never have incomplete
     // type. In addition, EmitTentativeDefinition makes sure that we
     // never attempt to emit a tentative definition if a real one

clang/lib/Sema/SemaDecl.cpp

if (isa<C11NoReturnAttr>(NewAttribute)) {
         S.Diag(Def->getLocation(), diag::err_alignas_missing_on_definition)
           << AA;
         S.Diag(NewAttribute->getLocation(), diag::note_alignas_on_declaration)
           << AA;
         NewAttributes.erase(NewAttributes.begin() + I);
         --E;
         continue;
       }
+    } else if (isa<LoaderUninitializedAttr>(NewAttribute)) {
+      // If there is a C definition followed by a redeclaration with this
+      // attribute then there are two different definitions. In C++, prefer the
+      // standard diagnostics.
+      if (!S.getLangOpts().CPlusPlus) {
+        S.Diag(NewAttribute->getLocation(),
+               diag::err_loader_uninitialized_redeclaration);
+        S.Diag(Def->getLocation(), diag::note_previous_definition);
+        NewAttributes.erase(NewAttributes.begin() + I);
+        --E;
+        continue;
+      }
     } else if (isa<SelectAnyAttr>(NewAttribute) &&
                cast<VarDecl>(New)->isInline() &&
                !cast<VarDecl>(New)->isInlineSpecified()) {
       // Don't warn about applying selectany to implicitly inline variables.
       // Older compilers and language modes would require the use of selectany
       // to make such variables inline, and it would have no effect if we
       // honored it.
       ++I;
void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
   // OpenCL 1.1 6.5.2: "Variables allocated in the __local address space inside
   // a kernel function cannot be initialized."
   if (VDecl->getType().getAddressSpace() == LangAS::opencl_local) {
     Diag(VDecl->getLocation(), diag::err_local_cant_init);
     VDecl->setInvalidDecl();
     return;
   }
 
+  // The LoaderUninitialized attribute acts as a definition (of undef).
+  if (VDecl->hasAttr<LoaderUninitializedAttr>()) {
+    Diag(VDecl->getLocation(), diag::err_loader_uninitialized_cant_init);
+    VDecl->setInvalidDecl();
+    return;
+  }
+
   // Get the decls type and save a reference for later, since
   // CheckInitializerTypes may change it.
   QualType DclT = VDecl->getType(), SavT = DclT;
 
   // Expressions default to 'id' when we're in a debugger
   // and we are assigning it to a variable of Objective-C pointer type.
   if (getLangOpts().DebuggerCastResultToId && DclT->isObjCObjectPointerType() &&
       Init->getType() == Context.UnknownAnyTy) {
if (VarDecl *Var = dyn_cast<VarDecl>(RealDecl)) {
     if (!Var->isInvalidDecl() &&
         Var->getType().getAddressSpace() == LangAS::opencl_constant &&
         Var->getStorageClass() != SC_Extern && !Var->getInit()) {
       Diag(Var->getLocation(), diag::err_opencl_constant_no_init);
       Var->setInvalidDecl();
       return;
     }
 
+    if (!Var->isInvalidDecl() && RealDecl->hasAttr<LoaderUninitializedAttr>()) {
+      if (CXXRecordDecl *RD = Var->getType()->getAsCXXRecordDecl()) {
+        if (!RD->hasTrivialDefaultConstructor()) {
+          Diag(Var->getLocation(), diag::err_loader_uninitialized_trivial_ctor);
+          Var->setInvalidDecl();
+          return;
+        }
+      }
+      if (Var->getStorageClass() == SC_Extern) {
+        Diag(Var->getLocation(), diag::err_loader_uninitialized_extern_decl)
+            << Var;
+        Var->setInvalidDecl();
+        return;
+      }
+    }
+
     VarDecl::DefinitionKind DefKind = Var->isThisDeclarationADefinition();
     if (!Var->isInvalidDecl() && DefKind != VarDecl::DeclarationOnly &&
         Var->getType().hasNonTrivialToPrimitiveDefaultInitializeCUnion())
       checkNonTrivialCUnion(Var->getType(), Var->getLocation(),
                             NTCUC_DefaultInitializedObject, NTCUK_Init);
 
 
     switch (DefKind) {

clang/lib/Sema/SemaDeclAttr.cpp

 
 static void handleUninitializedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   assert(cast<VarDecl>(D)->getStorageDuration() == SD_Automatic &&
          "uninitialized is only valid on automatic duration variables");
   D->addAttr(::new (S.Context) UninitializedAttr(S.Context, AL));
 }
 
 static bool tryMakeVariablePseudoStrong(Sema &S, VarDecl *VD,
                                         bool DiagnoseFailure) {
   QualType Ty = VD->getType();
   if (!Ty->isObjCRetainableType()) {
     if (DiagnoseFailure) {
       S.Diag(VD->getBeginLoc(), diag::warn_ignored_objc_externally_retained)
           << 0;
     }
     return false;
   }
static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
   case ParsedAttr::AT_NoDestroy:
     handleDestroyAttr(S, D, AL);
     break;
 
   case ParsedAttr::AT_Uninitialized:
     handleUninitializedAttr(S, D, AL);
     break;
 
+  case ParsedAttr::AT_LoaderUninitialized:
+    handleSimpleAttribute<LoaderUninitializedAttr>(S, D, AL);
+    break;
+
   case ParsedAttr::AT_ObjCExternallyRetained:
     handleObjCExternallyRetainedAttr(S, D, AL);
     break;
 
   case ParsedAttr::AT_MIGServerRoutine:
     handleMIGServerRoutineAttr(S, D, AL);
     break;
 

clang/test/CodeGen/attr-loader-uninitialized.c

+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: @tentative_attr_first = global i32 undef, align 4
+int tentative_attr_first __attribute__((loader_uninitialized));
+int tentative_attr_first;
+
+// CHECK: @tentative_attr_second = global i32 undef, align 4
+int tentative_attr_second;
+int tentative_attr_second __attribute__((loader_uninitialized));
+
+// CHECK: @array = global [16 x float] undef, align 16
+float array[16] __attribute__((loader_uninitialized));
+
+typedef struct
+{
+  int x;
+  float y;
+} s;
+
+// CHECK: @i = global %struct.s undef, align 4
+s i __attribute__((loader_uninitialized));
+
+// CHECK: @private_extern_ok = hidden global i32 undef, align 4
+__private_extern__ int private_extern_ok __attribute__((loader_uninitialized));

clang/test/CodeGenCXX/attr-loader-uninitialized.cpp

+// RUN: %clang_cc1 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @defn = global i32 undef
+int defn  [[clang::loader_uninitialized]];
+
+// CHECK: @_ZL11defn_static = internal global i32 undef
+static int defn_static [[clang::loader_uninitialized]] __attribute__((used));
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::loader_uninitialized]];
+  return &data;
+}
+
+class trivial
+{
+  float x;
+};
+
+// CHECK: @ut = global %class.trivial undef
+trivial ut [[clang::loader_uninitialized]];
+
+// CHECK: @arr = global [32 x double] undef, align 16
+double arr[32] __attribute__((loader_uninitialized));
+
+// Defining as arr2[] [[clang..]] raises the error: attribute cannot be applied to types
+// CHECK: @arr2 = global [4 x double] undef, align 16
+double arr2 [[clang::loader_uninitialized]] [4];

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

 // CHECK-NEXT: GNUInline (SubjectMatchRule_function)
 // CHECK-NEXT: HIPPinnedShadow (SubjectMatchRule_variable)
 // CHECK-NEXT: Hot (SubjectMatchRule_function)
 // CHECK-NEXT: IBAction (SubjectMatchRule_objc_method_is_instance)
 // CHECK-NEXT: IFunc (SubjectMatchRule_function)
 // CHECK-NEXT: InitPriority (SubjectMatchRule_variable)
 // CHECK-NEXT: InternalLinkage (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: LTOVisibilityPublic (SubjectMatchRule_record)
+// CHECK-NEXT: LoaderUninitialized (SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: Lockable (SubjectMatchRule_record)
 // CHECK-NEXT: MIGServerRoutine (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_block)
 // CHECK-NEXT: MSStruct (SubjectMatchRule_record)
 // CHECK-NEXT: MicroMips (SubjectMatchRule_function)
 // CHECK-NEXT: MinSize (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: MinVectorWidth (SubjectMatchRule_function)
 // CHECK-NEXT: Mips16 (SubjectMatchRule_function)
 // CHECK-NEXT: MipsLongCall (SubjectMatchRule_function)

clang/test/Sema/attr-loader-uninitialized.c

+// RUN: %clang_cc1 %s -verify -fsyntax-only
+// See also attr-loader-uninitialized.cpp
+
+int good __attribute__((loader_uninitialized));
+static int local_ok __attribute__((loader_uninitialized));
+int hidden_ok __attribute__((visibility("hidden"))) __attribute__((loader_uninitialized));
+
+const int can_still_be_const __attribute__((loader_uninitialized));
+
+extern int external_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable 'external_rejected' cannot be declared both 'extern' and with the 'loader_uninitialized' attribute}}
+
+int noargs __attribute__((loader_uninitialized(0)));
+// expected-error@-1 {{'loader_uninitialized' attribute takes no arguments}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+int declaration_then_uninit_ok;
+int declaration_then_uninit_ok __attribute__((loader_uninitialized));
+
+int definition_then_uninit_rejected = 0;
+int definition_then_uninit_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{redeclaration cannot add 'loader_uninitialized' attribute}}
+// expected-note@-3 {{previous definition is here}}
+
+int tentative_repeated_ok __attribute__((loader_uninitialized));
+int tentative_repeated_ok __attribute__((loader_uninitialized));
+
+__private_extern__ int private_extern_can_be_initialised = 10;
+__private_extern__ int therefore_uninit_private_extern_ok __attribute__((loader_uninitialized));
+
+__private_extern__ int initialized_private_extern_rejected __attribute__((loader_uninitialized)) = 5;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+extern __attribute__((visibility("hidden"))) int extern_hidden __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable 'extern_hidden' cannot be declared both 'extern' and with the 'loader_uninitialized' attribute}}

clang/test/Sema/attr-loader-uninitialized.cpp

+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+static int local_ok __attribute__((loader_uninitialized));
+int hidden_ok __attribute__((visibility("hidden"))) __attribute__((loader_uninitialized));
+
+const int still_cant_be_const __attribute__((loader_uninitialized));
+// expected-error@-1 {{default initialization of an object of const type}}
+extern int external_rejected __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable 'external_rejected' cannot be declared both 'extern' and with the 'loader_uninitialized' attribute}}
+
+int noargs __attribute__((loader_uninitialized(0)));
+// expected-error@-1 {{'loader_uninitialized' attribute takes no arguments}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+void func() __attribute__((loader_uninitialized))
+// expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized));
+  // expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field;
+  // expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized));
+// expected-warning@-1 {{'loader_uninitialized' attribute only applies to global variables}}
+
+int redef_attr_first __attribute__((loader_uninitialized));
+int redef_attr_first;
+// expected-error@-1 {{redefinition of 'redef_attr_first'}}
+// expected-note@-3 {{previous definition is here}}
+
+int redef_attr_second;
+int redef_attr_second __attribute__((loader_uninitialized));
+// expected-warning@-1 {{attribute declaration must precede definition}}
+// expected-note@-3 {{previous definition is here}}
+// expected-error@-3 {{redefinition of 'redef_attr_second'}}
+// expected-note@-5 {{previous definition is here}}
+
+struct trivial {};
+
+trivial default_ok __attribute__((loader_uninitialized));
+trivial value_rejected  __attribute__((loader_uninitialized)) {};
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+struct nontrivial
+{
+  nontrivial() {}
+};
+
+nontrivial needs_trivial_ctor __attribute__((loader_uninitialized));
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute must have a trivial default constructor}}