Re: [PATCH v2] Fix static-binary lazy FPU context allocation

["Carlos O'Donell" <carlos at redhat dot com>]

On 09/05/2013 12:10 PM, Maciej W. Rozycki wrote:
> On Tue, 3 Sep 2013, Carlos O'Donell wrote:
> 
>>>  No regressions in mips-linux-gnu testing, o32, n64 and n32 ABIs.  OK to 
>>> apply?
>>
>> What existing test case checks for this?
> 
>  We've got math/test-fpucw that gives partial coverage; this won't check 
> static semantics though unless $(build-shared) is (unusually) set to `no'.

It's not enough to cover what you're fixing.

>> Could you please add a static test that verifies this is set correctly?
> 
>  The update below adds static coverage in all cases and extends dynamic 
> coverage to non-default __fpu_control initialisation; the only portable 
> way of doing that is I believe with the use of the _FPU_IEEE macro, so 
> I've chosen that (it is worth noting however that the use of anything but 
> _FPU_DEFAULT breaks ISO C compliance).

In my opinion the test case need not be portable as long as it tests 
the expected behaviour. Many of our test cases must actually be non-portable
to test the kinds of implementation details we are trying to verify.

>  Note that the test cases rely on the presence of the _FPU_GETCW macro so 
> some ports are not covered (Alpha).  Also the only Linux port that 
> currently actually produces the AT_FPUCW tag is Renesas SuperH.

That's fine and I accept that. It's not your position to need to fix
the machine ports.

>>> Index: glibc-fsf-trunk-quilt/elf/dl-support.c
>>> ===================================================================
>>> --- glibc-fsf-trunk-quilt.orig/elf/dl-support.c	2013-08-20 15:18:10.000000000 +0100
>>> +++ glibc-fsf-trunk-quilt/elf/dl-support.c	2013-08-21 20:07:55.169669153 +0100
>>> @@ -167,6 +167,8 @@ size_t _dl_phnum;
>>>  uint64_t _dl_hwcap __attribute__ ((nocommon));
>>>  uint64_t _dl_hwcap2 __attribute__ ((nocommon));
>>>  
>>
>> This needs a detailed comment here explaining the behaviour of this
>> internal variable. I would also welcome an expounding of the semantics
>> of __fpu_control and the behaviour for static and dynamic applications.
> 
>  I've added a note on _dl_fpu_control now; NB most of these variables are 
> undocumented here.

Thank you. I agree that most of the are undocumented. I wasn't around to
review those.

>  I'm not sure what you'd like to see on `__fpu_control' and where (an 
> expansion of the note in math/fpu_control.c perhaps?), I'll appreciate 
> guidance.  The semantics of this variable is supposed to be the same 
> whether a static or a dynamic app.

Don't worry about it. I do not wish to drag out your useful patch
to add additional comments. We can add these later.

>> In summary:
>> - Test case.
>> - Comment for _dl_fpu_control.
> 
>  Thanks for your review, here's an updated patch.  No failures with the 
> test-fpucw* cases on MIPS/Linux; regrettably I have no way to test 
> SuperH/Linux.  OK for this version (barring any `__fpu_control' doc 
> update)?
> 
>  BTW, I have realised I am supposed to add myself to wiki/MAINTAINERS -- 
> can you please grant me a write privilege to do so (ID: macro)?

I have vouched for you and added you to the EditorGroup. You can now
edit any page on the wiki. You can also add others to the EditorGroup.

> 2013-09-05  Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	* csu/init-first.c (_init): Remove the !SHARED condition around
> 	FPU control word initialization.
> 	* elf/dl-support.c (_dl_fpu_control): New variable.
> 	(_dl_aux_init) <AT_FPUCW>: Initialize it.
> 	* math/test-fpucw.c [!FPU_CONTROL] (FPU_CONTROL): New macro.
> 	(main): Replace _FPU_DEFAULT with FPU_CONTROL throughout.
> 	* math/test-fpucw-static.c: New file.
> 	* math/test-fpucw-ieee.c: New file.
> 	* math/test-fpucw-ieee-static.c: New file.
> 	* math/Makefile (tests): Add `test-fpucw-ieee' and 
> 	`$(tests-static)'.
> 	(tests-static): New variable.
> 	[($(build-shared),yes)] ($(addprefix $(objpfx),$(tests))): Move 
> 	dependency to...
> 	[($(build-shared),yes)]
> 	($(addprefix $(objpfx),$(filter-out $(tests-static),$(tests)))):
> 	... this.
> 	[($(build-shared),yes)] ($(addprefix $(objpfx),$(tests-static))):
> 	New dependency.

I'm happy with this, thanks for adding the extra static tests
and the comment.

Cheers,
Carlos.