[clang] deduplicate target-features for modules

[fmayer]

Previously, double passing a target feature would make the module
incompatible with a compilation unit that only passes it once.

The motivating problem is, when we pass -target-features +tagged-globals
as well as -fsanitize=hwaddress, which adds a second copy, the module
is incompatible with one built with only one -target-features +tagged-globals.

[llvmbot]

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Florian Mayer (fmayer)

Changes

Previously, double passing a target feature would make the module
incompatible with a compilation unit that only passes it once.

The motivating problem is, when we pass -target-features +tagged-globals
as well as -fsanitize=hwaddress, which adds a second copy, the module
is incompatible with one built with only one -target-features +tagged-globals.

---

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

2 Files Affected:

• (modified) clang/lib/Serialization/ASTReader.cpp (+3)
• (modified) clang/test/Modules/merge-target-features.cpp (+21)

diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 03b1b02859b81..db4fa68f6db08 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -495,7 +495,10 @@ static bool checkTargetOptions(const TargetOptions &TargetOpts,
   SmallVector<StringRef, 4> ReadFeatures(TargetOpts.FeaturesAsWritten.begin(),
                                          TargetOpts.FeaturesAsWritten.end());
   llvm::sort(ExistingFeatures);
+  ExistingFeatures.erase(llvm::unique(ExistingFeatures),
+                         ExistingFeatures.end());
   llvm::sort(ReadFeatures);
+  ReadFeatures.erase(llvm::unique(ReadFeatures), ReadFeatures.end());
 
   // We compute the set difference in both directions explicitly so that we can
   // diagnose the differences differently.
diff --git a/clang/test/Modules/merge-target-features.cpp b/clang/test/Modules/merge-target-features.cpp
index 73474e1388fb6..01d38a1eb0d13 100644
--- a/clang/test/Modules/merge-target-features.cpp
+++ b/clang/test/Modules/merge-target-features.cpp
@@ -10,6 +10,15 @@
 // RUN:   -target-cpu i386 -target-feature +sse2 \
 // RUN:   Inputs/merge-target-features/module.modulemap
 //
+// RUN: %clang_cc1 -fmodules -x c++ -fmodules-cache-path=%t \
+// RUN:   -iquote Inputs/merge-target-features \
+// RUN:   -fno-implicit-modules \
+// RUN:   -fmodule-map-file-home-is-cwd \
+// RUN:   -emit-module -fmodule-name=foo -o %t/foo-double.pcm \
+// RUN:   -triple i386-unknown-unknown \
+// RUN:   -target-cpu i386 -target-feature +sse2 -target-feature +sse2 \
+// RUN:   Inputs/merge-target-features/module.modulemap
+//
 // RUN: not %clang_cc1 -fmodules -x c++ -fmodules-cache-path=%t \
 // RUN:   -iquote Inputs/merge-target-features \
 // RUN:   -fno-implicit-modules \
@@ -60,6 +69,18 @@
 // MISMATCH: error: precompiled file '{{.*}}foo.pcm' was compiled with the target feature '+sse2' but the current translation unit is not
 // MISMATCH: error: current translation unit is compiled with the target feature '+cx16' but the precompiled file '{{.*}}foo.pcm' was not
 // MISMATCH: error: {{.*}} configuration mismatch
+//
+// RUN: %clang_cc1 -fmodules -x c++ -fmodules-cache-path=%t \
+// RUN:   -iquote Inputs/merge-target-features \
+// RUN:   -fno-implicit-modules \
+// RUN:   -fmodule-map-file-home-is-cwd \
+// RUN:   -fmodule-map-file=Inputs/merge-target-features/module.modulemap \
+// RUN:   -fmodule-file=%t/foo-double.pcm \
+// RUN:   -triple i386-unknown-unknown \
+// RUN:   -target-cpu i386 -target-feature +sse2 \
+// RUN:   -fsyntax-only merge-target-features.cpp 2>&1 \
+// RUN:   | FileCheck --allow-empty --check-prefix=DOUBLE %s
+// DOUBLE-NOT: error:
 
 #include "foo.h"
 

[fmayer]

Actually, there are more problems around how we handle + and - here.. Will re-send once I also addressed those

[fmayer]

Actually, let's take it one at a time and fix the other problems later.

[Bigcheese]

I would prefer we unique these during AST writing, or even in the CompilerInvocation directly. That would also handle the case for dependency scanning.

[fmayer]

See #187624 (comment) for the reasoning I did it like this. I don't have strong feelings but wanted to do the minimal change here.

[fmayer]

I guess implicit in this was: I don't think we should dedup in a member called AsWritten

[jansvoboda11]

Would it be possible to remove the AsWritten member entirely, perform this canonicalization during command-line parsing and switch to using Features everywhere?

[fmayer]

As mentioned in the linked comment, that is an option but makes the error messages more verbose

[jansvoboda11]

Can you explain why? Do targets inject features enabled by default into TargetOptions?

[fmayer]

my (cursory) understanding is that e.g. +sse2 also implicitly +sse

[fmayer]

Ping