[PATCH] D46965: [RISCV] Fix builtin fixup sizes (alternate approach)

[Alex Bradbury via Phabricator via llvm-commits]

asb created this revision.
asb added reviewers: shiva0217, apazos, simoncook.
Herald added subscribers: mgrang, edward-jones, zzheng, kito-cheng, niosHD, sabuasal, jordy.potman.lists, johnrusso, rbar.

This is a different approach to fixing the problem described in https://reviews.llvm.org/D46746. RISCVAsmBackend currently depends on the getSize helper function returning the number of bytes a fixup may change (note: some other backends have a similar helper named getFixupNumKindBytes). As noted in that review, this doesn't return the correct size for `FK_Data_1`, `FK_Data_2`, or `FK_Data_8` meaning that too few bytes will be written in the case of `FK_Data_8`, and there's the potential of writing outside the Data array for the smaller fixups.

https://reviews.llvm.org/D46746 extends getSize to recognise some of the builtin fixup types. Rather than having a function that needs to be kept up to date as new builtin or target-specific fixups are added, We can calculate an appropriate bound on the number of bytes that might be touched using Info.TargetSize and Info.TargetOffset.

This seems trivial, but posting for review in case there's something obvious I'm missing...


https://reviews.llvm.org/D46965

Files:
  lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp


Index: lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
===================================================================
--- lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
+++ lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
@@ -278,16 +278,6 @@
   }
 }
 
-static unsigned getSize(unsigned Kind) {
-  switch (Kind) {
-  default:
-    return 4;
-  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,
                                  MutableArrayRef<char> Data, uint64_t Value,
@@ -303,16 +293,13 @@
   Value <<= Info.TargetOffset;
 
   unsigned Offset = Fixup.getOffset();
-  unsigned FullSize = getSize(Fixup.getKind());
+  unsigned NumBytes = alignTo(Info.TargetSize + Info.TargetOffset, 8) / 8;
 
-#ifndef NDEBUG
-  unsigned NumBytes = (Info.TargetSize + 7) / 8;
   assert(Offset + NumBytes <= Data.size() && "Invalid fixup offset!");
-#endif
 
   // For each byte of the fragment that the fixup touches, mask in the
   // bits from the fixup value.
-  for (unsigned i = 0; i != FullSize; ++i) {
+  for (unsigned i = 0; i != NumBytes; ++i) {
     Data[Offset + i] |= uint8_t((Value >> (i * 8)) & 0xff);
   }
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D46965.147128.patch
Type: text/x-patch
Size: 1305 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180516/aa964696/attachment.bin>