jit: fix link on OS X and Solaris (PR jit/64089 and PR jit/84288)

Message ID 1518556781-10049-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • jit: fix link on OS X and Solaris (PR jit/64089 and PR jit/84288)
Related show

Commit Message

David Malcolm Feb. 13, 2018, 9:19 p.m.
libgccjit fails to link on OS X and Solaris due to jit/Make-lang.in,
due to the assumption there that the linker is GNU ld.  Specifically,
jit/Make-lang.in hardcodes the use of two options: --version-script
and -soname.

* on Darwin, --version-script doesn't seem to exist in the linker, and
  it uses -install_name rather than -soname.

* on Solaris, ld doesn't support --version-script.  However, the version
  script used for libgccjit.so doesn't use any gld extensions, so one can
  just use -M instead.

This patch fixes these issues by using variables emitted by gcc's "configure"
rather than hardcoding the options in jit/Make-lang.in.

It's based on the first part of Rainer's patch for PR jit/84288, but I
made the following changes:
* the GNU ld case in configure.ac wasn't setting ld_version_script_option.
  I set it to "--version-script" for that case.
* I moved the:
    LD_VERSION_SCRIPT_OPTION = @ld_version_script_option@
  from gcc/jit/Make-lang.in to gcc/Makefile.in, as the Make-lang.in files
  aren't substituted, only the gcc/Makefile.in.
  Rainer: how did this work for you?
* added LD_SONAME_OPTION, done in the same way
* conditionalized the usage of the options in Make-lang.in to cope with
  empty LD_VERSION_SCRIPT_OPTION (as is presumably the case on OS X).
  I used ($if condition,then-part[,else-part]) for this.
  I had to add a $(COMMA) since the "then-part" contains commas, which
  need to be treated as part of the "then-part", rather than separators
  for the "else-part".
  Hopefully this is compatible with every "make" implementation that we
  support.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.

I lightly tested the not-recognized case by hacking up the configure.ac
(on x86_64-pc-linux-gnu) and verifying that it links, and that a
smoketest of jit.dg/test-factorial works.

Does this fix the jit linker issues on OS X and Solaris?
(I didn't include the autogenerate configure changes)

gcc/ChangeLog:
	PR jit/64089
	PR jit/84288
	* Makefile.in (LD_VERSION_SCRIPT_OPTION, LD_SONAME_OPTION): New.
	* configure: Regenerate.
	* configure.ac ("linker --version-script option"): New.
	("linker soname option"): New.

gcc/jit/ChangeLog:
	PR jit/64089
	PR jit/84288
	* Make-lang.in (COMMA): New.
	(LIBGCCJIT_VERSION_SCRIPT_OPTION): New.
	(LIBGCCJIT_SONAME_OPTION): New.
	(jit): Move --version-script and -soname linker options to the
	above.
---
 gcc/Makefile.in      |  4 ++++
 gcc/configure.ac     | 38 ++++++++++++++++++++++++++++++++++++++
 gcc/jit/Make-lang.in | 17 +++++++++++++++--
 3 files changed, 57 insertions(+), 2 deletions(-)

Comments

Rainer Orth Feb. 13, 2018, 9:30 p.m. | #1
Hi David,

> libgccjit fails to link on OS X and Solaris due to jit/Make-lang.in,
> due to the assumption there that the linker is GNU ld.  Specifically,
> jit/Make-lang.in hardcodes the use of two options: --version-script
> and -soname.
>
> * on Darwin, --version-script doesn't seem to exist in the linker, and
>   it uses -install_name rather than -soname.
>
> * on Solaris, ld doesn't support --version-script.  However, the version
>   script used for libgccjit.so doesn't use any gld extensions, so one can
>   just use -M instead.
>
> This patch fixes these issues by using variables emitted by gcc's "configure"
> rather than hardcoding the options in jit/Make-lang.in.
>
> It's based on the first part of Rainer's patch for PR jit/84288, but I
> made the following changes:
> * the GNU ld case in configure.ac wasn't setting ld_version_script_option.
>   I set it to "--version-script" for that case.

That's weird: the configure.ac part starts with

ld_version_script_option='--version-script'

only overriding it in the Solaris case to keep things for other hosts
as they were.  Besides, I'm pretty sure I tested the patch on Solaris
with gas/gld to make certain that combo continues to work as is...

> * I moved the:
>     LD_VERSION_SCRIPT_OPTION = @ld_version_script_option@
>   from gcc/jit/Make-lang.in to gcc/Makefile.in, as the Make-lang.in files
>   aren't substituted, only the gcc/Makefile.in.
>   Rainer: how did this work for you?

It didn't: what I'd attached to the PR was an initial version of the
patch in the middle between manually hacking gcc/jit/Make-lang.in and
properly autoconfiguring things.

> * added LD_SONAME_OPTION, done in the same way
> * conditionalized the usage of the options in Make-lang.in to cope with
>   empty LD_VERSION_SCRIPT_OPTION (as is presumably the case on OS X).
>   I used ($if condition,then-part[,else-part]) for this.
>   I had to add a $(COMMA) since the "then-part" contains commas, which
>   need to be treated as part of the "then-part", rather than separators
>   for the "else-part".
>   Hopefully this is compatible with every "make" implementation that we
>   support.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu.
>
> I lightly tested the not-recognized case by hacking up the configure.ac
> (on x86_64-pc-linux-gnu) and verifying that it links, and that a
> smoketest of jit.dg/test-factorial works.
>
> Does this fix the jit linker issues on OS X and Solaris?

I'll give it a whirl tomorrow, including the jit-recording.c part of my
patch to allow the build to complete.

Thanks.
        Rainer
Rainer Orth Feb. 14, 2018, 1:36 p.m. | #2
Hi David,

>> * added LD_SONAME_OPTION, done in the same way
[...]
>> Does this fix the jit linker issues on OS X and Solaris?
>
> I'll give it a whirl tomorrow, including the jit-recording.c part of my
> patch to allow the build to complete.

actually, I've replaced the Makefile and configure parts of my patch
with yours and did a jit-only bootstrap on i386-pc-solaris2.11 with
as/ld and gas/ld.  Both went fine with a minor caveat: I noticed that
LD_SONAME_OPTION wasn't set in gcc/Makefile.  Fixed with the following
(so far untested) snippet:
diff --git a/gcc/configure.ac b/gcc/configure.ac
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -3715,6 +3715,12 @@ elif test x$gcc_cv_ld != x; then
       gcc_cv_ld_soname=yes
       ld_soname_option='-install_name'
       ;;
+    # Solaris 2 ld always supports -h.  It also supports --soname for GNU
+    # ld compatiblity since some Solaris 10 update.
+    *-*-solaris2*)
+      gcc_cv_ld_soname=yes
+      ld_soname_option='-h'
+      ;;
   esac
 fi
 # Don't AC_DEFINE result, only used in jit/Make-lang.in so far.
I've also checked that the original Solaris 10 release didn't support ld
-soname, so it's safer to always use the Solaris-native -h option
instead.

	Rainer
FX Feb. 14, 2018, 5:28 p.m. | #3
> Does this fix the jit linker issues on OS X and Solaris?

The patch fails to bootstrap on x86_64-apple-darwin17. gcc/config.log says:

gcc_cv_ld_version_script=no
ld_version_script_option='--version-script’

gcc/Makefile says:

LD_VERSION_SCRIPT_OPTION = --version-script
LD_SONAME_OPTION = -install_name

which makes it try to link with:

	      -Wl,--version-script,../../trunk/gcc/jit/libgccjit.map \
	      -Wl,-install_name,libgccjit.so.0

which fails with: ld: unknown option: --version-script

I think the patch to gcc/configure.ac should be:

+AC_MSG_CHECKING(linker --version-script option)
+gcc_cv_ld_version_script=no
+ld_version_script_option=''


FX
FX Feb. 14, 2018, 10:36 p.m. | #4
I can confirm that, with the attached revised patch, a bootstrap with --enable-languages=c,c++,jit --enable-host-shared is successful on macOS.

FX
FX March 6, 2018, 2:23 p.m. | #5
Hi David,

Any news on that patch?

Cheers,
FX
David Malcolm March 9, 2018, 2:14 p.m. | #6
On Wed, 2018-02-14 at 23:36 +0100, FX wrote:
> I can confirm that, with the attached revised patch, a bootstrap with
> --enable-languages=c,c++,jit --enable-host-shared is successful on
> macOS.
> 
> FX

Looks good to me; thanks for fixing.


Release managers:

I'd like to apply FX's patch here:
  https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00881/patch
to trunk, to fix the build of jit on OS X, and to make it easier to fix
it on Solaris.

This involves touching gcc/configure.ac (and configure).

I've successfully bootstrapped and regression-tested it on x86_64-pc-
linux-gnu.  FX reports above that it fixes the build on macOS, and
Rainer has an (untested) patch on top of it that ought to fix the build
on Solaris:
  https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00835.html

We're in stage 4, and the two bugs in question:
  PR jit/64089 ("libgccjit.so.0.0.1 linkage failure on darwin")
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64089

  PR jit/84288 ("Support jit on Solaris")
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84288
aren't regressions.

However, I believe this is a low-risk patch, and is mostly confined to
jit (and those targets).

Is it OK for trunk now, or does this need to wait until next stage 1?

Thanks
Dave
Jakub Jelinek March 9, 2018, 2:17 p.m. | #7
On Fri, Mar 09, 2018 at 09:14:13AM -0500, David Malcolm wrote:
> On Wed, 2018-02-14 at 23:36 +0100, FX wrote:
> > I can confirm that, with the attached revised patch, a bootstrap with
> > --enable-languages=c,c++,jit --enable-host-shared is successful on
> > macOS.
> > 
> > FX
> 
> Looks good to me; thanks for fixing.
> 
> 
> Release managers:
> 
> I'd like to apply FX's patch here:
>   https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00881/patch
> to trunk, to fix the build of jit on OS X, and to make it easier to fix
> it on Solaris.
> 
> Is it OK for trunk now, or does this need to wait until next stage 1?

Ok for trunk now.

	Jakub
Rainer Orth March 20, 2018, 8:38 a.m. | #8
Hi David,

> Release managers:
>
> I'd like to apply FX's patch here:
>   https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00881/patch
> to trunk, to fix the build of jit on OS X, and to make it easier to fix
> it on Solaris.
>
> This involves touching gcc/configure.ac (and configure).
>
> I've successfully bootstrapped and regression-tested it on x86_64-pc-
> linux-gnu.  FX reports above that it fixes the build on macOS, and
> Rainer has an (untested) patch on top of it that ought to fix the build
> on Solaris:
>   https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00835.html

I've now tested the patch (together with the one from PR jit/84288 for
several remaining issues).  I've needed another snippet for
Solaris/SPARC which links libkstat into xgcc and needs it in
libgccjit.so, too.  Bootstrapped without regressions on
i386-pc-solaris2.11 and sparc-sun-solaris2.11.

Ok for mainline?

	Rainer
David Malcolm March 20, 2018, 7:43 p.m. | #9
On Tue, 2018-03-20 at 09:38 +0100, Rainer Orth wrote:
> Hi David,
> 
> > Release managers:
> > 
> > I'd like to apply FX's patch here:
> >   https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00881/patch
> > to trunk, to fix the build of jit on OS X, and to make it easier to
> > fix
> > it on Solaris.
> > 
> > This involves touching gcc/configure.ac (and configure).
> > 
> > I've successfully bootstrapped and regression-tested it on x86_64-
> > pc-
> > linux-gnu.  FX reports above that it fixes the build on macOS, and
> > Rainer has an (untested) patch on top of it that ought to fix the
> > build
> > on Solaris:
> >   https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00835.html
> 
> I've now tested the patch (together with the one from PR jit/84288
> for
> several remaining issues).  I've needed another snippet for
> Solaris/SPARC which links libkstat into xgcc and needs it in
> libgccjit.so, too.  Bootstrapped without regressions on
> i386-pc-solaris2.11 and sparc-sun-solaris2.11.

FWIW I've successfully tested this on x86_64-pc-linux-gnu (regenerating
the gcc/configure), and, as jit maintainer, it looks good to me [1],
though it may still need RM approval given stage 4.

Dave

[1] ...though I have a slight preference for listing
$(EXTRA_GCC_LIBS) on the same line as $(EXTRA_GCC_OBJS) in jit/Make-
lang.in, so that these two items needed to embed the driver code into
the libgccjit shared library are visually grouped together.

> Ok for mainline?
> 
> 	Rainer
>
Rainer Orth March 20, 2018, 11:39 p.m. | #10
Hi Malcolm,

>> I've now tested the patch (together with the one from PR jit/84288
>> for
>> several remaining issues).  I've needed another snippet for
>> Solaris/SPARC which links libkstat into xgcc and needs it in
>> libgccjit.so, too.  Bootstrapped without regressions on
>> i386-pc-solaris2.11 and sparc-sun-solaris2.11.
>
> FWIW I've successfully tested this on x86_64-pc-linux-gnu (regenerating
> the gcc/configure), and, as jit maintainer, it looks good to me [1],
> though it may still need RM approval given stage 4.

thanks for trying this.

> [1] ...though I have a slight preference for listing
> $(EXTRA_GCC_LIBS) on the same line as $(EXTRA_GCC_OBJS) in jit/Make-
> lang.in, so that these two items needed to embed the driver code into
> the libgccjit shared library are visually grouped together.

I've selected the location of $(EXTRA_GCC_LIBS) in the link line to
match what gcc/Makefile.in does for xgcc etc.

	Rainer
David Malcolm March 20, 2018, 11:43 p.m. | #11
On Wed, 2018-03-21 at 00:39 +0100, Rainer Orth wrote:
> Hi Malcolm,
> 
> > > I've now tested the patch (together with the one from PR
> > > jit/84288
> > > for
> > > several remaining issues).  I've needed another snippet for
> > > Solaris/SPARC which links libkstat into xgcc and needs it in
> > > libgccjit.so, too.  Bootstrapped without regressions on
> > > i386-pc-solaris2.11 and sparc-sun-solaris2.11.
> > 
> > FWIW I've successfully tested this on x86_64-pc-linux-gnu
> > (regenerating
> > the gcc/configure), and, as jit maintainer, it looks good to me
> > [1],
> > though it may still need RM approval given stage 4.
> 
> thanks for trying this.
> 
> > [1] ...though I have a slight preference for listing
> > $(EXTRA_GCC_LIBS) on the same line as $(EXTRA_GCC_OBJS) in
> > jit/Make-
> > lang.in, so that these two items needed to embed the driver code
> > into
> > the libgccjit shared library are visually grouped together.
> 
> I've selected the location of $(EXTRA_GCC_LIBS) in the link line to
> match what gcc/Makefile.in does for xgcc etc.

Indeed, I don't want to bikeshed it - I care much more about whether it
works ;)
Richard Biener March 21, 2018, 8:21 a.m. | #12
On Tue, 20 Mar 2018, David Malcolm wrote:

> On Wed, 2018-03-21 at 00:39 +0100, Rainer Orth wrote:
> > Hi Malcolm,
> > 
> > > > I've now tested the patch (together with the one from PR
> > > > jit/84288
> > > > for
> > > > several remaining issues).  I've needed another snippet for
> > > > Solaris/SPARC which links libkstat into xgcc and needs it in
> > > > libgccjit.so, too.  Bootstrapped without regressions on
> > > > i386-pc-solaris2.11 and sparc-sun-solaris2.11.
> > > 
> > > FWIW I've successfully tested this on x86_64-pc-linux-gnu
> > > (regenerating
> > > the gcc/configure), and, as jit maintainer, it looks good to me
> > > [1],
> > > though it may still need RM approval given stage 4.
> > 
> > thanks for trying this.
> > 
> > > [1] ...though I have a slight preference for listing
> > > $(EXTRA_GCC_LIBS) on the same line as $(EXTRA_GCC_OBJS) in
> > > jit/Make-
> > > lang.in, so that these two items needed to embed the driver code
> > > into
> > > the libgccjit shared library are visually grouped together.
> > 
> > I've selected the location of $(EXTRA_GCC_LIBS) in the link line to
> > match what gcc/Makefile.in does for xgcc etc.
> 
> Indeed, I don't want to bikeshed it - I care much more about whether it
> works ;)

I'm fine with this patch.

Richard.

Patch

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index 6c37e46..903da58 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1116,6 +1116,10 @@  endif
 
 LANG_MAKEFRAGS = @all_lang_makefrags@
 
+# Used by gcc/jit/Make-lang.in
+LD_VERSION_SCRIPT_OPTION = @ld_version_script_option@
+LD_SONAME_OPTION = @ld_soname_option@
+
 # Flags to pass to recursive makes.
 # CC is set by configure.
 # ??? The choices here will need some experimenting with.
diff --git a/gcc/configure.ac b/gcc/configure.ac
index b7f9728..265920c 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -3640,6 +3640,44 @@  if test x"$gcc_cv_ld_static_dynamic" = xyes; then
 fi
 AC_MSG_RESULT($gcc_cv_ld_static_dynamic)
 
+AC_MSG_CHECKING(linker --version-script option)
+gcc_cv_ld_version_script=no
+ld_version_script_option='--version-script'
+if test $in_tree_ld = yes || test x"$gnu_ld" = xyes; then
+  gcc_cv_ld_version_script=yes
+  ld_version_script_option='--version-script'
+elif test x$gcc_cv_ld != x; then
+  case "$target" in
+    # Solaris 2 ld always supports -M.  It also supports a subset of
+    # --version-script since Solaris 11.4, but requires
+    # -z gnu-version-script-compat to activate.
+    *-*-solaris2*)
+      gcc_cv_ld_version_script=yes
+      ld_version_script_option='-M'
+      ;;
+  esac
+fi
+# Don't AC_DEFINE result, only used in jit/Make-lang.in so far.
+AC_MSG_RESULT($gcc_cv_ld_version_script)
+AC_SUBST(ld_version_script_option)
+
+AC_MSG_CHECKING(linker soname option)
+gcc_cv_ld_soname=no
+if test $in_tree_ld = yes || test x"$gnu_ld" = xyes; then
+  gcc_cv_ld_soname=yes
+  ld_soname_option='-soname'
+elif test x$gcc_cv_ld != x; then
+  case "$target" in
+    *-*-darwin*)
+      gcc_cv_ld_soname=yes
+      ld_soname_option='-install_name'
+      ;;
+  esac
+fi
+# Don't AC_DEFINE result, only used in jit/Make-lang.in so far.
+AC_MSG_RESULT($gcc_cv_ld_soname)
+AC_SUBST(ld_soname_option)
+
 if test x"$demangler_in_ld" = xyes; then
   AC_MSG_CHECKING(linker --demangle support)
   gcc_cv_ld_demangle=no
diff --git a/gcc/jit/Make-lang.in b/gcc/jit/Make-lang.in
index d4362a9..ba78f8e 100644
--- a/gcc/jit/Make-lang.in
+++ b/gcc/jit/Make-lang.in
@@ -51,6 +51,19 @@  LIBGCCJIT_FILENAME = \
 LIBGCCJIT_LINKER_NAME_SYMLINK = $(LIBGCCJIT_LINKER_NAME)
 LIBGCCJIT_SONAME_SYMLINK = $(LIBGCCJIT_SONAME)
 
+# Conditionalize the use of the LD_VERSION_SCRIPT_OPTION and
+# LD_SONAME_OPTION depending if configure found them, using $(if)
+# We have to define a COMMA here, otherwise the commas in the "true"
+# result are treated as separators by the $(if).
+COMMA := ,
+LIBGCCJIT_VERSION_SCRIPT_OPTION = \
+	$(if $(LD_VERSION_SCRIPT_OPTION),\
+	  -Wl$(COMMA)$(LD_VERSION_SCRIPT_OPTION)$(COMMA)$(srcdir)/jit/libgccjit.map)
+
+LIBGCCJIT_SONAME_OPTION = \
+	$(if $(LD_SONAME_OPTION), \
+	     -Wl$(COMMA)$(LD_SONAME_OPTION)$(COMMA)$(LIBGCCJIT_SONAME))
+
 jit: $(LIBGCCJIT_FILENAME) \
 	$(LIBGCCJIT_SYMLINK) \
 	$(LIBGCCJIT_LINKER_NAME_SYMLINK) \
@@ -85,8 +98,8 @@  $(LIBGCCJIT_FILENAME): $(jit_OBJS) \
 	     $(jit_OBJS) libbackend.a libcommon-target.a libcommon.a \
 	     $(CPPLIB) $(LIBDECNUMBER) $(LIBS) $(BACKENDLIBS) \
 	     $(EXTRA_GCC_OBJS) \
-	     -Wl,--version-script=$(srcdir)/jit/libgccjit.map \
-	     -Wl,-soname,$(LIBGCCJIT_SONAME)
+	     $(LIBGCCJIT_VERSION_SCRIPT_OPTION) \
+	     $(LIBGCCJIT_SONAME_OPTION)
 
 $(LIBGCCJIT_SONAME_SYMLINK): $(LIBGCCJIT_FILENAME)
 	ln -sf $(LIBGCCJIT_FILENAME) $(LIBGCCJIT_SONAME_SYMLINK)