[Fix] Speedup -Wunsafe-buffer-usage when using clang modules. #127161
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
matts1
force-pushed
the
push-lmuqnquptpuy
branch
from
14560a1 to
918e07f
Compare
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Matt (matts1) ChangesEach piece of code should have analysis run on it precisely once. However, if you build a module, and then build another module depending on it, the header file will have See https://issues.chromium.org/issues/351909443 for details and benchmarks. Full diff: https://github.com/llvm/llvm-project/pull/127161.diff 2 Files Affected:
diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp
index 589869d018657..849c899bbbc8b 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2546,14 +2546,27 @@ static void flushDiagnostics(Sema &S, const sema::FunctionScopeInfo *fscope) {
class CallableVisitor : public DynamicRecursiveASTVisitor {
private:
llvm::function_ref<void(const Decl *)> Callback;
+ const unsigned int TUModuleID;
public:
- CallableVisitor(llvm::function_ref<void(const Decl *)> Callback)
- : Callback(Callback) {
+ CallableVisitor(llvm::function_ref<void(const Decl *)> Callback,
+ unsigned int TUModuleID)
+ : Callback(Callback), TUModuleID(TUModuleID) {
ShouldVisitTemplateInstantiations = true;
ShouldVisitImplicitCode = false;
}
+ bool TraverseDecl(Decl *Node) override {
+ // For performance reasons, only validate the current translation unit's
+ // module, and not modules it depends on.
+ // See https://issues.chromium.org/issues/351909443 for details.
+ if (Node && Node->getOwningModuleID() == TUModuleID) {
+ return DynamicRecursiveASTVisitor::TraverseDecl(Node);
+ } else {
+ return true;
+ }
+ }
+
bool VisitFunctionDecl(FunctionDecl *Node) override {
if (cast<DeclContext>(Node)->isDependentContext())
return true; // Not to analyze dependent decl
@@ -2633,7 +2646,7 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings(
SourceLocation()) ||
(!Diags.isIgnored(diag::warn_unsafe_buffer_libc_call, SourceLocation()) &&
S.getLangOpts().CPlusPlus /* only warn about libc calls in C++ */)) {
- CallableVisitor(CallAnalyzers).TraverseTranslationUnitDecl(TU);
+ CallableVisitor(CallAnalyzers, TU->getOwningModuleID()).TraverseTranslationUnitDecl(TU);
}
}
diff --git a/clang/test/Modules/safe_buffers_optout.cpp b/clang/test/Modules/safe_buffers_optout.cpp
index 2129db65da752..8c3d6a235d399 100644
--- a/clang/test/Modules/safe_buffers_optout.cpp
+++ b/clang/test/Modules/safe_buffers_optout.cpp
@@ -95,18 +95,10 @@ int textual(int *p) {
// `safe_buffers_test_optout`, which uses another top-level module
// `safe_buffers_test_base`. (So the module dependencies form a DAG.)
-// No expected warnings from base.h because base.h is a separate
-// module and in a separate TU that is not textually included. The
-// explicit command that builds base.h has no `-Wunsafe-buffer-usage`.
-
-// [email protected]:3{{unsafe buffer access}}
-// [email protected]:3{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
-// expected-warning@test_sub1.h:5{{unsafe buffer access}}
-// expected-note@test_sub1.h:5{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
-// expected-warning@test_sub1.h:14{{unsafe buffer access}}
-// expected-note@test_sub1.h:14{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
-// expected-warning@test_sub2.h:5{{unsafe buffer access}}
-// expected-note@test_sub2.h:5{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+// No expected warnings from base.h, test_sub1, or test_sub2 because they are
+// in seperate modules, and the explicit commands that builds them have no
+// `-Wunsafe-buffer-usage`.
+
int foo(int * p) {
int x = p[5]; // expected-warning{{unsafe buffer access}} expected-note{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
#pragma clang unsafe_buffer_usage begin
@@ -129,14 +121,10 @@ int foo(int * p) {
// `safe_buffers_test_optout`, which uses another top-level module
// `safe_buffers_test_base`. (So the module dependencies form a DAG.)
-// [email protected]:3{{unsafe buffer access}}
-// [email protected]:3{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
-// expected-warning@test_sub1.h:5{{unsafe buffer access}}
-// expected-note@test_sub1.h:5{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
-// expected-warning@test_sub1.h:14{{unsafe buffer access}}
-// expected-note@test_sub1.h:14{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
-// expected-warning@test_sub2.h:5{{unsafe buffer access}}
-// expected-note@test_sub2.h:5{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
+// No expected warnings from base.h, test_sub1, or test_sub2 because they are
+// in seperate modules, and the explicit commands that builds them have no
+// `-Wunsafe-buffer-usage`.
+
int foo(int * p) {
int x = p[5]; // expected-warning{{unsafe buffer access}} expected-note{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}}
#pragma clang unsafe_buffer_usage begin
|
ChuanqiXu9
reviewed
Feb 17, 2025
matts1
force-pushed
the
push-lmuqnquptpuy
branch
from
918e07f to
ec6461f
Compare
ChuanqiXu9
approved these changes
Feb 17, 2025
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
matts1
force-pushed
the
push-lmuqnquptpuy
branch
from
ec6461f to
17b1184
Compare
atetubou
reviewed
Feb 17, 2025
See https://issues.chromium.org/issues/351909443 for details and benchmarks. This improves the performance of a file containing a single line, `#include <iostream>`, from ~1 second to ~100ms on my machine.
matts1
force-pushed
the
push-lmuqnquptpuy
branch
from
17b1184 to
46dc273
Compare
Collaborator
|
+cc @haoNoQ and @ivanaivanovska for -Wunsafe-buffer-usage |
Contributor
Author
|
@ChuanqiXu9 could you merge this since it's approved? |
|
@matts1 Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16030 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Each piece of code should have analysis run on it precisely once. However, if you build a module, and then build another module depending on it, the header file of the module you depend on will have
-Wunsafe-buffer-usagerun on it twice. This normally isn't a huge issue, but in the case of using the standard library as a module, simply adding the line#include <cstddef>increases compile times by 900ms (from 100ms to 1 second) on my machine. I believe this is because the standard library has massive modules, of which only a small part is used (the AST is ~700k lines), and because if what I've been told is correct, the AST is lazily generated, and-Wunsafe-buffer-usageforces it to be evaluated every time.See https://issues.chromium.org/issues/351909443 for details and benchmarks.