Bug 170351

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:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

108645

Attachments:

Description	Flags
The patch	fpizlo: review-
The patch v2	fpizlo: commit-queue-
The patch v3	mark.lam: commit-queue-
The patch v4	none

iscaro

Reported 2017-03-31 11:36:25 PDT

Currently the offline asm generator for 32-bit ARM code translates the 'call' meta-instruction (which may be found in LowLevelInterperter.asm and friends) to the ARM's BLX instrunction. The BLX instruction may be used for labels (immediates) and registers and one side effect of BLX is that it may switch the processor's instruction set. A 'BLX register' instruction will change/remain the processor state to ARM if the register_bit[0] is set to 0 or change/remain to Thumb if register_bit[0] is set to 1. However, a 'BLX label' instruction will always switch the processor state. It switches ARM to thumb and vice-versa. This behaviour is unwanted, since the C++ code and the offlineasm code are both compiled using the same instruction set, thus a instruction set change will likely produce a crash. In order to fix the problem the BL instruction can be used for labels. It will branch just like BLX, but it won't change the instruction set. BLX reference: http://infocenter.arm.com/help/topic/com.arm.doc.dui0489i/CIHBJCDC.html?resultof=%22%62%6c%78%22%20

Attachments
The patch (3.93 KB, patch) 2017-03-31 12:00 PDT, iscaro	fpizlo: review-	Details Formatted Diff Diff
The patch v2 (6.17 KB, patch) 2017-04-01 06:58 PDT, iscaro	fpizlo: commit-queue-	Details Formatted Diff Diff
The patch v3 (4.65 KB, patch) 2017-04-03 14:11 PDT, iscaro	mark.lam: commit-queue-	Details Formatted Diff Diff
The patch v4 (4.71 KB, patch) 2017-04-05 13:30 PDT, iscaro	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

iscaro

Comment 1 2017-03-31 12:00:44 PDT

Created attachment 305990 [details] The patch

Mark Lam

Comment 2 2017-03-31 13:06:26 PDT

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/

Filip Pizlo

Comment 3 2017-03-31 13:08:09 PDT

Comment on attachment 305990 [details] The patch Please conditionalize this change for not-iOS. On iOS, it's correct to use blx.

Filip Pizlo

Comment 4 2017-03-31 13:08:31 PDT

(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/

iscaro

Comment 5 2017-03-31 13:22:04 PDT

(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.

Csaba Osztrogonác

Comment 6 2017-04-01 04:12:32 PDT

I think we ran into this gold linker bug previously, see bug159880 for details.

iscaro

Comment 7 2017-04-01 06:58:12 PDT

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.

iscaro

Comment 8 2017-04-01 07:10:03 PDT

(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)?

Konstantin Tokarev

Comment 9 2017-04-01 10:42:27 PDT

gold supports only ELF, and Darwin uses Mach-O

Filip Pizlo

Comment 10 2017-04-01 11:30:11 PDT

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?

iscaro

Comment 11 2017-04-02 12:13:24 PDT

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.

Konstantin Tokarev

Comment 12 2017-04-02 12:19:28 PDT

(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)?

iscaro

Comment 13 2017-04-02 13:22:34 PDT

(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.

iscaro

Comment 14 2017-04-03 14:11:55 PDT

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.

Mark Lam

Comment 15 2017-04-05 10:28:23 PDT

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.

iscaro

Comment 16 2017-04-05 13:30:40 PDT

Created attachment 306306 [details] The patch v4 Here we go, a new version with the changes requested by Mark. Thanks for the review.

Mark Lam

Comment 17 2017-04-05 13:32:24 PDT

Comment on attachment 306306 [details] The patch v4 r=me

WebKit Commit Bot

Comment 18 2017-04-05 14:00:21 PDT

Comment on attachment 306306 [details] The patch v4 Clearing flags on attachment: 306306 Committed r214969: <http://trac.webkit.org/changeset/214969>

WebKit Commit Bot

Comment 19 2017-04-05 14:00:22 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.