diff mbox

[Fortran,Build] PR driver/46516: Add -lquadmath in the FE not as libgfortran.spec file

Message ID 4CE4FE82.7080305@net-b.de
State New
Headers show

Commit Message

Tobias Burnus Nov. 18, 2010, 10:22 a.m. UTC 
This patch reverts the linking behaviour to the very old patch:

The front end adds the -lquadmath (in fortran/gfortranspec.c) rather 
than generating libgfortran.spec and placing it along the 
libgfortran.{a,so,*} file, which causes all kinds of errors.

The automatic addition of -lquadmath is guarded by LIBGCC2_HAS_TF_MODE, 
which should be a sufficiently safe check.

Build and tested on x86-64-linux.
OK for the trunk?

(Actually, I do not know whose approval I need. Probably "Fortran" with 
libgomp being trivial/obvious; however, I would not mind to see comments 
from Driver or Build.)

Tobias

Comments

Jakub Jelinek Nov. 18, 2010, 10:31 a.m. UTC | #1 
On Thu, Nov 18, 2010 at 11:22:58AM +0100, Tobias Burnus wrote:
> This patch reverts the linking behaviour to the very old patch:
> 
> The front end adds the -lquadmath (in fortran/gfortranspec.c) rather
> than generating libgfortran.spec and placing it along the
> libgfortran.{a,so,*} file, which causes all kinds of errors.
> 
> The automatic addition of -lquadmath is guarded by
> LIBGCC2_HAS_TF_MODE, which should be a sufficiently safe check.

The macro looks misnamed to me, it is something only i?86/x86_64
define, which is good for libquadmath, but there are many other
targets that have TF mode, but as long double rather than __float128,
and those won't have libquadmath (talking about sparc*/powerpc*/s390* etc.).

	Jakub
Tobias Burnus Nov. 18, 2010, 11:05 a.m. UTC | #2 
On 11/18/2010 11:31 AM, Jakub Jelinek wrote:
> On Thu, Nov 18, 2010 at 11:22:58AM +0100, Tobias Burnus wrote:
>> The automatic addition of -lquadmath is guarded by
>> LIBGCC2_HAS_TF_MODE, which should be a sufficiently safe check.
> The macro looks misnamed to me, it is something only i?86/x86_64
> define,
and ia64

> which is good for libquadmath, but there are many other
> targets that have TF mode, but as long double rather than __float128,
> and those won't have libquadmath (talking about sparc*/powerpc*/s390* etc.).

I agree that it is misnamed - and that it is confusing. (The need for a 
comment why one cannot use LIBGCC2_HAS_TF_MODE directly already 
indicates that the macro is not well named, cf. gcc/config/dfp-bit.h.)

I terms of the patch, one could make it more explicit by using:

   #if LONG_DOUBLE_HAS_TF_MODE \
&& LIBGCC2_LONG_DOUBLE_TYPE_SIZE != 128

However, it then seems to make more sense to rename the macro ...

Tobias
Jakub Jelinek Nov. 18, 2010, 11:14 a.m. UTC | #3 
On Thu, Nov 18, 2010 at 12:05:15PM +0100, Tobias Burnus wrote:
> On 11/18/2010 11:31 AM, Jakub Jelinek wrote:
> >On Thu, Nov 18, 2010 at 11:22:58AM +0100, Tobias Burnus wrote:
> >>The automatic addition of -lquadmath is guarded by
> >>LIBGCC2_HAS_TF_MODE, which should be a sufficiently safe check.
> >The macro looks misnamed to me, it is something only i?86/x86_64
> >define,
> and ia64
> 
> >which is good for libquadmath, but there are many other
> >targets that have TF mode, but as long double rather than __float128,
> >and those won't have libquadmath (talking about sparc*/powerpc*/s390* etc.).
> 
> I agree that it is misnamed - and that it is confusing. (The need
> for a comment why one cannot use LIBGCC2_HAS_TF_MODE directly
> already indicates that the macro is not well named, cf.
> gcc/config/dfp-bit.h.)
> 
> I terms of the patch, one could make it more explicit by using:
> 
>   #if LONG_DOUBLE_HAS_TF_MODE \
> && LIBGCC2_LONG_DOUBLE_TYPE_SIZE != 128

Well, you can'd do that in the driver.
LIBGCC2_* macros are really meant for use in libgcc code only (could be
in other target libraries, but definitely not in the driver).
The thing is that e.g. on alpha/sparc*/powerpc* we have
/* Define this to set long double type size to use in libgcc2.c, which can
   not depend on target_flags.  */
#ifdef __LONG_DOUBLE_128__
#define LIBGCC2_LONG_DOUBLE_TYPE_SIZE 128
#else
#define LIBGCC2_LONG_DOUBLE_TYPE_SIZE 64
#endif

and __LONG_DOUBLE_128__ is a macro defined by the compiler depending on
the compiler switches.  If you use it in the driver, it will depend on
whatever macro has been defined in the host compiler (if any).
LIBGCC2_HAS_TF_MODE currently happens to work because it is only
defined to 1 on the targets you care about and not defined at all elsewhere,
but e.g. as soon as the support would be dependent on compiler switches on
some target this wouldn't work anymore.

	Jakub
Tobias Burnus Nov. 18, 2010, 11:28 a.m. UTC | #4 
On 11/18/2010 12:14 PM, Jakub Jelinek wrote:
>>    #if LONG_DOUBLE_HAS_TF_MODE \
>> &&  LIBGCC2_LONG_DOUBLE_TYPE_SIZE != 128
> Well, you can'd do that in the driver. [...] If you use it in
> the driver, it will depend on whatever macro has been defined
> in the host compiler (if any).
> LIBGCC2_HAS_TF_MODE currently happens to work because it is only
> defined to 1 on the targets you care about and not defined at all elsewhere,
> but e.g. as soon as the support would be dependent on compiler switches on
> some target this wouldn't work anymore.

OK. Do you have then a suggestion how to handle this?

A spec file makes a lot of trouble (in terms of finding the correct 
one). And in particular, gcc/gcc.c loads first the FE specific .spec 
file before it sets up the multilib path. That causes the wrong 
libgfortran.spec to be loaded - or on systems without multilib and 
unusual lib directory (such as most x86-64-linux systems), it completely 
fails to load the file. I have not yet debugged the issues with multilib 
testing, which were recently reported on gcc@. Changing the order, i.e. 
first setting up the multilib paths, should work, but as the comment 
indicates it has been on purpose that the path is set only after the FE 
spec files.

Using a compile-time constant like LIBGCC2_HAS_TF_MODE perfectly works, 
but rather by chance and not by design - and thus it can easily break if 
one modifies the compiler.

And choice number 3: Requiring the user to add the -lquadmath library 
fails, as the "-lgfortran -lm" are added after all user-specified 
libraries. (Besides, many user won't know what to do if they get a 
linkage error.)

I am open for suggestions.

Tobias
Tobias Burnus Nov. 18, 2010, 4:13 p.m. UTC | #5 
On 11/18/2010 12:28 PM, Tobias Burnus wrote:
> OK. Do you have then a suggestion how to handle this?

Matz had a suggestion (cf. PR for a longer quote from IRC): One adds a 
front-end hook to allow adding a front-end .spec file to 
linker_command_spec. The latter is read late enough that set_multipath() 
was executed already and thus the spec file can be found in the correct 
directory.

Thoughs? Suggestions? Comments? I do not want to spend a lot of my time 
implementing a dead end...

Tobias,
who has already spend more time on GCC lately than planned :-(
diff mbox

Patch

gcc/fortran/
2010-11-18  Tobias Burnus  <burnus@net-b.de>

	PR driver/46516
	* gfortranspec.c (find_spec_file): Remove.
	(lang_specific_driver): Remove spec file handling.
	(add_arg_libgfortran): Link -lquadmath.

gcc/testsuite/
2010-11-18  Tobias Burnus  <burnus@net-b.de>

	PR driver/46516
	* lib/gcc-defs.exp (gcc-set-multilib-library-path):
	Remove option handling.
	* lib/gfortran.exp (gfortran_link_flags, gfortran_init):
	Remove libgfortran.spec handling.

libgomp/
2010-11-18  Tobias Burnus  <burnus@net-b.de>

	PR driver/46516
	* configure.ac: Remove libgfortran.spec handling.
	* configure: Regenerate.

libgfortran/
2010-11-18  Tobias Burnus  <burnus@net-b.de>

	PR driver/46516
	* libgfortran.spec.in: Remove
	* acinclude.m4: Remove --as-needed check.
	* configure.ac: Remove spec file handling.
	* Makefile.am: Ditto.
	* configure: Regenerate.
	* Makefile.in: Regenerate.
Index: libgomp/ChangeLog
===================================================================
--- libgomp/ChangeLog	(revision 166895)
+++ libgomp/ChangeLog	(working copy)
@@ -2,7 +2,7 @@ 
 	    Tobias Burnus  <burnus@net-b.de>
 
 	PR fortran/32049
-	* configure.ac: 
+	* configure.ac: Touch libgfortran.spec.
 	* configure: Regenerate.
 
 2010-10-06  Marcus Shawcroft  <marcus.shawcroft@arm.com>
Index: libgomp/configure.ac
===================================================================
--- libgomp/configure.ac	(revision 166895)
+++ libgomp/configure.ac	(working copy)
@@ -140,10 +140,6 @@  AC_SUBST(enable_static)
 
 AM_MAINTAINER_MODE
 
-# Create a spec file, so that compile/link tests don't fail
-test -f libgfortran.spec || touch libgfortran.spec
-FCFLAGS="$FCFLAGS -L."
-
 # We need gfortran to compile parts of the library
 # We can't use AC_PROG_FC because it expects a fully working gfortran.
 #AC_PROG_FC(gfortran)
@@ -159,7 +155,7 @@  case `echo $GFORTRAN` in
     fi ;;
 esac
 AC_PROG_FC(gfortran)
-FCFLAGS="$FCFLAGS -Wall -L../libgfortran"
+FCFLAGS="$FCFLAGS -Wall"
 
 # For libtool versioning info, format is CURRENT:REVISION:AGE
 libtool_VERSION=1:0:0
Index: libgfortran/Makefile.am
===================================================================
--- libgfortran/Makefile.am	(revision 166895)
+++ libgfortran/Makefile.am	(working copy)
@@ -34,10 +34,9 @@  LTLDFLAGS = $(shell $(SHELL) $(top_srcdi
 	    -no-undefined -bindir "$(bindir)"
 
 toolexeclib_LTLIBRARIES = libgfortran.la
-toolexeclib_DATA = libgfortran.spec
 libgfortran_la_LINK = $(LINK) $(libgfortran_la_LDFLAGS)
 libgfortran_la_LDFLAGS = -version-info `grep -v '^\#' $(srcdir)/libtool-version` $(LTLDFLAGS) $(LIBQUADLIB) -lm $(extra_ldflags_libgfortran) $(version_arg)
-libgfortran_la_DEPENDENCIES = $(version_dep) libgfortran.spec $(LIBQUADLIB_DEP)
+libgfortran_la_DEPENDENCIES = $(version_dep) $(LIBQUADLIB_DEP)
 
 myexeclib_LTLIBRARIES = libgfortranbegin.la
 myexeclibdir = $(libdir)/gcc/$(target_alias)/$(gcc_version)$(MULTISUBDIR)
Index: libgfortran/acinclude.m4
===================================================================
--- libgfortran/acinclude.m4	(revision 166895)
+++ libgfortran/acinclude.m4	(working copy)
@@ -278,7 +278,6 @@  esac])
 
 dnl Check whether we have a __float128 type
 AC_DEFUN([LIBGFOR_CHECK_FLOAT128], [
-  LIBQUADSPEC=
   AC_CACHE_CHECK([whether we have a usable __float128 type],
                  libgfor_cv_have_float128, [
     AC_TRY_LINK([
@@ -303,34 +302,7 @@  AC_DEFUN([LIBGFOR_CHECK_FLOAT128], [
   if test "x$libgfor_cv_have_float128" = xyes; then
     AC_DEFINE(HAVE_FLOAT128, 1, [Define if have a usable __float128 type.])
 
-    dnl Check whether -Wl,--as-needed is supported
-    dnl 
-    dnl Turn warnings into error to avoid testsuite breakage.  So enable
-    dnl AC_LANG_WERROR, but there's currently (autoconf 2.64) no way to turn
-    dnl it off again.  As a workaround, save and restore werror flag like
-    dnl AC_PATH_XTRA.
-    dnl Cf. http://gcc.gnu.org/ml/gcc-patches/2010-05/msg01889.html
-    ac_xsave_[]_AC_LANG_ABBREV[]_werror_flag=$ac_[]_AC_LANG_ABBREV[]_werror_flag
-    AC_CACHE_CHECK([whether --as-needed works],
-      [libgfor_cv_have_as_needed],
-      [
-      save_LDFLAGS="$LDFLAGS"
-      LDFLAGS="$LDFLAGS -Wl,--as-needed -lm -Wl,--no-as-needed"
-      libgfor_cv_have_as_needed=no
-      AC_LANG_WERROR
-      AC_LINK_IFELSE([AC_LANG_PROGRAM([])],
-		     [libgfor_cv_have_as_needed=yes],
-		     [libgfor_cv_have_as_needed=no])
-      LDFLAGS="$save_LDFLAGS"
-      ac_[]_AC_LANG_ABBREV[]_werror_flag=$ac_xsave_[]_AC_LANG_ABBREV[]_werror_flag
-    ])
-
     dnl For static libgfortran linkage, depend on libquadmath only if needed.
-    if test "x$libgfor_cv_have_as_needed" = xyes; then
-      LIBQUADSPEC="%{static-libgfortran:--as-needed} -lquadmath %{static-libgfortran:--no-as-needed}"
-    else
-      LIBQUADSPEC="-lquadmath"
-    fi
     if test -f ../libquadmath/libquadmath.la; then
       LIBQUADLIB=../libquadmath/libquadmath.la
       LIBQUADLIB_DEP=../libquadmath/libquadmath.la
@@ -342,8 +314,6 @@  AC_DEFUN([LIBGFOR_CHECK_FLOAT128], [
     fi
   fi
 
-  dnl For the spec file
-  AC_SUBST(LIBQUADSPEC)
   AC_SUBST(LIBQUADLIB)
   AC_SUBST(LIBQUADLIB_DEP)
   AC_SUBST(LIBQUADINCLUDE)
Index: libgfortran/configure.ac
===================================================================
--- libgfortran/configure.ac	(revision 166895)
+++ libgfortran/configure.ac	(working copy)
@@ -72,8 +72,6 @@  AM_ENABLE_MULTILIB(, ..)
 #AC_MSG_NOTICE($build / $host / $target / $host_alias / $target_alias); sleep 5
 
 # Are we being configured with some form of cross compiler?
-# NB: We don't actually need to know this just now, but when, say, a test
-#     suite is included, we'll have to know.
 if test "$build" != "$host"; then
   LIBGFOR_IS_NATIVE=false
   GCC_NO_EXECUTABLES
@@ -111,9 +109,6 @@  esac
 AC_SUBST(toolexecdir)
 AC_SUBST(toolexeclibdir)
 
-# Create a spec file, so that compile/link tests don't fail
-test -f libgfortran.spec || touch libgfortran.spec
-
 # Check the compiler.
 # The same as in boehm-gc and libstdc++. Have to borrow it from there.
 # We must force CC to /not/ be precious variables; otherwise
@@ -518,6 +513,5 @@  fi
 # Write our Makefile and spec file.
 AC_CONFIG_FILES([
 Makefile
-libgfortran.spec
 ])
 AC_OUTPUT
Index: libgfortran/libgfortran.spec.in
===================================================================
--- libgfortran/libgfortran.spec.in	(revision 166895)
+++ libgfortran/libgfortran.spec.in	(working copy)
@@ -1,8 +0,0 @@ 
-#
-# This spec file is read by gfortran when linking.
-# It is used to specify the libraries we need to link in, in the right
-# order.
-#
-
-%rename lib liborig
-*lib: @LIBQUADSPEC@ -lm %(libgcc) %(liborig)
Index: gcc/testsuite/lib/gcc-defs.exp
===================================================================
--- gcc/testsuite/lib/gcc-defs.exp	(revision 166897)
+++ gcc/testsuite/lib/gcc-defs.exp	(working copy)
@@ -250,10 +250,9 @@  proc gcc-set-multilib-library-path { com
     }
 
     set libpath ":${rootme}"
-    set options [lrange $compiler 1 end]
     set compiler [lindex $compiler 0]
     if { [is_remote host] == 0 && [which $compiler] != 0 } {
-	foreach i "[exec $compiler $options --print-multi-lib]" {
+	foreach i "[exec $compiler --print-multi-lib]" {
 	    set mldir ""
 	    regexp -- "\[a-z0-9=_/\.-\]*;" $i mldir
 	    set mldir [string trimright $mldir "\;@"]
Index: gcc/testsuite/lib/gfortran.exp
===================================================================
--- gcc/testsuite/lib/gfortran.exp	(revision 166897)
+++ gcc/testsuite/lib/gfortran.exp	(working copy)
@@ -103,22 +103,11 @@  proc gfortran_link_flags { paths } {
       if [file exists "${gccpath}/libgfortran/libgforbegin.a"] {
           append flags "-L${gccpath}/libgfortran "
       }
-      if [file exists "${gccpath}/libquadmath/.libs/libquadmath.a"] {
-          # Some targets use libquadmath.a%s in their specs, so they need a -B option
-          # for uninstalled testing.
-          append flags "-B${gccpath}/libquadmath/.libs "
-          append flags "-L${gccpath}/libquadmath/.libs "
-          append ld_library_path ":${gccpath}/libquadmath/.libs"
-      }
-      if [file exists "${gccpath}/libquadmath/.libs/libquadmath.${shlib_ext}"] {
-	  append flags "-L${gccpath}/libquadmath/.libs "
-	  append ld_library_path ":${gccpath}/libquadmath/.libs"
-      }
       if [file exists "${gccpath}/libiberty/libiberty.a"] {
           append flags "-L${gccpath}/libiberty "
       }
       append ld_library_path \
-	[gcc-set-multilib-library-path { $GFORTRAN_UNDER_TEST } ]
+	[gcc-set-multilib-library-path $GFORTRAN_UNDER_TEST]
     }
 
     set_ld_library_path_env_vars
@@ -161,12 +150,7 @@  proc gfortran_init { args } {
 	    if { [is_remote host] || ! [info exists TESTING_IN_BUILD_TREE] } {
 		set GFORTRAN_UNDER_TEST [transform gfortran]
 	    } else {
-		if [info exists TOOL_OPTIONS] {
-	            set specpath [get_multilibs ${TOOL_OPTIONS}]
-		} else {
-		    set specpath [get_multilibs]
-		}
-		set GFORTRAN_UNDER_TEST [findfile $base_dir/../../gfortran "$base_dir/../../gfortran -B$base_dir/../../ -L$specpath/libgfortran" [findfile $base_dir/gfortran "$base_dir/gfortran -B$base_dir/" [transform gfortran]]]
+		set GFORTRAN_UNDER_TEST [findfile $base_dir/../../gfortran "$base_dir/../../gfortran -B$base_dir/../../" [findfile $base_dir/gfortran "$base_dir/gfortran -B$base_dir/" [transform gfortran]]]
 	    }
 	}
     }
Index: gcc/fortran/gfortranspec.c
===================================================================
--- gcc/fortran/gfortranspec.c	(revision 166897)
+++ gcc/fortran/gfortranspec.c	(working copy)
@@ -63,8 +63,9 @@  along with GCC; see the file COPYING3.
 #define FORTRAN_LIBRARY "gfortran"
 #endif
 
-/* Name of the spec file.  */
-#define SPEC_FILE "libgfortran.spec"
+#ifndef QUADMATH_LIBRARY
+#define QUADMATH_LIBRARY "quadmath"
+#endif
 
 /* The original argument list and related info is copied here.  */
 static unsigned int g77_xargc;
@@ -75,27 +76,6 @@  static void append_arg (const struct cl_
 static unsigned int g77_newargc;
 static struct cl_decoded_option *g77_new_decoded_options;
 
-
-/* Return full path name of spec file if it is in DIR, or NULL if
-   not.  */
-static char *
-find_spec_file (const char *dir)
-{
-  const char dirsep_string[] = { DIR_SEPARATOR, '\0' };
-  char *spec;
-  struct stat sb;
-
-  spec = XNEWVEC (char, strlen (dir) + sizeof (SPEC_FILE) + 4);
-  strcpy (spec, dir);
-  strcat (spec, dirsep_string);
-  strcat (spec, SPEC_FILE);
-  if (!stat (spec, &sb))
-    return spec;
-  free (spec);
-  return NULL;
-}
-
-
 /* Return whether strings S1 and S2 are both NULL or both the same
    string.  */
 
@@ -192,6 +172,18 @@  add_arg_libgfortran (bool force_static A
   if (force_static)
     append_option (OPT_Wl_, "-Bdynamic", 1);
 #endif
+
+#ifdef LIBGCC2_HAS_TF_MODE
+#ifdef USE_LD_AS_NEEDED
+  if (force_static)
+    append_option (OPT_Wl_, "--as-needed", 1);
+#endif
+  append_option (OPT_l, QUADMATH_LIBRARY, 1);
+#ifdef USE_LD_AS_NEEDED
+  if (force_static)
+    append_option (OPT_Wl_, "--no-as-needed", 1);
+#endif
+#endif
 }
 
 void
@@ -223,9 +215,6 @@  lang_specific_driver (struct cl_decoded_
   /* Whether we should link a static libgfortran. */
   int static_lib = 0; 
 
-  /* The path to the spec file.  */
-  char *spec_file = NULL;
-
   /* Whether we need to link statically.  */
   int static_linking = 0;
 
@@ -310,12 +299,6 @@  For more information about these matters
 	     cool facility for handling --help and --verbose --help.  */
 	  return;
 
-	case OPT_L:
-	  if (!spec_file)
-	    spec_file = find_spec_file (decoded_options[i].arg);
-	  break;
-
-
 	default:
 	  break;
 	}
@@ -446,11 +429,6 @@  For more information about these matters
 
 #endif
 
-  /* Read the specs file corresponding to libgfortran.
-     If we didn't find the spec file on the -L path, then we hope it
-     is somewhere in the standard install areas.  */
-  append_option (OPT_specs_, spec_file == NULL ? SPEC_FILE : spec_file, 1);
-
   if (verbose && g77_new_decoded_options != g77_x_decoded_options)
     {
       fprintf (stderr, _("Driving:"));