clang integrated assembler generates incorrect relocations for mips32 #65020
Comments
|
Hi @MaskRay I am not sure if you happen to know how to figure out this or shed a light? |
|
@llvm/issue-subscribers-backend-mips |
|
cc @yingopq |
|
FYI, I have spent some time on investigating this issue, and I can create a much simpler case to reproduce this too. Simply put, if the static local variable is defined before the code that references it, everything works fine; if the variable is defined after that, it breaks. By the way, I am trying to fix it on my own, but I am a beginner to compiler(assembler), so I spent a lot of time studying the assembler code as well. Any suggestion is appreciated. Best regards |
|
@wzssyqa Ping. |
OK, I am researching this issue. |
Please let me know if you need more info |
Could you support aes-mips.c and file above mentioned simple case? Thanks. |
|
@yingopq The symbol After digging into the code, I found that while parsing the assembly, all the symbols would be examined, and when However, I am not familiar with LLVM, I have no idea how to fix up this after first round parsing, maybe a second parse? |
|
It is not easy to "fix", but we can claim that this is not a bug ;) In GAS's documents: So we can support diff --git a/llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp b/llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
index 3c673ae938fd..d22deb9a6587 100644
--- a/llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
+++ b/llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
@@ -2914,14 +2914,21 @@ bool MipsAsmParser::loadAndAddSymbolAddress(const MCExpr *SymExpr,
}
bool IsPtr64 = ABI.ArePtrs64bit();
+
+ if (StringRef(Res.getSymA()->getLoc().getPointer()).starts_with(".L"))
+ printf (".L symbol\n");
+
bool IsLocalSym =
Res.getSymA()->getSymbol().isInSection() ||
Res.getSymA()->getSymbol().isTemporary() ||
+ StringRef(Res.getSymA()->getLoc().getPointer()).starts_with(".L") ||
(Res.getSymA()->getSymbol().isELF() &&
cast<MCSymbolELF>(Res.getSymA()->getSymbol()).getBinding() ==
ELF::STB_LOCAL);
bool UseXGOT = STI->hasFeature(Mips::FeatureXGOT) && !IsLocalSym;
+ printf ("%d\n", IsLocalSym);
+
// The case where the result register is $25 is somewhat special. If the
// symbol in the final relocation is external and not modified with a
// constant then we must use R_MIPS_CALL16 instead of R_MIPS_GOT16 |
|
Ohh, currently |
|
Hi @wzssyqa thank you for your explanation and quick implementation for ".L", but I still have some questions:
|
|
I have a method and was implementing and testing. When mips asm parse instruction |
Sounds wonderful. Looking forward to it |
Yes. We should fix it, and I find that the link refused to link these object.
Yes. For OpenSSL, we may need to put the .rodata before .text. |
For asm code like: xx: la $2,$yy $yy: nop MIPS O32 use `$` as PrivateGlobalPrefix, while another `$` is added for getOrCreateSymbol, thus: error: Undefined temporary symbol $$yy We also set symbols that starts with `.L` as local for O32. See: llvm#65020.
For asm code like: xx: la $2,$yy $yy: nop MIPS O32 use `$` as PrivateGlobalPrefix, while another `$` is added for getOrCreateSymbol, thus: error: Undefined temporary symbol $$yy We also set symbols that starts with `.L` as local for O32. See: llvm#65020.
|
Hi @wzssyqa |
For asm code like: xx: la $2,$yy $yy: nop MIPS O32 use `$` as PrivateGlobalPrefix, while another `$` is added for getOrCreateSymbol, thus: error: Undefined temporary symbol $$yy We also set symbols that starts with `.L` as local for O32. See: llvm#65020.
…la macro (#80644) When parsing the `la` macro, we add a duplicate `$` prefix in `getOrCreateSymbol`, leading to `error: Undefined temporary symbol $$yy` for code like: ``` xx: la $2,$yy $yy: nop ``` Remove the duplicate prefix. In addition, recognize `.L`-prefixed symbols as local for O32. See: #65020. --------- Co-authored-by: Fangrui Song <i@maskray.me>
…la macro (llvm#80644) When parsing the `la` macro, we add a duplicate `$` prefix in `getOrCreateSymbol`, leading to `error: Undefined temporary symbol $$yy` for code like: ``` xx: la $2,$yy $yy: nop ``` Remove the duplicate prefix. In addition, recognize `.L`-prefixed symbols as local for O32. See: llvm#65020. --------- Co-authored-by: Fangrui Song <i@maskray.me> (cherry picked from commit c007fbb)
…la macro (llvm#80644) When parsing the `la` macro, we add a duplicate `$` prefix in `getOrCreateSymbol`, leading to `error: Undefined temporary symbol $$yy` for code like: ``` xx: la $2,$yy $yy: nop ``` Remove the duplicate prefix. In addition, recognize `.L`-prefixed symbols as local for O32. See: llvm#65020. --------- Co-authored-by: Fangrui Song <i@maskray.me> (cherry picked from commit c007fbb)
It has been merged. If you have interesting, you can try to figure out a PR to OpenSSL. |
test result: patch diff --git a/llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp b/llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
index 36aab383da68..f9f6704230f5 100644
--- a/llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
+++ b/llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp
@@ -149,6 +149,7 @@ class MipsAsmParser : public MCTargetAsmParser {
// directive.
bool IsLittleEndian;
bool IsPicEnabled;
+ bool HasParseRdata;
bool IsCpRestoreSet;
int CpRestoreOffset;
unsigned GPReg;
@@ -555,6 +556,7 @@ public:
IsPicEnabled = getContext().getObjectFileInfo()->isPositionIndependent();
IsCpRestoreSet = false;
+ HasParseRdata = false;
CpRestoreOffset = -1;
GPReg = ABI.GetGlobalPtr();
@@ -2920,11 +2922,40 @@ bool MipsAsmParser::loadAndAddSymbolAddress(const MCExpr *SymExpr,
(Res.getSymA()->getSymbol().isELF() &&
cast<MCSymbolELF>(Res.getSymA()->getSymbol()).getBinding() ==
ELF::STB_LOCAL);
+
// For O32, "$"-prefixed symbols are recognized as temporary while
// .L-prefixed symbols are not (PrivateGlobalPrefix is "$"). Recognize ".L"
// manually.
if (ABI.IsO32() && Res.getSymA()->getSymbol().getName().starts_with(".L"))
IsLocalSym = true;
+ else {
+ if (HasParseRdata == false) {
+ StringRef CurrentASMContent = StringRef(IDLoc.getPointer());
+
+ //Get local symbol name LocalSymbol from "la $number, localsymbolname\n ... "
+ size_t NewlineIndex = CurrentASMContent.find_first_of('\n');
+ size_t CommaIndex = CurrentASMContent.find_first_of(',');
+ size_t SymbolLength = NewlineIndex - CommaIndex - 2;
+ StringRef LocalSymbol = CurrentASMContent.take_front(NewlineIndex).take_back(SymbolLength);
+
+ //Get and check if ".rdata" section exist.
+ size_t RdataIndex = CurrentASMContent.find( ".rdata");
+ if (RdataIndex != StringRef::npos) {
+ StringRef Rdata = CurrentASMContent.substr(RdataIndex);
+
+ //Check if rdata section contain local symbol.
+ if (1 == Rdata.contains(LocalSymbol)) {
+ //Check if "LocalSymbol:" exist.
+ size_t A = Rdata.find(LocalSymbol);
+ size_t B = Rdata.find(':', A);
+ if (B - A == LocalSymbol.size()) {
+ IsLocalSym = true;
+ LLVM_DEBUG(dbgs() << DEBUG_TYPE << ": Has definition of local symbol " << LocalSymbol << " after 'la' instruction" << "\n");
+ }
+ }
+ }
+ }
+ }
bool UseXGOT = STI->hasFeature(Mips::FeatureXGOT) && !IsLocalSym;
// The case where the result register is $25 is somewhat special. If the
@@ -8073,6 +8104,7 @@ bool MipsAsmParser::parseDirectiveTpRelWord() {
if (getLexer().isNot(AsmToken::EndOfStatement))
return Error(getLexer().getLoc(),
"unexpected token, expected end of statement");
+ HasParseRdata = true;
Parser.Lex(); // Eat EndOfStatement token.
return false;
}
|
|
@wzssyqa Could you help review this patch? Thanks! |
… for mips32 When mips asm parse instruction la, check whether .rdata section was accessed and parsed, if it was not, then check the following statement, check if local symbol(eg "hello:") was in .rdata section, if it was in it, IsLocalSym is true. Fix llvm#65020
…or mips32 Modify asm scan starting point to ensure scaning .rdata section before .text section. Save begin location, check if have .rdata section, confirm where to begin parse. If have it and .rdata after .text, begin parse from .rdata, when go to Eof, jump to begin location and then continue parse. If did not have .rdata or .rdata is before .text, jump to begin location and then parse. Fix llvm#65020
Hi @MaskRay , this is the simple demo of this issue. The current progress for this issue is that now we can use ".L" to denote local symbols, but gas can even work without using ".L". I suppose the latest PR is trying to fix this. |
When I build openssl with clang 16, the built binary
openssljust crashes. Then I tried to narrow down to the issue, and it seems it is related to the integrated assembler of clang, because it generates different object from the one generated by gas, and finally the linker would complain that "can't find matching R_MIPS_LO16 relocation for R_MIPS_GOT16"I have preprocessed the problematic assembly file, so it can be easily verified.
aes-mips.s.zip
Reproduced steps:
clang -target mipsel-buildroot-linux-musl -fPIC -pthread -mabi=32 -mips2 -Wa,--noexecstack -Qunused-arguments -Wall -O3 -c -o aes-mips.o aes-mips.sllvm-readelf -r aes-mips.oR_MIPS_GOT16without correspondingR_MIPS_LO16If we tried the assembler from binutils.
mipsel-buildroot-linux-musl-as -march mips2 -mabi 32 -call_nonpic -EL -KPIC --noexecstack -o gas-aes-mips.o aes-mips.sllvm-readelf -r gas-aes-mips.oThe text was updated successfully, but these errors were encountered: