[lld][macho] Rename test to fix spelling #160938
Conversation
|
@llvm/pr-subscribers-lld-macho Author: Ellis Hoag (ellishg) ChangesThe test name had a typo from #140307. Fix it. I realized cstring ordering is not supported when string deduplication is turned off. We could easily call Full diff: https://github.com/llvm/llvm-project/pull/160938.diff 2 Files Affected:
diff --git a/lld/MachO/SyntheticSections.cpp b/lld/MachO/SyntheticSections.cpp
index 979a4ee6d8133..228b84db21c2a 100644
--- a/lld/MachO/SyntheticSections.cpp
+++ b/lld/MachO/SyntheticSections.cpp
@@ -1687,6 +1687,9 @@ void CStringSection::writeTo(uint8_t *buf) const {
void CStringSection::finalizeContents() {
uint64_t offset = 0;
+ // TODO: Call buildCStringPriorities() to support cstring ordering when
+ // deduplication is off, although this may negatively impact build
+ // performance.
for (CStringInputSection *isec : inputs) {
for (const auto &[i, piece] : llvm::enumerate(isec->pieces)) {
if (!piece.live)
diff --git a/lld/test/MachO/ordre-file-cstring.s b/lld/test/MachO/order-file-cstring.s
similarity index 100%
rename from lld/test/MachO/ordre-file-cstring.s
rename to lld/test/MachO/order-file-cstring.s
|
|
@llvm/pr-subscribers-lld Author: Ellis Hoag (ellishg) ChangesThe test name had a typo from #140307. Fix it. I realized cstring ordering is not supported when string deduplication is turned off. We could easily call Full diff: https://github.com/llvm/llvm-project/pull/160938.diff 2 Files Affected:
diff --git a/lld/MachO/SyntheticSections.cpp b/lld/MachO/SyntheticSections.cpp
index 979a4ee6d8133..228b84db21c2a 100644
--- a/lld/MachO/SyntheticSections.cpp
+++ b/lld/MachO/SyntheticSections.cpp
@@ -1687,6 +1687,9 @@ void CStringSection::writeTo(uint8_t *buf) const {
void CStringSection::finalizeContents() {
uint64_t offset = 0;
+ // TODO: Call buildCStringPriorities() to support cstring ordering when
+ // deduplication is off, although this may negatively impact build
+ // performance.
for (CStringInputSection *isec : inputs) {
for (const auto &[i, piece] : llvm::enumerate(isec->pieces)) {
if (!piece.live)
diff --git a/lld/test/MachO/ordre-file-cstring.s b/lld/test/MachO/order-file-cstring.s
similarity index 100%
rename from lld/test/MachO/ordre-file-cstring.s
rename to lld/test/MachO/order-file-cstring.s
|
|
|
||
| void CStringSection::finalizeContents() { | ||
| uint64_t offset = 0; | ||
| // TODO: Call buildCStringPriorities() to support cstring ordering when |
There was a problem hiding this comment.
Aside from the comment, would it make sense to log this information to the user if deduplication is not enabled and c-string ordering is specified?
There was a problem hiding this comment.
That's a good idea, but unfortunately we can't tell if an orderfile contains cstrings or just normal symbols without looking at the file itself. They both use the same -order_file flag.
There was a problem hiding this comment.
Okay that's fair. We'd have to read in the orderfile at some point when specified. In theory, we could thread that information of whether an orderfile contains cstring up to here, but it might not be worth that complexity if this specific case is something a user would even care about.
The test name had a typo from llvm#140307. Fix it. I realized cstring ordering is not supported when string deduplication is turned off. We could easily call `buildCStringPriorities()` in `CStringSection::finalizeContents()`, but I worry it might harm build performance since it creates multiple vectors and searches though maps. If users are not deduplicating strings, they probably won't care to order them, but it would be good to support this.
The test name had a typo from #140307. Fix it.
I realized cstring ordering is not supported when string deduplication is turned off. We could easily call
buildCStringPriorities()inCStringSection::finalizeContents(), but I worry it might harm build performance since it creates multiple vectors and searches though maps. If users are not deduplicating strings, they probably won't care to order them, but it would be good to support this.