Switch magic numbers to library functions in fixup

[dhoekwater]

Summary

Using masks and bounds that are magic numbers
means that defects in bounds checking and offset
masking are subtle and hard to detect.
https://reviews.llvm.org/D152841 is an example
of this type of defect. Switching to clearly
readable library functions makes defects less
obfuscated.

[efriedma]

I like using isInt() and isUInt() more.

isShiftedUInt() also checks the low bits, so I think this changes the diagnostic output in some cases?

I don't like Value != alignTo<4>(Value); alignTo<4>(Value) doesn't represent any actual relevant value.

Not sure about replacing 0x3ffffff with maskTrailingOnes<uint64_t>(26); it's really verbose, and the name isn't great. Maybe if we can come up with some shorter alternative? Or maybe just write ((1ULL << 26) - 1)?

[peter.smith]

I have similar opinions to efriedma. Definitely like using isUint and isInt more. I'd prefer to use isAligned rather than alignTo.

I'm so used to seeing trailing masks, I do find maskTrailingOnes less intuitive, but will go with consensus on that front. Perhaps make a simpler/shorter name with a using?

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

175

While I think they will give the same result as this is a no-op for an already 4-byte aligned quantity it is a more extensive calculation as alignTo aligns to the next 4-byte boundary.

Would isAligned(Value, 4) in Support/Alignment.h be better?

[dhoekwater]

Fixed the isShiftedUInt() issue that changed behavior.

Replaced alignTo<4> stuff with isAligned(Align{4}, x). That's a neat utility function I didn't know existed.

I also don't love how verbose maskTrailingOnes<uint64_t>(n) is. I do personally feel like anything that
directly indicates the number of bits we're taking is better than a magic number mask, but I also haven't
developed the trained eye that tells me "0x1FFFFF is 21 bits" at a glance. Is there a more terse helper
function to express "get the low n bits of this number"? Ideally, we'd have something that clearly communicates
the meaning of something like n & 0x3, (n & 0x1ffffc) >> 2, or (n & 0x1fffff000ULL) >> 12. If not,
I'll just revert the masking changes.

[dhoekwater]

Rebase onto main and undo masking changes. This *should* be the final push.

[MaskRay]

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

175

For this case, the original Value & 0x3 seems clearer to me than the new !isAligned(Align{4}, Value)

[dhoekwater]

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

175

I think using a library function here is more clear, but maybe that's because the average reader of this code is more comfortable with visualizing bitmasks in their head than I am.

This kind of comes back to the "is it clearer to have the reader visualize hex values in their head or have it spelled out?" conversation regarding masks. In this case, the intent is to convey that we are checking if Value is 4-byte aligned.

The original version is shorter, but it has more logical steps to arrive at the semantic meaning ("we're checking if Value is aligned to a 4-byte boundary") than the new version.

[arsenm]

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

202

I'm mostly surprised by the parameter ordering of isAligned, I'd expect the alignment value to be the second argument

[MaskRay]

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

202

I am still of the opinion that isAligned(Align{4}, Value)) is a very awkward way to test alignment. We don't do this in other places, and I don't think it's a good suggestion.

isUInt is fine.

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

case FK_SecRel_4:
     return 4;
 
   case FK_Data_8:
     return 8;
   }
 }
 
 static unsigned AdrImmBits(unsigned Value) {
-  unsigned lo2 = Value & 0x3;
-  unsigned hi19 = (Value & 0x1ffffc) >> 2;
+  unsigned lo2 = Value & maskTrailingOnes<unsigned>(2);
+  unsigned hi19 = (Value >> 2) & maskTrailingOnes<unsigned>(19);
   return (hi19 << 5) | (lo2 << 29);
 }
 
 static uint64_t adjustFixupValue(const MCFixup &Fixup, const MCValue &Target,
                                  uint64_t Value, MCContext &Ctx,
                                  const Triple &TheTriple, bool IsResolved) {
   int64_t SignedValue = static_cast<int64_t>(Value);
   switch (Fixup.getTargetKind()) {
   default:
     llvm_unreachable("Unknown fixup kind!");
   case AArch64::fixup_aarch64_pcrel_adr_imm21:
     if (!isInt<21>(SignedValue))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
-    return AdrImmBits(Value & 0x1fffffULL);
+    return AdrImmBits(Value & maskTrailingOnes<uint64_t>(21));
   case AArch64::fixup_aarch64_pcrel_adrp_imm21:
     assert(!IsResolved);
     if (TheTriple.isOSBinFormatCOFF()) {
       if (!isInt<21>(SignedValue))
         Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
-      return AdrImmBits(Value & 0x1fffffULL);
+      return AdrImmBits(Value & maskTrailingOnes<uint64_t>(21));
     }
-    return AdrImmBits((Value & 0x1fffff000ULL) >> 12);
+    return AdrImmBits((Value >> 12) & maskTrailingOnes<uint64_t>(21));
   case AArch64::fixup_aarch64_ldr_pcrel_imm19:
   case AArch64::fixup_aarch64_pcrel_branch19:
     // Signed 19-bit immediate which gets multiplied by 4
     if (!isInt<21>(SignedValue))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
-    if (Value & 0x3)
+    if (Value != alignTo<4>(Value))
       Ctx.reportError(Fixup.getLoc(), "fixup not sufficiently aligned");
     // Low two bits are not encoded.
-    return (Value >> 2) & 0x7ffff;
+    return (Value >> 2) & maskTrailingOnes<uint64_t>(19);
   case AArch64::fixup_aarch64_add_imm12:
   case AArch64::fixup_aarch64_ldst_imm12_scale1:
     if (TheTriple.isOSBinFormatCOFF() && !IsResolved)
-      Value &= 0xfff;
+      Value &= maskTrailingOnes<uint64_t>(12);
     // Unsigned 12-bit immediate
-    if (Value >= 0x1000)
+    if (!isUInt<12>(Value))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
     return Value;
   case AArch64::fixup_aarch64_ldst_imm12_scale2:
     if (TheTriple.isOSBinFormatCOFF() && !IsResolved)
-      Value &= 0xfff;
+      Value &= maskTrailingOnes<uint64_t>(12);
     // Unsigned 12-bit immediate which gets multiplied by 2
-    if (Value >= 0x2000)
+    if (!isShiftedUInt<12, 1>(Value))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
-    if (Value & 0x1)
+    if (Value != alignTo<2>(Value))
       Ctx.reportError(Fixup.getLoc(), "fixup must be 2-byte aligned");
     return Value >> 1;
   case AArch64::fixup_aarch64_ldst_imm12_scale4:
     if (TheTriple.isOSBinFormatCOFF() && !IsResolved)
-      Value &= 0xfff;
+      Value &= maskTrailingOnes<uint64_t>(12);
     // Unsigned 12-bit immediate which gets multiplied by 4
-    if (Value >= 0x4000)
+    if (!isShiftedUInt<12, 2>(Value))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
-    if (Value & 0x3)
+    if (Value != alignTo<4>(Value))
       Ctx.reportError(Fixup.getLoc(), "fixup must be 4-byte aligned");
     return Value >> 2;
   case AArch64::fixup_aarch64_ldst_imm12_scale8:
     if (TheTriple.isOSBinFormatCOFF() && !IsResolved)
-      Value &= 0xfff;
+      Value &= maskTrailingOnes<uint64_t>(12);
     // Unsigned 12-bit immediate which gets multiplied by 8
-    if (Value >= 0x8000)
+    if (!isShiftedUInt<12, 3>(Value))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
-    if (Value & 0x7)
+    if (Value != alignTo<8>(Value))
       Ctx.reportError(Fixup.getLoc(), "fixup must be 8-byte aligned");
     return Value >> 3;
   case AArch64::fixup_aarch64_ldst_imm12_scale16:
     if (TheTriple.isOSBinFormatCOFF() && !IsResolved)
-      Value &= 0xfff;
+      Value &= maskTrailingOnes<uint64_t>(12);
     // Unsigned 12-bit immediate which gets multiplied by 16
-    if (Value >= 0x10000)
+    if (!isShiftedUInt<12, 4>(Value))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
-    if (Value & 0xf)
+    if (Value != alignTo<16>(Value))
       Ctx.reportError(Fixup.getLoc(), "fixup must be 16-byte aligned");
     return Value >> 4;
   case AArch64::fixup_aarch64_movw: {
     AArch64MCExpr::VariantKind RefKind =
         static_cast<AArch64MCExpr::VariantKind>(Target.getRefKind());
     if (AArch64MCExpr::getSymbolLoc(RefKind) != AArch64MCExpr::VK_ABS &&
         AArch64MCExpr::getSymbolLoc(RefKind) != AArch64MCExpr::VK_SABS) {
       if (!RefKind) {
case AArch64::fixup_aarch64_movw: {
     }
     else if (Value > 0xFFFF) {
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
     }
     return Value;
   }
   case AArch64::fixup_aarch64_pcrel_branch14:
     // Signed 16-bit immediate
-    if (SignedValue > 32767 || SignedValue < -32768)
+    if (!isInt<16>(SignedValue))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
     // Low two bits are not encoded (4-byte alignment assumed).
-    if (Value & 0x3)
+    if (Value != alignTo(Value, 4))
       Ctx.reportError(Fixup.getLoc(), "fixup not sufficiently aligned");
-    return (Value >> 2) & 0x3fff;
+    return (Value >> 2) & maskTrailingOnes<uint64_t>(14);
   case AArch64::fixup_aarch64_pcrel_branch26:
   case AArch64::fixup_aarch64_pcrel_call26:
     // Signed 28-bit immediate
-    if (SignedValue > 134217727 || SignedValue < -134217728)
+    if (!isInt<28>(SignedValue))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
     // Low two bits are not encoded (4-byte alignment assumed).
-    if (Value & 0x3)
+    if (Value != alignTo(Value, 4))
       Ctx.reportError(Fixup.getLoc(), "fixup not sufficiently aligned");
-    return (Value >> 2) & 0x3ffffff;
+    return (Value >> 2) & maskTrailingOnes<uint64_t>(26);
   case FK_Data_1:
   case FK_Data_2:
   case FK_Data_4:
   case FK_Data_8:
   case FK_SecRel_2:
   case FK_SecRel_4:
     return Value;
   }