[lld] More info for aarch64 ldr/str misaligning error #135004
Conversation
Co-authored-by: Kirill Andreev <hindmost.one@gmail.com> Co-authored-by: Andrei Borzenkov <root@sandwitch.dev> Co-authored-by: Dmitrii Egorov <egorov.d.i@icloud.com> Co-authored-by: Danil Berestov <goosedb@yandex.ru> Co-authored-by: Ilya Baryshnikov <zlonast3@gmail.com>
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-lld-coff Author: Serge S. Gulin (GulinSS) ChangesIt adds details to lld error message for aarch64 misalign mistakes. The origin message: Very non-informative. What is the cause? Who knows. How to debug it? No easy ways. 🩸 😔 The new message is: Well, notable better. Now we know the instruction code, the actual offset and the reason of the relocation: object file + symbol name. Enough information to resolve any similar issues! Without these changes it would be notable more painful to deal with this. Full diff: https://github.com/llvm/llvm-project/pull/135004.diff 2 Files Affected:
diff --git a/lld/COFF/Chunks.cpp b/lld/COFF/Chunks.cpp
index 3494d1ba0ac02..90c33b52c4cbe 100644
--- a/lld/COFF/Chunks.cpp
+++ b/lld/COFF/Chunks.cpp
@@ -268,7 +268,7 @@ void applyArm64Imm(uint8_t *off, uint64_t imm, uint32_t rangeLimit) {
// Even if larger loads/stores have a larger range, limit the
// effective offset to 12 bit, since it is intended to be a
// page offset.
-static void applyArm64Ldr(uint8_t *off, uint64_t imm) {
+static void applyArm64Ldr(const Twine &msg_location, uint8_t *off, uint64_t imm) {
uint32_t orig = read32le(off);
uint32_t size = orig >> 30;
// 0x04000000 indicates SIMD/FP registers
@@ -276,7 +276,9 @@ static void applyArm64Ldr(uint8_t *off, uint64_t imm) {
if ((orig & 0x4800000) == 0x4800000)
size += 4;
if ((imm & ((1 << size) - 1)) != 0)
- error("misaligned ldr/str offset");
+ error(llvm::Twine {"misaligned ldr/str offset: 0x"} + llvm::Twine::utohexstr(imm)
+ + "@" + llvm::Twine::utohexstr(static_cast<uint64_t>(orig))
+ + " with align 2^" + llvm::Twine(size) + " from " + msg_location);
applyArm64Imm(off, imm >> size, size);
}
@@ -299,10 +301,10 @@ static void applySecRelHigh12A(const SectionChunk *sec, uint8_t *off,
applyArm64Imm(off, secRel & 0xfff, 0);
}
-static void applySecRelLdr(const SectionChunk *sec, uint8_t *off,
+static void applySecRelLdr(const SectionChunk *sec, const Twine &msg_location, uint8_t *off,
OutputSection *os, uint64_t s) {
if (checkSecRel(sec, os))
- applyArm64Ldr(off, (s - os->getRVA()) & 0xfff);
+ applyArm64Ldr(msg_location, off, (s - os->getRVA()) & 0xfff);
}
void applyArm64Branch26(uint8_t *off, int64_t v) {
@@ -323,14 +325,23 @@ static void applyArm64Branch14(uint8_t *off, int64_t v) {
or32(off, (v & 0x0000FFFC) << 3);
}
-void SectionChunk::applyRelARM64(uint8_t *off, uint16_t type, OutputSection *os,
+void SectionChunk::applyRelARM64(uint8_t *off, const coff_relocation &rel, OutputSection *os,
uint64_t s, uint64_t p,
uint64_t imageBase) const {
- switch (type) {
+ // COPY FROM: maybeReportRelocationToDiscarded
+ StringRef sym_name;
+ if (this->sym) {
+ sym_name = this->sym->getName();
+ } else {
+ COFFSymbolRef coffSym = check(this->file->getCOFFObj()->getSymbol(rel.SymbolTableIndex));
+ sym_name = check(this->file->getCOFFObj()->getSymbolName(coffSym));
+ }
+
+ switch (rel.Type) {
case IMAGE_REL_ARM64_PAGEBASE_REL21: applyArm64Addr(off, s, p, 12); break;
case IMAGE_REL_ARM64_REL21: applyArm64Addr(off, s, p, 0); break;
case IMAGE_REL_ARM64_PAGEOFFSET_12A: applyArm64Imm(off, s & 0xfff, 0); break;
- case IMAGE_REL_ARM64_PAGEOFFSET_12L: applyArm64Ldr(off, s & 0xfff); break;
+ case IMAGE_REL_ARM64_PAGEOFFSET_12L: applyArm64Ldr(sym_name + "@" + this->file->getName(), off, s & 0xfff); break;
case IMAGE_REL_ARM64_BRANCH26: applyArm64Branch26(off, s - p); break;
case IMAGE_REL_ARM64_BRANCH19: applyArm64Branch19(off, s - p); break;
case IMAGE_REL_ARM64_BRANCH14: applyArm64Branch14(off, s - p); break;
@@ -344,13 +355,13 @@ void SectionChunk::applyRelARM64(uint8_t *off, uint16_t type, OutputSection *os,
case IMAGE_REL_ARM64_SECREL: applySecRel(this, off, os, s); break;
case IMAGE_REL_ARM64_SECREL_LOW12A: applySecRelLow12A(this, off, os, s); break;
case IMAGE_REL_ARM64_SECREL_HIGH12A: applySecRelHigh12A(this, off, os, s); break;
- case IMAGE_REL_ARM64_SECREL_LOW12L: applySecRelLdr(this, off, os, s); break;
+ case IMAGE_REL_ARM64_SECREL_LOW12L: applySecRelLdr(this, sym_name + "@" + this->file->getName(), off, os, s); break;
case IMAGE_REL_ARM64_SECTION:
applySecIdx(off, os, file->symtab.ctx.outputSections.size());
break;
case IMAGE_REL_ARM64_REL32: add32(off, s - p - 4); break;
default:
- error("unsupported relocation type 0x" + Twine::utohexstr(type) + " in " +
+ error("unsupported relocation type 0x" + Twine::utohexstr(rel.Type) + " in " +
toString(file));
}
}
@@ -368,6 +379,7 @@ static void maybeReportRelocationToDiscarded(const SectionChunk *fromChunk,
// Get the name of the symbol. If it's null, it was discarded early, so we
// have to go back to the object file.
+ // COPY TO: SectionChunk::applyRelARM64.
ObjFile *file = fromChunk->file;
std::string name;
if (sym) {
@@ -457,7 +469,7 @@ void SectionChunk::applyRelocation(uint8_t *off,
applyRelARM(off, rel.Type, os, s, p, imageBase);
break;
case Triple::aarch64:
- applyRelARM64(off, rel.Type, os, s, p, imageBase);
+ applyRelARM64(off, rel, os, s, p, imageBase);
break;
default:
llvm_unreachable("unknown machine type");
@@ -829,10 +841,17 @@ void ImportThunkChunkARM::writeTo(uint8_t *buf) const {
}
void ImportThunkChunkARM64::writeTo(uint8_t *buf) const {
- int64_t off = impSymbol->getRVA() & 0xfff;
memcpy(buf, importThunkARM64, sizeof(importThunkARM64));
applyArm64Addr(buf, impSymbol->getRVA(), rva, 12);
- applyArm64Ldr(buf + 4, off);
+
+ StringRef fileName;
+ if (isa<DefinedImportData>(impSymbol))
+ fileName = static_cast<DefinedImportData *>(impSymbol)->getDLLName();
+ else
+ fileName = "<not_defined_import_data>";
+
+ int64_t off = impSymbol->getRVA() & 0xfff;
+ applyArm64Ldr(impSymbol->getName() + "@" + fileName, buf + 4, off);
}
// A Thumb2, PIC, non-interworking range extension thunk.
diff --git a/lld/COFF/Chunks.h b/lld/COFF/Chunks.h
index 9f68d5a325cc5..0e04f7b82cc20 100644
--- a/lld/COFF/Chunks.h
+++ b/lld/COFF/Chunks.h
@@ -280,7 +280,7 @@ class SectionChunk : public Chunk {
uint64_t p, uint64_t imageBase) const;
void applyRelARM(uint8_t *off, uint16_t type, OutputSection *os, uint64_t s,
uint64_t p, uint64_t imageBase) const;
- void applyRelARM64(uint8_t *off, uint16_t type, OutputSection *os, uint64_t s,
+ void applyRelARM64(uint8_t *off, const coff_relocation &rel, OutputSection *os, uint64_t s,
uint64_t p, uint64_t imageBase) const;
void getRuntimePseudoRelocs(std::vector<RuntimePseudoReloc> &res);
|
💪
|
@llvm/pr-subscribers-lld Author: Serge S. Gulin (GulinSS) ChangesIt adds details to lld error message for aarch64 misalign mistakes. The origin message: Very non-informative. What is the cause? Who knows. How to debug it? No easy ways. 🩸 😔 The new message is: Well, notable better. Now we know the instruction code, the actual offset and the reason of the relocation: object file + symbol name. Enough information to resolve any similar issues! Without these changes it would be notable more painful to deal with this. Full diff: https://github.com/llvm/llvm-project/pull/135004.diff 2 Files Affected:
diff --git a/lld/COFF/Chunks.cpp b/lld/COFF/Chunks.cpp
index 3494d1ba0ac02..90c33b52c4cbe 100644
--- a/lld/COFF/Chunks.cpp
+++ b/lld/COFF/Chunks.cpp
@@ -268,7 +268,7 @@ void applyArm64Imm(uint8_t *off, uint64_t imm, uint32_t rangeLimit) {
// Even if larger loads/stores have a larger range, limit the
// effective offset to 12 bit, since it is intended to be a
// page offset.
-static void applyArm64Ldr(uint8_t *off, uint64_t imm) {
+static void applyArm64Ldr(const Twine &msg_location, uint8_t *off, uint64_t imm) {
uint32_t orig = read32le(off);
uint32_t size = orig >> 30;
// 0x04000000 indicates SIMD/FP registers
@@ -276,7 +276,9 @@ static void applyArm64Ldr(uint8_t *off, uint64_t imm) {
if ((orig & 0x4800000) == 0x4800000)
size += 4;
if ((imm & ((1 << size) - 1)) != 0)
- error("misaligned ldr/str offset");
+ error(llvm::Twine {"misaligned ldr/str offset: 0x"} + llvm::Twine::utohexstr(imm)
+ + "@" + llvm::Twine::utohexstr(static_cast<uint64_t>(orig))
+ + " with align 2^" + llvm::Twine(size) + " from " + msg_location);
applyArm64Imm(off, imm >> size, size);
}
@@ -299,10 +301,10 @@ static void applySecRelHigh12A(const SectionChunk *sec, uint8_t *off,
applyArm64Imm(off, secRel & 0xfff, 0);
}
-static void applySecRelLdr(const SectionChunk *sec, uint8_t *off,
+static void applySecRelLdr(const SectionChunk *sec, const Twine &msg_location, uint8_t *off,
OutputSection *os, uint64_t s) {
if (checkSecRel(sec, os))
- applyArm64Ldr(off, (s - os->getRVA()) & 0xfff);
+ applyArm64Ldr(msg_location, off, (s - os->getRVA()) & 0xfff);
}
void applyArm64Branch26(uint8_t *off, int64_t v) {
@@ -323,14 +325,23 @@ static void applyArm64Branch14(uint8_t *off, int64_t v) {
or32(off, (v & 0x0000FFFC) << 3);
}
-void SectionChunk::applyRelARM64(uint8_t *off, uint16_t type, OutputSection *os,
+void SectionChunk::applyRelARM64(uint8_t *off, const coff_relocation &rel, OutputSection *os,
uint64_t s, uint64_t p,
uint64_t imageBase) const {
- switch (type) {
+ // COPY FROM: maybeReportRelocationToDiscarded
+ StringRef sym_name;
+ if (this->sym) {
+ sym_name = this->sym->getName();
+ } else {
+ COFFSymbolRef coffSym = check(this->file->getCOFFObj()->getSymbol(rel.SymbolTableIndex));
+ sym_name = check(this->file->getCOFFObj()->getSymbolName(coffSym));
+ }
+
+ switch (rel.Type) {
case IMAGE_REL_ARM64_PAGEBASE_REL21: applyArm64Addr(off, s, p, 12); break;
case IMAGE_REL_ARM64_REL21: applyArm64Addr(off, s, p, 0); break;
case IMAGE_REL_ARM64_PAGEOFFSET_12A: applyArm64Imm(off, s & 0xfff, 0); break;
- case IMAGE_REL_ARM64_PAGEOFFSET_12L: applyArm64Ldr(off, s & 0xfff); break;
+ case IMAGE_REL_ARM64_PAGEOFFSET_12L: applyArm64Ldr(sym_name + "@" + this->file->getName(), off, s & 0xfff); break;
case IMAGE_REL_ARM64_BRANCH26: applyArm64Branch26(off, s - p); break;
case IMAGE_REL_ARM64_BRANCH19: applyArm64Branch19(off, s - p); break;
case IMAGE_REL_ARM64_BRANCH14: applyArm64Branch14(off, s - p); break;
@@ -344,13 +355,13 @@ void SectionChunk::applyRelARM64(uint8_t *off, uint16_t type, OutputSection *os,
case IMAGE_REL_ARM64_SECREL: applySecRel(this, off, os, s); break;
case IMAGE_REL_ARM64_SECREL_LOW12A: applySecRelLow12A(this, off, os, s); break;
case IMAGE_REL_ARM64_SECREL_HIGH12A: applySecRelHigh12A(this, off, os, s); break;
- case IMAGE_REL_ARM64_SECREL_LOW12L: applySecRelLdr(this, off, os, s); break;
+ case IMAGE_REL_ARM64_SECREL_LOW12L: applySecRelLdr(this, sym_name + "@" + this->file->getName(), off, os, s); break;
case IMAGE_REL_ARM64_SECTION:
applySecIdx(off, os, file->symtab.ctx.outputSections.size());
break;
case IMAGE_REL_ARM64_REL32: add32(off, s - p - 4); break;
default:
- error("unsupported relocation type 0x" + Twine::utohexstr(type) + " in " +
+ error("unsupported relocation type 0x" + Twine::utohexstr(rel.Type) + " in " +
toString(file));
}
}
@@ -368,6 +379,7 @@ static void maybeReportRelocationToDiscarded(const SectionChunk *fromChunk,
// Get the name of the symbol. If it's null, it was discarded early, so we
// have to go back to the object file.
+ // COPY TO: SectionChunk::applyRelARM64.
ObjFile *file = fromChunk->file;
std::string name;
if (sym) {
@@ -457,7 +469,7 @@ void SectionChunk::applyRelocation(uint8_t *off,
applyRelARM(off, rel.Type, os, s, p, imageBase);
break;
case Triple::aarch64:
- applyRelARM64(off, rel.Type, os, s, p, imageBase);
+ applyRelARM64(off, rel, os, s, p, imageBase);
break;
default:
llvm_unreachable("unknown machine type");
@@ -829,10 +841,17 @@ void ImportThunkChunkARM::writeTo(uint8_t *buf) const {
}
void ImportThunkChunkARM64::writeTo(uint8_t *buf) const {
- int64_t off = impSymbol->getRVA() & 0xfff;
memcpy(buf, importThunkARM64, sizeof(importThunkARM64));
applyArm64Addr(buf, impSymbol->getRVA(), rva, 12);
- applyArm64Ldr(buf + 4, off);
+
+ StringRef fileName;
+ if (isa<DefinedImportData>(impSymbol))
+ fileName = static_cast<DefinedImportData *>(impSymbol)->getDLLName();
+ else
+ fileName = "<not_defined_import_data>";
+
+ int64_t off = impSymbol->getRVA() & 0xfff;
+ applyArm64Ldr(impSymbol->getName() + "@" + fileName, buf + 4, off);
}
// A Thumb2, PIC, non-interworking range extension thunk.
diff --git a/lld/COFF/Chunks.h b/lld/COFF/Chunks.h
index 9f68d5a325cc5..0e04f7b82cc20 100644
--- a/lld/COFF/Chunks.h
+++ b/lld/COFF/Chunks.h
@@ -280,7 +280,7 @@ class SectionChunk : public Chunk {
uint64_t p, uint64_t imageBase) const;
void applyRelARM(uint8_t *off, uint16_t type, OutputSection *os, uint64_t s,
uint64_t p, uint64_t imageBase) const;
- void applyRelARM64(uint8_t *off, uint16_t type, OutputSection *os, uint64_t s,
+ void applyRelARM64(uint8_t *off, const coff_relocation &rel, OutputSection *os, uint64_t s,
uint64_t p, uint64_t imageBase) const;
void getRuntimePseudoRelocs(std::vector<RuntimePseudoReloc> &res);
|
|
Both errors are: Are these due of old caches? Any clues would be helpful! |
GulinSS
changed the title
No, this isn't due to caches - your PR does not compile; did you try compiling it before submitting the PR? The reason is that in the latest git version of LLD there's a new call to Now as for the change itself. Yes, these errors are frustratingly unactionable in their current form; normally you wouldn't really be hitting them, but when developing a toolchain I guess it is possible to hit them. I have considered improving this diagnostic myself the last time I touched this area. But most code LLD is very much tuned for linking performance. Digging up info about the symbol name and file name and constructing that into a string (ok Then finally, if we are spending effort on prouducing a good diagnostic here, we should probably also make a testcase for it. (We didn't have a test for the previous error message, as it was mostly seen as something like an assert.) |
Ah, yes, thanks! I will handle that. Originally this commit had been prepared for LLVM-19 but rebased on actual master later. Sounds like I messed the setup and was unable to clearly check the build. Thanks again for the pointing.
I am completely agree with that but to make it by my hands some additional guiding is needed. 🙏 Originally this change was only for single purpose: collect more context about this error message. |
Yeah I'm not sure what the best way to solve this is (otherwise I would probably have made a fix long time ago already); let's see if others have good ideas about how to do it. (Also CC @MaskRay for more ideas about how to do this neatly.) This would be neat to do e.g. with an exception, but we don't use exceptions in the LLVM codebase. So perhaps we could return some error code inside the functions handling the relocations, and if the relocation functions indicate an error, we'd gather context about where it is, and print the error. |
|
The ELF port relies more heavily on In contrast, the COFF port seems to use errorOrWarn only with the /force:unresolved option. This approach could potentially be expanded for broader application. In addition, I have noticed that a few similar |
That is first what I tried. But it has lots of context to find the actual reason. Precise error message works better for such cases. No time wasting. |
There was a problem hiding this comment.
But most code LLD is very much tuned for linking performance. Digging up info about the symbol name and file name and constructing that into a string (ok
Twine), for use if we need to report an error, for every single relocation we do, feels quite unusual compared with how things normally are done in LLD. (LLD even takes the liberty and has the design that it is allowed to crash on erroneous inputs - even if printing a diagnostic obviously is nicer.) AlthoughTwines are quite cheap so it doesn't make much of a measurable practical difference. But I still think we'd want to go slightly differently about it, to only dig up and construct info about the location if we actually do need it. Or what do others, e.g. @rnk and @aganea think here?
I think having good diagnostics is important for the user experience. MSVC isn't particularly good at that and neither is LLD, although a bit better than MSVC. Clang and GCC have improved their diagnostics over the past decade. We have the opportunity to improve LLD largerly in that aspect; If we can provide more informative messages, with more context, we should absolutely do it.
In regards to performance, I wonder if we could extend Twine to incorporate llvm::function_ref. I found myself often in such occasions where we're torn between the informative aspect of the message, and not doing too much processing upfront for a error message that might never occur. If we shovel a lambda in the Twine, messages will only be prepared when needed, with no measurable impact if they don't occur.
| error("misaligned ldr/str offset"); | ||
| error(llvm::Twine {"misaligned ldr/str offset: 0x"} + llvm::Twine::utohexstr(imm) | ||
| + "@" + llvm::Twine::utohexstr(static_cast<uint64_t>(orig)) | ||
| + " with align 2^" + llvm::Twine(size) + " from " + msg_location); |
There was a problem hiding this comment.
I think it would be better to just display the alignment, ie. llvm::Twine(1 << size)
Definitely. Although this is a quite unusual/internal diagnostic; I'm not really sure how easy it is to even hit this one with just regular user code, unless the toolchain is faulty/broken. But nevertheless, for whoever is working on the potentially faulty toolchain it's not easy to pinpoint the issue unless we give more hints, so I'm all for improving the diagnostics here.
That's certainly a good option too. I wonder how much lowlevel overhead there is in such a lambda; probably not much at all? It mostly needs to make sure that any referenced/captured variable is stored in its canonical location in the stack frame of the caller, right? Another alternative would be to just pass in more of the variables we have in the callers, into the functions, allowing the functions themselves to format everything they want/need once they want to (possibly encapsulated in a helper function). Although I'm not sure if this any better/worse than just passing in a single lambda either. And it forces passing in a lot of parameters we don't usually need. The tricky part here is actually measuring the performance impact; I did try out this patch (on linking a large-ish binary like clang.exe - that's probably the biggest one I regularly build) and I'm not sure I see any measurable difference with this patch as-is, at least not anything really big. So it's hard to measure and figure out whether it's better to gather everything and just format one single |
Add to mind the fact I am a newbie at C++ ) but with more or less detailed guiding I could try.
It will happen for this case. I tried to make changes locally and it was one reason why I was needed to call procedures which may have impact explicitly on performance.
But adding some sort of laziness would be good here. |
It adds details to lld error message for aarch64 misalign mistakes.
The origin message:
Very non-informative. What is the cause? Who knows. How to debug it? No easy ways. 🩸 😔
The new message is:
Well, notable better. Now we know the instruction code, the actual offset and the reason of the relocation: object file + symbol name. Enough information to resolve any similar issues!
💪
Without these changes it would be notable more painful to deal with this.