Page MenuHomePhabricator

[MC][ELF] Work around R_MIPS_LO16 relocation handling problem
ClosedPublic

Authored by dim on May 3 2021, 11:10 AM.

Details

Summary

This fixes PR49821, and avoids "ld.lld: error: test.o:(.rodata.str1.1):
offset is outside the section" errors when linking MIPS objects with
negative R_MIPS_LO16 implicit addends.

ld.lld handles R_MIPS_HI16/R_MIPS_LO16 separately, not as a whole, so it
doesn't know that an R_MIPS_HI16 with implicit addend 1 and an
R_MIPS_LO16 with implicit addend -32768 represents 32768, which is in
range of a MergeInputSection. We could introduce a new RelExpr member
(like R_RISCV_PC_INDIRECT for R_RISCV_PCREL_HI20 / R_RISCV_PCREL_LO12)
but the complexity is unnecessary given that GNU as keeps the original
symbol for this case as well.

Diff Detail

Unit TestsFailed

TimeTest
20 msx64 debian > LLVM.MC/Mips::elf-relsym.s
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-mc -filetype=obj -triple mipsel-unknown-linux /mnt/disks/ssd0/agent/llvm-project/llvm/test/MC/Mips/elf-relsym.s -o - | /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-readobj --symbols - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/MC/Mips/elf-relsym.s
20 msx64 debian > LLVM.MC/Mips::xgot.s
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-mc -filetype=obj -triple mipsel-unknown-linux /mnt/disks/ssd0/agent/llvm-project/llvm/test/MC/Mips/xgot.s -o - | /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-readobj -r - | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/MC/Mips/xgot.s
70 msx64 windows > LLVM.MC/Mips::elf-relsym.s
Script: -- : 'RUN: at line 1'; c:\ws\w16e2-1\llvm-project\premerge-checks\build\bin\llvm-mc.exe -filetype=obj -triple mipsel-unknown-linux C:\ws\w16e2-1\llvm-project\premerge-checks\llvm\test\MC\Mips\elf-relsym.s -o - | c:\ws\w16e2-1\llvm-project\premerge-checks\build\bin\llvm-readobj.exe --symbols - | c:\ws\w16e2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16e2-1\llvm-project\premerge-checks\llvm\test\MC\Mips\elf-relsym.s
70 msx64 windows > LLVM.MC/Mips::xgot.s
Script: -- : 'RUN: at line 1'; c:\ws\w16e2-1\llvm-project\premerge-checks\build\bin\llvm-mc.exe -filetype=obj -triple mipsel-unknown-linux C:\ws\w16e2-1\llvm-project\premerge-checks\llvm\test\MC\Mips\xgot.s -o - | c:\ws\w16e2-1\llvm-project\premerge-checks\build\bin\llvm-readobj.exe -r - | c:\ws\w16e2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16e2-1\llvm-project\premerge-checks\llvm\test\MC\Mips\xgot.s

Event Timeline

dim created this revision.May 3 2021, 11:10 AM
dim requested review of this revision.May 3 2021, 11:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2021, 11:10 AM

Test?

Both R_MIPS_HI16 and R_MIPS_LO16 are considered as absolute relocations (the type is R_ABS).

R_ABS is ld.lld's annotation . It is not a standard term. You may need to mention the ld.lld qualifier.

dim updated this revision to Diff 342499.May 3 2021, 12:11 PM

Add test case, and update comment to explicitly mention ld.lld.

This fixes PR49821, and avoids "ld.lld: error: test.o:(.rodata.str1.1): offset is outside the section" errors when linking certain MIPS objects

... with negative R_MIPS_LO16 implicit addends.

A root of the problem is in the R_MIPS_LO16 relocation handling. Both R_MIPS_HI16 and R_MIPS_LO16 are considered as absolute relocations (the ...

Rephrase, with something like: ld.lld handles R_MIPS_HI16/R_MIPS_LO16 separately, not as a whole, so it doesn't know that an R_MIPS_HI16 with implicit addend 1 and an R_MIPS_LO16 with implicit addend -32768 represents 32768, which is in range of a MergeInputSection. We could introduce a new RelExpr member (like R_RISCV_PC_INDIRECT for R_RISCV_PCREL_HI20 / R_RISCV_PCREL_LO12) but the complexity is unnecessary given that GNU as keeps the original symbol for this case as well.

llvm/lib/MC/ELFObjectWriter.cpp
1385

Simplify as my main comment suggests, with potentially less wording.

llvm/test/MC/Mips/mips_lo16.s
4 ↗(On Diff #342499)

Remove hex bytes with --no-show-raw-insn

5 ↗(On Diff #342499)

Add mips64 for RELA test

dim updated this revision to Diff 342526.May 3 2021, 1:22 PM

Update comment and commit message, and add mips64 test.

This revision is now accepted and ready to land.May 3 2021, 1:45 PM
MaskRay accepted this revision.May 3 2021, 1:48 PM

A root of the problem is in the R_MIPS_LO16 relocation handling. Both

The description needs to be updated, too.

llvm/test/MC/Mips/mips_lo16.s
1 ↗(On Diff #342526)

Use the original local symbol for REL targets.

dim edited the summary of this revision. (Show Details)May 3 2021, 1:57 PM
This revision was landed with ongoing or failed builds.May 3 2021, 1:59 PM
This revision was automatically updated to reflect the committed changes.
dim added a comment.May 3 2021, 2:11 PM

Reverted due to the influence this had on some other test cases. I'll do a full check-all and go over them one by one, but it will take some time...

Please at least run check-llvm and also look at the Harbomaster failures.

emaste added a comment.May 3 2021, 3:38 PM

Presumably the MIPS tests changed in 72e75ca343c6f need to be changed back

jrtc27 added a comment.EditedMay 27 2021, 2:57 PM

In Debian we were seeing mipsel's stage2 LLVM 12 emitting nonsense textual IR like call void @llvm.memcpy.00G8.00G8.G32(i8* align 1 %3, i8* align 1 %4, i32 0, i1 false) and %0 = tail call float @llvm.nearbyint.toH(float %a) (see https://github.com/rust-lang/rust/issues/85562 / https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=988965), and another build saw different nonsense, but each build was consistent in what it would produce, suggesting miscompilation of LLVM, not run-time memory corruption.

Cherry-picking this patch seems to have fixed that. I don't know whether this is LLVM not emitting the correct HI16/LO16 pair (both in terms of the addends and in terms of being adjacent) for llvm/lib/IR/Function.cpp's string constant pointers or whether there's a bug in GNU ld where it silently does the wrong thing, but either way this does more than just work around an LLD deficiency.

This was in libLLVM.so, too, not a statically-linked Clang, so is R_MIPS_GOT16/R_MIPS_LO16, not R_MIPS_HI16/R_MIPS_LO16, which may make a difference.

dim added a subscriber: tstellar.May 28 2021, 12:29 PM

Hm so this is definitely something that we'd want in 12.0.1? @tstellar it doesn't change any ABI, so should be perfectly safe. I'll create a PR.