[RISCV] Fix builtin fixup sizes

[simoncook]

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).

[asb]

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?

[asb]

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.

[simoncook]

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]

Excellent, I've gone ahead and committed D46965.

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

static uint64_t adjustFixupValue(const MCFixup &Fixup, uint64_t Value,
 
   }
 }
 
 static unsigned getSize(unsigned Kind) {
   switch (Kind) {
   default:
     return 4;
+  case FK_Data_1:
+    return 1;
+  case FK_Data_2:
+    return 2;
+  case FK_Data_4:
+    return 4;
+  case FK_Data_8:
+    return 8;
   case RISCV::fixup_riscv_rvc_jump:
   case RISCV::fixup_riscv_rvc_branch:
     return 2;
   }
 }
 
 void RISCVAsmBackend::applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                                  const MCValue &Target,