This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Don't force absolute FK_Data_X fixups to relocs
ClosedPublic

Authored by edward-jones on Jun 17 2019, 4:32 AM.

Details

Summary

The current behavior of shouldForceRelocation forces relocations for the majority of fixups when relaxation is enabled. This makes sense for fixups which incorporate symbols but is unnecessary for simple data fixups where the fixup target is already resolved to an absolute value.

Diff Detail

Repository
rL LLVM

Event Timeline

edward-jones created this revision.Jun 17 2019, 4:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2019, 4:32 AM
edward-jones retitled this revision from [WIP][RISCV] Don't force absolute FK_Data_X fixups to relocs to [RISCV] Don't force absolute FK_Data_X fixups to relocs.

Rebased against master

asb requested changes to this revision.Jul 2 2019, 3:12 AM

Hi Ed, can you please add a test that shows this new behaviour? Perhaps it makes sense in test/MC/RISCV/linker-relaxation.s? (FK_Data_{4,8} produce R_RISCV_{32,64} when a relocation is emitted).

This revision now requires changes to proceed.Jul 2 2019, 3:12 AM

Added test, rebased

asb added a comment.Jul 8 2019, 8:31 AM

Hi Ed, I'm struggling a bit to follow the test case you're adding here. You're checking for the absence of R_RISCV_{32,64} but the relocations produced by these expressions are surely going to be R_RISCV_{ADD,SUB}{32,64}?

In the general case, aren't we required to emit relocations for these symbol differences? The symbols might be defined in a code section and relaxation might lead to the symbol location changing?

I realize that I hadn't associated this change with D61584

Normally you would get a R_RISCV_{ADD,SUB}{32,64} for the difference expression in the test case, however with the changes in D61584 a difference expression in .data will instead be folded (by MCExpr.cpp: EvaluateSymbolicAdd) to a FK_Data_{4,8} fixup which this change can then collapse to a constant value.

At the start of a CIE in .eh_frame there is a difference expression which expresses the length of the CIE. Before D61584 and this change the length field would end up zeroed with a R_RISCV_{32,64} or R_RISCV_{ADD,SUB}{32,64} relocation. This posed a problem because the linker parses the .eh_frame section ignoring the relocations, and the zero length would cause the parsing to error out.

asb accepted this revision.Jul 16 2019, 1:37 AM

Thanks for the clarification Ed. This looks good to me - thanks!

This revision is now accepted and ready to land.Jul 16 2019, 1:37 AM

Rebase, and fix another test of FDE

This revision was automatically updated to reflect the committed changes.