This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] modernize-deprecated-headers check should respect extern "C" blocks
ClosedPublic

Authored by steakhal on May 9 2022, 12:37 AM.

Details

Summary

The check should not report includes wrapped by extern "C" { ... } blocks,
such as:

#ifdef __cplusplus
extern "C" {
#endif

#include "assert.h" // <-- actual include

#ifdef __cplusplus
}
#endif

This pattern comes up sometimes in header files designed to be consumed
by both C and C++ source files.
The check now reports false reports when the header file is consumed by
a C++ translation unit.

In this change, I'm not emitting the reports immediately from the
PPCallback, rather aggregating them for further processing.
After all preprocessing is done, the matcher will be called on the
TranslationUnitDecl, ensuring that the check callback is called only
once.

Within that callback, I'm recursively visiting each decls, looking for
LinkageSpecDecls which represent the extern "C" specifier.
After this, I'm dropping all the reports coming from inside of it.
After the visitation is done, I'm emitting the reports I'm left with.


The patch did not show measurable runtime difference on the following projects:
llvm-project,contour,codechecker,qtbase,protobuf,bitcoin,xerces,libwebm,tinyxml2,postgres,ffmpeg,sqlite,openssl,vim,twin,curl,tmux,memcached

So, even though the O(N * M): N includes, M extern decls might seem rough at first, it turns out to be negligible in practice.

Diff Detail

Event Timeline

steakhal created this revision.May 9 2022, 12:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 12:37 AM
steakhal requested review of this revision.May 9 2022, 12:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 12:37 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steakhal updated this revision to Diff 428000.May 9 2022, 1:24 AM

Fix the RUN line

whisperity added inline comments.May 11 2022, 10:34 AM
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
57–60

This unconditionally assumes that an encountered LSD will be extern "C". However, extern "C++" is also possible (and supported by Clang). (And things like extern "Java" are also possible according to the standard, albeit not supported by Clang.)

79–121

(🔮: Same here. Is this state that might keep a dangling reference if multiple TUs are consumed in sequence?)

84

(Perhaps it is worth an explicit comment here that you do this at first glance unconventional match because the entire check's logic is calculated only on the "preprocessor" level.)

133

POSIX defines a "global" include limits.h which gives you the definition of macros like PATH_MAX. Will such code keep on working if the include is turned into climits?

clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.h
60

🔮: You are storing some TU-specific state here (SourceRange) that is not cleared. What happens if you run clang-tidy a.cpp b.cpp, i.e. two TUs in the same process execution?

steakhal updated this revision to Diff 428724.May 11 2022, 11:25 AM
  • Ignore extern "C++" blocks; also added a test for this.
  • Also ignore LinkageSpecDecls without braces.
  • Clear the IncludesToBeProcessed list at the end of each translation unit.
steakhal marked 2 inline comments as done.May 11 2022, 11:27 AM
steakhal added inline comments.
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
57–60

O.O I did not know its a thing :D
It seems like clang only supports C or C++.

79–121

How can I acquire the right SourceManager if not like this?
I haven't found any alternative.
What do you suggest?

133

IDK.

whisperity added inline comments.May 12 2022, 1:19 AM
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
79–121

Is the PP a different variable when a new translation unit is consumed by the same check instance? If so, then there is no problem. A simple debug print llvm::errs() << PP << '\n';, recompile, re-run with clang-tidy a.cpp b.cpp should show how the infrastructure behaves. I do not know the exact behaviour of the preprocessor layer.

whisperity added inline comments.May 12 2022, 1:22 AM
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
133

Did an investigation on Ubuntu with G++ 7.5. So it turns out that the GNU G++ standard library implementation, on Linux, is implemented in a way that it will include the POSIX-specific limits.h into the C Standard limits.h and thus also climits.

And

Out of these projects llvm-project,contour,codechecker,qtbase,protobuf,bitcoin,xerces,libwebm,tinyxml2,postgres,ffmpeg,sqlite,openssl,vim,twin,curl,tmux,memcached, it suppressed 5 reports

Well, Contour, tmux and Qt (and perhaps even LLVM's OS-specific support lib) SHOULD be heavily reliant on interfacing with POSIX stuff when compiled against Linux, so I guess we can say this transformation is not dangerous.

steakhal marked 6 inline comments as done.May 12 2022, 2:38 AM
steakhal added inline comments.
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
79–121

Okay, I've checked this. The lifetimes checks out. Thanks!

$ ./build/debug/bin/clang-tidy a.cpp b.cpp  -checks="-*,modernize-deprecated-headers"
DeprecatedHeadersCheck::registerPPCallbacks() acquires SM 0x27eb9e0
ctor IncludeModernizePPCallbacks acquires SM 0x27eb9e0
IncludeModernizePPCallbacks::InclusionDirective inserts to IncludesToBeProcessed using SM 0x27eb9e0
IncludeModernizePPCallbacks::InclusionDirective inserts to IncludesToBeProcessed using SM 0x27eb9e0
DeprecatedHeadersCheck::check() emits IncludesToBeProcessed using SM 0x27eb9e0
onEndOfTranslationUnit cleares IncludesToBeProcessed
1 warning generated.
dtor IncludeModernizePPCallbacks releases SM 0x27eb9e0
DeprecatedHeadersCheck::registerPPCallbacks() acquires SM 0x27eb9e0
ctor IncludeModernizePPCallbacks acquires SM 0x27eb9e0
IncludeModernizePPCallbacks::InclusionDirective inserts to IncludesToBeProcessed using SM 0x27eb9e0
IncludeModernizePPCallbacks::InclusionDirective inserts to IncludesToBeProcessed using SM 0x27eb9e0
DeprecatedHeadersCheck::check() emits IncludesToBeProcessed using SM 0x27eb9e0
onEndOfTranslationUnit cleares IncludesToBeProcessed
2 warnings generated.
dtor IncludeModernizePPCallbacks releases SM 0x27eb9e0
a.cpp:2:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
#include <assert.h>
         ^~~~~~~~~~
         <cassert>
b.cpp:2:10: warning: inclusion of deprecated C++ header 'assert.h'; consider using 'cassert' instead [modernize-deprecated-headers]
#include <assert.h>
         ^~~~~~~~~~
         <cassert>

LGTM. Some typos inline. Also I think this is a significant enough bug-fix that it warrants an entry in the Release Notes under Changes to existing checks.

Also, the tests are failing, but it fails on the formatting of the test file (???), and not the actual test itself. (Usually such issues would be sent back to Phab to appear an a machine-made inline comment, I am not sure what happened to that logic.)

changed files:
    clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
66–69

(llvm/ADT/STLExtras.h define a range-based lower_bound and upper_bound)

86

(Also: this is a fix to an issue of your own finding and not something that was reported on Bugzilla? Just to make sure we cross-reference and close tickets.)

steakhal marked 3 inline comments as done.May 12 2022, 4:07 AM

LGTM. Some typos inline. Also I think this is a significant enough bug-fix that it warrants an entry in the Release Notes under Changes to existing checks.

Also, the tests are failing, but it fails on the formatting of the test file (???), and not the actual test itself. (Usually such issues would be sent back to Phab to appear an a machine-made inline comment, I am not sure what happened to that logic.)

changed files:
    clang-tools-extra/test/clang-tidy/checkers/modernize-deprecated-headers-extern-c.cpp

Thanks.

(Also: this is a fix to an issue of your own finding and not something that was reported on Bugzilla? Just to make sure we cross-reference and close tickets.)

It was an internal ticket.

clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
66–69

Yup, but I'm using a different range.
The first call could be replaced by the llvm ranged version, but the second starts with Low instead of Begin.
At that point, I decided to keep the symmetry and use the std versions in both cases.

Well, I could llvm::make_range() and use the llvm::upper_bound but yea, no.

steakhal updated this revision to Diff 428904.May 12 2022, 4:08 AM
steakhal marked an inline comment as done.
  • Fix typos.
  • Mention this in the release notes.
  • clang-format the test file as well.
whisperity accepted this revision.May 12 2022, 4:14 AM
This revision is now accepted and ready to land.May 12 2022, 4:14 AM
thakis added a subscriber: thakis.May 13 2022, 8:03 AM

Looks like this breaks tests: http://45.33.8.238/linux/76033/step_8.txt

Please take a look and revert for now if it takes a while to fix.

steakhal reopened this revision.May 13 2022, 9:17 AM

It seems like we cannot use the sorting I did. The warning within the mylib.h header will have a different FileID than the original TU. Consequently, the upper_bound() lower_bound() range will be empty even if the extern "C" { ... } block.

I might need to fallback to the isBeforeInTranslationUnit or something like that. But I fear, if that's the case, the complexity will be O(n * m) for n extern "C" block and m includes. We will see.

This revision is now accepted and ready to land.May 13 2022, 9:17 AM
steakhal planned changes to this revision.May 13 2022, 9:18 AM
steakhal updated this revision to Diff 430012.May 17 2022, 5:57 AM
  • Copy the mylib.h under the build directory to have deterministic relative order to the LIT test file.
  • Use SourceManager::isBeforeInTranslationUnit instead of the fancy decomposed decl logarithmic search.
  • Add a test for including a system header containing a deprecated include.
  • Add REQUIRES: system-linux clause to the test.
This revision is now accepted and ready to land.May 17 2022, 5:57 AM
steakhal requested review of this revision.May 17 2022, 5:57 AM
steakhal edited the summary of this revision. (Show Details)May 17 2022, 6:00 AM
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
90

In modernize-macro-to-enum I had to similarly discard macros defined inside a top-level decl. I did decl(hasParent(translationUnitDecl())) to match the top-level decls. Would that simplify your check as well?

123

IMO it's poor style to define entities that are declared within a namespace outside the namespace within which they were declared.

Is there some reason this isn't defined with the rest of the methods on the callback class?

steakhal updated this revision to Diff 430295.May 18 2022, 2:21 AM
steakhal marked an inline comment as done.
  • Move all the details stuff into an anonymous namespace.
  • Pass the std::vector<IncludeMarker> &IncludesToBeProcessed directly to the callbacks.
  • Sink the IncludeMarker definition into the public part of DeprecatedHeadersCheck.
steakhal marked an inline comment as done.May 18 2022, 2:23 AM
steakhal added inline comments.
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp
90

The problem is that extern "C" {...} blocks might be within some namespace declarations.
https://godbolt.org/z/vr5jYx5az

123

I eradicated the use of detail namespace.
It indeed looks better.

steakhal marked an inline comment as done.May 20 2022, 7:10 AM

WDYT, are there any blocker issues with this patch stack?

This revision is now accepted and ready to land.May 20 2022, 10:16 AM