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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In D65857#1620933 <https://reviews.llvm.org/D65857#1620933>, @peter.smith wrote:

> In D65857#1619366 <https://reviews.llvm.org/D65857#1619366>, @pcc wrote:
>
> > MHO: The assembler is a low enough level component that the user can be presumed to know what they're doing, regardless of linker limitations. So I would prefer not to do this. If we do anything about this, we should document the limitations of the GNU linkers somewhere.
>
>
> It is a tricky balance, we don't want to rule out a reasonable use case, but ideally we want to detect problems as soon as possible. I think what I have here may be overly strict as if you happen to know that the result of a signed operation is positive then this can work, or if you happen to know you are linking with LLD.
>
> One possible compromise is some kind of strict mode, something like --strict-movw-relocs that people could enable to restrict the relocations to a GNU compatible subset for those that need it. The command line option could also act as a kind of documentation that is a bit more visible. Any thoughts?


Sure, that would be fine with me.



================
Comment at: lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:965
+  bool isMovKSymbolG3() const {
+    return isMovWSymbol({AArch64MCExpr::VK_ABS_G3});
+  }
----------------
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


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

https://reviews.llvm.org/D65857