Page MenuHomePhabricator

[RISCV] Fix builtin fixup sizes
AbandonedPublic

Authored by simoncook on May 11 2018, 4:28 AM.

Details

Summary

This adds the FK_Data_* fixup types to the size evaluation to return the correct size of each of these, rather than the default of 4. This allows applyFixup to correctly update the value of data without walking outside the bounds of the fixup (in the case of FK_Data_1 and FK_Data 2), or truncating the value being set (as in the case of FK_Data_8).

Diff Detail

Repository
rL LLVM

Event Timeline

simoncook created this revision.May 11 2018, 4:28 AM
asb added a comment.EditedMay 14 2018, 2:28 PM

Thanks Simon. I can see that truncation of FK_Data_8 is a correctness issue and addressing that in getSize seems most sensible.

I wonder if changing getSize is the correct place to be fixing FK_DATA_1 and FK_DATA_2 though. After all, all the target-specific fixups are fully dependent on fixup overflow not taking place. If (for instance) it's possible for an FK_DATA_1 fixup to give a value that's greater than 8 bits, maybe the bug is in adjustFixupValue?

It looks like SystemZMCAsmBackend::applyFixup demonstrates an even better approach: don't rely on a helper function at all and just use getFixupKindInfo to get the size (in bits) of the fixup. What do you think about adapting that sort of logic instead?

In D46746#1098532, @asb wrote:

Thanks Simon. I can see that truncation of FK_Data_8 is a correctness issue and addressing that in getSize seems most sensible.

I wonder if changing getSize is the correct place to be fixing FK_DATA_1 and FK_DATA_2 though. After all, all the target-specific fixups are fully dependent on fixup overflow not taking place. If (for instance) it's possible for an FK_DATA_1 fixup to give a value that's greater than 8 bits, maybe the bug is in adjustFixupValue?

It looks like SystemZMCAsmBackend::applyFixup demonstrates an even better approach: don't rely on a helper function at all and just use getFixupKindInfo to get the size (in bits) of the fixup. What do you think about adapting that sort of logic instead?

I looked at this in more detail, and of course we're already using getFixupKindInfo to find the size in bits of the fixup. I think D46965 is a reasonable alternative approach - could you please check I'm not missing anything obvious.

I agree, the route you have taken in D46965 seems the best route to go down. Unless we end up with some fixup where we want to manipulate bytes that wouldn't be covered by that calculation (it seems unlikely), then all this table is doing is duplicating what information we already have, so I think it's best to use your calculation.

asb resigned from this revision.May 23 2018, 4:42 AM

Excellent, I've gone ahead and committed D46965.

simoncook abandoned this revision.May 23 2018, 5:26 AM