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

["Maciej W. Rozycki" <macro at codesourcery dot com>]

On Fri, 6 Sep 2013, Carlos O'Donell wrote:

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

 I have no doubt here.

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

 I think there is no need to go beyond the portable _FPU_IEEE macro here 
though; with the tests proposed we have the default and an overridden case 
covered.  If any particular different use case pops out in the future, 
then it can be covered separately for the architectures affected.

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

 OK.

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

 Thanks.

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

 Thank you for your review.

 Does anyone else has anything to add, any comments or objections perhaps?  
Have we reached consensus?

  Maciej