This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Enable non-complex math builtins from C99 for Bionic


Hi Joseph, thanks for your comments.

I updated the patch:

1) The function name as a second argument in libc_has_function target
hook was removed - was not usefull so far.
2) By using contrib/config-list.mk (thanks for the hint - great tool!)
and analysing tm.h files and what is included in them I have checked
197 targets. That analysis includes all issues that you raised in your
comments - everything is fixed now. I don't like that sometimes we
have to redefine the version of the hook back to the default one due
to a poisoning of including elfos.h, but I couldn't find a better
solution - I commented all those cases.

Regtesting is in progress now. I have already tested the patch before,
so I don't expect to see any new problems.

If all the tests pass, is the patch OK for trunk?

thanks,
Alexander

2013/7/9 Joseph S. Myers <joseph@codesourcery.com>:
> On Thu, 28 Mar 2013, Alexander Ivchenko wrote:
>
>> Hi,
>>
>> 4.8 is now branched, lets come back to the discussion that we had
>> before. I updated the patch a little
>> bit since we now have linux-protos.h and linux-android.c files.
>>
>> I tried to preserve the avaiability of c99 for all targets, but it's
>> pretty difficult, because we are changing
>> the defaults. Passing an empty string as second argument doesn't look
>> very good, but on the other hand
>> the user has one clear way for checking the presence of a certain
>> function. But of course we can create
>> another function, that will call targetm.libc_has_function
>> (function_class, "") within itself.
>
> Observations on this patch version:
>
> * You still shouldn't be calling with "" as function name (or 0, in
> i386.md) unless there's a good reason the relevant function can't readily
> be identified.  (Actually, the TARGET_C99_FUNCTIONS checks in i386.md seem
> wrong - the instruction patterns expand things inline and the semantics of
> an isinf insn pattern are nothing to do with whether the target library
> has C99 functions; C99 doesn't provide isinf as a function anyway, only as
> a macro.)  If you don't yet need the name in any hook implementation, feel
> free to define the hooks without the name argument and it can be added
> later if needed.
>
> * I don't see how your change for Darwin preserves the existing semantics;
> you appear to make it use no_c99_libc_has_function, but the existing
> semantics are that it has C99 functions except for 32-bit powerpc Darwin
> versions older than 10.3.
>
> * I don't see how your change preserves semantics for Solaris either - you
> add a definition in sol2.h (for Solaris 9) but don't override it in
> sol2-10.h (for Solaris 10, which has the C99 functions).
>
> * I don't see anything to preserve semantics for AIX 4.3 and 5.1 (the AIX
> versions for which TARGET_C99_FUNCTIONS was not defined).
>
> * I don't see anything to preserve semantics for alpha*-dec-*vms*.
>
> * I don't see anything to preserve semantics for hppa*-*-hpux*.
>
> * I don't see anything to preserve semantics for
> i[34567]86-pc-msdosdjgpp*.
>
> * I don't see anything to preserve semantics for i[34567]86-*-cygwin*, or
> x86_64-*-cygwin*, or i[34567]86-*-mingw*, or x86_64-*-mingw*, or
> i[34567]86-*-interix[3-9]*.
>
> * It looks rather like microblaze*-*-* don't use elfos.h, so meaning
> semantics aren't preserved for those (non-Linux) targets either.  Now, I
> don't know if there's a good reason for not using that file (ask the
> architecture maintainer), but in any case semantics should be preserved.
>
> * I don't see anything to preserve semantics for mmix-knuth-mmixware, or
> pdp11-*-*, or picochip-* (more non-ELF targets).
>
> * The patch is missing the poisoning of the removed target macros in
> system.h.
>
> Now, for verifying semantics are unchanged for existing targets, you might
> want to adapt contrib/config-list.mk to show in some way the
> TARGET_C99_FUNCTIONS setting resulting from tm.h (before your patch) or
> the new hook setting (after your patch), so you can compare for all the
> targets listed there.  That's not perfect - the list of targets may not be
> complete - but if you fix all the issues listed above then
> before-and-after comparison for lots of targets using config-list.mk would
> be a good way of gaining confidence in the lack of unintended changes in
> the patch.
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Attachment: libc_has_function_05.patch
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]