Closed Quuxplusone closed 6 years ago
Bugzilla Link | PR23000 |
Status | RESOLVED FIXED |
Importance | P normal |
Reported by | Jörg Sonnenberger (joerg@NetBSD.org) |
Reported on | 2015-03-23 15:53:25 -0700 |
Last modified on | 2018-02-21 14:27:55 -0800 |
Version | trunk |
Hardware | PC Linux |
CC | compnerd@compnerd.org, dimitry@andric.com, john.brawn@arm.com, kristof.beyls@gmail.com, llvm-bugs@lists.llvm.org, nicolasweber@gmx.de, oliver.stannard@arm.com, rengolin@gmail.com, smithp352@googlemail.com, t.p.northover@gmail.com |
Fixed by commit(s) | |
Attachments | |
Blocks | PR20422 |
Blocked by | |
See also |
Adjusting the example slightly to:
AES_Te:
.word 1,2,3,4,5,6
.word 1,2,3,4,5,6
.word 1,2,3,4,5,6
.word 1,2,3,4,5,6
.word 1,2,3,4,5,6
.word 1,2,3,4,5,6
.word 1,2,3,4,5,6
.word 1,2,3,4,5,6
.word 1,2,3,4,5,6
.word 1,2,3,4,5,6
.word 1,2,3,4,5,6
AES_encrypt:
sub r10,r3,#(AES_encrypt-AES_Te) @ Te
sub r10,r3,#(6*11*4)
.word (AES_encrypt-AES_Te)
.word (6*11*4)
Then assembling with clang and disassembling with llvm-dis I see
AES_encrypt:
108: 08 a1 43 e2 sub r10, r3, #8, #2
10c: 42 af 43 e2 sub r10, r3, #264
$d.2:
110: 08 01 00 00 andeq r0, r0, r8, lsl #2
114: 08 01 00 00 andeq r0, r0, r8, lsl #2
It's only the sub with an immediate that's a label difference that's been
misassembled.
Yes, that makes sense. Ive not had enough time to fully isolate the issue. The adjustment is being evaluated correctly. It seems that the issue comes in really late (near the the of the fragment layout phase).
The core of the issue is that add and sub take a 12bit immediate with optional shifting, but we use a 16bit fix up when we can't evaluate the immediate. A new ARM-specific fix up is needed for this,
Blocking Chromium meta, since this seems to be the last of the Chromium bugs with IAS.
(It looks like the last needed for boringssl in chromium, but not the last for all of chromium I think)
Oh, sorry. I stand corrected. :-)
This seems fixed on trunk:
...
100: 05 00 00 00 andeq r0, r0, r5
104: 06 00 00 00 andeq r0, r0, r6
AES_encrypt:
108: 42 af 43 e2 sub r10, r3, #264
10c: 42 af 43 e2 sub r10, r3, #264
$d:
110: 08 01 00 00 andeq r0, r0, r8, lsl #2
114: 08 01 00 00 andeq r0, r0, r8, lsl #2
Saleem, did you fix this? I can't find the commit...
This could also have been "accidentally" fixed by another commit... Peter was
working around add/sub fixups and making some refactoring around it.
No, I hadn't had a chance to work on it. I would assume that it was Peter's work. However, I think that we should at least check in some tests for the future.
Good point. I can do that.
Tests in r276858.
This isn't fixed for boringssl in a chromium bug (which is probably the same
source that the openssl bit is reduced from). Maybe it's due to thumb:
thakis@thakis:~/src/chrome/src/out/gnand$ cat ../../third_party/boringssl/linux-
arm/crypto/aes/bsaes-armv7.S
.arch armv7-a
.fpu neon
.text
.syntax unified @ ARMv7-capable assembler is expected to handle this
.thumb
.type _bsaes_decrypt8,%function
.align 4
_bsaes_decrypt8:
adr r6,_bsaes_decrypt8
vldmia r4!, {q9} @ round 0 key
add r6,r6,#.LM0ISR-_bsaes_decrypt8
.type _bsaes_const,%object
.align 6
_bsaes_const:
.LM0ISR:@ InvShiftRows constants
.quad 0x0a0e0206070b0f03, 0x0004080c0d010509
thakis@thakis:~/src/chrome/src/out/gnand$ ../../third_party/llvm-
build/Release+Asserts/bin/clang -
B../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-
4.9/prebuilt/linux-x86_64/bin --target=arm-linux-androideabi -march=armv7-a -
mfloat-abi=softfp -mthumb -mtune=generic-armv7-a -mfpu=neon -g2 --
sysroot=../../third_party/android_tools/ndk/platforms/android-16/arch-arm -c
../../third_party/boringssl/linux-arm/crypto/aes/bsaes-armv7.S
../../third_party/boringssl/linux-arm/crypto/aes/bsaes-armv7.S:13:12: error:
invalid operand for instruction
add r6,r6,#.LM0ISR-_bsaes_decrypt8
^
(that was a somewhat reduced repro, not the original file, of course)
Hi Nico,
Thanks for the new snippet, I can now reproduce it on ARM and Thumb. The old
snippet still doesn't fail, so I'll abandon that investigation and look at the
new reduced case.
cheers,
--renato
This is still an issue, even though the diagnostic changed slightly. With the
same repro as above:
../../third_party/llvm-build/Release+Asserts/bin/clang -
B../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-
4.9/prebuilt/linux-x86_64/bin --target=arm-linux-androideabi -march=armv7-a -
mfloat-abi=softfp -mthumb -mtune=generic-armv7-a -mfpu=neon -g2 --
sysroot=../../third_party/android_tools/ndk/platforms/android-16/arch-arm -c
foo.S
-bash: ../../third_party/llvm-build/Release+Asserts/bin/clang: No such file or
directory
Nicos-MacBook-Pro:src thakis$ cd out/gn
Nicos-MacBook-Pro:gn thakis$ ../../third_party/llvm-
build/Release+Asserts/bin/clang -
B../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-
4.9/prebuilt/linux-x86_64/bin --target=arm-linux-androideabi -march=armv7-a -
mfloat-abi=softfp -mthumb -mtune=generic-armv7-a -mfpu=neon -g2 --
sysroot=../../third_party/android_tools/ndk/platforms/android-16/arch-arm -c
../../foo.S
../../foo.S:13:12: error: immediate operand must be in the range [0,4095]
add r6,r6,#.LM0ISR-_bsaes_decrypt8
^
The new message is due to a change by Oliver to clean up the asm messages. It shouldn't change the initial bug.
I think that I managed to fix this as part of pr28647 Support of thumb2's
modified immediate assembly syntax is incomplete (committed Mon Jun 5 2017)
With trunk llvm-mc or clang I can assemble the latest reproducer without error
and produce a file that disassembles to:
00000000 <_bsaes_decrypt8>:
0: f2af 0604 subw r6, pc, #4
4: ecf4 2b04 vldmia r4!, {d18-d19}
8: f106 0640 add.w r6, r6, #64 ; 0x40
c: bf00 nop
e: bf00 nop
10: bf00 nop
12: bf00 nop
14: bf00 nop
16: bf00 nop
18: bf00 nop
1a: bf00 nop
1c: bf00 nop
1e: bf00 nop
20: bf00 nop
22: bf00 nop
24: bf00 nop
26: bf00 nop
28: bf00 nop
2a: bf00 nop
2c: bf00 nop
2e: bf00 nop
30: bf00 nop
32: bf00 nop
34: bf00 nop
36: bf00 nop
38: bf00 nop
3a: bf00 nop
3c: bf00 nop
3e: bf00 nop
00000040 <_bsaes_const>:
40: 0f03 070b 0206 0a0e 0509 0d01 080c 0004
Can you take another look to see if there is still a problem? If not we should
be able to resolve this pr.
Marking this as resolved, since it was fixed months ago.