This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] Fix static-binary lazy FPU context allocation
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: "Maciej W. Rozycki" <macro at codesourcery dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Fri, 06 Sep 2013 02:26:12 -0400
- Subject: Re: [PATCH v2] Fix static-binary lazy FPU context allocation
- Authentication-results: sourceware.org; auth=none
- References: <alpine dot DEB dot 1 dot 10 dot 1308221904520 dot 8514 at tp dot orcam dot me dot uk> <52260408 dot 2030809 at redhat dot com> <alpine dot DEB dot 1 dot 10 dot 1309042303200 dot 29360 at tp dot orcam dot me dot uk>
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.