[clang] correctly handle +/- features when matching modules#187624
Closed
fmayer wants to merge 4890 commits into
Conversation
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Florian Mayer (fmayer) ChangesBy sorting and then comparing, we make +sse2 -sse2 equal to Full diff: https://github.com/llvm/llvm-project/pull/187624.diff 2 Files Affected:
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index db4fa68f6db08..6738c19880b11 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -489,16 +489,30 @@ static bool checkTargetOptions(const TargetOptions &TargetOpts,
#undef CHECK_TARGET_OPT
// Compare feature sets.
- SmallVector<StringRef, 4> ExistingFeatures(
- ExistingTargetOpts.FeaturesAsWritten.begin(),
- ExistingTargetOpts.FeaturesAsWritten.end());
- 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());
+ std::set<StringRef> ExistingFeatures;
+ std::set<StringRef> ReadFeatures;
+
+ for (StringRef Name : ExistingTargetOpts.FeaturesAsWritten) {
+ if (Name.size() < 2)
+ continue;
+ StringRef Feature = Name.substr(1);
+ if (Name[0] == '+')
+ ExistingFeatures.emplace(Feature);
+ else if (Name[0] == '-')
+ if (auto It = ExistingFeatures.find(Feature);
+ It != ExistingFeatures.end())
+ ExistingFeatures.erase(Feature);
+ }
+ for (StringRef Name : TargetOpts.FeaturesAsWritten) {
+ if (Name.size() < 2)
+ continue;
+ StringRef Feature = Name.substr(1);
+ if (Name[0] == '+')
+ ReadFeatures.emplace(Feature);
+ else if (Name[0] == '-')
+ if (auto It = ReadFeatures.find(Feature); It != ReadFeatures.end())
+ ReadFeatures.erase(Feature);
+ }
// We compute the set difference in both directions explicitly so that we can
// diagnose the differences differently.
@@ -518,10 +532,12 @@ static bool checkTargetOptions(const TargetOptions &TargetOpts,
if (Diags) {
for (StringRef Feature : UnmatchedReadFeatures)
Diags->Report(diag::err_ast_file_targetopt_feature_mismatch)
- << /* is-existing-feature */ false << ModuleFilename << Feature;
+ << /* is-existing-feature */ false << ModuleFilename
+ << (std::string("+") + Feature.str());
for (StringRef Feature : UnmatchedExistingFeatures)
Diags->Report(diag::err_ast_file_targetopt_feature_mismatch)
- << /* is-existing-feature */ true << ModuleFilename << Feature;
+ << /* is-existing-feature */ true << ModuleFilename
+ << (std::string("+") + Feature.str());
}
return !UnmatchedReadFeatures.empty() || !UnmatchedExistingFeatures.empty();
diff --git a/clang/test/Modules/merge-target-features.cpp b/clang/test/Modules/merge-target-features.cpp
index 01d38a1eb0d13..f662c36dade4e 100644
--- a/clang/test/Modules/merge-target-features.cpp
+++ b/clang/test/Modules/merge-target-features.cpp
@@ -19,6 +19,24 @@
// RUN: -target-cpu i386 -target-feature +sse2 -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-plusminus.pcm \
+// RUN: -triple i386-unknown-unknown \
+// RUN: -target-cpu i386 -target-feature +sse2 -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-minusplus.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 \
@@ -81,6 +99,54 @@
// RUN: -fsyntax-only merge-target-features.cpp 2>&1 \
// RUN: | FileCheck --allow-empty --check-prefix=DOUBLE %s
// DOUBLE-NOT: error:
+//
+// 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-plusminus.pcm \
+// RUN: -triple i386-unknown-unknown \
+// RUN: -target-cpu i386 \
+// RUN: -fsyntax-only merge-target-features.cpp 2>&1 \
+// RUN: | FileCheck --allow-empty --check-prefix=PLUSMINUS %s
+// PLUSMINUS-NOT: error:
+//
+// 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-minusplus.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=MINUSPLUS %s
+// MINUSPLUS-NOT: error:
+//
+// 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.pcm \
+// RUN: -triple i386-unknown-unknown \
+// RUN: -target-cpu i386 -target-feature +sse2 -target-feature invalid \
+// RUN: -fsyntax-only merge-target-features.cpp 2>&1 \
+// RUN: | FileCheck --allow-empty --check-prefix=IGNORED %s
+// IGNORED-NOT: error:
+//
+// 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.pcm \
+// RUN: -triple i386-unknown-unknown \
+// RUN: -target-cpu i386 -target-feature +sse2 -target-feature x \
+// RUN: -fsyntax-only merge-target-features.cpp 2>&1 \
+// RUN: | FileCheck --allow-empty --check-prefix=IGNORED2 %s
+// IGNORED2-NOT: error:
#include "foo.h"
|
Contributor
Author
|
Another option would have been to use |
github-actions
Bot
deleted the
users/fmayer/spr/clang-correctly-handle-features-when-matching-modules
branch
github-actions
Bot
deleted the branch
users/fmayer/spr/main.clang-correctly-handle-features-when-matching-modules
Using old GCC (7.5 in this case), we get a compile error about not being able to deduce the template paramerter: ``` /llvm/llvm/tools/llubi/lib/Interpreter.cpp:770:14: error: no viable constructor or deduction guide for deduction of template arguments of 'std::vector' 770 | return std::vector(Vec.begin() + Offset, Vec.begin() + Offset + DstSize); ``` The error was introduced in #194345. Just specify the element type. --------- Signed-off-by: Nick Sarnie <nick.sarnie@intel.com>
The offload case is building a fresh filepath and there's no file extension to replace. Just directly append the file extension to avoid clobbering part of the path name if the triple contains a period. Avoids confusing test updates in future triple patch.
Pass `--linker classic` or `--linker parallel` on every `dsymutil` invocation instead of relying on the implicit default. This preserves the existing coverage in preparation for toggling the default in the future. Tests previously exercising only one linker now mirror the RUN block for the other, sharing FileCheck prefixes. Not all tests are compatible, and I've added a FIXME to make them easy to spot.
Remove unreachable addConst() on the by-value return path of addHandleAccessFunction. No caller passes IsConstReturn=true with IsRef=false. The existing AST tests ( StructuredBuffers-AST.hlsl, ByteAddressBuffers-AST.hlsl, TypedBuffers-AST.hlsl) already assert the by-value Load return type is non-const, so behavior is verified unchanged. Assisted by: Github Copilot Fixes #194982
…5343) RNBRemote::GetJSONThreadsInfo() has a bool mode switch: Only exception-related information information about threads which had an exception, or full information about all threads. The exception-related information is what ends up in the `jstopinfo` key in the stop packet, asciihex encoded. The full information is what is sent for the `jThreadsInfo` packet, with full information for all threads at a public stop. When I added the `added-binaries` and `detailed-binaries-info` keys to the thread description, I incorrectly put this in the exception related block of this method. Move that in to the "full information" section of the method, so we don't duplicate the information that is included in the stop packet, asciihex encoded at that. rdar://176001611
Fixes lit test comment from #195286
…ease (#195219) We are trying to move a way from using secrets associated with the llvmbot account, so this drops another one of its users.
…(NFCI) (#195220) Collect the inputs, derived pointers, and mutable evaluation state of DWARFExpression::Evaluate into a file-local EvalContext struct passed by reference to the static helpers. Two incidental fixes making this not-quite NFC: 1. Evaluate_DW_OP_deref_size renamed to Evaluate_DW_OP_deref and takes the LocationAtom, so error messages name the actual opcode. 2. ResolveLoadAddress no longer crashes on null exe_ctx (uses eval_ctx.target, which is null-safe). I initially prototyped a visitor-like design, centered around a DWARFEvaluator class with one method per opcode and shared state as members. I discarded it because moving the simple operations out of the switch hurt readability and increased mental overhead. Its only real benefit were the cleaner signatures, which this patch achives by using the new EvalContext.
Remove the `#include <sys/personality.h>` from the implementation header. On buildbots, the generated header does not exist at compile time, so `-idirafter/usr/include` pulls in glibc's version instead. Glibc's header uses `__BEGIN_DECLS` and `__THROW`, which are unavailable in the freestanding build. The include was unnecessary. The function signature only uses basic types. Fixes buildbot failures introduced by #195065: - libc-aarch64-ubuntu-fullbuild-dbg - libc-x86_64-debian-fullbuild-dbg-asan - libc-x86_64-debian-fullbuild-dbg - libc-x86_64-debian-gcc-fullbuild-dbg
…eleases (#195312) We are trying to move a way from using secrets associated with the llvmbot account, so this drops another one of its users.
#195315) We are trying to move a way from using secrets associated with the llvmbot account, so this drops another one of its users.
Copy what the HIP path does, except use "Unused" instead of "Generic". Avoids assertions in future patch.
…91528) This PR adjusts how the string table is generated for PSV versions 1 and 2. Previously, the string name would be unconditionally added to the string table, when it should only be added in version 3. Adds a test to verify there is no entry name in the string table for older PSV versions. Fixes #117267 Assisted by: Github Copilot
) UnfoldConstant returns a boolean and logs errors to a stream. However, the prevailing pattern is to return an Error object and let the caller decide what to do with it. Currently, these error messages are dumped out to stdout when LLDB encoutners an issue. I decided not to change that behavior with this commit to keep it simple and self-contained. I also reformatted and simplified some code in UnfoldConstant where it was relatively easy.
By sorting and then comparing, we made +sse2 -sse2 equal to -sse2 +sse2, where the former has sse2 disabled, and the latter enabled. I verified this is actually the case by compiling the following: ``` #ifdef __SSE2__ #error X #endif ``` Pull Request: #187624
Created using spr 1.3.7 [skip ci]
fmayer
requested review from
DeinAlptraum,
Endilll,
QuietMisdreavus,
aaupov,
andykaylor,
ayermolo,
banach-space,
bcardosolopes,
lamb-j,
lanza,
maksfb,
paschalis-mpeis,
rafaelauler,
snprajwal,
xlauko,
yavtuk,
yota9 and
yozhu
as code owners
Contributor
Author
|
Sorry for the spam. I tried to restore this change after the branch was accidentally deleted by automation. |
Contributor
Author
|
Sorry for the spam. I tried to restore this change after the branch was accidentally deleted by automation. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 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.
By sorting and then comparing, we made +sse2 -sse2 equal to
-sse2 +sse2, where the former has sse2 disabled, and the latter
enabled. I verified this is actually the case by compiling the
following: