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

[Jon Chesterfield via Phabricator via cfe-commits]

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)]];
  #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.


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jon Chesterfield via Phabricator via cfe-commits]

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


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

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

[Erich Keane via Phabricator via cfe-commits]

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


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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


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

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

[Erich Keane via Phabricator via cfe-commits]

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


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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


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

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

[Erich Keane via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Erich Keane via Phabricator via cfe-commits]

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


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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


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

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

[Nico Weber via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D74361



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jon Chesterfield via Phabricator via cfe-commits]

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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.c
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,60 @@
+// 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}}
Index: clang/test/Sema/attr-loader-uninitialized.c
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.c
@@ -0,0 +1,37 @@
+// 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_uninitiali

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

[Jon Chesterfield via Phabricator via cfe-commits]

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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.c
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,60 @@
+// 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}}
Index: clang/test/Sema/attr-loader-uninitialized.c
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.c
@@ -0,0 +1,37 @@
+// 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 c

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

[John McCall via Phabricator via cfe-commits]

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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.c
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,60 @@
+// 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}}
Index: clang/test/Sema/attr-loader-uninitialized.c
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.c
@@ -0,0 +1,37 @@
+// 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));
+//

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

[Jon Chesterfield via Phabricator via cfe-commits]

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

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.c
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,60 @@
+// 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}}
Index: clang/test/Sema/attr-loader-uninitialized.c
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.c
@@ -0,0 +1,37 @@
+// 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-

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

[Jon Chesterfield via Phabricator via cfe-commits]

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: clang/include/clang/Basic/DiagnosticSemaKinds.td:5344
+def err_loader_uninitialized_extern_decl : Error<
+  "external declaration of variable cannot have the 'loader_uninitialized' 
attribute">;
 def err_block_extern_cant_init : Error<

aaron.ballman wrote:
> 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"?
Better, thanks. Also discovered clang-format has learned to do sensible things 
with tablegen so applied it to the new defs.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

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: clang/include/clang/Basic/DiagnosticSemaKinds.td:5344
+def err_loader_uninitialized_extern_decl : Error<
+  "external declaration of variable cannot have the 'loader_uninitialized' 
attribute">;
 def err_block_extern_cant_init : Error<

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"?


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jon Chesterfield via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.c
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,60 @@
+// 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 {{external declaration of variable cannot have 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}}
Index: clang/test/Sema/attr-loader-uninitialized.c
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.c
@@ -0,0 +1,37 @@
+// 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 {{external declaration of variable cannot have 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 c

[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/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 end of the sentence. The comment can probably be 
re-flowed with the preceding sentence too.



Comment at: clang/lib/Sema/SemaDecl.cpp:11949
 
+  // The LoaderUninitialized attribute acts as a definition (of undef)
+  if (VDecl->hasAttr()) {

Missing full stop.



Comment at: clang/lib/Sema/SemaDecl.cpp:12377
+  }
+  if (Var->getStorageClass() == SC_Extern) {
+Diag(Var->getLocation(), diag::err_loader_uninitialized_extern);

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?



Comment at: clang/test/CodeGen/attr-loader-uninitialized.c:4
+// CHECK: @tentative_attr_first = weak global i32 undef, align 4
+int tentative_attr_first __attribute__((loader_uninitialized));
+int tentative_attr_first;

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



Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:4
+// CHECK: @defn = global i32 undef
+int defn  [[clang::loader_uninitialized]];
+

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


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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)
- 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?


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.c
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __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 with 'loader_uninitialized' attribute cannot have external linkage}}
+
+int noargs __attribute__((loader_uninitialized(0)));
+// expected-error@-1 {{'loader_uninitialized' attribute takes no arguments}} 
+
+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}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+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}}
Index: clang/test/Sema/attr-loader-uninitialized.c
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+// See also attr-loader-uninitialized.cpp
+
+int good __attribute__((loader_uninitialized));
+
+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));
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -65,6 +65,7 @@
 // CHECK-NEXT: InitPriority (SubjectMatchRule_variable)
 // CHECK-NEXT: InternalLinkage (SubjectMatchRule_variable, SubjectMatchRule_function, SubjectMatchRule_record)
 // CHECK-NEXT: LTOVisibilityPublic (SubjectMatch

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

[Jon Chesterfield via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __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 with 'loader_uninitialized' attribute cannot have external linkage}}
+
+int noargs __attribute__((loader_uninitialized(0)));
+// expected-error@-1 {{'loader_uninitialized' attribute takes no arguments}} 
+
+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}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+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}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -65,6 +65,7 @@
 // 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)
Index: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
@@ -0,0 +1,29 @@
+// 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::loade

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

[Jon Chesterfield via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+
+const int still_cant_be_const __attribute__((loader_uninitialized)); // expected-error {{default initialization of an object of const type}}
+extern int external_should_be_rejected __attribute__((loader_uninitialized));
+
+int noargs __attribute__((loader_uninitialized(0))); // expected-error {{'loader_uninitialized' attribute takes no arguments}} 
+
+void func() __attribute__((loader_uninitialized)) // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field; // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized)); // expected-warning {{'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}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
+
+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}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -65,6 +65,7 @@
 // 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)
Index: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::loader_uninitialized]];
+  return &data;
+}
+
+// No code emitted
+extern int extern_unhelpful_but_harmless [[clang::loader_uninitialized]];
+
+// 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));
+
+class trivial
+{
+  float x;

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

[Jon Chesterfield via Phabricator via cfe-commits]

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


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

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

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:
> > rjmccall wrote:
> > > Quuxplusone wrote:
> > > > rjmccall wrote:
> > > > > JonChesterfield wrote:
> > > > > > Quuxplusone wrote:
> > > > > > > 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?
> > > > > > 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.
> > > > > 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.
> > > > 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'

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

[John McCall via Phabricator via cfe-commits]

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:
> > Quuxplusone wrote:
> > > rjmccall wrote:
> > > > JonChesterfield wrote:
> > > > > Quuxplusone wrote:
> > > > > > 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?
> > > > > 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.
> > > > 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.
> > > 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";
> > > 

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

[John McCall via Phabricator via cfe-commits]

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:
> > JonChesterfield wrote:
> > > Quuxplusone wrote:
> > > > 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?
> > > 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.
> > 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.
> 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.
> 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 

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

[Jon Chesterfield via Phabricator via cfe-commits]

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;
+

Quuxplusone wrote:
> rjmccall wrote:
> > JonChesterfield wrote:
> > > Quuxplusone wrote:
> > > > 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?
> > > 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.
> > 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.
> 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.
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 {}; // de

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

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:
> > Quuxplusone wrote:
> > > 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?
> > 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.
> 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.
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.


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

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

[John McCall via Phabricator via cfe-commits]

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


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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 loader is expensive.
+  }];

rjmccall wrote:
> 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.
That's a lot better! Thank you. Updated the patch to use your wording verbatim.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7436
+  case ParsedAttr::AT_LoaderUninitialized:
+handleLoaderUninitializedAttr(S, D, AL);
+break;

aaron.ballman wrote:
> 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(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.
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.



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:
> 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?
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 defau

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

[Jon Chesterfield via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D74361/new/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+
+const int still_cant_be_const __attribute__((loader_uninitialized)); // expected-error {{default initialization of an object of const type}}
+extern int external_should_be_rejected __attribute__((loader_uninitialized));
+
+int noargs __attribute__((loader_uninitialized(0))); // expected-error {{'loader_uninitialized' attribute takes no arguments}} 
+
+void func() __attribute__((loader_uninitialized)) // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field; // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized)); // expected-warning {{'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}}
+
+int init_rejected __attribute__((loader_uninitialized)) = 42;
+// expected-error@-1 {{variable with 'loader_uninitialized' attribute cannot have an initializer}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -65,6 +65,7 @@
 // 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)
Index: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::loader_uninitialized]];
+  return &data;
+}
+
+// No code emitted
+extern int extern_unhelpful_but_harmless [[clang::loader_uninitialized]];
+
+// 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));
+
+class trivial
+{
+  float x;
+};
+
+// CHECK: @ut = global %class.trivial undef
+trivial ut [[clang::loader_uninitialized]];
+
+struct nontrivial
+{
+  nontrivial() : x(3.14) {}
+  double x;
+};
+
+// CHECK: @unt = global %struct.nontrivial undef
+nontrivial unt [[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
+// CH

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

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


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

[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(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

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

[John McCall via Phabricator via cfe-commits]

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:

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



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7436
+  case ParsedAttr::AT_LoaderUninitialized:
+handleLoaderUninitializedAttr(S, D, AL);
+break;

JonChesterfield wrote:
> aaron.ballman wrote:
> > If you don't need any custom semantic checking, you can remove that 
> > function and instead call 
> > `handleSimpleAttribute(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.


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7436
+  case ParsedAttr::AT_LoaderUninitialized:
+handleLoaderUninitializedAttr(S, D, AL);
+break;

aaron.ballman wrote:
> If you don't need any custom semantic checking, you can remove that function 
> and instead call `handleSimpleAttribute(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.



Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:14
+// CHECK: @tentative = global i32 undef
+int tentative  [[clang::loader_uninitialized]];
+

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






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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D74361

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-loader-uninitialized.c
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+const int still_cant_be_const __attribute__((loader_uninitialized)); // expected-error {{default initialization of an object of const type}}
+extern int external __attribute__((loader_uninitialized));
+
+int noargs __attribute__((loader_uninitialized(0))); // expected-error {{'loader_uninitialized' attribute takes no arguments}} 
+
+void func() __attribute__((loader_uninitialized)) // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field; // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized)); // expected-warning {{'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}}
+
+// Would like sema to reject this, but that is not yet implemented
+int multiple_definitions __attribute__((loader_uninitialized)) = 42;
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -65,6 +65,7 @@
 // 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)
Index: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::loader_uninitialized]];
+  return &data;
+}
+
+// No code emitted
+extern int extern_unhelpful_but_harmless [[clang::loader_uninitialized]];
+
+// 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: @nominally_zero_init = global i32 undef
+int nominally_zero_init  [[clang::loader_uninitialized]] = 0;
+
+// CHECK: @nominally_value_init = global i32 undef
+int nominally_value_init  [[clang::loader_uninitialized]] = 4;
+
+class trivial
+{
+  float x;
+};
+
+// CHECK: @ut = global %class.trivial undef
+trivial ut [[clang::loader_uninitialized]];
+
+struct nontrivial
+{
+  nontrivial() : x(3.14) {}
+  double x;
+};
+
+// CHECK: @unt = global %struct.nontrivial undef
+nontrivial unt [[clang::loader_uninitialized]];
+
+// CHECK: @arr = global [32 x double] undef, align 16
+double arr[32] __attribute__((loader_uninitialized));
+

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

[Aaron Ballman via Phabricator via cfe-commits]

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 initialised by the loader.
+This is useful for variables that are always written to before use where the

initialised -> initialized



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7436
+  case ParsedAttr::AT_LoaderUninitialized:
+handleLoaderUninitializedAttr(S, D, AL);
+break;

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



Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:14
+// CHECK: @tentative = global i32 undef
+int tentative  [[clang::loader_uninitialized]];
+

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.



Comment at: clang/test/Sema/attr-loader-uninitialized.cpp:19
+
+} __attribute__((loader_uninitialized)); // expected-warning 
{{'loader_uninitialized' attribute only applies to global variables}}

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


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-loader-uninitialized.cpp

Index: clang/test/Sema/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-loader-uninitialized.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((loader_uninitialized));
+const int still_cant_be_const __attribute__((loader_uninitialized)); // expected-error {{default initialization of an object of const type}}
+extern int external __attribute__((loader_uninitialized));
+
+void func() __attribute__((loader_uninitialized)) // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+{
+  int local __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static int sl __attribute__((loader_uninitialized));
+}
+
+struct s {
+  __attribute__((loader_uninitialized)) int field; // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
+
+  static __attribute__((loader_uninitialized)) int sfield;
+
+} __attribute__((loader_uninitialized)); // expected-warning {{'loader_uninitialized' attribute only applies to global variables}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -65,6 +65,7 @@
 // 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)
Index: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-loader-uninitialized.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::loader_uninitialized]];
+  return &data;
+}
+
+// No code emitted
+extern int extern_unhelpful_but_harmless [[clang::loader_uninitialized]];
+
+// CHECK: @tentative = global i32 undef
+int tentative  [[clang::loader_uninitialized]];
+
+// CHECK: @_ZL16tentative_static = internal global i32 undef
+static int tentative_static [[clang::loader_uninitialized]] __attribute__((used));
+
+// CHECK: @nominally_zero_init = global i32 undef
+int nominally_zero_init  [[clang::loader_uninitialized]] = 0;
+
+// CHECK: @nominally_value_init = global i32 undef
+int nominally_value_init  [[clang::loader_uninitialized]] = 4;
+
+class trivial
+{
+  float x;
+};
+
+// CHECK: @ut = global %class.trivial undef
+trivial ut [[clang::loader_uninitialized]];
+
+struct nontrivial
+{
+  nontrivial() : x(3.14) {}
+  double x;
+};
+
+// CHECK: @unt = global %struct.nontrivial undef
+nontrivial unt [[clang::loader_uninitialized]];
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6505,6 +6505,11 @@
   D->addAttr(::new (S.Context) UninitializedAttr(S.Context, AL));
 }
 
+static void handleLoaderUninitializedAttr(Sema &S, Decl *D,
+const ParsedAttr &AL) {
+  D->addAttr(::new (S.Context) LoaderUninitializedAttr(S.Context, AL));
+}
+
 static bool tryMakeVariablePseudoStrong(Sema &S, VarDecl *VD,
 bool DiagnoseFailure) {
   QualType Ty = VD->getType();
@@ -7427,6 +7432,10 @@
 handleUninitializedAttr(S, D, AL);
 break;
 
+  case ParsedAttr::AT_LoaderUninitialized:
+handleLoaderUninitializedAttr(S, D, AL);
+break;
+
   case ParsedAttr::AT_ObjCExternallyRetained:
 handleObjCExternallyRetainedAttr(S, D, AL);
 break;
Index: clang/lib/CodeGen/CodeGenModule.cpp

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

[Jon Chesterfield via Phabricator via cfe-commits]

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


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

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

[John McCall via Phabricator via cfe-commits]

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




Comment at: clang/include/clang/Basic/Attr.td:3436
+  let Subjects = SubjectList<[GlobalVar]>;
+  let Documentation = [Undocumented];
+}

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.


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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]];

Quuxplusone wrote:
> 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](https://wg21.link/p1144) (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.
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.


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

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

[Arthur O'Dwyer via Phabricator via cfe-commits]

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++ 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](https://wg21.link/p1144) (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.


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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
  clang/lib/AST/DeclBase.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenCXX/attr-no-zero-initializer.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-no-zero-initializer.cpp

Index: clang/test/Sema/attr-no-zero-initializer.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-no-zero-initializer.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only
+
+int good __attribute__((no_zero_initializer));
+const int still_cant_be_const __attribute__((no_zero_initializer)); // expected-error {{default initialization of an object of const type}}
+extern int external __attribute__((no_zero_initializer));
+
+void func() __attribute__((no_zero_initializer)) // expected-warning {{'no_zero_initializer' attribute only applies to global variables}}
+{
+  int local __attribute__((no_zero_initializer)); // expected-warning {{'no_zero_initializer' attribute only applies to global variables}}
+
+  static int sl __attribute__((no_zero_initializer));
+}
+
+struct s {
+  __attribute__((no_zero_initializer)) int field; // expected-warning {{'no_zero_initializer' attribute only applies to global variables}}
+
+  static __attribute__((no_zero_initializer)) int sfield;
+
+} __attribute__((no_zero_initializer)); // expected-warning {{'no_zero_initializer' attribute only applies to global variables}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -94,6 +94,7 @@
 // CHECK-NEXT: NoStackProtector (SubjectMatchRule_function)
 // CHECK-NEXT: NoThreadSafetyAnalysis (SubjectMatchRule_function)
 // CHECK-NEXT: NoThrow (SubjectMatchRule_hasType_functionType)
+// CHECK-NEXT: NoZeroInitializer (SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NotTailCalled (SubjectMatchRule_function)
 // CHECK-NEXT: OSConsumed (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: OSReturnsNotRetained (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property, SubjectMatchRule_variable_is_parameter)
Index: clang/test/CodeGenCXX/attr-no-zero-initializer.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/attr-no-zero-initializer.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -std=c++11 -emit-llvm -o - %s | FileCheck %s
+
+// CHECK: @_ZZ4funcvE4data = internal global i32 undef
+int* func(void)
+{
+  static int data [[clang::no_zero_initializer]];
+  return &data;
+}
+
+// No code emitted
+extern int extern_unhelpful_but_harmless [[clang::no_zero_initializer]];
+
+// CHECK: @tentative = global i32 undef
+int tentative  [[clang::no_zero_initializer]];
+
+// CHECK: @_ZL16tentative_static = internal global i32 undef
+static int tentative_static [[clang::no_zero_initializer]] __attribute__((used));
+
+// CHECK: @nominally_zero_init = global i32 undef
+int nominally_zero_init  [[clang::no_zero_initializer]] = 0;
+
+// CHECK: @nominally_value_init = global i32 undef
+int nominally_value_init  [[clang::no_zero_initializer]] = 4;
+
+class trivial
+{
+  float x;
+};
+
+// CHECK: @ut = global %class.trivial undef
+trivial ut [[clang::no_zero_initializer]];
+
+struct nontrivial
+{
+  nontrivial() : x(3.14) {}
+  double x;
+};
+
+// CHECK: @unt = global %struct.nontrivial undef
+nontrivial unt [[clang::no_zero_initializer]];
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6505,6 +6505,11 @@
   D->addAttr(::new (S.Context) UninitializedAttr(S.Context, AL));
 }
 
+static void handleNoZeroInitializerAttr(Sema &S, Decl *D,
+const ParsedAttr &AL) {
+  D->addAttr(::new (S.Context) NoZeroInitializerAttr(S.Context, AL));
+}
+
 static bool tryMakeVariablePseudoStrong(Sema &S, VarDecl *VD,
 bool DiagnoseFailure) {
   QualType Ty = VD->getType();
@@ -7427,6 +7432,10 @@
 handleUninitializedAttr(S, D, AL);
 break;
 
+  case ParsedAttr::AT_NoZeroInitializer:
+handleNoZeroInitializerAttr(S, D, AL);
+break;
+
   case ParsedAttr::AT_ObjCExternallyRetained:
 handleObjCExternallyRetainedAttr(S, D, AL);
 break;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGe

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

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield marked an inline comment as done.
JonChesterfield added inline comments.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6509
+static void handleNoZeroInitializerAttr(Sema &S, Decl *D, const ParsedAttr 
&AL) {
+  D->addAttr(::new (S.Context) NoZeroInitializerAttr(S.Context, AL));
+}

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


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

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

[Jon Chesterfield via Phabricator via cfe-commits]

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


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