[PATCH] D156337: [clang] Allow setting the uninitialized attribute on record

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

In D156337#4558410 , @aaron.ballman 
wrote:

> Was there an RFC for this extension to the attribute? (There doesn't need to 
> be one, I'm just wondering if there's more background info on what's driving 
> this patch forward and discussion around the design.)
>
> I'd like some more details about how this attribute impacts class 
> hierarchies. e.g., if you put the attribute on the base class, does it impact 
> the derived class members as well, or just the base class members? Also, what 
> should happen in a case like this:
>
>   template 
>   void func() {
> Ty Val; // Does this know it's uninitialized? Or did we lose that 
> information because this isn't a type attribute?
>   }
>   
>   struct __attribute__((uninitialized)) S { int  value; };
>   
>   int main() {
> func();
>   }

All very relevant topics. Concerning inheritance, I'd say that if the base 
class is marked as uninitialized and the child class is not, then base members 
are uninitialized and child members are not.

Concerning your example, I'd expect `Val` to be uninitialized as it's type has 
the according attribute, but I also don't quite understand "Or did we lose that 
information because this isn't a type attribute?"


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

https://reviews.llvm.org/D156337

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

[PATCH] D156337: [clang] Allow setting the uninitialized attribute on record

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Was there an RFC for this extension to the attribute? (There doesn't need to be 
one, I'm just wondering if there's more background info on what's driving this 
patch forward and discussion around the design.)

I'd like some more details about how this attribute impacts class hierarchies. 
e.g., if you put the attribute on the base class, does it impact the derived 
class members as well, or just the base class members? Also, what should happen 
in a case like this:

  template 
  void func() {
Ty Val; // Does this know it's uninitialized? Or did we lose that 
information because this isn't a type attribute?
  }
  
  struct __attribute__((uninitialized)) S { int  value; };
  
  int main() {
func();
  }




Comment at: clang/include/clang/Basic/AttrDocs.td:5789
+command-line parameter, forcing variables to remain uninitialized.
+When set on a struct or class, all stack variables of this type are affected.
+





Comment at: clang/lib/CodeGen/CGDecl.cpp:1908
 
-  // Note: constexpr already initializes everything correctly.
-  LangOptions::TrivialAutoVarInitKind trivialAutoVarInit =
-  (D.isConstexpr()
-   ? LangOptions::TrivialAutoVarInitKind::Uninitialized
-   : (D.getAttr()
-  ? LangOptions::TrivialAutoVarInitKind::Uninitialized
-  : getContext().getLangOpts().getTrivialAutoVarInit()));
+  LangOptions::TrivialAutoVarInitKind trivialAutoVarInit;
+  if (D.isConstexpr()) {




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

https://reviews.llvm.org/D156337

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

[PATCH] D156337: [clang] Allow setting the uninitialized attribute on record

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 545074.
serge-sans-paille added a comment.

(rebased on `main`)


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

https://reviews.llvm.org/D156337

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenCXX/trivial-auto-var-init-attribute.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-uninitialized.c
  clang/test/SemaCXX/constant-expression-cxx14.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp

Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -1362,6 +1362,9 @@
   static_assert( == (Y));
 }
 
+struct __attribute__((uninitialized)) j { int v; constexpr j() {}};
+static_assert(j().v == 0, ""); // expected-error {{constant expression}} expected-note {{read of uninitialized object is not allowed in a constant expression}}
+
 namespace PR45133 {
   struct A { long x; };
 
Index: clang/test/SemaCXX/constant-expression-cxx14.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx14.cpp
+++ clang/test/SemaCXX/constant-expression-cxx14.cpp
@@ -850,6 +850,13 @@
 ({ if (true) {} }); // expected-note {{not supported}}
 return 0;
   }
+
+  // Make sure the uninitialized attribute does not silent constant expression
+  // warnings.
+  constexpr int i() {
+return ({ __attribute__((uninitialized)) int n; n; }); // expected-note {{read of uninitialized object}}
+  }
+  static_assert(i() == 0, ""); // expected-error {{constant expression}} expected-note {{in call}}
 }
 
 namespace VirtualFromBase {
Index: clang/test/Sema/attr-uninitialized.c
===
--- clang/test/Sema/attr-uninitialized.c
+++ clang/test/Sema/attr-uninitialized.c
@@ -18,4 +18,4 @@
 
 struct TheWordIsOut {
   __attribute((uninitialized)) int youre_doin_wrong; // expected-warning {{'uninitialized' attribute only applies to local variables}}
-} __attribute((uninitialized));  // expected-warning {{'uninitialized' attribute only applies to local variables}}
+} __attribute((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
@@ -188,7 +188,7 @@
 // CHECK-NEXT: TargetVersion (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
-// CHECK-NEXT: Uninitialized (SubjectMatchRule_variable_is_local)
+// CHECK-NEXT: Uninitialized (SubjectMatchRule_variable_is_local, SubjectMatchRule_record)
 // CHECK-NEXT: UnsafeBufferUsage (SubjectMatchRule_function)
 // CHECK-NEXT: UseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
Index: clang/test/CodeGenCXX/trivial-auto-var-init-attribute.cpp
===
--- clang/test/CodeGenCXX/trivial-auto-var-init-attribute.cpp
+++ clang/test/CodeGenCXX/trivial-auto-var-init-attribute.cpp
@@ -38,4 +38,24 @@
 }
 #pragma clang attribute pop
 
+struct [[clang::uninitialized]] uninitialized_record {
+  int i;
+};
+
+// UNINIT-LABEL:  test_record_attribute_uninitialized(
+// UNINIT:  alloca
+// UNINIT-NEXT: call void
+// ZERO-LABEL:test_record_attribute_uninitialized(
+// ZERO:  alloca
+// ZERO-NOT:  !annotation
+// ZERO-NEXT: call void
+// PATTERN-LABEL: test_record_attribute_uninitialized(
+// PATTERN:  alloca
+// PATTERN-NOT:  !annotation
+// PATTERN-NEXT: call void
+void test_record_attribute_uninitialized() {
+  uninitialized_record some;
+  used(some);
+}
+
 } // extern "C"
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -8457,8 +8457,10 @@
 }
 
 static void handleUninitializedAttr(Sema , Decl *D, const ParsedAttr ) {
-  assert(cast(D)->getStorageDuration() == SD_Automatic &&
- "uninitialized is only valid on automatic duration variables");
+  assert((isa(D) ||
+  (cast(D)->getStorageDuration() == SD_Automatic)) &&
+ "uninitialized is only valid on automatic duration variables and "
+ "record type");
   D->addAttr(::new (S.Context) UninitializedAttr(S.Context, AL));
 }
 
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -1905,13 +1905,17 @@
   const 

[PATCH] D156337: [clang] Allow setting the uninitialized attribute on record

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 545060.
serge-sans-paille added a comment.

Take reviewers comment into account.


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

https://reviews.llvm.org/D156337

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenCXX/trivial-auto-var-init-attribute.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-uninitialized.c
  clang/test/SemaCXX/constant-expression-cxx14.cpp
  clang/test/SemaCXX/constant-expression-cxx2a.cpp

Index: clang/test/SemaCXX/constant-expression-cxx2a.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx2a.cpp
+++ clang/test/SemaCXX/constant-expression-cxx2a.cpp
@@ -1362,6 +1362,9 @@
   static_assert( == (Y));
 }
 
+struct __attribute__((uninitialized)) j { int v; constexpr j() {}};
+static_assert(j().v == 0, ""); // expected-error {{constant expression}} expected-note {{read of uninitialized object is not allowed in a constant expression}}
+
 namespace PR45133 {
   struct A { long x; };
 
Index: clang/test/SemaCXX/constant-expression-cxx14.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx14.cpp
+++ clang/test/SemaCXX/constant-expression-cxx14.cpp
@@ -849,6 +849,13 @@
 ({ if (true) {} }); // expected-note {{not supported}}
 return 0;
   }
+
+  // Make sure the uninitialized attribute does not silent constant expression
+  // warnings.
+  constexpr int i() {
+return ({ __attribute__((uninitialized)) int n; n; }); // expected-note {{read of uninitialized object}}
+  }
+  static_assert(i() == 0, ""); // expected-error {{constant expression}} expected-note {{in call}}
 }
 
 namespace VirtualFromBase {
Index: clang/test/Sema/attr-uninitialized.c
===
--- clang/test/Sema/attr-uninitialized.c
+++ clang/test/Sema/attr-uninitialized.c
@@ -18,4 +18,4 @@
 
 struct TheWordIsOut {
   __attribute((uninitialized)) int youre_doin_wrong; // expected-warning {{'uninitialized' attribute only applies to local variables}}
-} __attribute((uninitialized));  // expected-warning {{'uninitialized' attribute only applies to local variables}}
+} __attribute((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
@@ -188,7 +188,7 @@
 // CHECK-NEXT: TargetVersion (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
-// CHECK-NEXT: Uninitialized (SubjectMatchRule_variable_is_local)
+// CHECK-NEXT: Uninitialized (SubjectMatchRule_variable_is_local, SubjectMatchRule_record)
 // CHECK-NEXT: UnsafeBufferUsage (SubjectMatchRule_function)
 // CHECK-NEXT: UseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
Index: clang/test/CodeGenCXX/trivial-auto-var-init-attribute.cpp
===
--- clang/test/CodeGenCXX/trivial-auto-var-init-attribute.cpp
+++ clang/test/CodeGenCXX/trivial-auto-var-init-attribute.cpp
@@ -38,4 +38,24 @@
 }
 #pragma clang attribute pop
 
+struct [[clang::uninitialized]] uninitialized_record {
+  int i;
+};
+
+// UNINIT-LABEL:  test_record_attribute_uninitialized(
+// UNINIT:  alloca
+// UNINIT-NEXT: call void
+// ZERO-LABEL:test_record_attribute_uninitialized(
+// ZERO:  alloca
+// ZERO-NOT:  !annotation
+// ZERO-NEXT: call void
+// PATTERN-LABEL: test_record_attribute_uninitialized(
+// PATTERN:  alloca
+// PATTERN-NOT:  !annotation
+// PATTERN-NEXT: call void
+void test_record_attribute_uninitialized() {
+  uninitialized_record some;
+  used(some);
+}
+
 } // extern "C"
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -8438,8 +8438,10 @@
 }
 
 static void handleUninitializedAttr(Sema , Decl *D, const ParsedAttr ) {
-  assert(cast(D)->getStorageDuration() == SD_Automatic &&
- "uninitialized is only valid on automatic duration variables");
+  assert((isa(D) ||
+  (cast(D)->getStorageDuration() == SD_Automatic)) &&
+ "uninitialized is only valid on automatic duration variables and "
+ "record type");
   D->addAttr(::new (S.Context) UninitializedAttr(S.Context, AL));
 }
 
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -1902,13 

[PATCH] D156337: [clang] Allow setting the uninitialized attribute on record

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added inline comments.



Comment at: clang/lib/CodeGen/CGDecl.cpp:1906-1908
+  if (D.isConstexpr())
+// Note: constexpr already initializes everything correctly.
+trivialAutoVarInit = LangOptions::TrivialAutoVarInitKind::Uninitialized;

Should we have curly braces here?



Comment at: clang/lib/CodeGen/CGDecl.cpp:1909
+trivialAutoVarInit = LangOptions::TrivialAutoVarInitKind::Uninitialized;
+  else if (D.getAttr())
+trivialAutoVarInit = LangOptions::TrivialAutoVarInitKind::Uninitialized;





Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8442
+  assert(((isa(D) &&
+   cast(D)->getStorageDuration() == SD_Automatic) ||
+  isa(D)) &&

AFAIK `cast` does `isa`, since it is already done here we can probably switch 
to `dyn_cast`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156337

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

[PATCH] D156337: [clang] Allow setting the uninitialized attribute on record

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I think a test verifying that during constant evaluation we still flag the read 
of a local tagged uninitialized as ill-formed would be nice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156337

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

[PATCH] D156337: [clang] Allow setting the uninitialized attribute on record

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a reviewer: clang-language-wg.
shafik added a comment.

Adding clang-language-wg for more visibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156337

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

[PATCH] D156337: [clang] Allow setting the uninitialized attribute on record

[serge via Phabricator via cfe-commits]

serge-sans-paille created this revision.
serge-sans-paille added reviewers: jfb, aaron.ballman.
Herald added a subscriber: jdoerfert.
Herald added a project: All.
serge-sans-paille requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This attribute would then affect all stack variables of that type, when the 
type has been evaluated as ok wrt. initialisation, or a for performance / 
security trade-offs.
An example of use could be container with preallocated storage like 
SmallVector, if we consider that it is too costly to initialize that storage.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156337

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGenCXX/trivial-auto-var-init-attribute.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Sema/attr-uninitialized.c

Index: clang/test/Sema/attr-uninitialized.c
===
--- clang/test/Sema/attr-uninitialized.c
+++ clang/test/Sema/attr-uninitialized.c
@@ -18,4 +18,4 @@
 
 struct TheWordIsOut {
   __attribute((uninitialized)) int youre_doin_wrong; // expected-warning {{'uninitialized' attribute only applies to local variables}}
-} __attribute((uninitialized));  // expected-warning {{'uninitialized' attribute only applies to local variables}}
+} __attribute((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
@@ -188,7 +188,7 @@
 // CHECK-NEXT: TargetVersion (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
-// CHECK-NEXT: Uninitialized (SubjectMatchRule_variable_is_local)
+// CHECK-NEXT: Uninitialized (SubjectMatchRule_variable_is_local, SubjectMatchRule_record)
 // CHECK-NEXT: UnsafeBufferUsage (SubjectMatchRule_function)
 // CHECK-NEXT: UseHandle (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: VecReturn (SubjectMatchRule_record)
Index: clang/test/CodeGenCXX/trivial-auto-var-init-attribute.cpp
===
--- clang/test/CodeGenCXX/trivial-auto-var-init-attribute.cpp
+++ clang/test/CodeGenCXX/trivial-auto-var-init-attribute.cpp
@@ -38,4 +38,24 @@
 }
 #pragma clang attribute pop
 
+struct [[clang::uninitialized]] uninitialized_record {
+  int i;
+};
+
+// UNINIT-LABEL:  test_record_attribute_uninitialized(
+// UNINIT:  alloca
+// UNINIT-NEXT: call void
+// ZERO-LABEL:test_record_attribute_uninitialized(
+// ZERO:  alloca
+// ZERO-NOT:  !annotation
+// ZERO-NEXT: call void
+// PATTERN-LABEL: test_record_attribute_uninitialized(
+// PATTERN:  alloca
+// PATTERN-NOT:  !annotation
+// PATTERN-NEXT: call void
+void test_record_attribute_uninitialized() {
+  uninitialized_record some;
+  used(some);
+}
+
 } // extern "C"
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -8438,8 +8438,11 @@
 }
 
 static void handleUninitializedAttr(Sema , Decl *D, const ParsedAttr ) {
-  assert(cast(D)->getStorageDuration() == SD_Automatic &&
- "uninitialized is only valid on automatic duration variables");
+  assert(((isa(D) &&
+   cast(D)->getStorageDuration() == SD_Automatic) ||
+  isa(D)) &&
+ "uninitialized is only valid on automatic duration variables and "
+ "record type");
   D->addAttr(::new (S.Context) UninitializedAttr(S.Context, AL));
 }
 
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -1902,13 +1902,17 @@
   const Address Loc =
   locIsByrefHeader ? emission.getObjectAddress(*this) : emission.Addr;
 
-  // Note: constexpr already initializes everything correctly.
-  LangOptions::TrivialAutoVarInitKind trivialAutoVarInit =
-  (D.isConstexpr()
-   ? LangOptions::TrivialAutoVarInitKind::Uninitialized
-   : (D.getAttr()
-  ? LangOptions::TrivialAutoVarInitKind::Uninitialized
-  : getContext().getLangOpts().getTrivialAutoVarInit()));
+  LangOptions::TrivialAutoVarInitKind trivialAutoVarInit;
+  if (D.isConstexpr())
+// Note: constexpr already initializes everything correctly.
+trivialAutoVarInit = LangOptions::TrivialAutoVarInitKind::Uninitialized;
+  else if (D.getAttr())
+trivialAutoVarInit = LangOptions::TrivialAutoVarInitKind::Uninitialized;
+  else if (const auto *RecordTy = D.getType()->getAsRecordDecl();