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] Make -fvisibility={} and -ftype-visibility={} benign options. #71985

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

ributzka
Copy link
Collaborator

@ributzka ributzka commented Nov 10, 2023

Both options do not affect the AST content that is serialized into the PCM. This
commit includes the following changes:

1.) Mark -fvisibility={} and -ftype-visibility={} as benign options. That
means they are no longer considered part of the module hash, which can
reduce the number of module variants.

2.) Add a test to verify the generated LLVM IR is not affected by the default
visibiliy mode in the module.

3.) Add a test to clang-scan-deps to ensure only one module is build, even if
the above mentioned options are used.

This fixes rdar://118246054.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 10, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 10, 2023

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Juergen Ributzka (ributzka)

Changes

Both options do not affect the AST content that is serialized into the PCM. This
commit includes the following changes:

1.) Mark -fvisibility={} and -ftype-visibility={} as benign options. That
means they are no longer considered part of the module hash, which can
reduce the number of module variants.

2.) Add a test to clang-scan-deps to ensure only one module is build, even if
the above mentioned options are used.

This fixes rdar://118246054.


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

2 Files Affected:

  • (modified) clang/include/clang/Basic/LangOptions.def (+2-2)
  • (added) clang/test/ClangScanDeps/strip-visibility.c (+59)
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index c541ccefdd5fbe1..6ad6ccbe4a78c5f 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -349,9 +349,9 @@ LANGOPT(
     "type's inheritance model would be determined under the Microsoft ABI")
 
 ENUM_LANGOPT(GC, GCMode, 2, NonGC, "Objective-C Garbage Collection mode")
-ENUM_LANGOPT(ValueVisibilityMode, Visibility, 3, DefaultVisibility,
+BENIGN_ENUM_LANGOPT(ValueVisibilityMode, Visibility, 3, DefaultVisibility,
              "default visibility for functions and variables [-fvisibility]")
-ENUM_LANGOPT(TypeVisibilityMode, Visibility, 3, DefaultVisibility,
+BENIGN_ENUM_LANGOPT(TypeVisibilityMode, Visibility, 3, DefaultVisibility,
              "default visibility for types [-ftype-visibility]")
 LANGOPT(SetVisibilityForExternDecls, 1, 0,
         "apply global symbol visibility to external declarations without an explicit visibility")
diff --git a/clang/test/ClangScanDeps/strip-visibility.c b/clang/test/ClangScanDeps/strip-visibility.c
new file mode 100644
index 000000000000000..3c8de700b12bab1
--- /dev/null
+++ b/clang/test/ClangScanDeps/strip-visibility.c
@@ -0,0 +1,59 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full -mode preprocess-dependency-directives > %t/result.txt
+
+// RUN: FileCheck %s -input-file %t/result.txt
+
+// Verify that there's a single version of module A.
+
+// CHECK:        "modules": [
+// CHECK-NEXT:     {
+// CHECK:            "command-line": [
+// CHECK-NOT:          "-fvisibility="
+// CHECK-NOT:          "-ftype-visibility="
+// CHECK:            ]
+// CHECK:            "name": "A"
+// CHECK:          }
+// CHECK-NOT:        "name": "A"
+// CHECK:        "translation-units"
+
+//--- cdb.json.template
+[
+  {
+    "directory": "DIR",
+    "command": "clang -Imodules/A -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -fsyntax-only DIR/t1.c",
+    "file": "DIR/t1.c"
+  },
+  {
+    "directory": "DIR",
+    "command": "clang -Imodules/A -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -fvisibility=hidden -fsyntax-only DIR/t2.c",
+    "file": "DIR/t2.c"
+  },
+  {
+    "directory": "DIR",
+    "command": "clang -Imodules/A -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-modules -fimplicit-module-maps -fvisibility=hidden -fvisibility-ms-compat -fsyntax-only DIR/t3.c",
+    "file": "DIR/t3.c"
+  }
+]
+
+//--- modules/A/module.modulemap
+
+module A {
+  umbrella header "A.h"
+}
+
+//--- modules/A/A.h
+
+typedef int A_t;
+extern int a(void);
+
+//--- t1.c
+#include "A.h"
+
+//--- t2.c
+#include "A.h"
+
+//--- t3.c
+#include "A.h"

@ributzka ributzka marked this pull request as draft November 13, 2023 22:42
@ributzka ributzka marked this pull request as ready for review November 14, 2023 22:38
@llvmbot llvmbot added the clang:modules C++20 modules and Clang Header Modules label Nov 14, 2023
Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

…ons.

Both options do not affect the AST content that is serialized into the PCM. This
commit includes the following changes:

1.) Mark `-fvisibility={}` and `-ftype-visibility={}` as benign options. That
    means they are no longer considered part of the module hash, which can
    reduce the number of module variants.

2.) Add a test to verify the generated LLVM IR is not affected by the default
    visibiliy mode in the module.

3.) Add a test to clang-scan-deps to ensure only one module is build, even if
    the above mentioned options are used.

This fixes rdar://118246054.
@ributzka ributzka merged commit d3b75c4 into llvm:main Nov 16, 2023
2 of 3 checks passed
@ributzka ributzka deleted the pr/strip-visibility branch November 16, 2023 16:41
sr-tream pushed a commit to sr-tream/llvm-project that referenced this pull request Nov 20, 2023
…ons. (llvm#71985)

Both options do not affect the AST content that is serialized into the PCM. This
commit includes the following changes:
    
1.) Mark `-fvisibility={}` and `-ftype-visibility={}` as benign options.That
     means they are no longer considered part of the module hash, which can
     reduce the number of module variants.
    
2.) Add a test to verify the generated LLVM IR is not affected by the default
     visibiliy mode in the module.

3.) Add a test to clang-scan-deps to ensure only one module is build, even if
      the above mentioned options are used.
    
This fixes rdar://118246054.
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
…ons. (llvm#71985)

Both options do not affect the AST content that is serialized into the PCM. This
commit includes the following changes:
    
1.) Mark `-fvisibility={}` and `-ftype-visibility={}` as benign options.That
     means they are no longer considered part of the module hash, which can
     reduce the number of module variants.
    
2.) Add a test to verify the generated LLVM IR is not affected by the default
     visibiliy mode in the module.

3.) Add a test to clang-scan-deps to ensure only one module is build, even if
      the above mentioned options are used.
    
This fixes rdar://118246054.
ributzka added a commit to apple/llvm-project that referenced this pull request Jan 23, 2024
…ons. (llvm#71985)

Both options do not affect the AST content that is serialized into the PCM. This
commit includes the following changes:

1.) Mark `-fvisibility={}` and `-ftype-visibility={}` as benign options.That
     means they are no longer considered part of the module hash, which can
     reduce the number of module variants.

2.) Add a test to verify the generated LLVM IR is not affected by the default
     visibiliy mode in the module.

3.) Add a test to clang-scan-deps to ensure only one module is build, even if
      the above mentioned options are used.

This fixes rdar://118246054.

(cherry picked from commit d3b75c4)
ributzka added a commit to apple/llvm-project that referenced this pull request Jan 24, 2024
…ons. (llvm#71985)

Both options do not affect the AST content that is serialized into the PCM. This
commit includes the following changes:

1.) Mark `-fvisibility={}` and `-ftype-visibility={}` as benign options.That
     means they are no longer considered part of the module hash, which can
     reduce the number of module variants.

2.) Add a test to verify the generated LLVM IR is not affected by the default
     visibiliy mode in the module.

3.) Add a test to clang-scan-deps to ensure only one module is build, even if
      the above mentioned options are used.

This fixes rdar://118246054.

(cherry picked from commit d3b75c4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants