Bug 170351 – JavaScriptCore: Do not use BLX for immediates (ARM-32)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[iscaro]

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

[iscaro]

Created attachment 305990 [details]
The patch

[Mark Lam]

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 on attachment 305990 [details]
The patch

Please conditionalize this change for not-iOS.  On iOS, it's correct to use blx.

[Filip Pizlo]

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

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

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

[iscaro]

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]

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

gold supports only ELF, and Darwin uses Mach-O

[Filip Pizlo]

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]

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]

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

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

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 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]

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 on attachment 306306 [details]
The patch v4

r=me

[WebKit Commit Bot]

Comment on attachment 306306 [details]
The patch v4

Clearing flags on attachment: 306306

Committed r214969: <http://trac.webkit.org/changeset/214969>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.