-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[AIX][TOC] -mtocdata/-mno-tocdata fix non deterministic iteration order #86840
[AIX][TOC] -mtocdata/-mno-tocdata fix non deterministic iteration order #86840
Conversation
Failure with testcase toc-conf.c observed when building with LLVM_REVERSE_ITERATION=ON. Changing from using llvm::StringSet to std::set<llvm:StringRef> to ensure iteration order is deterministic. Note: the functionality of the feature does not require a specific iteration order, however, this will allow testing to be consistent. From llvm docs: The advantages of std::set are that its iterators are stable (deleting or inserting an element from the set does not affect iterators or pointers to other elements) and that iteration over the set is guaranteed to be in sorted order.
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Zaara Syeda (syzaara) ChangesFailure with testcase toc-conf.c observed when building with LLVM_REVERSE_ITERATION=ON. Full diff: https://github.com/llvm/llvm-project/pull/86840.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/AIX.cpp b/clang/lib/Driver/ToolChains/AIX.cpp
index 6e089903a3158a..7a62b0f9aec419 100644
--- a/clang/lib/Driver/ToolChains/AIX.cpp
+++ b/clang/lib/Driver/ToolChains/AIX.cpp
@@ -471,7 +471,7 @@ static void addTocDataOptions(const llvm::opt::ArgList &Args,
// the global setting of tocdata in TOCDataGloballyinEffect.
// Those that have the opposite setting to TOCDataGloballyinEffect, are added
// to ExplicitlySpecifiedGlobals.
- llvm::StringSet<> ExplicitlySpecifiedGlobals;
+ std::set<llvm::StringRef> ExplicitlySpecifiedGlobals;
for (const auto Arg :
Args.filtered(options::OPT_mtocdata_EQ, options::OPT_mno_tocdata_EQ)) {
TOCDataSetting ArgTocDataSetting =
@@ -486,7 +486,7 @@ static void addTocDataOptions(const llvm::opt::ArgList &Args,
ExplicitlySpecifiedGlobals.erase(Val);
}
- auto buildExceptionList = [](const llvm::StringSet<> &ExplicitValues,
+ auto buildExceptionList = [](const std::set<llvm::StringRef> &ExplicitValues,
const char *OptionSpelling) {
std::string Option(OptionSpelling);
bool IsFirst = true;
@@ -495,7 +495,7 @@ static void addTocDataOptions(const llvm::opt::ArgList &Args,
Option += ",";
IsFirst = false;
- Option += E.first();
+ Option += E.str();
}
return Option;
};
diff --git a/clang/test/Driver/toc-conf.c b/clang/test/Driver/toc-conf.c
index 80d92ee1a90b4b..7b2d5122ebc6ce 100644
--- a/clang/test/Driver/toc-conf.c
+++ b/clang/test/Driver/toc-conf.c
@@ -23,7 +23,7 @@ void func() {
// CHECK-CONF1-NOT: warning:
// CHECK-CONF1: "-cc1"{{.*}}" "-mno-tocdata"
-// CHECK-CONF1: "-mtocdata=g2,g1"
+// CHECK-CONF1: "-mtocdata=g1,g2"
// CHECK-CONF2-NOT: warning:
// CHECK-CONF2: "-cc1"{{.*}}" "-mtocdata"
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unfortunate we have to ditch the StringSet because of the iteration order since the semantics of the option are the same regardless of the order the names are printed in.
Now that we are constructing the list in sorted order we should change these to assertions that they are already sorted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Zaara pointed out offline that we can't rely on the input being sorted despite this change because you can invoke the front end directly with an argument list in unsorted order.
Failure with testcase toc-conf.c observed when building with LLVM_REVERSE_ITERATION=ON.
Changing from using llvm::StringSet to std::setllvm:StringRef to ensure iteration order is deterministic. Note: the functionality of the feature does not require a specific iteration order, however, this will allow testing to be consistent.
From llvm docs:
The advantages of std::set are that its iterators are stable (deleting or inserting an element from the set does not affect iterators or pointers to other elements) and that iteration over the set is guaranteed to be in sorted order.