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

[Jon Chesterfield via Phabricator via cfe-commits]

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



================
Comment at: clang/lib/Sema/SemaDecl.cpp:12377
+      }
+      if (Var->getStorageClass() == SC_Extern) {
+        Diag(Var->getLocation(), diag::err_loader_uninitialized_extern);
----------------
aaron.ballman wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74361