Re: [PATCH v3 2/4] libffi/test: Fix compilation for build sysroot
On Mon, 30 Mar 2020, Mike Stump wrote:
> > I have actually considered extracting the bits already, but I hesitated
> > putting that forward that as having looked at the part that we require I
> > have thought it to be very messy:
>
> Yeah, sometimes it's like that. I glanced at the work, if you think
> it's a step forward, I'd support importing it, my take, import from
> upstream isn't a bad way to go.
So I looked into it some more and interestingly enough all the commits I
have listed in the previous message have already been made as of libffi's
commit c82cc159426d ("Merge pull request #166 from chevah/master") that we
imported with our commit b1760f7f915a ("Merge libffi to upstream commit
c82cc159426d8d4402375fa1ae3f045b9cf82e16").
That merge was extensively discussed starting from:
, however no
mention was as to why the local.exp part had been omitted from the merge.
Perhaps it was considered not necessary for integrating with the GCC tree,
although I would think that keeping the divergence to the minimum is
preferable, and it looks to me that our requirements boil down essentially
to adding multilib support and making some further, minor tweaks to match
the rest of the tree. Whereas the diff between the Makefile systems as at
libffi's commit c82cc159426d and our commit b1760f7f915a looks to me quite
substantial.
Perhaps Richard will be able to provide some input here.
> > So I am in favour of retaining the mechanism rather than using my earlier
> > proposal, however I'm in two minds as to how to proceed. Integrating the
> > change as it is will make us having clutter left in the tree after `make
> > distclean', but we can do it right away.
>
> I'd support this. distclean is one rm -rf away from being clean enough.
> I'd not let that gate or hold up the import.
While doing further work on finding a solution that would be acceptable
(to me anyway), I actually found two further upstream libffi commits:
- commit 6b6df1a7bb37 ("[PATCH] Adds `local.exp` to CLEANFILES"),
- commit 6cf0dea78a5a ("[PATCH] Change CLEANFILES to DISTCLEANFILES"),
both beyond our merge point, that fix this shortcoming. Still there's no
Makefile dependency, so if configure.ac is patched or local.exp removed,
then it is not regenerated, and all that would not be required if what
automake provides was used.
> If there is work that we want that's more to do with in tree building
> and testing (sys root fun, multilibs), while not ideal, we can deviate
> from upstream to support that. Though, if there is a way to naturally
> support that, that upstream can accept, that'd be better.
I did some work now to reduce the divergence and will be posting patch
series shortly to both upstream libffi and our version. Hopefully that'll
be acceptable, at least the initial, minimal change from each series.
If not, for a reference, here's an updated version of the patch I posted
last time. It includes the two upstream libffi commits I have mentioned
above.
Let's see how it goes. Thank you for your input.
Maciej
---
libffi/Makefile.am |3 +++
libffi/Makefile.in |4
libffi/configure |5 +
libffi/configure.ac |5 +
libffi/testsuite/Makefile.am |2 ++
libffi/testsuite/Makefile.in |1 +
6 files changed, 20 insertions(+)
gcc-test-libffi-cc-for-target.diff
Index: gcc/libffi/Makefile.am
===
--- gcc.orig/libffi/Makefile.am
+++ gcc/libffi/Makefile.am
@@ -15,6 +15,9 @@ EXTRA_DIST = LICENSE ChangeLog.v1 Change
libffi.xcodeproj/project.pbxproj\
libtool-ldflags
+# local.exp is generated by configure
+DISTCLEANFILES = local.exp
+
# Automake Documentation:
# If your package has Texinfo files in many directories, you can use the
# variable TEXINFO_TEX to tell Automake where to find the canonical
Index: gcc/libffi/Makefile.in
===
--- gcc.orig/libffi/Makefile.in
+++ gcc/libffi/Makefile.in
@@ -454,6 +454,9 @@ EXTRA_DIST = LICENSE ChangeLog.v1 Change
libtool-ldflags
+# local.exp is generated by configure
+DISTCLEANFILES = local.exp
+
# Automake Documentation:
# If your package has Texinfo files in many directories, you can use the
# variable TEXINFO_TEX to tell Automake where to find the canonical
@@ -1674,6 +1677,7 @@ installcheck: installcheck-recursive
-rm -f src/x86/$(am__dirstamp)
-rm -f src/xtensa/$(DEPDIR)/$(am__dirstamp)
-rm -f src/xtensa/$(am__dirstamp)
+ -test -z "$(DISTCLEANFILES)" || rm -f $(DISTCLEANFILES)
maintainer-clean-generic:
@echo "This command is intended for maintainers to use"
Index: gcc/libffi/configure
===
--- gcc.orig/libffi/configure
+++ gcc/libffi/c
Re: [PATCH v3 2/4] libffi/test: Fix compilation for build sysroot
On Mar 26, 2020, at 3:00 PM, Maciej W. Rozycki wrote: > > I have actually considered extracting the bits already, but I hesitated > putting that forward that as having looked at the part that we require I > have thought it to be very messy: Yeah, sometimes it's like that. I glanced at the work, if you think it's a step forward, I'd support importing it, my take, import from upstream isn't a bad way to go. > So I am in favour of retaining the mechanism rather than using my earlier > proposal, however I'm in two minds as to how to proceed. Integrating the > change as it is will make us having clutter left in the tree after `make > distclean', but we can do it right away. I'd support this. distclean is one rm -rf away from being clean enough. I'd not let that gate or hold up the import. If there is work that we want that's more to do with in tree building and testing (sys root fun, multilibs), while not ideal, we can deviate from upstream to support that. Though, if there is a way to naturally support that, that upstream can accept, that'd be better.
Re: [PATCH v3 2/4] libffi/test: Fix compilation for build sysroot
On Tue, 24 Mar 2020, Mike Stump wrote:
> > Have we made any conclusions WRT the way to move forward with this stuff?
> > Things remain broken and I'd prefer to get the issues off the plate while
> > the stuff is hot, or at least mildly warm. I'm about to get distracted
> > with other work.
>
> It's unfortunate that upstream has anything that prevents it from us
> just importing it all and calling it done.
>
> Anyway, if you see a path forward for grabbing all the Makefile/config
> stuff and leaving the abi changing stuff out, and just important that,
> we can do a partial import. I say this without reviewing the diffs from
> upstream and how many there are and what's in them. I'm hoping things
> are nicely segregated and reasonably small otherwise.
Thank you for your input.
I have actually considered extracting the bits already, but I hesitated
putting that forward that as having looked at the part that we require I
have thought it to be very messy: the .exp file is handcrafted with an
inline piece of scriptery buried in `configure.ac' and never cleaned, not
even with `make distclean', nor equipped with any Makefile dependencies.
Clearly it must have been written by someone who hasn't been accustomed to
working with GNU autotools.
The ultimate change is very small, but it has only been created gradually
with four commits in the upstream libffi repository, none of which applies
cleanly to ours and most of which include unrelated stuff. They are:
- commit 8308984e479e ("[PATCH] Make sure we're running dejagnu tests with
the right compiler."),
- commit 2d9b3939751b ("[PATCH] Fix for closures with sunpro compiler"),
- commit 0c3824702d3d ("[PATCH] Always set CC_FOR_TARGET for dejagnu, to
make the testsuite respect $CC"),
- commit 7d698125b1f0 ("[PATCH] Use the proper C++ compiler to run C++
tests") -- not yet needed in our libffi version as no tests are marked
C++.
-- at . I have now extracted the
relevant bits from the four commits and the result is below.
I have pushed it through my testing and it fixes the test results just
like my earlier proposal; in fact libffi.log files are the same modulo
timestamps and one number that is randomly generated. It is worth noting
however that the multilib discovery logic in `libffi-init' has not been
updated unlike with my proposal and it continues using the compiler
hardcoded within rather than one set with CC_FOR_TARGET/CXX_FOR_TARGET.
That uses a mechanism (*_FOR_TARGET, interpreted within `target_compile')
different from one we do (*_UNDER_TEST, used to set `compiler=' in the
invocation of `target_compile'), and ignores TOOL_EXECUTABLE altogether.
It makes sense however semantically to me for a standalone library test
suite to consider the compiler just a tool in testing and not the object
under test. Plus it makes it easy to define compilers for the various
languages required.
So I am in favour of retaining the mechanism rather than using my earlier
proposal, however I'm in two minds as to how to proceed. Integrating the
change as it is will make us having clutter left in the tree after `make
distclean', but we can do it right away. Fixing the problems with the
change upstream in libffi first and then merging the result back into our
tree will avoid getting the clutter, but will likely take time.
I'll sleep on it yet, and meanwhile I'll be happy to hear suggestions.
I have also cc-ed the libffi mailing list for a possible further insight.
Maciej
---
libffi/configure |5 +
libffi/configure.ac |5 +
libffi/testsuite/Makefile.am |2 ++
libffi/testsuite/Makefile.in |1 +
4 files changed, 13 insertions(+)
Index: gcc/libffi/configure
===
--- gcc.orig/libffi/configure
+++ gcc/libffi/configure
@@ -14961,6 +14961,11 @@ _ACEOF
+cat > local.exp <&5
$as_echo_n "checking whether to enable maintainer-specific portions of
Makefiles... " >&6; }
Index: gcc/libffi/configure.ac
===
--- gcc.orig/libffi/configure.ac
+++ gcc/libffi/configure.ac
@@ -61,6 +61,11 @@ AC_PROG_LIBTOOL
# Test for 64-bit build.
AC_CHECK_SIZEOF([size_t])
+cat > local.exp <
Re: [PATCH v3 2/4] libffi/test: Fix compilation for build sysroot
On Mar 17, 2020, at 2:52 PM, Maciej W. Rozycki wrote: > > On Fri, 28 Feb 2020, H.J. Lu wrote: >> >> Libffi 3.4 ABI was changed to support CET. But it isn't the first >> time ABI change for libffi, > > Have we made any conclusions WRT the way to move forward with this stuff? > Things remain broken and I'd prefer to get the issues off the plate while > the stuff is hot, or at least mildly warm. I'm about to get distracted > with other work. It's unfortunate that upstream has anything that prevents it from us just importing it all and calling it done. Anyway, if you see a path forward for grabbing all the Makefile/config stuff and leaving the abi changing stuff out, and just important that, we can do a partial import. I say this without reviewing the diffs from upstream and how many there are and what's in them. I'm hoping things are nicely segregated and reasonably small otherwise. That could unblock your concerns and you can leave the rest for some else that can play with the abi.
Re: [PATCH v3 2/4] libffi/test: Fix compilation for build sysroot
On Fri, 28 Feb 2020, H.J. Lu wrote: > > > > However that hack has been actually made to address this very problem > > > > discussed with this submission, so why not simply sync our copy of > > > > libffi > > > > with the upstream version? Then we can decide if changing the hack into > > > > something cleaner is worth the hassle. > > > > > > I'd love to sync with upstream libffi. In fact, I have done it on my > > > users/hjl/cet/master > > > branch: > > > > > > https://gitlab.com/x86-gcc/gcc/-/commit/9090e840b8464ce0f684e305eb75ff4655d05deb > > I think we'd like to update as well, but isn't there an ABI change in libffi > > that has to be fixed first? > > Libffi 3.4 ABI was changed to support CET. But it isn't the first > time ABI change for libffi, Have we made any conclusions WRT the way to move forward with this stuff? Things remain broken and I'd prefer to get the issues off the plate while the stuff is hot, or at least mildly warm. I'm about to get distracted with other work. Maciej
Re: [PATCH v3 2/4] libffi/test: Fix compilation for build sysroot
On Fri, Feb 28, 2020 at 1:16 PM Jeff Law wrote: > > On Fri, 2020-02-28 at 05:54 -0800, H.J. Lu wrote: > > On Fri, Feb 28, 2020 at 5:44 AM Maciej W. Rozycki wrote: > > > On Fri, 28 Feb 2020, H.J. Lu wrote: > > > > > > > > libffi/ > > > > > * configure.ac: Add testsuite/libffi-site-extra.exp to output > > > > > files. > > > > > * configure: Regenerate. > > > > > * testsuite/libffi-site-extra.exp.in: New file. > > > > > * testsuite/Makefile.am (EXTRA_DEJAGNU_SITE_CONFIG): New > > > > > variable. > > > > > * testsuite/Makefile.in: Regenerate. > > > > > * testsuite/lib/libffi.exp (libffi-init): Handle > > > > > GCC_UNDER_TEST. > > > > > (libffi_target_compile): Likewise. > > > > > > > > Upstream libffi has local.exp. Is that possible to use the same file? > > > > > > Thanks for the suggestion; I didn't realise we are so out of date WRT the > > > upstream version. > > > > > > I find the way local.exp has been wired in rather hackish as it makes the > > > template buried in `configure.ac' and also it requires running `autoconf' > > > whenever there is a need to change it. > > > > > > However that hack has been actually made to address this very problem > > > discussed with this submission, so why not simply sync our copy of libffi > > > with the upstream version? Then we can decide if changing the hack into > > > something cleaner is worth the hassle. > > > > I'd love to sync with upstream libffi. In fact, I have done it on my > > users/hjl/cet/master > > branch: > > > > https://gitlab.com/x86-gcc/gcc/-/commit/9090e840b8464ce0f684e305eb75ff4655d05deb > I think we'd like to update as well, but isn't there an ABI change in libffi > that has to be fixed first? Libffi 3.4 ABI was changed to support CET. But it isn't the first time ABI change for libffi, -- H.J.
Re: [PATCH v3 2/4] libffi/test: Fix compilation for build sysroot
On Fri, 2020-02-28 at 05:54 -0800, H.J. Lu wrote: > On Fri, Feb 28, 2020 at 5:44 AM Maciej W. Rozycki wrote: > > On Fri, 28 Feb 2020, H.J. Lu wrote: > > > > > > libffi/ > > > > * configure.ac: Add testsuite/libffi-site-extra.exp to output > > > > files. > > > > * configure: Regenerate. > > > > * testsuite/libffi-site-extra.exp.in: New file. > > > > * testsuite/Makefile.am (EXTRA_DEJAGNU_SITE_CONFIG): New > > > > variable. > > > > * testsuite/Makefile.in: Regenerate. > > > > * testsuite/lib/libffi.exp (libffi-init): Handle > > > > GCC_UNDER_TEST. > > > > (libffi_target_compile): Likewise. > > > > > > Upstream libffi has local.exp. Is that possible to use the same file? > > > > Thanks for the suggestion; I didn't realise we are so out of date WRT the > > upstream version. > > > > I find the way local.exp has been wired in rather hackish as it makes the > > template buried in `configure.ac' and also it requires running `autoconf' > > whenever there is a need to change it. > > > > However that hack has been actually made to address this very problem > > discussed with this submission, so why not simply sync our copy of libffi > > with the upstream version? Then we can decide if changing the hack into > > something cleaner is worth the hassle. > > I'd love to sync with upstream libffi. In fact, I have done it on my > users/hjl/cet/master > branch: > > https://gitlab.com/x86-gcc/gcc/-/commit/9090e840b8464ce0f684e305eb75ff4655d05deb I think we'd like to update as well, but isn't there an ABI change in libffi that has to be fixed first? jeff
Re: [PATCH v3 2/4] libffi/test: Fix compilation for build sysroot
On Fri, Feb 28, 2020 at 5:44 AM Maciej W. Rozycki wrote: > > On Fri, 28 Feb 2020, H.J. Lu wrote: > > > > libffi/ > > > * configure.ac: Add testsuite/libffi-site-extra.exp to output > > > files. > > > * configure: Regenerate. > > > * testsuite/libffi-site-extra.exp.in: New file. > > > * testsuite/Makefile.am (EXTRA_DEJAGNU_SITE_CONFIG): New > > > variable. > > > * testsuite/Makefile.in: Regenerate. > > > * testsuite/lib/libffi.exp (libffi-init): Handle GCC_UNDER_TEST. > > > (libffi_target_compile): Likewise. > > > > Upstream libffi has local.exp. Is that possible to use the same file? > > Thanks for the suggestion; I didn't realise we are so out of date WRT the > upstream version. > > I find the way local.exp has been wired in rather hackish as it makes the > template buried in `configure.ac' and also it requires running `autoconf' > whenever there is a need to change it. > > However that hack has been actually made to address this very problem > discussed with this submission, so why not simply sync our copy of libffi > with the upstream version? Then we can decide if changing the hack into > something cleaner is worth the hassle. I'd love to sync with upstream libffi. In fact, I have done it on my users/hjl/cet/master branch: https://gitlab.com/x86-gcc/gcc/-/commit/9090e840b8464ce0f684e305eb75ff4655d05deb -- H.J.
Re: [PATCH v3 2/4] libffi/test: Fix compilation for build sysroot
On Fri, 28 Feb 2020, H.J. Lu wrote: > > libffi/ > > * configure.ac: Add testsuite/libffi-site-extra.exp to output > > files. > > * configure: Regenerate. > > * testsuite/libffi-site-extra.exp.in: New file. > > * testsuite/Makefile.am (EXTRA_DEJAGNU_SITE_CONFIG): New > > variable. > > * testsuite/Makefile.in: Regenerate. > > * testsuite/lib/libffi.exp (libffi-init): Handle GCC_UNDER_TEST. > > (libffi_target_compile): Likewise. > > Upstream libffi has local.exp. Is that possible to use the same file? Thanks for the suggestion; I didn't realise we are so out of date WRT the upstream version. I find the way local.exp has been wired in rather hackish as it makes the template buried in `configure.ac' and also it requires running `autoconf' whenever there is a need to change it. However that hack has been actually made to address this very problem discussed with this submission, so why not simply sync our copy of libffi with the upstream version? Then we can decide if changing the hack into something cleaner is worth the hassle. Maciej
Re: [PATCH v3 2/4] libffi/test: Fix compilation for build sysroot
On Thu, Feb 27, 2020 at 5:18 PM Maciej W. Rozycki wrote: > > Fix a problem with the libffi testsuite using a method to determine the > compiler to use resulting in the tool being different from one the > library has been built with, and causing a catastrophic failure from the > inability to actually choose any compiler at all in a cross-compilation > configuration. > > Address this problem by providing a DejaGNU configuration file defining > the compiler to use, via the GCC_UNDER_TEST TCL variable, set from $CC > by autoconf, which will have all the required options set for the target > compiler to build executables in the environment configured, removing > failures like: > > FAIL: libffi.call/closure_fn0.c -W -Wall -Wno-psabi -O0 (test for excess > errors) > Excess errors: > default_target_compile: No compiler to compile with > UNRESOLVED: libffi.call/closure_fn0.c -W -Wall -Wno-psabi -O0 compilation > failed to produce executable > > and bringing overall test results for the `riscv64-linux-gnu' target > (here with the `x86_64-linux-gnu' host and RISC-V QEMU in the Linux user > emulation mode as the target board) from: > > === libffi Summary === > > # of unexpected failures708 > # of unresolved testcases 708 > # of unsupported tests 30 > > to: > > === libffi Summary === > > # of expected passes1934 > # of unsupported tests 28 > > Also respect the TOOL_EXECUTABLE TCL variable for a standalone run via > `runtest' and remove an unused TOOL_OPTIONS TCL variable instance. > > libffi/ > * configure.ac: Add testsuite/libffi-site-extra.exp to output > files. > * configure: Regenerate. > * testsuite/libffi-site-extra.exp.in: New file. > * testsuite/Makefile.am (EXTRA_DEJAGNU_SITE_CONFIG): New > variable. > * testsuite/Makefile.in: Regenerate. > * testsuite/lib/libffi.exp (libffi-init): Handle GCC_UNDER_TEST. > (libffi_target_compile): Likewise. Upstream libffi has local.exp. Is that possible to use the same file? -- H.J.
[PATCH v3 2/4] libffi/test: Fix compilation for build sysroot
Fix a problem with the libffi testsuite using a method to determine the compiler to use resulting in the tool being different from one the library has been built with, and causing a catastrophic failure from the inability to actually choose any compiler at all in a cross-compilation configuration. Address this problem by providing a DejaGNU configuration file defining the compiler to use, via the GCC_UNDER_TEST TCL variable, set from $CC by autoconf, which will have all the required options set for the target compiler to build executables in the environment configured, removing failures like: FAIL: libffi.call/closure_fn0.c -W -Wall -Wno-psabi -O0 (test for excess errors) Excess errors: default_target_compile: No compiler to compile with UNRESOLVED: libffi.call/closure_fn0.c -W -Wall -Wno-psabi -O0 compilation failed to produce executable and bringing overall test results for the `riscv64-linux-gnu' target (here with the `x86_64-linux-gnu' host and RISC-V QEMU in the Linux user emulation mode as the target board) from: === libffi Summary === # of unexpected failures708 # of unresolved testcases 708 # of unsupported tests 30 to: === libffi Summary === # of expected passes1934 # of unsupported tests 28 Also respect the TOOL_EXECUTABLE TCL variable for a standalone run via `runtest' and remove an unused TOOL_OPTIONS TCL variable instance. libffi/ * configure.ac: Add testsuite/libffi-site-extra.exp to output files. * configure: Regenerate. * testsuite/libffi-site-extra.exp.in: New file. * testsuite/Makefile.am (EXTRA_DEJAGNU_SITE_CONFIG): New variable. * testsuite/Makefile.in: Regenerate. * testsuite/lib/libffi.exp (libffi-init): Handle GCC_UNDER_TEST. (libffi_target_compile): Likewise. --- Changes from v2: - Revert to v1. - Rename testsuite/libffi-test-support.exp.in to testsuite/libffi-site-extra.exp.in. Changes from v1: - Remove testsuite/libffi-test-support.exp.in and the associated changes. - Pass $(CC) via `--tool_exec' in $(AM_RUNTESTFLAGS). --- libffi/configure |3 ++- libffi/configure.ac |2 +- libffi/testsuite/Makefile.am |2 ++ libffi/testsuite/Makefile.in |5 - libffi/testsuite/lib/libffi.exp | 16 ++-- libffi/testsuite/libffi-site-extra.exp.in |1 + 6 files changed, 24 insertions(+), 5 deletions(-) gcc-test-libffi-gcc-under-test.diff Index: gcc/libffi/configure === --- gcc.orig/libffi/configure +++ gcc/libffi/configure @@ -16662,7 +16662,7 @@ ac_config_commands="$ac_config_commands ac_config_links="$ac_config_links include/ffitarget.h:src/$TARGETDIR/ffitarget.h" -ac_config_files="$ac_config_files include/Makefile include/ffi.h Makefile testsuite/Makefile man/Makefile libffi.pc" +ac_config_files="$ac_config_files include/Makefile include/ffi.h Makefile testsuite/Makefile testsuite/libffi-site-extra.exp man/Makefile libffi.pc" cat >confcache <<\_ACEOF @@ -17829,6 +17829,7 @@ do "include/ffi.h") CONFIG_FILES="$CONFIG_FILES include/ffi.h" ;; "Makefile") CONFIG_FILES="$CONFIG_FILES Makefile" ;; "testsuite/Makefile") CONFIG_FILES="$CONFIG_FILES testsuite/Makefile" ;; +"testsuite/libffi-site-extra.exp") CONFIG_FILES="$CONFIG_FILES testsuite/libffi-site-extra.exp" ;; "man/Makefile") CONFIG_FILES="$CONFIG_FILES man/Makefile" ;; "libffi.pc") CONFIG_FILES="$CONFIG_FILES libffi.pc" ;; Index: gcc/libffi/configure.ac === --- gcc.orig/libffi/configure.ac +++ gcc/libffi/configure.ac @@ -377,6 +377,6 @@ test -d src/$TARGETDIR || mkdir src/$TAR AC_CONFIG_LINKS(include/ffitarget.h:src/$TARGETDIR/ffitarget.h) -AC_CONFIG_FILES(include/Makefile include/ffi.h Makefile testsuite/Makefile man/Makefile libffi.pc) +AC_CONFIG_FILES(include/Makefile include/ffi.h Makefile testsuite/Makefile testsuite/libffi-site-extra.exp man/Makefile libffi.pc) AC_OUTPUT Index: gcc/libffi/testsuite/Makefile.am === --- gcc.orig/libffi/testsuite/Makefile.am +++ gcc/libffi/testsuite/Makefile.am @@ -11,6 +11,8 @@ RUNTEST = `if [ -f $(top_srcdir)/../deja echo $(top_srcdir)/../dejagnu/runtest ; \ else echo runtest; fi` +EXTRA_DEJAGNU_SITE_CONFIG = libffi-site-extra.exp + AM_RUNTESTFLAGS = CLEANFILES = *.exe core* *.log *.sum Index: gcc/libffi/testsuite/Makefile.in === --- gcc.orig/libffi/testsuite/Makefile.in +++ gcc/libffi/testsuite/Makefile.in @@ -106,7 +106,7 @@ am__configure_deps = $(am__aclocal_m4_de DIST_COMMON = $(srcdir)/Makefile.am mkinstalldirs = $(SHELL) $(top_srcdir)/../mkinstalldirs CONFIG
