Patchwork [v2] Fix libgfortran cross compile configury w.r.t newlib

login
register
mail settings
Submitter Marcus Shawcroft
Date Oct. 1, 2013, 11:40 a.m.
Message ID <524AB4AD.6070508@arm.com>
Download mbox | patch
Permalink /patch/279431/
State New
Headers show

Comments

Marcus Shawcroft - Oct. 1, 2013, 11:40 a.m.
On 30/09/13 13:40, Marcus Shawcroft wrote:

>> Well, I thought this patch would work for me, but it does not.  It looks
>> like gcc_no_link is set to 'no' on my target because, technically, I can
>> link even if I don't use a linker script.  I just can't find any
>> functions.
>>

> In which case gating on gcc_no_link could be replaced with a test that
> looks to see if we can link with the library.  Perhaps looking for
> exit() or some such that might reasonably be expected to be present.
>
> For example:
>
> AC_CHECK_FUNC(exit)
> if test "x${with_newlib}" = "xyes" -a "x${ac_cv_func_exit}" = "xno"; then
>
> /Marcus
>
>
>
>


Patch attached.

/Marcus

2013-10-01  Marcus Shawcroft  <marcus.shawcroft@arm.com>

         * configure.ac (AC_CHECK_FUNCS_ONCE): Add for exit() then make
         existing AC_CHECK_FUNCS_ONCE dependent on outcome.
Steve Ellcey - Oct. 1, 2013, 3:19 p.m.
On Tue, 2013-10-01 at 12:40 +0100, Marcus Shawcroft wrote:
> On 30/09/13 13:40, Marcus Shawcroft wrote:
> 
> >> Well, I thought this patch would work for me, but it does not.  It looks
> >> like gcc_no_link is set to 'no' on my target because, technically, I can
> >> link even if I don't use a linker script.  I just can't find any
> >> functions.
> >>
> 
> > In which case gating on gcc_no_link could be replaced with a test that
> > looks to see if we can link with the library.  Perhaps looking for
> > exit() or some such that might reasonably be expected to be present.
> >
> > For example:
> >
> > AC_CHECK_FUNC(exit)
> > if test "x${with_newlib}" = "xyes" -a "x${ac_cv_func_exit}" = "xno"; then
> >
> > /Marcus

This patch works on my mips-mti-elf target.

Steve Ellcey
sellcey@mips.com
Marcus Shawcroft - Oct. 8, 2013, 2:25 p.m.
On 1 October 2013 12:40, Marcus Shawcroft <marcus.shawcroft@arm.com> wrote:

> Patch attached.
>
> /Marcus
>
> 2013-10-01  Marcus Shawcroft  <marcus.shawcroft@arm.com>
>
>         * configure.ac (AC_CHECK_FUNCS_ONCE): Add for exit() then make
>         existing AC_CHECK_FUNCS_ONCE dependent on outcome.

Ping.
Marcus Shawcroft - Oct. 15, 2013, 11:31 a.m.
On 1 October 2013 12:40, Marcus Shawcroft <marcus.shawcroft@arm.com> wrote:
> On 30/09/13 13:40, Marcus Shawcroft wrote:
>
>>> Well, I thought this patch would work for me, but it does not.  It looks
>>> like gcc_no_link is set to 'no' on my target because, technically, I can
>>> link even if I don't use a linker script.  I just can't find any
>>> functions.
>>>
>
>> In which case gating on gcc_no_link could be replaced with a test that
>> looks to see if we can link with the library.  Perhaps looking for
>> exit() or some such that might reasonably be expected to be present.
>>
>> For example:
>>
>> AC_CHECK_FUNC(exit)
>> if test "x${with_newlib}" = "xyes" -a "x${ac_cv_func_exit}" = "xno"; then
>>
>> /Marcus
>>
>>
>>
>>
>
>
> Patch attached.
>
> /Marcus
>
> 2013-10-01  Marcus Shawcroft  <marcus.shawcroft@arm.com>
>
>         * configure.ac (AC_CHECK_FUNCS_ONCE): Add for exit() then make
>         existing AC_CHECK_FUNCS_ONCE dependent on outcome.

Ping^2

/Marcus
Richard Earnshaw - Oct. 15, 2013, 2:13 p.m.
On 15/10/13 12:31, Marcus Shawcroft wrote:
> On 1 October 2013 12:40, Marcus Shawcroft <marcus.shawcroft@arm.com> wrote:
>> On 30/09/13 13:40, Marcus Shawcroft wrote:
>>
>>>> Well, I thought this patch would work for me, but it does not.  It looks
>>>> like gcc_no_link is set to 'no' on my target because, technically, I can
>>>> link even if I don't use a linker script.  I just can't find any
>>>> functions.
>>>>
>>
>>> In which case gating on gcc_no_link could be replaced with a test that
>>> looks to see if we can link with the library.  Perhaps looking for
>>> exit() or some such that might reasonably be expected to be present.
>>>
>>> For example:
>>>
>>> AC_CHECK_FUNC(exit)
>>> if test "x${with_newlib}" = "xyes" -a "x${ac_cv_func_exit}" = "xno"; then
>>>
>>> /Marcus
>>>
>>>
>>>
>>>
>>
>>
>> Patch attached.
>>
>> /Marcus
>>
>> 2013-10-01  Marcus Shawcroft  <marcus.shawcroft@arm.com>
>>
>>         * configure.ac (AC_CHECK_FUNCS_ONCE): Add for exit() then make
>>         existing AC_CHECK_FUNCS_ONCE dependent on outcome.
> 
> Ping^2
> 
> /Marcus
> 

OK provided no Fortran maintainer objects within the next 24 hours.

R.
Tobias Burnus - Oct. 15, 2013, 8:17 p.m.
Marcus Shawcroft wrote:>> 2013-10-01  Marcus Shawcroft 
<marcus.shawcroft@arm.com>
 >>
 >>          * configure.ac (AC_CHECK_FUNCS_ONCE): Add for exit() then make
 >>          existing AC_CHECK_FUNCS_ONCE dependent on outcome.
 >
 > Ping^2

For configure patches, I am never quite sure whether they should be 
reviewed by a build maintainer or by the library maintainer. In any 
case, the change looks reasonable to me and as no other newlib user has 
complained, it is also okay from my side.

Tobias
Mike Stump - Oct. 15, 2013, 9:35 p.m.
On Oct 15, 2013, at 1:17 PM, Tobias Burnus <burnus@net-b.de> wrote:
> Marcus Shawcroft wrote:>> 2013-10-01  Marcus Shawcroft <marcus.shawcroft@arm.com>
> >>
> >>          * configure.ac (AC_CHECK_FUNCS_ONCE): Add for exit() then make
> >>          existing AC_CHECK_FUNCS_ONCE dependent on outcome.
> >
> > Ping^2
> 
> For configure patches, I am never quite sure whether they should be reviewed by a build maintainer or by the library maintainer. In any case, the change looks reasonable to me and as no other newlib user has complained, it is also okay from my side.

Would be nice for a build/config person to weigh in or to upgrade and make bullet proof the system against such failures.  My take, by default, the compile line should do something useful, and that should be enough for autoconf style tests to smell the library.  Now, we can observe that Steve's mips-mti-elf newlib port apparently violates that, but it is lost on me why that is and why that is a good thing.  Steve?  Is there some reason why by default, a suitable linker script can't be added by the compiler, or, if none suitable, a stub one that isn't suitable in general, but, is complete enough to allow autoconf to function normally?
Marcus Shawcroft - Oct. 24, 2013, 2:53 p.m.
On 15 October 2013 22:35, Mike Stump <mikestump@comcast.net> wrote:

> Would be nice for a build/config person to weigh in or to upgrade and make bullet proof the system against such failures.  My take, by default, the compile line should do something useful, and that should be enough for autoconf style tests to smell the library.  Now, we can observe that Steve's mips-mti-elf newlib port apparently violates that, but it is lost on me why that is and why that is a good thing.  Steve?  Is there some reason why by default, a suitable linker script can't be added by the compiler, or, if none suitable, a stub one that isn't suitable in general, but, is complete enough to allow autoconf to function normally?


Steve,

Can your build be fixed allowing us to back out:
http://gcc.gnu.org/ml/fortran/2013-06/msg00038.html

?

I'd really like to make some progress on this, while my proposed patch
does resolve the regression introduced by the above patch I am
concerned that this is going in the wrong direction and that we
should, as Mike suggests above fix the build issue such that autoconf
behaves, rather than attempting to hardwire configure details of
newlib into libgfortran...

/Marcus
Steve Ellcey - Oct. 24, 2013, 4:47 p.m.
On Thu, 2013-10-24 at 15:53 +0100, Marcus Shawcroft wrote:

> Steve,
> 
> Can your build be fixed allowing us to back out:
> http://gcc.gnu.org/ml/fortran/2013-06/msg00038.html
> 
> ?
> 
> I'd really like to make some progress on this, while my proposed patch
> does resolve the regression introduced by the above patch I am
> concerned that this is going in the wrong direction and that we
> should, as Mike suggests above fix the build issue such that autoconf
> behaves, rather than attempting to hardwire configure details of
> newlib into libgfortran...
> 
> /Marcus

I am not sure how we would fix the build issue to allow us to not
hardcode the newlib configure details into the libgfortran configure
script.  The linker script that needs to be used to get a good link is
different depending on what options one is compiling with on a multilib
MIPS system and I have no idea on how one could create (or use) a dummy
linker script.  Ideally, I think we would want a check that does not
depend on linking at all.

Note that this problem is not just in libgfortran, it affects libstdc++
and libjava too and those configure scripts also have hardcoded the
newlib assumptions.  The only difference between libgfortran and the
other two libraries is that the newlib assumptions for libgfortran are
not static like they are for libstdc++ and libjava.  They vary (for one
function) based on whether or not long double is supported.

If we want to come up with a better solution it should be used for all
the libraries and not just for libgfortran.

Steve Ellcey
sellcey@mips.com
Marcus Shawcroft - Nov. 6, 2013, 5:32 p.m.
On 24 October 2013 17:47, Steve Ellcey <sellcey@mips.com> wrote:

> I am not sure how we would fix the build issue to allow us to not
> hardcode the newlib configure details into the libgfortran configure
> script.  The linker script that needs to be used to get a good link is
> different depending on what options one is compiling with on a multilib
> MIPS system and I have no idea on how one could create (or use) a dummy
> linker script.  Ideally, I think we would want a check that does not
> depend on linking at all.
>
> Note that this problem is not just in libgfortran, it affects libstdc++
> and libjava too and those configure scripts also have hardcoded the
> newlib assumptions.  The only difference between libgfortran and the
> other two libraries is that the newlib assumptions for libgfortran are
> not static like they are for libstdc++ and libjava.  They vary (for one
> function) based on whether or not long double is supported.

Exactly, hard wiring the newlib interface into the configury of other
libraries is questionable, but the patch applied to libgfortran goes
beyond hard wiring details of the interface, it hard wires incorrect
details of the interface when sizeof(long double) != sizeof(double).

The argument that the patch is OK because libjava and libstdc++ also
use this idiom, is also rather questionable, because the libgfortran
patch goes beyond the idiom used in those other libraries by hard
wiring an assumption that does not hold universally.

On that basis, I think the the libgfortran patch should be reverted
since it caused a regression.

/Marcus
Steve Ellcey - Nov. 12, 2013, 4:41 p.m.
On Wed, 2013-11-06 at 17:32 +0000, Marcus Shawcroft wrote:

> Exactly, hard wiring the newlib interface into the configury of other
> libraries is questionable, but the patch applied to libgfortran goes
> beyond hard wiring details of the interface, it hard wires incorrect
> details of the interface when sizeof(long double) != sizeof(double).

Actually, libstdc++ hardwires incorrect information too.  In its
configure it checks to see if long_double_math_on_this_cpu is set to
'yes' and then defines the *L math routines if it is not set.  But the
variable is never set to true anywhere.  The comment in configure.ac
says:

  # At some point, we should differentiate between architectures
  # like x86, which have long double versions, and alpha/powerpc/etc.,
  # which don't. For the time being, punt.

I tested to see if this approach would work with libgfortran and MIPS by
not defining HAVE_STRTOLD and I could still build and run the testsuite.
In fact (at least on MIPS) we never use strtold because
GFC_REAL_16_IS_FLOAT128 is defined and that takes precedence and calls a
different routine to do the conversion.

> The argument that the patch is OK because libjava and libstdc++ also
> use this idiom, is also rather questionable, because the libgfortran
> patch goes beyond the idiom used in those other libraries by hard
> wiring an assumption that does not hold universally.

libgfortran is no better or worse then libstdc++ in this regard.

> On that basis, I think the the libgfortran patch should be reverted
> since it caused a regression.
> 
> /Marcus

I understand that you don't like the patch and that you don't think
'libstdc++ does it too' is an adequate rationale, but at some point
being able to build is more important then having a clean looking
configure script and if we revert my patch, I can't build Fortran on
MIPS.

If you don't want to check in you patch that checks to see if we can
link in the exit routine, I would support removing the HAVE_STRTOLD
setting completely or putting it under a long_double_math_on_this_cpu
check (that is always false) like the libstdc++ configure script has.
Either of these changes would allow me to build.  But just removing my
original patch leaves me unable to build at all and I don't know of a
'clean' way to fix that and apparently neither did the people working on
the libstdc++ and libjava configure scripts so I do object to reverting
my patch.

Steve Ellcey
sellcey@mips.com

Patch

diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index 4609eba..ac0c02f 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -261,7 +261,8 @@  GCC_HEADER_STDINT(gstdint.h)
 AC_CHECK_MEMBERS([struct stat.st_blksize, struct stat.st_blocks, struct stat.st_rdev])
 
 # Check for library functions.
-if test "x${with_newlib}" = "xyes"; then
+AC_CHECK_FUNC(exit)
+if test "x${with_newlib}" = "xyes" -a "x${ac_cv_func_exit}" = "xno"; then
    # We are being configured with a cross compiler.  AC_REPLACE_FUNCS
    # may not work correctly, because the compiler may not be able to
    # link executables.