Summary: | JavaScriptCore: Do not use BLX for immediates (ARM-32) | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | iscaro <iscaro> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | annulen, barbieri, buildbot, commit-queue, fpizlo, iscaro, jfbastien, keith_miller, mark.lam, msaboff, ossy, saam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 108645 | ||||||||||||
Attachments: |
|
Description
iscaro
2017-03-31 11:36:25 PDT
Created attachment 305990 [details]
The patch
Comment on attachment 305990 [details] The patch View in context: https://bugs.webkit.org/attachment.cgi?id=305990&action=review I was wondering why this never posed a problem before for us before. I did a local build, and see that the compiled LowLevelInterpreter.o does emit blx instructions. However, in the llinked version of the code, the linker changed the blx into bl instructions. If the toolchain does not fix this for us, then this bug would have manifested. Hence, r=me > Source/JavaScriptCore/ChangeLog:8 > + 'call' meta-instruction (which may be found in LowLevelInterperter.asm typo: /LowLevelInterperter.asm/LowLevelInterpreter.asm/ > Source/JavaScriptCore/ChangeLog:16 > + This behaviour is unwanted, since the C++ code and the offlineasm code /offlineasm code/offlineasm generated code/ Comment on attachment 305990 [details]
The patch
Please conditionalize this change for not-iOS. On iOS, it's correct to use blx.
(In reply to Mark Lam from comment #2) > Comment on attachment 305990 [details] > The patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=305990&action=review > > I was wondering why this never posed a problem before for us before. I did > a local build, and see that the compiled LowLevelInterpreter.o does emit blx > instructions. However, in the llinked version of the code, the linker > changed the blx into bl instructions. If the toolchain does not fix this > for us, then this bug would have manifested. It's right to use blx on iOS. > > Hence, r=me > > > Source/JavaScriptCore/ChangeLog:8 > > + 'call' meta-instruction (which may be found in LowLevelInterperter.asm > > typo: /LowLevelInterperter.asm/LowLevelInterpreter.asm/ > > > Source/JavaScriptCore/ChangeLog:16 > > + This behaviour is unwanted, since the C++ code and the offlineasm code > > /offlineasm code/offlineasm generated code/ (In reply to Filip Pizlo from comment #3) > Comment on attachment 305990 [details] > The patch > > Please conditionalize this change for not-iOS. On iOS, it's correct to use > blx. Hello, I'll adapt the code. Just wondering how it works on iOS? Is the linker changing the instruction? I'll send a new patch soon. Btw, thanks for the review Filip and Mark. I think we ran into this gold linker bug previously, see bug159880 for details. Created attachment 306047 [details]
The patch v2
I made a v2 patch which still uses BLX for iOS devices as requested by Filip.
Please send me some thoughts. I also set the commit-queue flag to '?' as instruced by Mark.
(In reply to Csaba Osztrogonác from comment #6) > I think we ran into this gold linker bug previously, see bug159880 for > details. This bug is very interesting. I can confirm that I was using the gold linker in my local build. The backtrace is similar to the one reported by my gdb. I must say that I was using an older version of WebKit and your patch was not there, therefore gold was being used. Do you think that this might be the reason for iOS not be affected (I mean, gold is not used when compiling iOS stuff)? gold supports only ELF, and Darwin uses Mach-O Comment on attachment 306047 [details]
The patch v2
I think this is very roundabout. Introducing a setting that you have to hack into the .asm file with a dummy if statement is super dirty. We should have the offlineasm program know what platform it's on. That's better since we'll never have a fat binary that is both iOS and non-iOS-arm. As it stands the introduction of a new settings flag doubles the runtime of offlineasm for everyone.
Would it be enough for you to detect in arm.rb if you're running on Darwin?
Hello, > I think this is very roundabout. Introducing a setting that you have to hack > into the .asm file with a dummy if statement is super dirty. We should have > the offlineasm program know what platform it's on. That's better since > we'll never have a fat binary that is both iOS and non-iOS-arm. As it stands > the introduction of a new settings flag doubles the runtime of offlineasm > for everyone. I'm sorry for this 'dirty' code Filip. I saw that there were already some pieces of code doing that (just like the armv7 if above PLATFORM_IOS) and then I though it was the right way to do. To be honest, I tried to add a command line option to offlineasm which flagged that the target was iOS. However, I could not find a easy way to do that. > Would it be enough for you to detect in arm.rb if you're running on Darwin? Yes, it would be enough. What do you think about adding the following constant at arm.rb? IS_IOS = ((RUBY_PLATFORM =~ /darwin/i) != nil) Sounds good? Thanks. (In reply to iscaro from comment #11) Could you check if blx to bl change is still needed when you don't use gold (i.e. if you cherry-pick r203446)? (In reply to Konstantin Tokarev from comment #12) > (In reply to iscaro from comment #11) > > Could you check if blx to bl change is still needed when you don't use gold > (i.e. if you cherry-pick r203446)? I confirm that it does not crash when gold is not used. Created attachment 306108 [details]
The patch v3
So here's another version. This new version will check if we're compiling for iOS in arm.rb.
Comment on attachment 306108 [details] The patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=306108&action=review > Source/JavaScriptCore/ChangeLog:24 > + this change the BL instruction will only be used on non iOS targets. Replace /non iOS/non-darwin/ since that's what the patch actually does. > Source/JavaScriptCore/offlineasm/arm.rb:97 > +IS_IOS = ((RUBY_PLATFORM =~ /darwin/i) != nil) Let's rename IS_IOS to OS_DARWIN. The name should reflect what it's really checking. Created attachment 306306 [details]
The patch v4
Here we go, a new version with the changes requested by Mark.
Thanks for the review.
Comment on attachment 306306 [details]
The patch v4
r=me
Comment on attachment 306306 [details] The patch v4 Clearing flags on attachment: 306306 Committed r214969: <http://trac.webkit.org/changeset/214969> All reviewed patches have been landed. Closing bug. |