Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][modules] Strip LLVM options #75405

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Conversation

ributzka
Copy link
Collaborator

Currently, the dep scanner does not remove LLVM options from the argument list.
Since LLVM options shouldn't affect the AST, it is safe to remove them all.

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Dec 13, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 13, 2023

@llvm/pr-subscribers-clang

Author: Juergen Ributzka (ributzka)

Changes

Currently, the dep scanner does not remove LLVM options from the argument list.
Since LLVM options shouldn't affect the AST, it is safe to remove them all.


Full diff: https://github.com/llvm/llvm-project/pull/75405.diff

2 Files Affected:

  • (modified) clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp (+2)
  • (added) clang/test/ClangScanDeps/strip-llvm-args.m (+49)
diff --git a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
index 4a3cd054f23d92..bfaa897851041d 100644
--- a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -119,6 +119,8 @@ makeCommonInvocationForModuleBuild(CompilerInvocation CI) {
   // units.
   CI.getFrontendOpts().Inputs.clear();
   CI.getFrontendOpts().OutputFile.clear();
+  // LLVM options are not going to affect the AST
+  CI.getFrontendOpts().LLVMArgs.clear();
 
   // TODO: Figure out better way to set options to their default value.
   CI.getCodeGenOpts().MainFileName.clear();
diff --git a/clang/test/ClangScanDeps/strip-llvm-args.m b/clang/test/ClangScanDeps/strip-llvm-args.m
new file mode 100644
index 00000000000000..a2ab1b4ab5c767
--- /dev/null
+++ b/clang/test/ClangScanDeps/strip-llvm-args.m
@@ -0,0 +1,49 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb1.json.template > %t/cdb1.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb1.json -format experimental-full > %t/result1.txt
+// RUN: FileCheck %s -input-file %t/result1.txt
+
+// Verify that secondary actions get stripped, and that there's a single version
+// of module A.
+
+// CHECK:        "modules": [
+// CHECK-NEXT:     {
+// CHECK:            "command-line": [
+// CHECK-NOT:          "-treat-scalable-fixed-error-as-warning"
+// CHECK:            ]
+// CHECK:            "name": "A"
+// CHECK:          }
+// CHECK-NOT:        "name": "A"
+// CHECK:        "translation-units"
+
+//--- cdb1.json.template
+[
+  {
+    "directory": "DIR",
+    "command": "clang -Imodules/A -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -fsyntax-only DIR/t1.m",
+    "file": "DIR/t1.m"
+  },
+  {
+    "directory": "DIR",
+    "command": "clang -Imodules/A -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -mllvm -stackmap-version=2 -fsyntax-only DIR/t2.m",
+    "file": "DIR/t2.m"
+  }
+]
+
+//--- modules/A/module.modulemap
+
+module A {
+  umbrella header "A.h"
+}
+
+//--- modules/A/A.h
+
+typedef int A_t;
+
+//--- t1.m
+@import A;
+
+//--- t2.m
+@import A;

Copy link
Contributor

@jansvoboda11 jansvoboda11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Currently, the dep scanner does not remove LLVM options from the argument list.
Since LLVM options shouldn't affect the AST, it is safe to remove them all.
@ributzka
Copy link
Collaborator Author

Thanks for the reviews.

@ributzka ributzka merged commit e007551 into llvm:main Dec 14, 2023
3 of 4 checks passed
@ributzka ributzka deleted the pr/strip-llvm-args branch December 14, 2023 19:29
ributzka added a commit to apple/llvm-project that referenced this pull request Jan 23, 2024
Currently, the dep scanner does not remove LLVM options from the
argument list.
Since LLVM options shouldn't affect the AST, it is safe to remove them
all.

(cherry picked from commit e007551)

Conflicts:
	clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
ributzka added a commit to apple/llvm-project that referenced this pull request Jan 24, 2024
Currently, the dep scanner does not remove LLVM options from the
argument list.
Since LLVM options shouldn't affect the AST, it is safe to remove them
all.

(cherry picked from commit e007551)

Conflicts:
	clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants