Page MenuHomePhabricator

[Sparc] invalid adjustments in TLS_LE/TLS_LDO relocations removed
ClosedPublic

Authored by fedor.sergeev on Jul 18 2017, 8:40 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

fedor.sergeev created this revision.Jul 18 2017, 8:40 AM

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

joerg added a subscriber: joerg.Jul 18 2017, 9:48 AM

Test cases?

jyknight edited edge metadata.Jul 18 2017, 10:11 AM

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.

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?

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

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

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

fedor.sergeev added inline comments.Jul 23 2017, 12:41 PM
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.

fedor.sergeev marked an inline comment as done.Jul 23 2017, 12:42 PM
This revision was automatically updated to reflect the committed changes.

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 added a subscriber: hans.Jul 26 2017, 8:55 AM

Should this patch be merged to 5.0?

joerg added a comment.Jul 26 2017, 2:05 PM

Merging would be reasonable, yes.

hans added a comment.Jul 26 2017, 2:37 PM

Merging would be reasonable, yes.

Thanks, r309187.

In D35567#822098, @hans wrote:

Merging would be reasonable, yes.

Thanks, r309187.

Thanks to you, @hans!