diff mbox

Remove incorrect register mov in floorf on x86_64

Message ID 20150814103330.GA15748@spoyarek.pnq.redhat.com
State New
Headers show

Commit Message

Siddhesh Poyarekar Aug. 14, 2015, 10:33 a.m. UTC 
The change in 0b5395f052ee09cd7e3d219af4e805c38058afb5 replaced calls
to __get_cpu_features@plt followed by a mov from rax to rdx, with a
single macro LOAD_RTLD_GLOBAL_RO_RDX.  It is pretty clear that there
was a typo in s_floorf due to which the (now incorrect) mov was not
removed.  This patch removes that mov.

Siddhesh

	* sysdeps/x86_64/fpu/multiarch/s_floorf.S (__floorf): Remove
	unnecessary movq.

---
 sysdeps/x86_64/fpu/multiarch/s_floorf.S | 1 -
 1 file changed, 1 deletion(-)

Comments

Ondřej Bílka Aug. 14, 2015, 11:12 a.m. UTC | #1 
On Fri, Aug 14, 2015 at 04:03:30PM +0530, Siddhesh Poyarekar wrote:
> The change in 0b5395f052ee09cd7e3d219af4e805c38058afb5 replaced calls
> to __get_cpu_features@plt followed by a mov from rax to rdx, with a
> single macro LOAD_RTLD_GLOBAL_RO_RDX.  It is pretty clear that there
> was a typo in s_floorf due to which the (now incorrect) mov was not
> removed.  This patch removes that mov.
> 
It isn't incorrect, just dead move. Ok for me.
Siddhesh Poyarekar Aug. 14, 2015, 11:19 a.m. UTC | #2 
On Fri, Aug 14, 2015 at 01:12:04PM +0200, Ondřej Bílka wrote:
> It isn't incorrect, just dead move. Ok for me.

No, it loads an arbitrary value (we don't know what is in %rax at that
point) into %rdx and overwrites the struct address.

Siddhesh
H.J. Lu Aug. 14, 2015, 12:34 p.m. UTC | #3 
On Fri, Aug 14, 2015 at 4:19 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
> On Fri, Aug 14, 2015 at 01:12:04PM +0200, Ondřej Bílka wrote:
>> It isn't incorrect, just dead move. Ok for me.
>
> No, it loads an arbitrary value (we don't know what is in %rax at that
> point) into %rdx and overwrites the struct address.
>

Thanks for catching it. I checked in your patch together with the same
fix for sysdeps/x86_64/fpu/multiarch/s_nearbyint.S
Carlos O'Donell Aug. 14, 2015, 8:05 p.m. UTC | #4 
On 08/14/2015 08:34 AM, H.J. Lu wrote:
> On Fri, Aug 14, 2015 at 4:19 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
>> On Fri, Aug 14, 2015 at 01:12:04PM +0200, Ondřej Bílka wrote:
>>> It isn't incorrect, just dead move. Ok for me.
>>
>> No, it loads an arbitrary value (we don't know what is in %rax at that
>> point) into %rdx and overwrites the struct address.
>>
> 
> Thanks for catching it. I checked in your patch together with the same
> fix for sysdeps/x86_64/fpu/multiarch/s_nearbyint.S
 
Why didn't testing catch this? Is it because this is a multiarch that
didn't get tested?

c.
H.J. Lu Aug. 14, 2015, 8:36 p.m. UTC | #5 
On Fri, Aug 14, 2015 at 1:05 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 08/14/2015 08:34 AM, H.J. Lu wrote:
>> On Fri, Aug 14, 2015 at 4:19 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
>>> On Fri, Aug 14, 2015 at 01:12:04PM +0200, Ondřej Bílka wrote:
>>>> It isn't incorrect, just dead move. Ok for me.
>>>
>>> No, it loads an arbitrary value (we don't know what is in %rax at that
>>> point) into %rdx and overwrites the struct address.
>>>
>>
>> Thanks for catching it. I checked in your patch together with the same
>> fix for sysdeps/x86_64/fpu/multiarch/s_nearbyint.S
>
> Why didn't testing catch this? Is it because this is a multiarch that
> didn't get tested?

RAX contains a valid address, but not the correct address.  Since
my machine has SSE4.1, any version works.  But machines without
SSE4.1 may fail.
Carlos O'Donell Aug. 14, 2015, 8:45 p.m. UTC | #6 
On 08/14/2015 04:36 PM, H.J. Lu wrote:
> On Fri, Aug 14, 2015 at 1:05 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 08/14/2015 08:34 AM, H.J. Lu wrote:
>>> On Fri, Aug 14, 2015 at 4:19 AM, Siddhesh Poyarekar <siddhesh@redhat.com> wrote:
>>>> On Fri, Aug 14, 2015 at 01:12:04PM +0200, Ondřej Bílka wrote:
>>>>> It isn't incorrect, just dead move. Ok for me.
>>>>
>>>> No, it loads an arbitrary value (we don't know what is in %rax at that
>>>> point) into %rdx and overwrites the struct address.
>>>>
>>>
>>> Thanks for catching it. I checked in your patch together with the same
>>> fix for sysdeps/x86_64/fpu/multiarch/s_nearbyint.S
>>
>> Why didn't testing catch this? Is it because this is a multiarch that
>> didn't get tested?
> 
> RAX contains a valid address, but not the correct address.  Since
> my machine has SSE4.1, any version works.  But machines without
> SSE4.1 may fail.
 
OK, that's what I figured. I'll see what we can do to make this better.

Cheers,
Carlos.
Ondřej Bílka Aug. 18, 2015, 6:01 a.m. UTC | #7 
On Fri, Aug 14, 2015 at 04:49:49PM +0530, Siddhesh Poyarekar wrote:
> On Fri, Aug 14, 2015 at 01:12:04PM +0200, Ondřej Bílka wrote:
> > It isn't incorrect, just dead move. Ok for me.
> 
> No, it loads an arbitrary value (we don't know what is in %rax at that
> point) into %rdx and overwrites the struct address.
> 
sorry, i somewhat looked at moving function address and thought its
overwriting it. you are rigth.
diff mbox

Patch

diff --git a/sysdeps/x86_64/fpu/multiarch/s_floorf.S b/sysdeps/x86_64/fpu/multiarch/s_floorf.S
index f60f662..9d67847 100644
--- a/sysdeps/x86_64/fpu/multiarch/s_floorf.S
+++ b/sysdeps/x86_64/fpu/multiarch/s_floorf.S
@@ -23,7 +23,6 @@ 
 ENTRY(__floorf)
 	.type	__floorf, @gnu_indirect_function
 	LOAD_RTLD_GLOBAL_RO_RDX
-	movq	%rax, %rdx
 	leaq	__floorf_sse41(%rip), %rax
 	HAS_CPU_FEATURE (SSE4_1)
 	jnz	2f