[PATCH] D114172: [ARM] implement support for ALU/LDR PC-relative group relocations

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/ARM.cpp:431
+    rem = val;
+    val &= (uint64_t)0xffffffU >> lz;
+  } while (group--);
----------------
ardb wrote:
> MaskRay wrote:
> > ardb wrote:
> > > MaskRay wrote:
> > > > MaskRay wrote:
> > > > > MaskRay wrote:
> > > > > > 
> > > > > val is uint32_t. Remove `(uint64_t)`?
> > > > Is `(uint64_t)0xffffff >> 32` possible? If lz can be 32, casting to `(uint64_t)` LGTM but it needs a comment. I am not familiar with the encoding and do not know whether lz can be 32.
> > > Yes, lz will be 32 if val == 0. This may happen when using multiple groups if the value can be fully encoded by the previous group(s).
> > > 
> > > Note that in that case, doing &= should not have any effect, but a 32-bit shift of a 32-bit type is better avoided entirely, I'd assume?
> > Unaddressed: needs a uint64_t cast to fix UB. `val &= (uint64_t)0xffffff >> lz;  // lz may be 32` 
> > 
> This is no longer true: lz == 32 only occurs if rem == 0x0, in which case we break out of the loop before.
Thx. Would be nice to have the lz==32 test if not exist yet.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114172/new/

https://reviews.llvm.org/D114172