crazy_linker: isnanf workaround, reloaded.

crazy_linker: isnanf workaround, reloaded.

The __isnanf only exists on android 21+, so provide a fallback for the
fallback for versions 16-20.

BUG=735003
Review-Url: https://codereview.chromium.org/2945293002
Cr-Commit-Position: refs/heads/master@{#481737}
Committed: https://chromium.googlesource.com/chromium/src/+/938998bd58a2fabb04556833a298f3c7facd5135

[Nico, 2017-06-21 15:36:11]

The CQ bit was checked by thakis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 15:36:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-06-21 15:50:01]

thakis@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[Nico, 2017-06-21 15:50:02]

It's not clear yet if we'll need this, but if we do I'd like to have it ready.

https://codereview.chromium.org/2945293002/diff/1/third_party/android_crazy_l...
File third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp
(right):

https://codereview.chromium.org/2945293002/diff/1/third_party/android_crazy_l...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:68:
return (bits & 0x7f800000) == 0x7f800000;
I think this is right, but please take a careful look. I checked that both
32-bit arm and intel assembly produced by clang do not just call isnanf again,
so either clang wasn't smart enough to figure out what this does, or it did but
decided against turning it into a lib call again, or I implemented isnanf
incorrectly.

Here's the assembly:

00000000 <_ZN5crazy12_GLOBAL__N_112local_isnanfEf>:
   0:	f000 41ff 	and.w	r1, r0, #2139095040	; 0x7f800000
   4:	2000      	movs	r0, #0
   6:	f1b1 4fff 	cmp.w	r1, #2139095040	; 0x7f800000
   a:	bf08      	it	eq
   c:	2001      	moveq	r0, #1
   e:	4770      	bx	lr


00000000 <_ZN5crazy12_GLOBAL__N_112local_isnanfEf>:
   0:	55                   	push   %ebp
   1:	89 e5                	mov    %esp,%ebp
   3:	83 e4 fc             	and    $0xfffffffc,%esp
   6:	b9 00 00 80 7f       	mov    $0x7f800000,%ecx
   b:	8b 55 08             	mov    0x8(%ebp),%edx
   e:	21 ca                	and    %ecx,%edx
  10:	31 c0                	xor    %eax,%eax
  12:	39 ca                	cmp    %ecx,%edx
  14:	0f 94 c0             	sete   %al
  17:	89 ec                	mov    %ebp,%esp
  19:	5d                   	pop    %ebp
  1a:	c3                   	ret   

(that stack realignment on x86 is so painful to see :-/)

[commit-bot: I haz the power, 2017-06-21 15:51:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 15:51:03]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Nico, 2017-06-21 16:53:32]

The CQ bit was checked by thakis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 16:53:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 17:42:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 17:42:28]

Dry run: This issue passed the CQ dry run.

[DmitrySkiba, 2017-06-21 18:51:09]

On 2017/06/21 15:50:02, Nico wrote:
> It's not clear yet if we'll need this, but if we do I'd like to have it ready.
> 
>
https://codereview.chromium.org/2945293002/diff/1/third_party/android_crazy_l...
> File third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp
> (right):
> 
>
https://codereview.chromium.org/2945293002/diff/1/third_party/android_crazy_l...
> third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:68:
> return (bits & 0x7f800000) == 0x7f800000;
> I think this is right, but please take a careful look. I checked that both
> 32-bit arm and intel assembly produced by clang do not just call isnanf again,
> so either clang wasn't smart enough to figure out what this does, or it did
but
> decided against turning it into a lib call again, or I implemented isnanf
> incorrectly.
> 
> Here's the assembly:
> 
> 00000000 <_ZN5crazy12_GLOBAL__N_112local_isnanfEf>:
>    0:	f000 41ff 	and.w	r1, r0, #2139095040	; 0x7f800000
>    4:	2000      	movs	r0, #0
>    6:	f1b1 4fff 	cmp.w	r1, #2139095040	; 0x7f800000
>    a:	bf08      	it	eq
>    c:	2001      	moveq	r0, #1
>    e:	4770      	bx	lr
> 
> 
> 00000000 <_ZN5crazy12_GLOBAL__N_112local_isnanfEf>:
>    0:	55                   	push   %ebp
>    1:	89 e5                	mov    %esp,%ebp
>    3:	83 e4 fc             	and    $0xfffffffc,%esp
>    6:	b9 00 00 80 7f       	mov    $0x7f800000,%ecx
>    b:	8b 55 08             	mov    0x8(%ebp),%edx
>    e:	21 ca                	and    %ecx,%edx
>   10:	31 c0                	xor    %eax,%eax
>   12:	39 ca                	cmp    %ecx,%edx
>   14:	0f 94 c0             	sete   %al
>   17:	89 ec                	mov    %ebp,%esp
>   19:	5d                   	pop    %ebp
>   1a:	c3                   	ret   
> 
> (that stack realignment on x86 is so painful to see :-/)

Hmm, can we make local_isnanf() take uint32_t instead of float? You're casting
it anyway, and from the assembly it passes the argument in r0 anyway. That
should reduce chances of clang recognizing isnanf implementation I think.

[Nico, 2017-06-21 18:54:11]

It seems to work as is, and doesn't that have calling convention
implications? I didn't look it up, but doesn't x86_64 pass floats in
different registers?

On Jun 21, 2017 2:51 PM, <dskiba@chromium.org> wrote:

On 2017/06/21 15:50:02, Nico wrote:
> It's not clear yet if we'll need this, but if we do I'd like to have it
ready.
>
>
https://codereview.chromium.org/2945293002/diff/1/third_
party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp
> File third_party/android_crazy_linker/src/src/crazy_linker_
shared_library.cpp
> (right):
>
>
https://codereview.chromium.org/2945293002/diff/1/third_
party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp#newcode68
> third_party/android_crazy_linker/src/src/crazy_linker_
shared_library.cpp:68:
> return (bits & 0x7f800000) == 0x7f800000;
> I think this is right, but please take a careful look. I checked that both
> 32-bit arm and intel assembly produced by clang do not just call isnanf
again,
> so either clang wasn't smart enough to figure out what this does, or it
did
but
> decided against turning it into a lib call again, or I implemented isnanf
> incorrectly.
>
> Here's the assembly:
>
> 00000000 <_ZN5crazy12_GLOBAL__N_112local_isnanfEf>:
> 0: f000 41ff and.w r1, r0, #2139095040 ; 0x7f800000
> 4: 2000 movs r0, #0
> 6: f1b1 4fff cmp.w r1, #2139095040 ; 0x7f800000
> a: bf08 it eq
> c: 2001 moveq r0, #1
> e: 4770 bx lr
>
>
> 00000000 <_ZN5crazy12_GLOBAL__N_112local_isnanfEf>:
> 0: 55 push %ebp
> 1: 89 e5 mov %esp,%ebp
> 3: 83 e4 fc and $0xfffffffc,%esp
> 6: b9 00 00 80 7f mov $0x7f800000,%ecx
> b: 8b 55 08 mov 0x8(%ebp),%edx
> e: 21 ca and %ecx,%edx
> 10: 31 c0 xor %eax,%eax
> 12: 39 ca cmp %ecx,%edx
> 14: 0f 94 c0 sete %al
> 17: 89 ec mov %ebp,%esp
> 19: 5d pop %ebp
> 1a: c3 ret
>
> (that stack realignment on x86 is so painful to see :-/)

Hmm, can we make local_isnanf() take uint32_t instead of float? You're
casting
it anyway, and from the assembly it passes the argument in r0 anyway. That
should reduce chances of clang recognizing isnanf implementation I think.

https://codereview.chromium.org/2945293002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rmcilroy, 2017-06-21 21:17:01]

https://codereview.chromium.org/2945293002/diff/20001/third_party/android_cra...
File third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp
(right):

https://codereview.chromium.org/2945293002/diff/20001/third_party/android_cra...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:68:
return (bits & 0x7f800000) == 0x7f800000;
This would return true for infinity ( + / - 0x7f800000) as well as nans (any
non-zero value in the significand). We should only return true for the nans.
Looks like glibc does this with some fancy arithmetic to avoid branches:
   bits &= 0x7fffffff;
   bits = 0x7f800000 - bits;
   return (int)(((u_int32_t)(bits))>>31)

[Nico, 2017-06-22 19:34:15]

The CQ bit was checked by thakis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 19:34:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-06-22 19:35:03]

Excellent catch, thanks!

I don't expect isnanf to be called in an inner loop, so I went with an imho less
clever, but not branch-free version.

00000000 <_ZN5crazy12_GLOBAL__N_112local_isnanfEf>:
   0:	f000 41ff 	and.w	r1, r0, #2139095040	; 0x7f800000
   4:	f36f 50df 	bfc	r0, #23, #9
   8:	f1b1 4fff 	cmp.w	r1, #2139095040	; 0x7f800000
   c:	bf18      	it	ne
   e:	2000      	movne	r0, #0
  10:	4770      	bx	lr

[commit-bot: I haz the power, 2017-06-22 20:20:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-22 20:20:27]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2017-06-22 21:28:37]

https://codereview.chromium.org/2945293002/diff/40001/third_party/android_cra...
File third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp
(right):

https://codereview.chromium.org/2945293002/diff/40001/third_party/android_cra...
third_party/android_crazy_linker/src/src/crazy_linker_shared_library.cpp:70:
return bits & 0x7fffff;
One difference between this and the real isnanf is this returns a random
positive integer, whereas isnanf only returns 1. No idea if this will ever be an
issue, but given the unlikelihood of this function ever being installed (and so
the pain in debugging if there ever is an issue) I'd prefer if we only returned
1 or 0 here.

[Nico, 2017-06-22 21:39:09]

Done


00000000 <_ZN5crazy12_GLOBAL__N_112local_isnanfEf>:
   0:	f000 41ff 	and.w	r1, r0, #2139095040	; 0x7f800000
   4:	2200      	movs	r2, #0
   6:	f36f 50df 	bfc	r0, #23, #9
   a:	f1b1 4fff 	cmp.w	r1, #2139095040	; 0x7f800000
   e:	bf08      	it	eq
  10:	2201      	moveq	r2, #1
  12:	2800      	cmp	r0, #0
  14:	bf18      	it	ne
  16:	2001      	movne	r0, #1
  18:	4010      	ands	r0, r2
  1a:	4770      	bx	lr

[Nico, 2017-06-22 21:49:26]

The CQ bit was checked by thakis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 21:50:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rmcilroy, 2017-06-22 23:10:55]

LGTM, thanks.

[Nico, 2017-06-22 23:37:21]

The CQ bit was unchecked by thakis@chromium.org

[Nico, 2017-06-22 23:37:24]

The CQ bit was checked by thakis@chromium.org

[Nico, 2017-06-22 23:37:28]

The CQ bit was checked by thakis@chromium.org

[commit-bot: I haz the power, 2017-06-22 23:37:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 00:20:10]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498174648034810,
"parent_rev": "c485d6c706fe27268948c43e71ef9f68e743bcf7", "commit_rev":
"938998bd58a2fabb04556833a298f3c7facd5135"}

[commit-bot: I haz the power, 2017-06-23 00:20:24]

Description was changed from

==========
crazy_linker: isnanf workaround, reloaded.

The __isnanf only exists on android 21+, so provide a fallback for the
fallback for versions 16-20.

BUG=735003
==========

to

==========
crazy_linker: isnanf workaround, reloaded.

The __isnanf only exists on android 21+, so provide a fallback for the
fallback for versions 16-20.

BUG=735003

Review-Url: https://codereview.chromium.org/2945293002
Cr-Commit-Position: refs/heads/master@{#481737}
Committed:
https://chromium.googlesource.com/chromium/src/+/938998bd58a2fabb04556833a298...
==========

[commit-bot: I haz the power, 2017-06-23 00:20:25]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/938998bd58a2fabb04556833a298...