llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Matt (matts1) <details> <summary>Changes</summary> 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 will have `-Wunsafe-buffer-usage` run 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-usage` forces it to be evaluated every time. 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: - (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+16-3) - (modified) clang/test/Modules/safe_buffers_optout.cpp (+8-20) ``````````diff 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`. - -// expected-warning@base.h:3{{unsafe buffer access}} -// expected-note@base.h: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.) -// expected-warning@base.h:3{{unsafe buffer access}} -// expected-note@base.h: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 `````````` </details> https://github.com/llvm/llvm-project/pull/127161 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
