[clang] Fix sorting module headers #73146
Conversation
Struct Module::Header is not a POD type. As such, qsort() and llvm::array_pod_sort() must not be used to sort it. This became an issue with the new implementation of qsort() in glibc 2.39 that is not guaranteed to be a stable sort, causing Headers to be re-ordered and corrupted. Replace the usage of llvm::array_pod_sort() with std::stable_sort() in order to fix this issue. The signature of compareModuleHeaders() has to be modified. This commit also fixes commit d3676d4 that caused all headers to have NameAsWritten set to a 0-length string without adapting compareModuleHeaders() to the new field. Fixes llvm#73145.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Tulio Magno Quites Machado Filho (tuliom) ChangesStruct Module::Header is not a POD type. As such, qsort() and llvm::array_pod_sort() must not be used to sort it. This became an issue with the new implementation of qsort() in glibc 2.39 that is not guaranteed to be a stable sort, causing Headers to be re-ordered and corrupted. Replace the usage of llvm::array_pod_sort() with std::stable_sort() in order to fix this issue. The signature of compareModuleHeaders() has to be modified. This commit also fixes commit d3676d4 that caused all headers to have NameAsWritten set to a 0-length string without adapting compareModuleHeaders() to the new field. Fixes #73145. Full diff: https://github.com/llvm/llvm-project/pull/73146.diff 1 Files Affected:
diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 00e13c9be4a7d73..7ba5a5f8cbf973d 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -2509,9 +2509,11 @@ void ModuleMapParser::parseHeaderDecl(MMToken::TokenKind LeadingToken,
<< FixItHint::CreateReplacement(CurrModuleDeclLoc, "framework module");
}
-static int compareModuleHeaders(const Module::Header *A,
- const Module::Header *B) {
- return A->NameAsWritten.compare(B->NameAsWritten);
+static bool compareModuleHeaders(const Module::Header &A,
+ const Module::Header &B) {
+ return A.NameAsWritten < B.NameAsWritten ||
+ (A.NameAsWritten == B.NameAsWritten &&
+ A.PathRelativeToRootModuleDirectory < B.PathRelativeToRootModuleDirectory);
}
/// Parse an umbrella directory declaration.
@@ -2574,7 +2576,7 @@ void ModuleMapParser::parseUmbrellaDirDecl(SourceLocation UmbrellaLoc) {
}
// Sort header paths so that the pcm doesn't depend on iteration order.
- llvm::array_pod_sort(Headers.begin(), Headers.end(), compareModuleHeaders);
+ std::stable_sort(Headers.begin(), Headers.end(), compareModuleHeaders);
for (auto &Header : Headers)
Map.addHeader(ActiveModule, std::move(Header), ModuleMap::TextualHeader);
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I've just fixed the code format. |
Sorry, I don't quite understand tnhis - but I guess the second field ( PathRelativeToRootModuleDirectory ) comparison was to address this bug? It'd probably be good to fix that separately, so it can be discussed in more detail, etc. |
@dwblaikie Yes.
The commit is already very small. Splitting it wouldn't help with bisect, as we would continue having a broken commit. After the both issues are understood, it becomes easy to understand why they're being made. |
Not sure I understand - presumably this bug has existed for a while, separate from the qsort issue? So fixing it separately seems good so that patches do one thing clearly - makes it easy to review, easy to identify root causes more narrowly, easy to revert, etc. |
|
OK. I modified this PR in order to only make the changes that fix #73145 . |
Thanks, really appreciate your patience/understanding here - I know it's a bit of a fuss to split things up, repost, etc. |
Struct Module::Header is not a POD type. As such, qsort() and llvm::array_pod_sort() must not be used to sort it. This became an issue with the new implementation of qsort() in glibc 2.39 that is not guaranteed to be a stable sort, causing Headers to be re-ordered and corrupted. Replace the usage of llvm::array_pod_sort() with std::stable_sort() in order to fix this issue. The signature of compareModuleHeaders() has to be modified. Fixes #73145. (cherry picked from commit cf1bde3)
Local branch amd-gfx af24aa3 Merged main:076ec9f5f5bf into amd-gfx:f74e82fc358f Remote branch main cf1bde3 [clang] Fix sorting module headers (llvm#73146)
Struct Module::Header is not a POD type. As such, qsort() and llvm::array_pod_sort() must not be used to sort it. This became an issue with the new implementation of qsort() in glibc 2.39 that is not guaranteed to be a stable sort, causing Headers to be re-ordered and corrupted.
Replace the usage of llvm::array_pod_sort() with std::stable_sort() in order to fix this issue. The signature of compareModuleHeaders() has to be modified.
Fixes #73145.