Bug 170351

Summary: JavaScriptCore: Do not use BLX for immediates (ARM-32)
Product: WebKit Reporter: iscaro <iscaro>
Component: JavaScriptCoreAssignee: 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 Flags
The patch
fpizlo: review-
The patch v2
fpizlo: commit-queue-
The patch v3
mark.lam: commit-queue-
The patch v4 none

Description iscaro 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
Comment 1 iscaro 2017-03-31 12:00:44 PDT
Created attachment 305990 [details]
The patch
Comment 2 Mark Lam 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/
Comment 3 Filip Pizlo 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.
Comment 4 Filip Pizlo 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/
Comment 5 iscaro 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.
Comment 6 Csaba Osztrogonác 2017-04-01 04:12:32 PDT
I think we ran into this gold linker bug previously, see bug159880 for details.
Comment 7 iscaro 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.
Comment 8 iscaro 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)?
Comment 9 Konstantin Tokarev 2017-04-01 10:42:27 PDT
gold supports only ELF, and Darwin uses Mach-O
Comment 10 Filip Pizlo 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?
Comment 11 iscaro 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.
Comment 12 Konstantin Tokarev 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)?
Comment 13 iscaro 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.
Comment 14 iscaro 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.
Comment 15 Mark Lam 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.
Comment 16 iscaro 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.
Comment 17 Mark Lam 2017-04-05 13:32:24 PDT
Comment on attachment 306306 [details]
The patch v4

r=me
Comment 18 WebKit Commit Bot 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>
Comment 19 WebKit Commit Bot 2017-04-05 14:00:22 PDT
All reviewed patches have been landed.  Closing bug.