This is an archive of the discontinued LLVM Phabricator instance.

[AIX][TLS] Add support for TLSGD relocations to XCOFF objects
ClosedPublic

Authored by NeHuang on Apr 9 2021, 9:47 AM.

Details

Summary

This patch adds support for TLSGD relocations to XCOFF objects:

  • Add branch absolute reloction R_RBA, R_TLS relocation for the variable offset for the tlsgd model and R_TLSM for the region handle for the tlsgd model
  • Properly set the relocation fixed values for R_TLS and R_TLSM
  • Emit the TCEntry with the variant kind in the XCOFFStreamer

Diff Detail

Event Timeline

NeHuang created this revision.Apr 9 2021, 9:47 AM
NeHuang requested review of this revision.Apr 9 2021, 9:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2021, 9:47 AM
NeHuang edited the summary of this revision. (Show Details)Apr 9 2021, 9:47 AM
NeHuang edited the summary of this revision. (Show Details)
DiggerLin added inline comments.Apr 14 2021, 1:45 PM
llvm/lib/MC/MCELFStreamer.cpp
462 ↗(On Diff #336502)

the patch is for aix , why there is changed on the MCELFStreamer.cpp ?

NeHuang updated this revision to Diff 337555.Apr 14 2021, 2:07 PM

Address the review comments to remove unnecessary changes for ELF.

NeHuang marked an inline comment as done.Apr 14 2021, 2:08 PM

Patch looks pretty good. IIUC we were not printing the '@gd' specifier before because it is assumed by default by the system assembler. Now that we are adding object file support we can no longer rely of the system assembler to assume the specifier based on the storage mapping class ,and need to add the new variant kinds so we create the correct relocations, which has the side effect of also changing the asm we emit. I know I am being pedantic, but I think we should land that part of the change and the update to the asm test in an initial patch even though the object writing is the motivation for adding it.

llvm/lib/Target/PowerPC/PPC.h
132

Can we keep using the MO_TLSGD_FLAG for this, and differentiate based on checking that the system is AIX?

Patch looks pretty good. IIUC we were not printing the '@gd' specifier before because it is assumed by default by the system assembler. Now that we are adding object file support we can no longer rely of the system assembler to assume the specifier based on the storage mapping class ,and need to add the new variant kinds so we create the correct relocations, which has the side effect of also changing the asm we emit. I know I am being pedantic, but I think we should land that part of the change and the update to the asm test in an initial patch even though the object writing is the motivation for adding it.

I agree, lets split out the ASM portion from the object code portion.

NeHuang updated this revision to Diff 339221.Apr 21 2021, 6:59 AM

Address review comments to

sfertile added inline comments.May 3 2021, 10:28 AM
llvm/test/CodeGen/PowerPC/aix-tls-xcoff-reloc-large.ll
527

I don't know if we have decided on any formatting guidelines for these types of tests so that we have a consistent feel across all the tests in the back end, but if we have let me know and we can ignore this comment.

I think for this output here, the only important part is the instruction, while the instructions that have relocations the important part also includes the instructions address. From that I think should have lit checks along the lines of the following:

DIS:              virt-addr (idx: N)    Label:
DIS-NEXT:                                  Instr operands
...
DIS-NEXT:    virt-addr:  {{.*}}     Instr operands
DIS-NEXT:    virt-addr: Reloc (idx: M) Sym_Name
...

where we include the virtual address on labels, instructions that are referenced by relocations, and the relocations themselves. In the instructions that are referenced by relocations we need to add a regex to consume the raw data, but in the other instructions we just ignore everything preceding the instruction mnemonic.

576

When we get to the data section, whats important has flipped and the virtual address and the contents of memory is important, while the text that comes after the memory contents typically isn't important (in fact its typically misleading 😆) and so I think we should drop the text following the raw data.

A minor nit: FWIW I think we should try to align the contents of the checks also. eg:

DIS:            00000090 (idx: 11) storesTIInit[DS]:
DIS-NEXT:               90: 00 00 00 00
DIS-NEXT: 00000090: R_POS  (idx: 5) .storesTIInit
...
NeHuang updated this revision to Diff 343022.May 5 2021, 6:18 AM

Addressed review comments

  • Check the important contents in the test case
  • Use regex to check the instructions referened by reloctions
  • Fix the alignment
NeHuang marked 2 inline comments as done.May 5 2021, 6:18 AM
sfertile accepted this revision.May 6 2021, 6:33 AM

Thanks for the updates Victor, LGTM.

This revision is now accepted and ready to land.May 6 2021, 6:33 AM
This revision was landed with ongoing or failed builds.May 6 2021, 7:05 AM
This revision was automatically updated to reflect the committed changes.