LLVM 12 regression: ld.lld: error: test.o:(.rodata.str1.1): offset is outside the section

[andrewrk]

Bugzilla Link	49821
Resolution	FIXED
Resolved on	Jun 08, 2021 16:17
Version	trunk
OS	All
Blocks	#48661
CC	@DimitryAndric,@emaste,@MaskRay,@slacka,@nicolas17,@atanasyan,@tstellar
Fixed by commit(s)	7e83a7f fec90b2

Extended Description

A regression in LLVM12 (tested on rc2, rc4) is causing Zig test suite to fail on the target mips-linux.

ld.lld: error: /home/andy/Downloads/zig/zig-cache/o/7da08f6c0f092ce043d5af1591728354/test.o:(.rodata.str1.1): offset is outside the section
The following command exited with error code 1:
/home/andy/Downloads/zig/build-llvm12/zig test /home/andy/Downloads/zig/test/stage1/behavior.zig --test-name-prefix behavior-mips-linux-none-Debug-bare-multi --cache-dir /home/andy/Downloads/zig/zig-cache --global-cache-dir /home/andy/.cache/zig --name test -target mips-linux-none --test-cmd qemu-mips --test-cmd-bin -I /home/andy/Downloads/zig/test -L /home/andy/local/llvm12-release/lib -isystem /home/andy/local/llvm12-release/include --override-lib-dir /home/andy/Downloads/zig/lib

downstream issue: ziglang/zig#8155

content copied here for convenience:

Minimal reproducer:

	.text
foo:
	lui	$2, %hi(bar)
	addiu	$2, $2, %lo(bar)

	.section	.rodata.str1.1,"aMS",@progbits,1
	.zero 0x8000
bar:
	.asciz	"hello"

The 0x8000 gap simulates the previous content in the .rodata.str1.1 section, it's big enough to get sign-extended and small enough to fit in 16bit. The problem is in LLD's MarkLive::resolveReloc, the code is not able to handle the HI16/LO16 relocation pairs.

It's the 32bit one that's showing this problem:

llvm-mc-12 -triple mipsel-linux-none -filetype obj -o - /tmp/foo.s | llvm-objdump-12 -dr -

00000000 <foo>:
       0: 01 00 02 3c  	lui	$2, 1 <foo+0x1>
			00000000:  R_MIPS_HI16	.rodata.str1.1
       4: 00 80 42 24  	addiu	$2, $2, -32768 <foo+0xffffffffffff8000>
			00000004:  R_MIPS_LO16	.rodata.str1.1

[andrewrk]

assigned to @tstellar

[nicolas17]

In what previous LLVM version did it work correctly?

[andrewrk]

LLVM 11

[slacka]

Can we get a bisect on this?

[nicolas17]

The reason why I asked when it worked is that the minimal reproducer failed on LLVM11 too for me, so I couldn't bisect, but I think I figured out what I was doing wrong before. Bisect running!

[nicolas17]

I got the bisect-run condition backwards (ugh) but it landed me on (a revert of) https://reviews.llvm.org/D64327 which seems plausible. Running it properly now, hopefully I will get the same result.

[nicolas17]

I confirm doing the bisect correctly ends up here: https://reviews.llvm.org/rG72e75ca343c6ff927a2242efee3f4640943eedd6

[MC][ELF] Allow STT_SECTION referencing SHF_MERGE on REL targets

This relands D64327 with a more specific workaround for R_386_GOTOFF
(gold<2.34 bug https://sourceware.org/bugzilla/show_bug.cgi?id=16794)

.debug_info has quite a few .debug_str relocations (R_386_32/R_ARM_ABS32).
The original workaround was too general and introduced too many .L symbols
used just as relocation targets.

From the original review:

  ... it reduced the size of a big ARM-32 debug image by 33%. It contained ~68M
  of relocations symbols out of total ~71M symbols (96% of symbols table was
  generated for relocations with symbol).

So if I understand this correctly, lld never supported this, but llvm 11 MC didn't emit it and llvm 12 does? That explains my problem bisecting it before, I was only re-running lld (and in llvm 11 it failed in the same way given the same .o) but I wasn't re-assembling the .s into .o.

[DimitryAndric]

Same here when building mips world for FreeBSD:

...
--- libcam.so.7.full ---
building shared library libcam.so.7
/home/dim/obj/main/home/dim/src/llvm-12-update/freebsd14-amd64/tmp/usr/bin/cc -target mips-unknown-freebsd14.0 --sysroot=/home/dim/obj/main/home/dim/src/llvm-12-update/mips.mips/tmp -B/home/dim/obj/main/home/dim/src/llvm-12-update/mips.mips/tmp/usr/bin -EB -mabi=32 -shared -Wl,-x -Wl,--fatal-warnings -Wl,--warn-shared-textrel -o libcam.so.7.full -Wl,-soname,libcam.so.7 camlib.pico scsi_cmdparse.pico scsi_all.pico scsi_da.pico scsi_sa.pico cam.pico ata_all.pico nvme_all.pico smp_all.pico -lsbuf
ld: error: scsi_all.pico:(.rodata.str1.1): offset is outside the section
cc: error: linker command failed with exit code 1 (use -v to see invocation)
*** [libcam.so.7.full] Error code 1

I guess reverting rG72e75ca343c6ff927a2242efee3f4640943eedd6 is not feasible?

[DimitryAndric]

I confirm doing the bisect correctly ends up here:
https://reviews.llvm.org/rG72e75ca343c6ff927a2242efee3f4640943eedd6

[MC][ELF] Allow STT_SECTION referencing SHF_MERGE on REL targets

This relands D64327 with a more specific workaround for R_386_GOTOFF
(gold<2.34 bug https://sourceware.org/bugzilla/show_bug.cgi?id=16794)

Alternatively, maybe put an exception there for EM_MIPS in the mean time?

[atanasyan]

I confirm doing the bisect correctly ends up here:
https://reviews.llvm.org/rG72e75ca343c6ff927a2242efee3f4640943eedd6

[MC][ELF] Allow STT_SECTION referencing SHF_MERGE on REL targets

This relands D64327 with a more specific workaround for R_386_GOTOFF
(gold<2.34 bug https://sourceware.org/bugzilla/show_bug.cgi?id=16794)

Alternatively, maybe put an exception there for EM_MIPS in the mean time?

I think now it's the best workaround.

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 type is R_ABS). When we calculate R_MIPS_HI16's addend we find a paired R_MIPS_LO16, generate combined addend and return correct symbol value from the InputSectionBase::getRelocTargetVA. For R_MIPS_LO16 we do not do that and use its addend as is. Probably we should introduce new expression type for these relocations (like R_RISCV_PC_INDIRECT for R_RISCV_PCREL_HI20 / R_RISCV_PCREL_LO12) and calculate a correct symbol value for both relocations at the InputSectionBase::getRelocTargetVA.

Unfortunately I do not have a time to fix it by myself right now.

[DimitryAndric]

As a local workaround I now have:

--- a/contrib/llvm-project/llvm/lib/MC/ELFObjectWriter.cpp
+++ b/contrib/llvm-project/llvm/lib/MC/ELFObjectWriter.cpp
@@ -1394,8 +1394,11 @@ bool ELFObjectWriter::shouldRelocateWithSymbol(const MCAssembler &Asm,

   // gold<2.34 incorrectly ignored the addend for R_386_GOTOFF (9)
   // (http://sourceware.org/#16794 ).

•  if (TargetObjectWriter->getEMachine() == ELF::EM_386 &&
  

•      Type == ELF::R_386_GOTOFF)
  

•  if ((TargetObjectWriter->getEMachine() == ELF::EM_386 &&
  

•       Type == ELF::R_386_GOTOFF) ||
  

•      (TargetObjectWriter->getEMachine() == ELF::EM_MIPS &&
  

•       !hasRelocationAddend()))
  

•    return true;
  

  }

[emaste]

I'd suggest we split the

•      (TargetObjectWriter->getEMachine() == ELF::EM_MIPS &&
  

•       !hasRelocationAddend()))
  

out to a separate condition with a new comment describing the issue from comment #​9.

[DimitryAndric]

Review posted at https://reviews.llvm.org/D101773.

[DimitryAndric]

Fix re-landed in:
https://reviews.llvm.org/rG7e83a7f1fdfcc2edde61f0a535f9d7a56f531db9

Tom, can we please merge this to the 12.x branch after it has baked a little?

(I'm removing bug 48902 from the "Blocks:" field, but not bug 49317)

[andrewrk]

I just tested with LLVM 12.0.1 RC1, and the bug is still present. Did the fix get cherry picked into the release/12.x branch?

[DimitryAndric]

I just tested with LLVM 12.0.1 RC1, and the bug is still present. Did the
fix get cherry picked into the release/12.x branch?

Not yet, it seems; but it's still marked as a blocker for bug 49317, so hopefully it'll get picked up for rc2.

[tstellar]

The MC/Mips/elf-relsym.s test fails when I backport this patch. Can someone take a look at this?

[DimitryAndric]

Additional patch for 7e83a7f1fdfcc2edde61f0a535f9d7a56f531db9 cherry-pick

The MC/Mips/elf-relsym.s test fails when I backport this patch. Can someone
take a look at this?

For some reason, 12.0 outputs the symbols in a different order, so the following adjustment is needed, on top of the cherry-pick:

diff --git a/llvm/test/MC/Mips/elf-relsym.s b/llvm/test/MC/Mips/elf-relsym.s
index 207cc265ca1b..b8c2f89e82e6 100644
--- a/llvm/test/MC/Mips/elf-relsym.s
+++ b/llvm/test/MC/Mips/elf-relsym.s
@@ -4,17 +4,17 @@

// CHECK: Symbols [
// CHECK: Symbol {
-// CHECK: Name: $CPI0_0
-// CHECK: }
-// CHECK: Symbol {
-// CHECK: Name: $CPI0_1
-// CHECK: }
-// CHECK: Symbol {
// CHECK: Name: $.str
// CHECK: }
// CHECK: Symbol {
// CHECK: Name: $.str1
// CHECK: }
+// CHECK: Symbol {
+// CHECK: Name: $CPI0_0
+// CHECK: }
+// CHECK: Symbol {
+// CHECK: Name: $CPI0_1
+// CHECK: }
// CHECK: ]

    .text

(I don't think FileCheck can be told the order doesn't matter...)

[tstellar]

Merged: fec90b2