Page MenuHomePhabricator

[clang-tidy] Check for dynamically initialized statics in headers.
ClosedPublic

Authored by czhang on Jun 3 2019, 3:47 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

czhang created this revision.Jun 3 2019, 3:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2019, 3:47 PM
czhang edited the summary of this revision. (Show Details)Jun 3 2019, 3:49 PM
lebedev.ri retitled this revision from clang-tidy: Check for dynamically initialized statics in headers. to [clang-tidy] Check for dynamically initialized statics in headers..Jun 3 2019, 3:52 PM
lebedev.ri edited subscribers, added: cfe-commits; removed: llvm-commits.
lebedev.ri added inline comments.
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
40 ↗(On Diff #202813)

Most of the matching should be done here, not in check().

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
37 ↗(On Diff #202813)

Will be good idea to address FIXME or remove if it's irrelevant.

54 ↗(On Diff #202813)

Will be good idea to address FIXME or remove if it's irrelevant.

55 ↗(On Diff #202813)

Could it be const auto *?

clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
9 ↗(On Diff #202813)

Will be good idea to provide example.

Eugene.Zelenko changed the repository for this revision from rL LLVM to rG LLVM Github Monorepo.
Eugene.Zelenko removed a project: Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2019, 5:13 PM
czhang updated this revision to Diff 203462.Jun 6 2019, 4:16 PM
czhang marked an inline comment as done.

Addressed comments.

czhang marked 4 inline comments as done.Jun 6 2019, 4:17 PM
czhang marked an inline comment as done.
czhang added inline comments.
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
40 ↗(On Diff #202813)

I believe that there is no way to use the AST matching language to express hasConstantDeclaration.

Hmm, but there already is clang's -Wglobal-constructors, that fires on some of these:
https://godbolt.org/z/rSnNuu
You are sure those extra warning this check produces ontop of -Wglobal-constructors are correct?
If so, maybe -Wglobal-constructors should be extended instead?

czhang added a comment.Jun 6 2019, 4:55 PM

Hmm, but there already is clang's -Wglobal-constructors, that fires on some of these:
https://godbolt.org/z/rSnNuu
You are sure those extra warning this check produces ontop of -Wglobal-constructors are correct?
If so, maybe -Wglobal-constructors should be extended instead?

Yes, this change is for variables with static lifetimes that are not necessarily globally scoped. The name -global-constructors would be a misnomer. In addition, this check is intended purely for header files, for reasons documented in the .rst file. Although similar, I do not believe this check would make sense as an extension to -global-constructors.

aaron.ballman added inline comments.Jul 1 2019, 6:08 AM
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
39 ↗(On Diff #203462)

Do you want to restrict this matcher to only variable declarations that have initializers, or are you also intending for this check to cover cases like:

// At file scope.
struct S { S(); } s;
55 ↗(On Diff #203462)

We have an AST matcher for this (isExpansionInSystemHeader()).

40 ↗(On Diff #202813)

You can write a local AST matcher to hoist this functionality into the check() call, though. Some of it is already exposed for you, like hasInitializer(), isValueDependent(), and isConstexpr().

czhang marked an inline comment as done.Jul 8 2019, 10:37 AM
czhang added inline comments.
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
39 ↗(On Diff #203462)

I think only variables with static storage are relevant to the stated goal of the checker.

55 ↗(On Diff #203462)

Isn't this for system headers only, not just included 'user' headers?

aaron.ballman added inline comments.Jul 9 2019, 8:59 AM
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
39 ↗(On Diff #203462)

Variables declared at file scope have static storage duration by default.

55 ↗(On Diff #203462)

Ahh, good point! Still, this should be trivial to make a local AST matcher for.

czhang updated this revision to Diff 209071.Jul 10 2019, 2:46 PM

Hoist constant initializer check and rebase

czhang marked 4 inline comments as done.Jul 10 2019, 2:50 PM
czhang added inline comments.
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
55 ↗(On Diff #203462)

Seems like many of the google tidy checks choose to relegate this header checking (whence I copied this bit) in the checker, rather than match registration time. Not sure what the pros and cons are of hoisting, but I left it there to follow the convention of the other checks.

40 ↗(On Diff #202813)

Perhaps not done in the most elegant way, but there is some amount of non-trivial side effecting caused by checkInitIsICE that makes me a little wary try to chain this more declaratively.

aaron.ballman added inline comments.Jul 23 2019, 8:25 AM
clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
9 ↗(On Diff #202813)

Agreed, I'd like to see an example in the docs.

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
33–35 ↗(On Diff #209071)

What problems can be caused here? Typically, dynamic init is only problematic when it happens before main() is executed (because of initialization order dependencies), but that doesn't apply to local statics.

czhang marked 3 inline comments as done.Aug 1 2019, 12:55 PM
czhang added inline comments.
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
33–35 ↗(On Diff #209071)

Consider the case when synchronization is disabled for static initialization, and two threads call foo2 for the first time. It may be the case that they both try and initialize the static variable at the same time with different values (since the dynamic initializer may not be pure), creating a race condition.

czhang updated this revision to Diff 212890.Aug 1 2019, 1:03 PM
czhang marked an inline comment as done.

Add example in documentation.

czhang marked an inline comment as done.Aug 1 2019, 1:03 PM
aaron.ballman added inline comments.Aug 2 2019, 6:25 AM
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
33–35 ↗(On Diff #209071)

Consider the case when synchronization is disabled for static initialization

This is a compiler bug, though: http://eel.is/c++draft/stmt.dcl#4.sentence-3

czhang marked 2 inline comments as done.Aug 2 2019, 9:48 AM
czhang added inline comments.
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
33–35 ↗(On Diff #209071)

Sorry, I guess I didn't make it clear enough in the rst documentation file, but this check is for those who explicitly enable the -fno-threadsafe-statics flag because they provide their own synchronization. Then they would like to check if the headers they didn't write may possibly run into this issue when compiling with this flag.

aaron.ballman added inline comments.Aug 2 2019, 10:00 AM
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
33–35 ↗(On Diff #209071)

Ah! Thank you for the explanation. In that case, this behavior makes more sense, but I think you should only warn if the user has enabled that feature flag rather than always warning.

czhang marked 2 inline comments as done.Aug 2 2019, 11:12 AM
czhang added inline comments.
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
33–35 ↗(On Diff #209071)

I haven't been able to find much documentation on how to actually make a tidy check run against a feature flag. Is it possible to do this? I would think that said users would manually run this check on their header files.

lebedev.ri added inline comments.Aug 2 2019, 11:18 AM
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
33–35 ↗(On Diff #209071)

Sorry, I guess I didn't make it clear enough in the rst documentation file, but this check is for those who explicitly enable the -fno-threadsafe-statics flag because they provide their own synchronization.

I too want to see this explicitly spelled out in documentation.

Then they would like to check if the headers they didn't write may possibly run into this issue when compiling with this flag.

I'm very much not a fan of this solution.
Are you sure that is not exposed in LangOptions, e.g. as ThreadsafeStatics?

czhang updated this revision to Diff 213102.Aug 2 2019, 11:41 AM
czhang marked an inline comment as done.

Be more explicit about -fno-threadsafe-statics

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
33–35 ↗(On Diff #209071)

I'm very much not a fan of this solution.
Are you sure that is not exposed in LangOptions, e.g. as ThreadsafeStatics?

Can you clarify what you mean? Are you not a fan of users disabling synchronization and providing their own? Or are you agreeing with Aaron that we should only enable this check with ThreadsafeStatics is enabled?

Either way, I have put a check against LangOpts for this now.

czhang marked 2 inline comments as done.Aug 2 2019, 11:42 AM

I think this last update clarified the concerns i raised in last comment.
But, did you mean to update the tests? They should have broke now that you require !getLangOpts().ThreadsafeStatics.

clang-tools-extra/docs/clang-tidy/checks/bugprone-dynamic-static-initializers.rst
11 ↗(On Diff #213102)

escape it:

``-fno-threadsafe-statics``
aaron.ballman added inline comments.Aug 5 2019, 5:34 AM
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
47 ↗(On Diff #213102)

Why is this check disabled for C++? I would expect dynamic init of a static in a C++ header file would be flagged by this check.

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
1 ↗(On Diff #213102)

I'm a bit confused.

  1. Why is this file a .hpp if the check is currently supposed to be C-only?
  2. Why are there no #include directives in the test if the check is supposed to only work on header files?

As best I can tell, this is a moral equivalent to doing: clang -x c whatever.h, and so this should be a compilation unit and not a header file. @alexfh, do you have thoughts there?

czhang marked an inline comment as done.Aug 5 2019, 9:39 AM
czhang added inline comments.
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
47 ↗(On Diff #213102)

I'm confused now. If the language is not C++, we do an early return; that is, the check is run if we are on C++. Perhaps the early return is too confusing?

lebedev.ri added inline comments.Aug 5 2019, 9:42 AM
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
47 ↗(On Diff #213102)

Then the question is opposite, is this meaningless for C?

czhang marked an inline comment as done.Aug 5 2019, 10:10 AM
czhang added inline comments.
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
47 ↗(On Diff #213102)

C can only initialize statics with constants, right?

aaron.ballman added inline comments.Aug 5 2019, 10:38 AM
clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp
47 ↗(On Diff #213102)

Oye, my eyes saw one thing and my brain saw another. :-P You're right, @czhang, this code is correct as-is (C only initializes statics with a constant).

czhang updated this revision to Diff 214275.Aug 8 2019, 6:54 PM

Fix test and formatting.

czhang marked an inline comment as done.Aug 8 2019, 6:55 PM
alexfh added inline comments.Aug 20 2019, 6:54 AM
clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
1 ↗(On Diff #213102)

Why is this file a .hpp if the check is currently supposed to be C-only?

This seems to have been cleared in a different comment: the check is C++-only.

Why are there no #include directives in the test if the check is supposed to only work on header files?

Since the check is using a list of extensions to figure out, if a file is a header, it's not overly important to actually add a separate file that includes this one. And there's a certain benefit in convenience of not doing so (it's nice when CHECK-... and RUN: directives just work). At least one more check - misc-definitions-in-headers - does it the same way.

aaron.ballman accepted this revision.Aug 20 2019, 7:52 AM

LGTM!

clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp
1 ↗(On Diff #213102)

Awesome, thank you for weighing in, @alexfh!

This revision is now accepted and ready to land.Aug 20 2019, 7:52 AM
ychen added a subscriber: ychen.Aug 20 2019, 5:18 PM

Since Charlie has completed his internship before LGTM, I'll commit this on his behalf.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2019, 12:59 PM