[PATCH] D54154: [ELF][MIPS] Use MIPS R6 `sigrie` as a trap instruction

[James Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

In https://reviews.llvm.org/D54154#1289551, @ruiu wrote:

> Yeah, we could alternatively represent a trap instruction as an ArrayRef<uint8_t> which can vary in size, but that's somewhat minor point, and I believe always writing 4 bytes using memcpy is much faster than copying ArrayRef contents because the former can be highly optimized by a compiler that can inline memcpy function.


Yeah, I considered that as a possibility, but it doesn't seem worth it. Your `uint8_t[4]` proposal seems like the best option overall to me, and can always be revisited if it turns out to be a problem (I don't suppose we'll be seeing an IA-64 backend needing 16 bytes for its instruction!).



================
Comment at: ELF/Arch/Mips.cpp:61
+  // Set `sigrie 1` as a trap instruction.
+  if (Config->IsLE == sys::IsLittleEndianHost)
+    write32le(&TrapInstr, 0x04170001);
----------------
jrtc27 wrote:
> atanasyan wrote:
> > jrtc27 wrote:
> > > Maybe I'm being stupid, but won't this do the wrong thing on a big-endian host? If, say, we're building for mips64 on mips64, `Config->IsLE` and `sys::IsLittleEndianHost` will both be false, so we'll take the `write32le` path, which will write `0x04170001` (represented in-memory natively as big-endian) as a little-endian value, i.e. incorrectly byte-swapping? It seems to me like this should be just a check for `if (Config->IsLE)`?
> > Thanks a lot. The only remaining question - how could I write this nice piece of code? :)
> I went and verified this because, despite having many dealings with it, it's too easy to get confused about endianness-related code. So, yes, my belief was correct, and I didn't inadvertently mislead you :) Running on powerpc64-linux-gnu:
> 
> ```
> -- Testing: 1 tests, 1 threads --
> FAIL: lld :: ELF/mips-got16-relocatable.s (1 of 1)
> ******************** TEST 'lld :: ELF/mips-got16-relocatable.s' FAILED ********************
> Script:
> --
> : 'RUN: at line 5';   /home/jrtc27/src/llvm/Build/bin/llvm-mc -filetype=obj -triple=mips-unknown-linux -mcpu=mips32r6 -o /home/jrtc27/src/llvm/Build/tools/lld/test/ELF/Output/mips-got16-relocatable.s.tmp.o /home/jrtc27/src/llvm/llvm/tools/lld/test/ELF/mips-got16-relocatable.s
> : 'RUN: at line 6';   /home/jrtc27/src/llvm/Build/bin/ld.lld -r -o /home/jrtc27/src/llvm/Build/tools/lld/test/ELF/Output/mips-got16-relocatable.s.tmp /home/jrtc27/src/llvm/Build/tools/lld/test/ELF/Output/mips-got16-relocatable.s.tmp.o /home/jrtc27/src/llvm/Build/tools/lld/test/ELF/Output/mips-got16-relocatable.s.tmp.o
> : 'RUN: at line 7';   /home/jrtc27/src/llvm/Build/bin/llvm-objdump -d -r /home/jrtc27/src/llvm/Build/tools/lld/test/ELF/Output/mips-got16-relocatable.s.tmp | /home/jrtc27/src/llvm/Build/bin/FileCheck -check-prefix=OBJ /home/jrtc27/src/llvm/llvm/tools/lld/test/ELF/mips-got16-relocatable.s
> : 'RUN: at line 8';   /home/jrtc27/src/llvm/Build/bin/ld.lld -shared -o /home/jrtc27/src/llvm/Build/tools/lld/test/ELF/Output/mips-got16-relocatable.s.tmp.so /home/jrtc27/src/llvm/Build/tools/lld/test/ELF/Output/mips-got16-relocatable.s.tmp
> : 'RUN: at line 9';   /home/jrtc27/src/llvm/Build/bin/llvm-objdump -d /home/jrtc27/src/llvm/Build/tools/lld/test/ELF/Output/mips-got16-relocatable.s.tmp.so | /home/jrtc27/src/llvm/Build/bin/FileCheck -check-prefix=SO /home/jrtc27/src/llvm/llvm/tools/lld/test/ELF/mips-got16-relocatable.s
> --
> Exit Code: 1
> 
> Command Output (stderr):
> --
> /home/jrtc27/src/llvm/llvm/tools/lld/test/ELF/mips-got16-relocatable.s:17:13: error: OBJ-NEXT: expected string not found in input
> # OBJ-NEXT: 8: 04 17 00 01 sigrie 1
>             ^
> <stdin>:10:2: note: scanning from here
>  8: 01 00 17 04 <unknown>
>  ^
> 
> --
> 
> ********************
> Testing Time: 0.22s
> ********************
> Failing Tests (1):
>     lld :: ELF/mips-got16-relocatable.s
> 
>   Unexpected Failures: 1
> ```
... and finally if I change this line to just `if (Config->IsLE)` it does indeed pass as expected on powerpc64-linux-gnu, so LGTM from my (unimportant) point of view once that's fixed. Alternatively you could use your `Config->IsLE == sys::IsLittleEndianHost` check, but explicitly do the byte swapping when they don't match (i.e. your `write32le` becomes the identity, and `write32be` becomes a byte swap, but both inlined and constant propagated), like the PPC64 target.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D54154