Skip to content

AArch64-specific workaround: int::to_i32() seems to be flawed#4716

Closed
akosthekiss wants to merge 10 commits intoservo:rustup_20150109from
akosthekiss:pr-rustup_20150109-util-au-from_px
Closed

AArch64-specific workaround: int::to_i32() seems to be flawed#4716
akosthekiss wants to merge 10 commits intoservo:rustup_20150109from
akosthekiss:pr-rustup_20150109-util-au-from_px

Conversation

@akosthekiss
Copy link
Copy Markdown
Contributor

@akosthekiss akosthekiss commented Jan 25, 2015

In the call chain Au::from_px(int) -> Au::from::<int>(int) -> int::to_i32(),
the last item seems to be flawed and returns None for valid arguments as well
(e.g., for 16*60=960). The error seems to stem from Rust, and for now, it can be
fixed in Servo only with a workaround. Casting the argument of from_px to from
int to i32 changes the call chain (-> Au::from::<i32>(i32) ->
i32::to_i32()) so that it works correctly.

jdm and others added 10 commits January 24, 2015 12:56
@jdm @Ms2ger
Update rustc to 00b112c45a604fa6f4b59af2a40c9deeadfdb7c6/rustc-1.0.0-…
2df302f 
…dev.
@Ms2ger
fixup! Update rustc to 00b112c45a604fa6f4b59af2a40c9deeadfdb7c6/rustc…
4456b7a 
…-1.0.0-dev.
@Ms2ger
Fix crash.
f400d79
@Ms2ger
reftests
ffabed4
@jdm
Fix a bunch of unit tests. test-unit still doesn't build.
c095810
@akosthekiss
Fix script: use c_char instead of i8
07c3608
@jdm
Merge pull request servo#4714 from akiss77/pr-rustup_20150109-script-…
343f3d2 
…u8-c_char

Fix script: use `c_char` instead of `i8`
@Ms2ger
Fix a closure
599d2ea
@Ms2ger
Remove deque unit tests.
5fe1b45
@akosthekiss
AArch64-specific workaround: int::to_i32() seems to be flawed
4ba0466 
In the call chain `Au::from_px(int)` -> `Au::from::<int>(int)` -> `int::to_i32()`,
the last item seems to be flawed and returns `None` for valid arguments as well
(e.g., for 16*60=960). The error seems to stem from Rust, and for now, it can be
fixed in Servo only with a workaround. Casting the argument of `from_px` to from
`int` to `i32` changes the call chain (-> `Au::from::<i32>(i32)` ->
`i32::to_i32()`) so that it works correctly.
@highfive
Copy link
Copy Markdown

highfive commented Jan 25, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @kmcallister (or someone else) soon.

@akosthekiss
Copy link
Copy Markdown
Contributor Author

akosthekiss commented Jan 25, 2015

FYI: an excerpt from the AArch64 disassembly of int::to_it32():

   0x000000555ad0df10 <+96>:    bl  0x555ad0dfbc <_ZN3num7i32.Int9min_value20hf62321fc03b2fe06gpbE>
   0x000000555ad0df14 <+100>:   str w0, [sp,#20]
   0x000000555ad0df18 <+104>:   bl  0x555ad0dfc4 <_ZN3num7i32.Int9max_value20hd133b1995f993960wpbE>
   0x000000555ad0df1c <+108>:   str w0, [sp,#16]
   0x000000555ad0df20 <+112>:   ldrsw   x8, [sp,#20]
   0x000000555ad0df24 <+116>:   mov w0, w8
   0x000000555ad0df28 <+120>:   mov w8, w0
   0x000000555ad0df2c <+124>:   ldr x9, [sp,#24]
=> 0x000000555ad0df30 <+128>:   cmp x8, x9

The ldrsw at 0x555ad0df20 correctly sign extends i32::min_value to the 64 bit x8 register (resulting 0xffffffff80000000), but then the two (quite superfluous) moves at 0x555ad0df24 and 0x555ad0df28 erroneously clear the top 32 bits of x8. Thus, by the time we reach the cmp at 0x555ad0df30, which would check whether the value in x9 is greater than i32::min_value, it actually checks whether x9 is greater than 0x80000000 (i.e., 2147483648, a positive value!).

@akosthekiss akosthekiss mentioned this pull request Jan 25, 2015
Closed
@jdm jdm force-pushed the rustup_20150109 branch from 657f608 to 24d614a Compare January 26, 2015 15:58
@akosthekiss
Copy link
Copy Markdown
Contributor Author

akosthekiss commented Jan 26, 2015

Withdrawing this request: it seems possible to work around the rustc bug with no modification required on Servo side.

@akosthekiss akosthekiss deleted the pr-rustup_20150109-util-au-from_px branch January 27, 2015 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@akosthekiss @highfive @jdm @Ms2ger