Page MenuHomePhabricator

Implement __attribute__((require_constant_initialization)) for safe static initialization.
ClosedPublic

Authored by EricWF on Aug 10 2016, 5:04 PM.

Details

Summary

This attribute specifies expectations about the initialization of static and
thread local variables. Specifically that the variable has a
constant initializer
according to the rules of [basic.start.static]. Failure to meet this expectation
will result in an error.

Static objects with constant initializers avoid hard-to-find bugs caused by
the indeterminate order of dynamic initialization. They can also be safely
used by other static constructors across translation units.

This attribute acts as a compile time assertion that the requirements
for constant initialization have been met. Since these requirements change
between dialects and have subtle pitfalls it's important to fail fast instead
of silently falling back on dynamic initialization.

c++
  // -std=c++14
  #define SAFE_STATIC __attribute__((require_constant_initialization)) static
  struct T {
    constexpr T(int) {}
    ~T();
  };
  SAFE_STATIC T x = {42}; // OK.
  SAFE_STATIC T y = 42; // error: variable does not have a constant initializer
  // copy initialization is not a constant expression on a non-literal type.

This attribute can only be applied to objects with static or thread-local storage
duration.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
rsmith edited edge metadata.Aug 11 2016, 7:33 PM

Have you considered modelling this as an attribute on the declaration of the variable instead of as a separate check? If so, why do you prefer this approach? (If I were to suggest this for standardization, an attribute is the approach I'd probably take.)

lib/AST/Expr.cpp
2653–2662 ↗(On Diff #67790)

This doesn't look right: just because we're calling a constexpr constructor with constant arguments doesn't imply that the initializer is constant.

EricWF marked an inline comment as done.Aug 11 2016, 7:58 PM

Have you considered modelling this as an attribute on the declaration of the variable instead of as a separate check? If so, why do you prefer this approach? (If I were to suggest this for standardization, an attribute is the approach I'd probably take.)

An attribute would be a much better approach! I only went this root because I figured out how to do it. If you could give me a starting point I'll change it over to an attribute.

EricWF updated this revision to Diff 67821.Aug 12 2016, 4:57 AM
EricWF retitled this revision from Implement __has_constant_initializer(obj) expression traits. to Implement __attribute__((require_constant_initialization)) attribute.
EricWF updated this object.
EricWF edited edge metadata.

Switch to an attribute based implementation as suggested by @rsmith.

EricWF updated this revision to Diff 67822.Aug 12 2016, 5:04 AM
EricWF retitled this revision from Implement __attribute__((require_constant_initialization)) attribute to Implement __attribute__((require_constant_initialization)) for safe static initialization..
  • Remove test from previous implementation.
EricWF marked an inline comment as done.Aug 12 2016, 5:08 AM
EricWF added inline comments.
lib/AST/Expr.cpp
2653–2662 ↗(On Diff #67790)

I changed this to use VD->checkInitICE() when evaluating a constexpr constructor or if VD->isInitKnownICE(). Otherwise it falls back on isConstantInitializer()

aaron.ballman added inline comments.Aug 12 2016, 5:32 AM
include/clang/Basic/Attr.td
1384

This should not use the GCC spelling because GCC doesn't support the attribute. Do you expect this attribute to do anything useful in C? If not, this should just be a CXX11 spelling in the clang namespace.

1385

You might want to make a custom SubsetSubject for this. Check out NormalVar, SharedVar, and the likes in Attr.td. Then you can get rid of the custom handling in SemaDeclAttr.cpp.

include/clang/Basic/AttrDocs.td
835

Should be no need to specify the heading manually since there's only one spelling for the attribute.

847

s/acts an a/acts as a

include/clang/Basic/DiagnosticSemaKinds.td
6842

I would reword this to: "'require_constant_initialization' attribute only applies to variables with static or thread-local storage duration"

test/SemaCXX/attr-require-constant-initialization.cpp
2

This file should also get tests that ensure we properly diagnose the attribute when it doesn't apply to a declaration (such as a local variable, as well as nonsense like a function), test that it does not accept arguments, etc.

5–7

Does this comment actually apply?

EricWF updated this revision to Diff 67827.Aug 12 2016, 6:28 AM
EricWF marked 7 inline comments as done.

Address @aaron.ballman's review comments.

include/clang/Basic/Attr.td
1384

Are CXX11 attributes usable in C++03? If not perhaps we should provide both?

test/SemaCXX/attr-require-constant-initialization.cpp
5–7

No.

majnemer added inline comments.Aug 12 2016, 9:30 AM
lib/Sema/SemaDecl.cpp
10388–10390

You could use a lambda to abstract away the "check if it's cached, if not query" logic.

10485

Any reason not to use the already existing err_init_element_not_constant?

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 implementation approaches seem feasible: either check that the VarDecl has constant initialization at the point when the attribute is attached in SemaDeclAttr.cpp, or attach the attribute without checking and instead perform the check from Sema::CheckCompleteVariableDeclaration, around where we currently call isConstantInitializer to produce a warning on non-constant initialization -- the effect of the attribute would then be to promote that warning to an error.

Sorry about the redundant previous comment, the thread got forked by the change of subject and I missed the updated patch.

lib/Sema/SemaDecl.cpp
10484–10485

I think this check is incorrect: we perform constant initialization (to zero) for globals with no initializer.

10500

Instead of diagnosing the condition separately (and getting both a warning and an error for the same situation), it would seem preferable to change this to produce either diag::warn_global_constructor or your new error depending on whether the attribute is present. This would also remove the duplicate error messages if the attribute is specified on an object that is also marked constexpr.

EricWF updated this revision to Diff 67983.Aug 14 2016, 1:10 PM
EricWF marked 2 inline comments as done.

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 diagnostic, but I can't find where it's being emitted.

lib/Sema/SemaDecl.cpp
10484–10485

Agreed. Technically not "constant initialization" but every bit as safe.

10485

Any reason not to use the already existing err_init_element_not_constant?

I hadn't considered it, but the error text seems misleading, since it may select a constructor that's not a valid constant expression even when every element in the initializer is.

10500

Already I think I've dealt with the duplicate diagnostics.

10528

I can't figure out where the diagnostic is this comment is coming from. Hopefully I'm just missing something simple.

EricWF marked an inline comment as done.Aug 14 2016, 1:22 PM
EricWF added inline comments.
lib/Sema/SemaDecl.cpp
10528

Nevermind, I kept reading "global destructor" as "global constructor" and was looking for the wrong thing.

EricWF updated this revision to Diff 67986.Aug 14 2016, 1:33 PM
EricWF marked an inline comment as done.

Get all tests passing regarding duplicate warnings between __attribute__((requires_constant_initialization)) and -Wglobal-constructors. The warning will not emit a duplicate warning about global constructors, but it will still warn if the variable has a global destructor, which make sense to me.

I believe this is ready to go and I have addressed all concerns.

EricWF updated this object.Aug 14 2016, 1:34 PM
EricWF updated this revision to Diff 67987.Aug 14 2016, 1:40 PM

Fix missing semicolon.

rsmith added inline comments.Aug 15 2016, 2:50 PM
include/clang/Basic/Attr.td
1384

Providing both seems fine, but regardless this should be a GNU spelling not a GCC spelling, because GCC doesn't support the attribute.

include/clang/Basic/AttrDocs.td
836–840

This reads a bit awkwardly, since you don't actually say what the attribute does until the second sentence. Maybe fold the first two sentences together:

"This attribute specifies that the variable to which it is attached is intended to have a constant initializer according to the rules of [basic.start.static]. The variable is required to have static or thread storage duration. If the initialization of the variable is not a constant initializer, an error will be produced."

842

Static objects -> Static storage duration variables

844

static constructors -> dynamic initializers?

858

OK even though T has a non-trivial destructor? This makes the variable unsafe to use during program shutdown in the general case.

include/clang/Basic/DiagnosticSemaKinds.td
2575

thread-local -> thread, per standard terminology

6842

It may be useful for this diagnostic to say why we consider this to be an error (that is, mention that there is a require_constant_initialization attribute attached to the variable). The attribute will typically be hidden behind a (perhaps unfamiliar to the reader) macro, so it may not be obvious if we don't point it out.

lib/Sema/SemaDecl.cpp
10519–10520

Falling back to checkConstInit here will suppress the warning on some cases that are not technically constant initializers (but that Clang can emit as constants regardless). Is that what you want? If so, you should update the documentation to say that instead of saying that we only check for a constant initializer.

test/SemaCXX/attr-require-constant-initialization.cpp
8–16

Just use _Static_assert.

97–101

Did you mean for this to still be here?

EricWF marked 7 inline comments as done.Aug 15 2016, 3:14 PM
EricWF added inline comments.
include/clang/Basic/AttrDocs.td
844

Changed "by other static constructors" -> "during dynamic initialization"

858

Right, but I want this attribute to be able to work with (A) the union trick for "trivial" destructors and (B) variables not used during shutdown.
I was planning on following this up with an additional feature to aid in the shutdown case as well, but I think there is value in separating the features.

Currently -Wglobal-destructors will still warn on that declaration, so at least the unsafe shutdown is not silently missed.

Does this behavior make sense to you?

include/clang/Basic/DiagnosticSemaKinds.td
6842

"variable does not have a constant initializer as required by 'require_constant_initializer' attribute"?

Or do we want the diagnostic to point to the attribute token in a "required from here"-like note?

lib/Sema/SemaDecl.cpp
10519–10520

Falling back to checkConstInit here will suppress the warning on some cases that are not technically constant initializers (but that Clang can emit as constants regardless). Is that what you want?

Not really. I would prefer this strictly conform to the standard so it can be used to portably detect possible dynamic initializers on other toolchains.

What would the correct fallback here be?

test/SemaCXX/attr-require-constant-initialization.cpp
97–101

No.

EricWF updated this revision to Diff 68094.Aug 15 2016, 3:16 PM
EricWF marked 2 inline comments as done.

Address most of @rsmiths review comments.

rsmith added inline comments.Aug 15 2016, 4:23 PM
include/clang/Basic/AttrDocs.td
858

Sure, if that's a conscious design decision for the attribute.

include/clang/Basic/DiagnosticSemaKinds.td
6842

A separate note pointed at the attribute would make it clearer how the attribute got involved (especially via the macro expansion backtrace).

lib/Sema/SemaDecl.cpp
10519–10520

In C++11 onwards, checkInitIsICE is the right thing to use in all cases. In C++98, checkInitIsICE is appropriate for globals of integral type; we do not have an implementation of strict constant expression checking for other types. Perhaps just documenting that (and maybe adding a FIXME here) is the best you can do for now, assuming you're not interested in implementing the C++98 rules.

EricWF added inline comments.Aug 15 2016, 5:14 PM
lib/Sema/SemaDecl.cpp
10519–10520

SGTM. I think I'll fall back to isConstantInitializer() in C++03. Do you have an example of what checkConstantInitializer() gets wrong in C++03?

rsmith added inline comments.Aug 15 2016, 5:29 PM
lib/Sema/SemaDecl.cpp
10519–10520
struct A {
  ~A();
  int n;
} a;
int *p = &a.n; // constant initializer, but not a C++98 address constant expression
EricWF updated this revision to Diff 68121.EditedAug 15 2016, 6:08 PM

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

struct PODType { int x; int y; };
__attribute__((require_constant_initialization)) PODType pod; // error in C++11, OK in C++03.
aaron.ballman added inline comments.Aug 30 2016, 2:04 PM
include/clang/Basic/AttrDocs.td
836

I don't think I ever heard whether this attribute was usable in C or not. I suspect it's not particularly needed in C. If that's true, we should state that in the documentation, and consider making the attribute a C++-only attribute?

test/SemaCXX/attr-require-constant-initialization.cpp
2

Can you run this file through clang-format?

8–10

This doesn't appear to be needed by the test.

12

Hah, I've not seen 0+ before in a verify comment, that's a neat trick!

EricWF marked 3 inline comments as done.Aug 30 2016, 2:50 PM
EricWF added inline comments.
include/clang/Basic/AttrDocs.td
836

I think it would be somewhat useful in C. In particular if you have globals initialized from values you don't own. Example:

#include <y.h>
ATTR int x = y; // Will diagnose if  becomes 'y' is a non-constant expression.

However the current implementation only handles C++ and I'm satisfied with that. I'll mark the attribute as C++ only and document that change.

EricWF updated this revision to Diff 69761.Aug 30 2016, 2:53 PM

Address all comments except making the attribute C++ only.

EricWF updated this revision to Diff 69775.Aug 30 2016, 4:07 PM

Make the attribute C++ only.

EricWF marked an inline comment as done.Aug 30 2016, 4:08 PM
EricWF added inline comments.
utils/TableGen/ClangAttrEmitter.cpp
2794

This is needed so that the COnly and CPlusPlus entries in Attr.td don't create the same function name.

rsmith added inline comments.Aug 30 2016, 5:03 PM
include/clang/Basic/AttrDocs.td
836

For the record, in C the program is ill-formed if the initializer for a global is non-constant, so it doesn't seem like this attribute would ever be useful.

EricWF added inline comments.Aug 30 2016, 5:20 PM
include/clang/Basic/AttrDocs.td
836

Ah I didn't know that!

Are there any remaining issues with this patch?

aaron.ballman accepted this revision.Sep 1 2016, 12:45 PM
aaron.ballman edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Sep 1 2016, 12:45 PM
EricWF closed this revision.Sep 2 2016, 11:33 AM
EricWF updated this revision to Diff 70200.Sep 2 2016, 11:56 AM
EricWF edited edge metadata.

Updating against recent ToT.

EricWF reopened this revision.Sep 2 2016, 11:56 AM
This revision is now accepted and ready to land.Sep 2 2016, 11:56 AM
EricWF closed this revision.Sep 2 2016, 12:01 PM