Include sysdep.h before stdio.h in ldlex.c

Include sysdep.h before stdio.h in ldlex.c

[Richard Sandiford]

As Franz pointed out recently, binutils 2.20 and trunk fail to build
on AIX hosts because of an include-ordering bug in ldlex.c.  The code
generated by flex includes stdio.h _before_ any of C code in ldlex.l,
which in our case means that it includes stdio.h before sysdep.h.

sysdep.h uses config.h to set macros like _LARGE_FILES to the values
we want, then includes the headers that depend on those macros.
So on AIX we end up including stdio.h twice, once asking for 32-bit
offsets and once asking for 64-bit offsets.

The attached patch is a bit ugly, but it does fix the problem.
Better suggestions welcome!

Tested on powerpc-ibm-aix5.3.0 and x86_64-linux-gnu.  OK to apply
to trunk and branch?

Richard


ld/
	* Makefile.am (ldlex.o): Depend on ldlex-wrapper.c too, and compile
	that instead of ldlex.c.
	* Makefile.in: Regenerate.
	* ldlex.l (sysdep.h): Don't include here.
	* ldlex-wrapper.c: New file.

Index: ld/Makefile.am
===================================================================
--- ld/Makefile.am	2010-01-12 20:30:19.000000000 +0000
+++ ld/Makefile.am	2010-02-08 19:21:35.000000000 +0000
@@ -480,16 +480,16 @@ endif
 	$(COMPILE) -c `test -f ldgram.c || echo $(srcdir)/`ldgram.c $(NO_WERROR)
 endif
 
-ldlex.o: ldlex.c
+ldlex.o: ldlex-wrapper.c ldlex.c
 if am__fastdepCC
-	$(COMPILE) -MT $@ -MD -MP -MF $(DEPDIR)/$*.Tpo -c -o $@ `test -f ldlex.c || echo $(srcdir)/`ldlex.c $(NO_WERROR)
+	$(COMPILE) -MT $@ -MD -MP -MF $(DEPDIR)/$*.Tpo -c -o $@ $(srcdir)/ldlex-wrapper.c $(NO_WERROR)
 	mv -f $(DEPDIR)/$*.Tpo $(DEPDIR)/$*.Po
 else
 if AMDEP
-	source='ldlex.c' object='$@' libtool=no @AMDEPBACKSLASH@
+	source='ldlex-wrapper.c' object='$@' libtool=no @AMDEPBACKSLASH@
 	DEPDIR=$(DEPDIR) $(CCDEPMODE) $(depcomp) @AMDEPBACKSLASH@
 endif
-	$(COMPILE) -c `test -f ldlex.c || echo $(srcdir)/`ldlex.c $(NO_WERROR)
+	$(COMPILE) -c ldlex-wrapper.c $(NO_WERROR)
 endif
 
 deffilep.o: deffilep.c
Index: ld/ldlex.l
===================================================================
--- ld/ldlex.l	2010-01-12 20:30:21.000000000 +0000
+++ ld/ldlex.l	2010-02-08 19:21:35.000000000 +0000
@@ -24,7 +24,6 @@
    Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
    MA 02110-1301, USA.  */
 
-#include "sysdep.h"
 #include "bfd.h"
 #include "safe-ctype.h"
 #include "bfdlink.h"
Index: ld/ldlex-wrapper.c
===================================================================
--- /dev/null	2010-02-08 19:11:56.502114510 +0000
+++ ld/ldlex-wrapper.c	2010-02-08 19:21:35.000000000 +0000
@@ -0,0 +1,2 @@
+#include "sysdep.h"
+#include "ldlex.c"

Re: Include sysdep.h before stdio.h in ldlex.c

[Ralf Wildenhues]

Hello Richard,

* Richard Sandiford wrote on Mon, Feb 08, 2010 at 08:39:50PM CET:
> ld/
> 	* Makefile.am (ldlex.o): Depend on ldlex-wrapper.c too, and compile
> 	that instead of ldlex.c.
> 	* Makefile.in: Regenerate.
> 	* ldlex.l (sysdep.h): Don't include here.
> 	* ldlex-wrapper.c: New file.
> 
> Index: ld/Makefile.am
> ===================================================================
> --- ld/Makefile.am	2010-01-12 20:30:19.000000000 +0000
> +++ ld/Makefile.am	2010-02-08 19:21:35.000000000 +0000
> @@ -480,16 +480,16 @@ endif
>  	$(COMPILE) -c `test -f ldgram.c || echo $(srcdir)/`ldgram.c $(NO_WERROR)
>  endif
>  
> -ldlex.o: ldlex.c
> +ldlex.o: ldlex-wrapper.c ldlex.c
>  if am__fastdepCC
> -	$(COMPILE) -MT $@ -MD -MP -MF $(DEPDIR)/$*.Tpo -c -o $@ `test -f ldlex.c || echo $(srcdir)/`ldlex.c $(NO_WERROR)
> +	$(COMPILE) -MT $@ -MD -MP -MF $(DEPDIR)/$*.Tpo -c -o $@ $(srcdir)/ldlex-wrapper.c $(NO_WERROR)
>  	mv -f $(DEPDIR)/$*.Tpo $(DEPDIR)/$*.Po
>  else
>  if AMDEP
> -	source='ldlex.c' object='$@' libtool=no @AMDEPBACKSLASH@
> +	source='ldlex-wrapper.c' object='$@' libtool=no @AMDEPBACKSLASH@
>  	DEPDIR=$(DEPDIR) $(CCDEPMODE) $(depcomp) @AMDEPBACKSLASH@
>  endif
> -	$(COMPILE) -c `test -f ldlex.c || echo $(srcdir)/`ldlex.c $(NO_WERROR)
> +	$(COMPILE) -c ldlex-wrapper.c $(NO_WERROR)

This will create ldlex-wrapper.o, not ldlex.o, for compilers other than
GCC.  I'm not sure whether this code still needs to be portable to
compilers that don't understand '-c -o', but if it does, then possible
ways out are either: add AM_PROG_CC_C_O to configure.in, or rename the
input file so that the object matches the source file with s/\.c$/.o/.
If not (i.e., it is fine to assume that '-c -o' works), then adding -o
$@ right after -c should work.  (The ordering helps depcomp.)

>  endif

Cheers,
Ralf

Re: Include sysdep.h before stdio.h in ldlex.c

[Nick Clifton]

Hi Richard,

> The attached patch is a bit ugly, but it does fix the problem.
> Better suggestions welcome!

Isn't this a generic problem for any AIX compiled program that uses flex 
and large files ?  If so, I wonder if someone has developed a less clunk 
workaround ?

> Tested on powerpc-ibm-aix5.3.0 and x86_64-linux-gnu.  OK to apply
> to trunk and branch?

I think that you ought to add a comment to ldlex-wrapper.c explaining 
why it is needed.  Otherwise someone might be tempted to come along and 
delete it.

Ralf also pointed out that:

> This will create ldlex-wrapper.o, not ldlex.o, for compilers other than
> GCC.  I'm not sure whether this code still needs to be portable to
> compilers that don't understand '-c -o', but if it does, then possible
> ways out are either: add AM_PROG_CC_C_O to configure.in, or rename the
> input file so that the object matches the source file with s/\.c$/.o/.
> If not (i.e., it is fine to assume that '-c -o' works), then adding -o
> $@ right after -c should work.  (The ordering helps depcomp.)

My feeling is that adding AM_PROG_CC_C_O would be the best option. 
Assuming of course that the AIX native compiler supports -c -o.  What do 
you think ?

Cheers
   Nick

Re: Include sysdep.h before stdio.h in ldlex.c

[Andreas Schwab]

Nick Clifton <nickc@redhat.com> writes:

> Isn't this a generic problem for any AIX compiled program that uses
> flex and large files ?  If so, I wonder if someone has developed a
> less clunk workaround ?

Actually it's a problem on any system that needs special defines for
large files.  If not causing build failures it causes the defines to be
ignored.  There is no dependency on off_t in BFD, so this has no
practical consequences yet.

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: Include sysdep.h before stdio.h in ldlex.c

[Richard Sandiford]

Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:
> This will create ldlex-wrapper.o, not ldlex.o, for compilers other than
> GCC.

*sigh* Well that was lame of me, sorry.

> I'm not sure whether this code still needs to be portable to
> compilers that don't understand '-c -o', but if it does, then possible
> ways out are either: add AM_PROG_CC_C_O to configure.in, or rename the
> input file so that the object matches the source file with s/\.c$/.o/.
> If not (i.e., it is fine to assume that '-c -o' works), then adding -o
> $@ right after -c should work.  (The ordering helps depcomp.)

I think I prefer calling the object file ldlex-wrapper.o instead.
That avoids the AM_PROC_CC_C_O thing and makes it more obvious
which file was compiled.

Does this look OK to you?  Nick?

(And Nick, to answer your question: yes, this is something that would
affect any target or package that tries to set these sort of macros in
a lex file.  Bit of a flex flaw really.)

Tested on x86_64-linux-gnu and powerpc-ibm-aix5.3.0 as before.

Richard


ld/
	* Makefile.am (CFILES): Add ldlex-wrapper.c.
	(OFILES): Replace ldlex.c with ldlex-wrapper.c.
	(ldlex.o): Replace with...
	(ldlex-wrapper.o): ...this new rule.
	(EXTRA_ld_new_SOURCES): Add ldlex.l.
	(ld_new_SOURCES): Replace ldlex.l with ldlex-wrapper.c.
	* Makefile.in: Regenerate.
	* ldlex.l (sysdep.h): Don't include here.
	* ldlex-wrapper.c: New file.

Index: ld/Makefile.am
===================================================================
--- ld/Makefile.am	2010-02-09 16:57:45.000000000 +0000
+++ ld/Makefile.am	2010-02-09 17:13:46.000000000 +0000
@@ -447,7 +447,7 @@ ALL_EMUL_EXTRA_OFILES = \
 
 CFILES = ldctor.c ldemul.c ldexp.c ldfile.c ldlang.c \
 	ldmain.c ldmisc.c ldver.c ldwrite.c lexsup.c \
-	mri.c ldcref.c pe-dll.c pep-dll.c
+	mri.c ldcref.c pe-dll.c pep-dll.c ldlex-wrapper.c
 
 HFILES = ld.h ldctor.h ldemul.h ldexp.h ldfile.h \
 	ldlang.h ldlex.h ldmain.h ldmisc.h ldver.h \
@@ -460,7 +460,7 @@ GENERATED_HFILES = ldgram.h ldemul-list.
 # tracking will not cause them to be built beforehand.
 BUILT_SOURCES = $(GENERATED_HFILES)
 
-OFILES = ldgram.o ldlex.o lexsup.o ldlang.o mri.o ldctor.o ldmain.o \
+OFILES = ldgram.o ldlex-wrapper.o lexsup.o ldlang.o mri.o ldctor.o ldmain.o \
 	ldwrite.o ldexp.o  ldemul.o ldver.o ldmisc.o \
 	ldfile.o ldcref.o ${EMULATION_OFILES} ${EMUL_EXTRA_OFILES}
 
@@ -480,16 +480,16 @@ endif
 	$(COMPILE) -c `test -f ldgram.c || echo $(srcdir)/`ldgram.c $(NO_WERROR)
 endif
 
-ldlex.o: ldlex.c
+ldlex-wrapper.o: ldlex-wrapper.c ldlex.c
 if am__fastdepCC
-	$(COMPILE) -MT $@ -MD -MP -MF $(DEPDIR)/$*.Tpo -c -o $@ `test -f ldlex.c || echo $(srcdir)/`ldlex.c $(NO_WERROR)
+	$(COMPILE) -MT $@ -MD -MP -MF $(DEPDIR)/$*.Tpo -c -o $@ $(srcdir)/ldlex-wrapper.c $(NO_WERROR)
 	mv -f $(DEPDIR)/$*.Tpo $(DEPDIR)/$*.Po
 else
 if AMDEP
-	source='ldlex.c' object='$@' libtool=no @AMDEPBACKSLASH@
+	source='ldlex-wrapper.c' object='$@' libtool=no @AMDEPBACKSLASH@
 	DEPDIR=$(DEPDIR) $(CCDEPMODE) $(depcomp) @AMDEPBACKSLASH@
 endif
-	$(COMPILE) -c `test -f ldlex.c || echo $(srcdir)/`ldlex.c $(NO_WERROR)
+	$(COMPILE) -c $(srcdir)/ldlex-wrapper.c $(NO_WERROR)
 endif
 
 deffilep.o: deffilep.c
@@ -1837,11 +1837,11 @@ ez8002.c: $(srcdir)/emulparams/z8002.sh 
 	${GENSCRIPTS} z8002 "$(tdir_z8002)"
 
 # We need this for automake to use YLWRAP.
-EXTRA_ld_new_SOURCES = deffilep.y
+EXTRA_ld_new_SOURCES = deffilep.y ldlex.l
 # Allow dependency tracking to work for these files, too.
 EXTRA_ld_new_SOURCES += pep-dll.c pe-dll.c
 
-ld_new_SOURCES = ldgram.y ldlex.l lexsup.c ldlang.c mri.c ldctor.c ldmain.c \
+ld_new_SOURCES = ldgram.y ldlex-wrapper.c lexsup.c ldlang.c mri.c ldctor.c ldmain.c \
 	ldwrite.c ldexp.c ldemul.c ldver.c ldmisc.c ldfile.c ldcref.c
 ld_new_DEPENDENCIES = $(EMULATION_OFILES) $(EMUL_EXTRA_OFILES) $(BFDLIB) $(LIBIBERTY) $(LIBINTL_DEP)
 ld_new_LDADD = $(EMULATION_OFILES) $(EMUL_EXTRA_OFILES) $(BFDLIB) $(LIBIBERTY) $(LIBINTL)
Index: ld/ldlex-wrapper.c
===================================================================
--- /dev/null	2010-01-13 08:32:49.424941899 +0000
+++ ld/ldlex-wrapper.c	2010-02-09 16:58:13.000000000 +0000
@@ -0,0 +1,2 @@
+#include "sysdep.h"
+#include "ldlex.c"
Index: ld/ldlex.l
===================================================================
--- ld/ldlex.l	2010-02-09 16:57:45.000000000 +0000
+++ ld/ldlex.l	2010-02-09 16:58:13.000000000 +0000
@@ -24,7 +24,6 @@
    Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
    MA 02110-1301, USA.  */
 
-#include "sysdep.h"
 #include "bfd.h"
 #include "safe-ctype.h"
 #include "bfdlink.h"

Re: Include sysdep.h before stdio.h in ldlex.c

[Nick Clifton]

Hi Richard,

> Does this look OK to you?  Nick?

I still think that you ought to have a comment in ldlex-wrapper.c 
explaining why it is necessary, but apart from that I have no other 
objections and I am happy to approve it.

Cheers
   Nick

> ld/
> 	* Makefile.am (CFILES): Add ldlex-wrapper.c.
> 	(OFILES): Replace ldlex.c with ldlex-wrapper.c.
> 	(ldlex.o): Replace with...
> 	(ldlex-wrapper.o): ...this new rule.
> 	(EXTRA_ld_new_SOURCES): Add ldlex.l.
> 	(ld_new_SOURCES): Replace ldlex.l with ldlex-wrapper.c.
> 	* Makefile.in: Regenerate.
> 	* ldlex.l (sysdep.h): Don't include here.
> 	* ldlex-wrapper.c: New file.

Re: Include sysdep.h before stdio.h in ldlex.c

[Richard Sandiford]

Nick Clifton <nickc@redhat.com> writes:
> Hi Richard,
>
>> Does this look OK to you?  Nick?
>
> I still think that you ought to have a comment in ldlex-wrapper.c 
> explaining why it is necessary,

Doh!  Sorry Nick, completely forgot to add that yesterday.
Here's what I used:

/* The flex output (ldlex.c) includes stdio.h before any of the C code
   in ldlex.l.  Make sure we include sysdep.h first, so that config.h
   can select the correct value of things like _FILE_OFFSET_BITS and
   _LARGE_FILES.  */

> but apart from that I have no other 
> objections and I am happy to approve it.

Applied, thanks.

Richard