[Sparc] invalid adjustments in TLS_LE/TLS_LDO relocations removed

[fedor.sergeev]

Summary

Some SPARC TLS relocations were applying nontrivial adjustments
to zero value, leading to unexpected non-zero values in ELF and then
Solaris linker failures.

Getting rid of these adjustments.

Fixes PR33825.

[fedor.sergeev]

With this change on top of trunk I'm able to manually bootstrap LLVM with clang on SPARC Solaris.

[joerg]

Test cases?

[jyknight]

Seems probably correct -- Value should always be zero, because ELFObjectWriter.cpp sets Value to 0 and moves the offset into the relocation if hasRelocationAddend() is true, which it is on sparc.

But, yea...it would be really neat if there were tests for the TLS fixups/relocations in llvm/test/MC/Sparc.

[fedor.sergeev]

Initially I thought to add something to test/MC/Sparc/sparc-relocations.s but then I realized that "llvm-readobj -r" used there to dump relocations
does not dump "Value" at all.

Can you recommend what tool would be appropriate to check Value of relocations for .o?
Is there anything besides llvm-readobj to use?

Any other ways to test this except dumping .o?

[fedor.sergeev]

Went through test/MC/AArch64/tls-reloc.s, llvm-mc -show-encoding used there does not work for my purpose either.
For the testcase from bug report it dumps this:

sethi %tle_hix22(_ZL14StackTraceHead), %i0 ! encoding: [0x31,0x00,0x00,0x00]
                                       !   fixup A - offset: 0, value: %tle_hix22(_ZL14StackTraceHead), kind: fixup_sparc_tls_le_hix22

[fedor.sergeev]

Finally I found a way to dump object file so value adjustments are visible - llvm-objdump -d works just fine.

[fedor.sergeev]

Adding a test for all the affected relocations.
Verified that it fails on non-fixed compiler :)

[fedor.sergeev]

test/MC/Sparc/sparc-tls-relocations.s

65–66 ↗

(On Diff #107838)

I was going to add complete asm sequence as generated by clang, but llvm-mc issues "invalid operand" on "@tldm_add", "@tldm_call" and "@tldo_add".
I'm not sure where the problem is, it will go on my TODO list to dig into it and at least file a bug.

I decided to comment these lines, so the rest of relocations get tested.

[jyknight]

Late LGTM. :) Comments for some future work.

test/MC/Sparc/sparc-tls-relocations.s

32 ↗

(On Diff #107838)

What's up with the "Unknown" here? That seems like another bug. And, wouldn't need the separate invocation of llvm-readobj if not for this.

65–66 ↗

(On Diff #107838)

It's because of isCodeGenOnly = 1 around TLS_ADDrr, TLS_LDrr, and TLS_CALL in SparcInstrInfo.td. I *think* that flag should just be deleted (leaving isAsmParserOnly = 1, because these cannot be parsed from an object file).

Fine to do in a subsequent change.

[hans]

Should this patch be merged to 5.0?

[joerg]

Merging would be reasonable, yes.

[hans]

In D35567#822050, @joerg wrote:

Merging would be reasonable, yes.

Thanks, r309187.

[fedor.sergeev]

In D35567#822098, @hans wrote:

In D35567#822050, @joerg wrote:

Merging would be reasonable, yes.

Thanks, r309187.

Thanks to you, @hans!

lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp

static unsigned adjustFixupValue(unsigned Kind, uint64_t Value) {
   case Sparc::fixup_sparc_pc10:
   case Sparc::fixup_sparc_got10:
   case Sparc::fixup_sparc_tls_gd_lo10:
   case Sparc::fixup_sparc_tls_ldm_lo10:
   case Sparc::fixup_sparc_tls_ie_lo10:
   case Sparc::fixup_sparc_lo10:
     return Value & 0x3ff;
 
-  case Sparc::fixup_sparc_tls_ldo_hix22:
-  case Sparc::fixup_sparc_tls_le_hix22:
-    return (~Value >> 10) & 0x3fffff;
-
-  case Sparc::fixup_sparc_tls_ldo_lox10:
-  case Sparc::fixup_sparc_tls_le_lox10:
-    return (~(~Value & 0x3ff)) & 0x1fff;
-
   case Sparc::fixup_sparc_h44:
     return (Value >> 22) & 0x3fffff;
 
   case Sparc::fixup_sparc_m44:
     return (Value >> 12) & 0x3ff;
 
   case Sparc::fixup_sparc_l44:
     return Value & 0xfff;
 
   case Sparc::fixup_sparc_hh:
     return (Value >> 42) & 0x3fffff;
 
   case Sparc::fixup_sparc_hm:
     return (Value >> 32) & 0x3ff;
 
+  case Sparc::fixup_sparc_tls_ldo_hix22:
+  case Sparc::fixup_sparc_tls_le_hix22:
+  case Sparc::fixup_sparc_tls_ldo_lox10:
+  case Sparc::fixup_sparc_tls_le_lox10:
+    assert(Value == 0 && "Sparc TLS relocs expect zero Value");
+    return 0;
+
   case Sparc::fixup_sparc_tls_gd_add:
   case Sparc::fixup_sparc_tls_gd_call:
   case Sparc::fixup_sparc_tls_ldm_add:
   case Sparc::fixup_sparc_tls_ldm_call:
   case Sparc::fixup_sparc_tls_ldo_add:
   case Sparc::fixup_sparc_tls_ie_ld:
   case Sparc::fixup_sparc_tls_ie_ldx:
   case Sparc::fixup_sparc_tls_ie_add: