Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF updated this revision to Diff 70200. EricWF added a comment. Updating against recent ToT. https://reviews.llvm.org/D23385 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/AttributeList.h lib/Sema/Se

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D23385 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF added a comment. Are there any remaining issues with this patch? https://reviews.llvm.org/D23385 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF added inline comments. Comment at: include/clang/Basic/AttrDocs.td:836 @@ +835,3 @@ + let Content = [{ +This attribute specifies that the variable to which it is attached is intended +to have a `constant initializer

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

rsmith added inline comments. Comment at: include/clang/Basic/AttrDocs.td:836 @@ +835,3 @@ + let Content = [{ +This attribute specifies that the variable to which it is attached is intended +to have a `constant initializer

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF marked an inline comment as done. Comment at: utils/TableGen/ClangAttrEmitter.cpp:2794 @@ -2794,1 +2793,3 @@ +if ((*I)->getValueAsBit("Negated")) { + FnName += "Not"; Test += "!"; This is needed so that the `COnly` and `CPlusPlus` entries in

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF updated this revision to Diff 69775. EricWF added a comment. Make the attribute C++ only. https://reviews.llvm.org/D23385 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/AttributeList.h lib/Sema/Se

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF updated this revision to Diff 69761. EricWF added a comment. Address all comments except making the attribute C++ only. https://reviews.llvm.org/D23385 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF marked 3 inline comments as done. Comment at: include/clang/Basic/AttrDocs.td:836 @@ +835,3 @@ + let Content = [{ +This attribute specifies that the variable to which it is attached is intended +to have a `constant initializer

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

aaron.ballman added inline comments. Comment at: include/clang/Basic/AttrDocs.td:836 @@ +835,3 @@ + let Content = [{ +This attribute specifies that the variable to which it is attached is intended +to have a `constant initializer

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF updated this revision to Diff 68121. EricWF added a comment. Do strict 'constant initializer' checking in C++11, but fall back to `isConstantInitializer()` in C++03. Add documentation about non-strict C++03 checking. This has weird results for POD types, for example: struct PODType {

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

rsmith added inline comments. Comment at: lib/Sema/SemaDecl.cpp:10519-10520 @@ +10518,4 @@ + auto *CE = dyn_cast(Init); + bool DiagErr = (var->isInitKnownICE() || (CE && CE->getConstructor()->isConstexpr())) + ? !var->checkInitIsICE() : !checkConstInit(); +

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF added inline comments. Comment at: lib/Sema/SemaDecl.cpp:10519-10520 @@ +10518,4 @@ + auto *CE = dyn_cast(Init); + bool DiagErr = (var->isInitKnownICE() || (CE && CE->getConstructor()->isConstexpr())) + ? !var->checkInitIsICE() : !checkConstInit(); +

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

rsmith added inline comments. Comment at: include/clang/Basic/AttrDocs.td:858 @@ +857,3 @@ + }; + SAFE_STATIC T x = {42}; // OK. + SAFE_STATIC T y = 42; // error: variable does not have a constant initializer Sure, if that's a conscious design decision for the

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF updated this revision to Diff 68094. EricWF marked 2 inline comments as done. EricWF added a comment. Address most of @rsmiths review comments. https://reviews.llvm.org/D23385 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF marked 7 inline comments as done. Comment at: include/clang/Basic/AttrDocs.td:844 @@ +843,3 @@ +the indeterminate order of dynamic initialization. They can also be safely +used by other static constructors across translation units. + rsmith wrote: > static

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

rsmith added inline comments. Comment at: include/clang/Basic/Attr.td:1384 @@ +1383,3 @@ +def RequireConstantInit : InheritableAttr { + let Spellings = [GCC<"require_constant_initialization">, + CXX11<"clang", "require_constant_initialization">]; ---

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF updated this revision to Diff 67987. EricWF added a comment. Fix missing semicolon. https://reviews.llvm.org/D23385 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/AttributeList.h lib/Sema/SemaDecl

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF updated this revision to Diff 67986. EricWF marked an inline comment as done. EricWF added a comment. Get all tests passing regarding duplicate warnings between `__attribute__((requires_constant_initialization))` and `-Wglobal-constructors`. The warning will not emit a duplicate warning a

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF marked an inline comment as done. Comment at: lib/Sema/SemaDecl.cpp:10528 @@ +10527,3 @@ +var->getLocation())) { + // Warn about globals which don't have a constant initializer. Don't + // warn about globals with a non-trivial

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF added inline comments. Comment at: lib/Sema/SemaDecl.cpp:10484-10485 @@ -10478,1 +10483,4 @@ + if (var->hasAttr() && !Init) + Diag(var->getLocation(), diag::err_require_constant_init_failed); + rsmith wrote: > I think this check is incorrect: we perf

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF updated this revision to Diff 67983. EricWF marked 2 inline comments as done. EricWF added a comment. Attempt to address Richards concerns about duplicate diagnostics and add tests for those cases. Unfortunately the tests are failing, because there is a duplicate -Wglobal-constructors di

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

rsmith added a comment. Sorry about the redundant previous comment, the thread got forked by the change of subject and I missed the updated patch. Comment at: lib/Sema/SemaDecl.cpp:10484-10485 @@ -10478,1 +10483,4 @@ + if (var->hasAttr() && !Init) + Diag(var->getLocation

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

rsmith added a comment. Attributes are specified in include/clang/Basic/Attr.td with a (hopefully) fairly self-explanatory declarative syntax. You'll then need to add code to lib/Sema/SemaDeclAttr.cpp to create a corresponding attribute object and attach it to the declaration. Two implementati

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

majnemer added inline comments. Comment at: lib/Sema/SemaDecl.cpp:10388-10390 @@ -10387,2 +10387,5 @@ + // Cache the result of checking for constant initialization. + Optional HasConstInit; + if (var->getTLSKind() == VarDecl::TLS_Static) { You could use a l

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF added inline comments. Comment at: include/clang/Basic/Attr.td:1384 @@ +1383,3 @@ +def RequireConstantInit : InheritableAttr { + let Spellings = [GCC<"require_constant_initialization">]; + let Subjects = SubjectList<[Var]>; aaron.ballman wrote: > This sho

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF updated this revision to Diff 67827. EricWF marked 7 inline comments as done. EricWF added a comment. Address @aaron.ballman's review comments. https://reviews.llvm.org/D23385 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:1384 @@ +1383,3 @@ +def RequireConstantInit : InheritableAttr { + let Spellings = [GCC<"require_constant_initialization">]; + let Subjects = SubjectList<[Var]>; This should not use the

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF marked an inline comment as done. Comment at: lib/AST/Expr.cpp:2653-2662 @@ -2651,4 +2652,12 @@ } - +if (CE->getConstructor()->isConstexpr() && +(CE->getConstructor()->getParent()->hasTrivialDestructor() || +AllowNonLiteral)) { +for (aut

Re: [PATCH] D23385: Implement __attribute__((require_constant_initialization)) for safe static initialization.

EricWF retitled this revision from "Implement __attribute__((require_constant_initialization)) attribute" to "Implement __attribute__((require_constant_initialization)) for safe static initialization.". EricWF updated this revision to Diff 67822. EricWF added a comment. - Remove test from previ