[Patch] Add to the libgfortran/newlib bodge to "detect" ftruncate support in ARM/AArch64/SH

[Patch] Add to the libgfortran/newlib bodge to "detect" ftruncate support in ARM/AArch64/SH

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]


Hi,

Steve's patch in 2013 [1] to fix the MIPS newlib/libgfortran build
causes subtle issues for an ARM/AArch64 newlib/libgfortran build. The
problem is that ARM/AArch64 (and SH) define a stub function for
ftruncate, which we would previously have auto-detected, but which is not
part of the hardwiring Steve added.

Continuing the tradition of building bodge on bodge on bodge, this patch
hardwires HAVE_FTRUNCATE on for ARM/AArch64/SH, which does fix the issue
I was seeing.

If this patch is acceptable for trunk, I'd also like to backport it to the
5.x and 4.9.x release branches.

I'm not quite sure how to effectively verify this patch. I've looked at
the generated config.h for aarch64-none-elf, arm-none-eabi and those come
out with HAVE_FTRUNCATE defined. I wanted to check mips-none-elf, but I
had no success there - the configure failed earlier when trying to link
executables.

I'd appreciate your help Steve to check that this patch works with
your build system.

Thanks,
James

[1]: https://gcc.gnu.org/ml/fortran/2013-09/msg00050.html

---

2015-08-14  James Greenhalgh  <james.greenhalgh@arm.com>

	* configure.ac: Define HAVE_FTRUNCATE for ARM/AArch64/SH newlib
	builds.
	* configure: Regenerate.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Patch-Add-to-the-libgfortran-newlib-bodge-to-detect-.patch --]
[-- Type: text/x-patch;  name=0001-Patch-Add-to-the-libgfortran-newlib-bodge-to-detect-.patch, Size: 719 bytes --]

diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index 35a8b39..adafb3f 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -295,6 +295,13 @@ if test "x${with_newlib}" = "xyes"; then
    if test x"long_double_math_on_this_cpu" = x"yes"; then
      AC_DEFINE(HAVE_STRTOLD, 1, [Define if you have strtold.])
    fi
+
+  # ARM, AArch64 and SH also provide ftruncate.
+  case "${host}" in
+     arm* | aarch64* | sh*)
+       AC_DEFINE(HAVE_FTRUNCATE, 1, [Define if you have ftruncate.])
+       ;;
+  esac
 else
    AC_CHECK_FUNCS_ONCE(getrusage times mkstemp strtof strtold snprintf \
    ftruncate chsize chdir getlogin gethostname kill link symlink sleep ttyname \

Re: [Patch] Add to the libgfortran/newlib bodge to "detect" ftruncate support in ARM/AArch64/SH

[Marcus Shawcroft]

On 20 August 2015 at 09:31, James Greenhalgh <james.greenhalgh@arm.com> wrote:
>
> Hi,
>
> Steve's patch in 2013 [1] to fix the MIPS newlib/libgfortran build
> causes subtle issues for an ARM/AArch64 newlib/libgfortran build. The
> problem is that ARM/AArch64 (and SH) define a stub function for
> ftruncate, which we would previously have auto-detected, but which is not
> part of the hardwiring Steve added.
>
> Continuing the tradition of building bodge on bodge on bodge, this patch
> hardwires HAVE_FTRUNCATE on for ARM/AArch64/SH, which does fix the issue
> I was seeing.

This is the second breakage I'm aware of due to the introduction of
this hardwire code, the first being related to strtold.  My
recollection is that it is only the mips target that requires the
newlib API hardwiring. Ideally we should rely only on the
AC_CHECK_FUNCS_ONCE probe code and avoid the hardwire entirely.

Perhaps a better approach for trunk would be something along the lines of:

case "${host}--x${with_newlib}" in
mips*--xyes)
  hardwire_newlib=1;;
esac
if test "${hardwire_newlib:-0}" -eq 1; then
  ... existing AC_DEFINES hardwire code
else
  ... existing AC_CHECK_FUNCS_ONCE probe code
fi

In effect limiting the hardwire to just the target which is unable to
probe.  For backport to 4.9 and 5 I think James' more conservative
patch is probably more appropriate.

What do folks think?

Cheers
/Marcus

Re: [Patch] Add to the libgfortran/newlib bodge to "detect" ftruncate support in ARM/AArch64/SH

[Steve Ellcey]

On Thu, 2015-08-20 at 09:31 +0100, James Greenhalgh wrote:

> I'd appreciate your help Steve to check that this patch works with
> your build system.
> 
> Thanks,
> James

Yes, this patch works fine with my builds for MIPS.

Steve Ellcey
sellcey@imgtec.com

Re: [Patch] Add to the libgfortran/newlib bodge to "detect" ftruncate support in ARM/AArch64/SH

[James Greenhalgh]

On Thu, Aug 20, 2015 at 10:50:47AM +0100, Marcus Shawcroft wrote:
> On 20 August 2015 at 09:31, James Greenhalgh <james.greenhalgh@arm.com> wrote:
> >
> > Hi,
> >
> > Steve's patch in 2013 [1] to fix the MIPS newlib/libgfortran build
> > causes subtle issues for an ARM/AArch64 newlib/libgfortran build. The
> > problem is that ARM/AArch64 (and SH) define a stub function for
> > ftruncate, which we would previously have auto-detected, but which is not
> > part of the hardwiring Steve added.
> >
> > Continuing the tradition of building bodge on bodge on bodge, this patch
> > hardwires HAVE_FTRUNCATE on for ARM/AArch64/SH, which does fix the issue
> > I was seeing.
> 
> This is the second breakage I'm aware of due to the introduction of
> this hardwire code, the first being related to strtold.  My
> recollection is that it is only the mips target that requires the
> newlib API hardwiring. Ideally we should rely only on the
> AC_CHECK_FUNCS_ONCE probe code and avoid the hardwire entirely.
> 
> Perhaps a better approach for trunk would be something along the lines of:
> 
> case "${host}--x${with_newlib}" in
> mips*--xyes)
>   hardwire_newlib=1;;
> esac
> if test "${hardwire_newlib:-0}" -eq 1; then
>   ... existing AC_DEFINES hardwire code
> else
>   ... existing AC_CHECK_FUNCS_ONCE probe code
> fi
> 
> In effect limiting the hardwire to just the target which is unable to
> probe.  For backport to 4.9 and 5 I think James' more conservative
> patch is probably more appropriate.
> 
> What do folks think?

(+CC fortran@gcc.gnu.org - who I should have CCed from the start).

This runs in to issues with a newlib build [1] (newlib provides a 'kill'
symbol for linking, but does not provide a declaration in signal.h, so
we take a -Werror=implicit-function-declaration).

I think that is something that we should think about fixing in newlib,
and I have a patch in the works that I'd like to start working through with
the newlib community.

In the mean time, I'd like to stick with this patch for trunk, GCC 5,
and GCC 4.9 (with a promise that I'll revisit it once I've worked through
what newlib wants to do).

Is the patch I proposed OK as a "FORNOW" for trunk, and as a full-time
bodge on 5, and 4.9?

Thanks,
James

---
[1]: https://sourceware.org/ml/newlib/2015/msg00630.html

Re: [Patch] Add to the libgfortran/newlib bodge to "detect" ftruncate support in ARM/AArch64/SH

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]


On Fri, Aug 21, 2015 at 11:05:47AM +0100, James Greenhalgh wrote:
> On Thu, Aug 20, 2015 at 10:50:47AM +0100, Marcus Shawcroft wrote:
> > On 20 August 2015 at 09:31, James Greenhalgh <james.greenhalgh@arm.com> wrote:
> > >
> > > Hi,
> > >
> > > Steve's patch in 2013 [1] to fix the MIPS newlib/libgfortran build
> > > causes subtle issues for an ARM/AArch64 newlib/libgfortran build. The
> > > problem is that ARM/AArch64 (and SH) define a stub function for
> > > ftruncate, which we would previously have auto-detected, but which is not
> > > part of the hardwiring Steve added.
> > >
> > > Continuing the tradition of building bodge on bodge on bodge, this patch
> > > hardwires HAVE_FTRUNCATE on for ARM/AArch64/SH, which does fix the issue
> > > I was seeing.
> >
> > This is the second breakage I'm aware of due to the introduction of
> > this hardwire code, the first being related to strtold.  My
> > recollection is that it is only the mips target that requires the
> > newlib API hardwiring. Ideally we should rely only on the
> > AC_CHECK_FUNCS_ONCE probe code and avoid the hardwire entirely.
> >
> > Perhaps a better approach for trunk would be something along the lines of:
> >
> > case "${host}--x${with_newlib}" in
> > mips*--xyes)
> >   hardwire_newlib=1;;
> > esac
> > if test "${hardwire_newlib:-0}" -eq 1; then
> >   ... existing AC_DEFINES hardwire code
> > else
> >   ... existing AC_CHECK_FUNCS_ONCE probe code
> > fi
> >
> > In effect limiting the hardwire to just the target which is unable to
> > probe.  For backport to 4.9 and 5 I think James' more conservative
> > patch is probably more appropriate.
> >
> > What do folks think?
>
> (+CC fortran@gcc.gnu.org - who I should have CCed from the start).
>
> This runs in to issues with a newlib build [1] (newlib provides a 'kill'
> symbol for linking, but does not provide a declaration in signal.h, so
> we take a -Werror=implicit-function-declaration).

This is what the patch you suggested would look like.

I've sent a patch to the newlib list [1] which unconditionally declares
'kill'. With that in place, we can then autodetect the presence of the
functions newlib provides. I'd expect that you would need to apply that
newlib patch if you were testing this patch locally.

I've tested this with a build of arm-none-eabi and aarch64-none-elf to
check that I now get HAVE_FTRUNCATE defined, and that the build completes.

OK?

Thanks,
James

---
2015-08-25  James Greenhalgh  <james.greenhalgh@arm.com>

	* configure.ac: Auto-detect newlib function support unless we
	know there are issues when configuring for a host.
	* configure: Regenerate.

---
[1]: https://sourceware.org/ml/newlib/2015/msg00632.html


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Re-Patch-Add-to-the-libgfortran-newlib-bodge-to-dete.patch --]
[-- Type: text/x-patch;  name=0001-Re-Patch-Add-to-the-libgfortran-newlib-bodge-to-dete.patch, Size: 687 bytes --]

diff --git a/libgfortran/configure.ac b/libgfortran/configure.ac
index 35a8b39..1e9914c 100644
--- a/libgfortran/configure.ac
+++ b/libgfortran/configure.ac
@@ -273,8 +273,13 @@ GCC_HEADER_STDINT(gstdint.h)
 
 AC_CHECK_MEMBERS([struct stat.st_blksize, struct stat.st_blocks, struct stat.st_rdev])
 
+case "${host}--x${with_newlib}" in
+  mips*--xyes)
+    hardwire_newlib=1;;
+esac
+
 # Check for library functions.
-if test "x${with_newlib}" = "xyes"; then
+if test "${hardwire_newlib:-0}" -eq 1; 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.

Re: [Patch] Add to the libgfortran/newlib bodge to "detect" ftruncate support in ARM/AArch64/SH

[FX]

> 2015-08-25  James Greenhalgh  <james.greenhalgh@arm.com>
> 
> 	* configure.ac: Auto-detect newlib function support unless we
> 	know there are issues when configuring for a host.
> 	* configure: Regenerate.

Thanks for CC’ing the fortran list.

Given that this is newlib-specific code, even though it’s in libgfortran configury, you should decide and commit what’s best. I don’t think we have any newlib expert in the Fortran maintainers.

Wait for 48 hours to see if anyone else objects, though.

Cheers,
FX

Re: [Patch] Add to the libgfortran/newlib bodge to "detect" ftruncate support in ARM/AArch64/SH

[James Greenhalgh]

On Tue, Aug 25, 2015 at 03:44:05PM +0100, FX wrote:
> > 2015-08-25  James Greenhalgh  <james.greenhalgh@arm.com>
> > 
> > 	* configure.ac: Auto-detect newlib function support unless we
> > 	know there are issues when configuring for a host.
> > 	* configure: Regenerate.
> 
> Thanks for CC’ing the fortran list.
> 
> Given that this is newlib-specific code, even though it’s in libgfortran
> configury, you should decide and commit what’s best. I don’t think we have
> any newlib expert in the Fortran maintainers.
> 
> Wait for 48 hours to see if anyone else objects, though.

OK, it has been 48 hours and I haven't seen any objections. The newlib
patch has now been committed.

I agree with Marcus' suggestion that we put the more comprehensive patch
(which requires the newlib fix) on trunk and my original patch (which does
not) on the release branches.

I'll go ahead with that later today.

Thanks,
James

Re: [Patch] Add to the libgfortran/newlib bodge to "detect" ftruncate support in ARM/AArch64/SH

[James Greenhalgh]

On Fri, Aug 28, 2015 at 10:40:31AM +0100, James Greenhalgh wrote:
> On Tue, Aug 25, 2015 at 03:44:05PM +0100, FX wrote:
> > > 2015-08-25  James Greenhalgh  <james.greenhalgh@arm.com>
> > > 
> > > 	* configure.ac: Auto-detect newlib function support unless we
> > > 	know there are issues when configuring for a host.
> > > 	* configure: Regenerate.
> > 
> > Thanks for CC’ing the fortran list.
> > 
> > Given that this is newlib-specific code, even though it’s in libgfortran
> > configury, you should decide and commit what’s best. I don’t think we have
> > any newlib expert in the Fortran maintainers.
> > 
> > Wait for 48 hours to see if anyone else objects, though.
> 
> OK, it has been 48 hours and I haven't seen any objections. The newlib
> patch has now been committed.
> 
> I agree with Marcus' suggestion that we put the more comprehensive patch
> (which requires the newlib fix) on trunk and my original patch (which does
> not) on the release branches.
> 
> I'll go ahead with that later today.

Now in place on trunk (r227301), gcc-5-branch (r227302) and gcc-4_9-branch
(r227304).

Give me a shout if you see issues in your build systems.

Thanks,
James

Re: [Patch] Add to the libgfortran/newlib bodge to "detect" ftruncate support in ARM/AArch64/SH

[Hans-Peter Nilsson]

(Pruned the CC list a bit as lists are included anyway)

On Fri, 28 Aug 2015, James Greenhalgh wrote:
> On Fri, Aug 28, 2015 at 10:40:31AM +0100, James Greenhalgh wrote:
> > On Tue, Aug 25, 2015 at 03:44:05PM +0100, FX wrote:
> > > > 2015-08-25  James Greenhalgh  <james.greenhalgh@arm.com>
> > > >
> > > > 	* configure.ac: Auto-detect newlib function support unless we
> > > > 	know there are issues when configuring for a host.
> > > > 	* configure: Regenerate.
> > >
> > > Thanks for CC?ing the fortran list.
> > >
> > > Given that this is newlib-specific code, even though it?s in libgfortran
> > > configury, you should decide and commit what?s best. I don?t think we have
> > > any newlib expert in the Fortran maintainers.
> > >
> > > Wait for 48 hours to see if anyone else objects, though.
> >
> > OK, it has been 48 hours and I haven't seen any objections. The newlib
> > patch has now been committed.
> >
> > I agree with Marcus' suggestion that we put the more comprehensive patch
> > (which requires the newlib fix) on trunk and my original patch (which does
> > not) on the release branches.
> >
> > I'll go ahead with that later today.
>
> Now in place on trunk (r227301), gcc-5-branch (r227302) and gcc-4_9-branch
> (r227304).
>
> Give me a shout if you see issues in your build systems.

Since you asked: I saw a build failure for cris-elf matching the
missing-kill-declaration issue, and I don't like much having to
take manual steps force a new newlib version. It isn't being
automatically updated because there are regressions in my gcc
test-suite results.  I guess autodetecting the kill-declaration
issue in libgfortran is unnecessary complicated, in presence of
a fixed newlib trunk.  All in all, I appreciate you don't force
a new newlib on release branches.

brgds, H-P

Re: [Patch] Add to the libgfortran/newlib bodge to "detect" ftruncate support in ARM/AArch64/SH

[James Greenhalgh]

On Sun, Aug 30, 2015 at 02:46:26PM +0100, Hans-Peter Nilsson wrote:
> (Pruned the CC list a bit as lists are included anyway)
> 
> On Fri, 28 Aug 2015, James Greenhalgh wrote:
> > On Fri, Aug 28, 2015 at 10:40:31AM +0100, James Greenhalgh wrote:
> > > On Tue, Aug 25, 2015 at 03:44:05PM +0100, FX wrote:
> > > > > 2015-08-25  James Greenhalgh  <james.greenhalgh@arm.com>
> > > > >
> > > > > 	* configure.ac: Auto-detect newlib function support unless we
> > > > > 	know there are issues when configuring for a host.
> > > > > 	* configure: Regenerate.
> > > >
> > > > Thanks for CC?ing the fortran list.
> > > >
> > > > Given that this is newlib-specific code, even though it?s in libgfortran
> > > > configury, you should decide and commit what?s best. I don?t think we have
> > > > any newlib expert in the Fortran maintainers.
> > > >
> > > > Wait for 48 hours to see if anyone else objects, though.
> > >
> > > OK, it has been 48 hours and I haven't seen any objections. The newlib
> > > patch has now been committed.
> > >
> > > I agree with Marcus' suggestion that we put the more comprehensive patch
> > > (which requires the newlib fix) on trunk and my original patch (which does
> > > not) on the release branches.
> > >
> > > I'll go ahead with that later today.
> >
> > Now in place on trunk (r227301), gcc-5-branch (r227302) and gcc-4_9-branch
> > (r227304).
> >
> > Give me a shout if you see issues in your build systems.
> 
> Since you asked: I saw a build failure for cris-elf matching the
> missing-kill-declaration issue, and I don't like much having to
> take manual steps force a new newlib version. It isn't being
> automatically updated because there are regressions in my gcc
> test-suite results.  I guess autodetecting the kill-declaration
> issue in libgfortran is unnecessary complicated, in presence of
> a fixed newlib trunk.  All in all, I appreciate you don't force
> a new newlib on release branches.

Hi,

I could postpone the pain until after the release of GCC 6, by that
point the newlib change will have a little longer to make it in to
people's trees. On the other hand, this seems like the best way to fix
the issue, and we are probably as well to do it now while we are still
sitting in stage 1.

I don't want to cause you too much inconvenience, so if you'd like, I
can revert the more comprehensive patch from trunk for now. I would be
very keen to push it again, either late in GCC 6 development, or soon
after the opening of GCC 7.

Otherwise, if you're happy enough with the fix staying in place, I'll
just leave it.

Sorry to have caused you any issues.

James

Re: [Patch] Add to the libgfortran/newlib bodge to "detect" ftruncate support in ARM/AArch64/SH

[Hans-Peter Nilsson]

On Thu, 3 Sep 2015, James Greenhalgh wrote:
> On Sun, Aug 30, 2015 at 02:46:26PM +0100, Hans-Peter Nilsson wrote:
> > (Pruned the CC list a bit as lists are included anyway)
> >
> > On Fri, 28 Aug 2015, James Greenhalgh wrote:
> > > Give me a shout if you see issues in your build systems.
> >
> > Since you asked: I saw a build failure for cris-elf matching the
> > missing-kill-declaration issue, and I don't like much having to
> > take manual steps force a new newlib version. It isn't being
> > automatically updated because there are regressions in my gcc
> > test-suite results.  I guess autodetecting the kill-declaration
> > issue in libgfortran is unnecessary complicated, in presence of
> > a fixed newlib trunk.  All in all, I appreciate you don't force
> > a new newlib on release branches.
>
> Hi,
>
> I could postpone the pain until after the release of GCC 6, by that
> point the newlib change will have a little longer to make it in to
> people's trees. On the other hand, this seems like the best way to fix
> the issue, and we are probably as well to do it now while we are still
> sitting in stage 1.

I do agree; the fault was with newlib for not declaring a
standard function that could actually be linked.  (It did cause
a linker *warning* for the stub function, but libtool didn't
care.)

> I don't want to cause you too much inconvenience, so if you'd like, I
> can revert the more comprehensive patch from trunk for now. I would be
> very keen to push it again, either late in GCC 6 development, or soon
> after the opening of GCC 7.
>
> Otherwise, if you're happy enough with the fix staying in place, I'll
> just leave it.

...that's why I said "All in all, I appreciate you don't force a
new newlib on release branches".  Maybe I wasn't clear: you
didn't; trunk is not a release branch.  Just wanted to cast a
vote for that decision to remain as-is.

> Sorry to have caused you any issues.

No worries.  If you hadn't asked to give a shout I'd have
remained silent - unless I'd have found other issues. :]

brgds, H-P