Page MenuHomePhabricator

[RISCV] Support resolving fixup_riscv_call and add to MCFixupKindInfo table

Authored by shiva0217 on May 21 2018, 12:23 AM.



And also adding static_assert after array declaration
to avoid missing any new fixup in MCFixupKindInfo in the future.

Fixup fixup_riscv_call by assembler if the function and callsite within the same compile unit and the linker relaxation disabled.

The patch is according to

Diff Detail


Event Timeline

shiva0217 created this revision.May 21 2018, 12:23 AM
asb added inline comments.May 23 2018, 4:55 AM
51 ↗(On Diff #147736)

This comment should explain why we need to preserve the relocation type. e.g. "We are unable to correctly resolve a fixup_riscv_call, so always emit a relocation."

90 ↗(On Diff #147736)

Thinking about it, bits should really be 64 for fixup_riscv_call. With that in place, wouldn't it be feasible to add support to adjustFixupValue so we actually can resolve the fixup?

92 ↗(On Diff #147736)

You can just use array_lengthof(Infos).

94 ↗(On Diff #147736)

"Not all fixup kinds added to Infos array" is probably sufficient.

asb requested changes to this revision.May 23 2018, 4:56 AM
This revision now requires changes to proceed.May 23 2018, 4:56 AM
asb added inline comments.May 23 2018, 5:36 AM
3 ↗(On Diff #147736)

I think a comment explaining the purpose of the test would be useful. Presumably it's something like "Check that not attempt is made to resolve a call fixups and a relocation is always emitted". Of course this would be unnecessary if we could add support for correctly resolving call fixups!

shiva0217 updated this revision to Diff 148397.May 24 2018, 6:03 AM
shiva0217 edited the summary of this revision. (Show Details)

Hi Alex.It seems that fixup fixup_riscv_call by assembler is workable. Thanks for your comments.

asb added inline comments.May 28 2018, 4:37 AM
280 ↗(On Diff #148397)

It would be helpful to have a comment to explain the addition of 0x800ULL.

1–3 ↗(On Diff #148397)

Now this test has changed to just checking the fixup is resolved when -mattr=-relax I think it makes much more sense as an MC layer test. Perhaps expanding test/MC/RISCV/fixups.s? We need some reassurance that the fixup calculation is correct, even for offsets > 12 bits.

It looks like you have to use .space as in fixups.s, as the following still results in a relocation, even though I would have expected it not to:

.equ addr, 0xdeadbeef
call addr
asb added inline comments.May 28 2018, 5:03 AM
1–3 ↗(On Diff #148397)

Sorry - I didn't turn my brain on for that comment. It's absolutely correct that the call to an absolute target results in a relocation, as of course call is pc-relative. Any way, more tests in test/MC/RISCV/fixups.s to give reassurance that resolvable fixups are encoded correctly would be really welcome.

shiva0217 updated this revision to Diff 148842.May 28 2018, 6:47 PM

Update test cases to address Alex's comments.

asb accepted this revision.May 29 2018, 12:12 PM

Thanks, looks good to me. Probably worth retitling the commit to e.g. "[RISCV] Support resolving fixup_riscv_call and add to MCFixupKindInfo table"

This revision is now accepted and ready to land.May 29 2018, 12:12 PM
shiva0217 retitled this revision from [RISCV] Add missing fixup_riscv_call in MCFixupKindInfo table to [RISCV] Support resolving fixup_riscv_call and add to MCFixupKindInfo table.May 29 2018, 6:09 PM
This revision was automatically updated to reflect the committed changes.