[PATCH] D65857: [MC][AArch64] Restrict use of signed relocation operators on MOV[NZK]

[Peter Smith via Phabricator via llvm-commits]

peter.smith marked an inline comment as done.
peter.smith added a comment.

Thanks for getting back to me, I'll look into making a strict mode. I'll also mention the use case you've identified with movk to my colleague from our GCC team, it is a pity that we don't have a R_AARCH64_PREL_G3_NC for the movk as although not ideal, as it is skipping an overflow check, it would permit GNU ld to be extended whilst retaining the property that you don't need to disassemble the binary to know how to evaluate a relocation.



================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:965
+  bool isMovKSymbolG3() const {
+    return isMovWSymbol({AArch64MCExpr::VK_ABS_G3});
+  }
----------------
pcc wrote:
> peter.smith wrote:
> > pcc wrote:
> > > We produce MOVK with PREL_G3 when the tagged-globals target feature is enabled, so we should allow this combination at least.
> > I hadn't realised that this was actively being used. Are you doing something like:
> > ```
> > movz x0, :prel_g0_nc: foo
> > movk x0, :prel_g1_nc: foo
> > movk x0, :prel_g2_nc: foo
> > movk x0, :prel_g3: foo
> > ```
> > 
> > This will work in BFD and gold if the result is positive as bit 30 is already 1 in a MOVK so orring with 1 won't change it, however if the result is negative then bfd/gold will clear bit 30, which will result in an OPC of 0 1 which is MOV <register> , this will likely result in a silent failure. Similarly as VK_PREL_G* is not present in fixMOVZ it won't get altered to a MOVN so the non-checking g0 won't alter it.
> > 
> > If you are happy with those restrictions, for example the offset from code to data in LD.bfd with the default linker script is always going to be positive, or are willing to only support the feature on LLD, then we should support it. I'm a bit nervous about fixMOVZ though.
> > 
> What I am doing is (see D65364 for more information):
> ```
> adrp x0, :pg_hi21_nc:global
> movk x0, #:prel_g3:global+4294967296
> add x0, x0, :lo12:global
> ``` 
> The second instruction is taking the pointer tag (i.e. a TBI pointer tag with bits 56-63 potentially set) from the address of `global` and combining it into `x0`. Note that the tagged-globals feature is only activated when passing `-fsanitize=hwaddress`.
> 
> It's possible for this offset to be "negative" in the sense that the pointer tag may be >= 0x80. From reading the ld.bfd source code it does appear that the linker will mangle the instruction in this case. On Android we currently require the use of lld to be able to use HWASAN since we use old versions of the GNU linkers that don't support the PREL relocations and are unlikely to upgrade them, so I think we are safe to use all 8 bits of the tag space. We may need to restrict the tag entropy to 7 bits on other operating systems in order to work around the GNU linker issue.
> 
> > I'm a bit nervous about fixMOVZ though.
> 
> It looks like fixMOVZ is only called for actual MOVZ instructions, unless I'm missing something.
> http://llvm-cs.pcc.me.uk/gen/lib/Target/AArch64/AArch64GenMCCodeEmitter.inc#9526
For the movz case I was worried about:

```
movz x0, :prel_g0_nc: foo
movk x0, :prel_g1_nc: foo
movk x0, :prel_g2_nc: foo
movk x0, :prel_g3: foo
```
which wouldn't work on BFD if the offset were always positive and fixMOVZ turned movz into movn, but as you aren't using movz then it doesn't matter. 


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

https://reviews.llvm.org/D65857