Re: Dejagnu: use -isystem to include system header files.

Re: Dejagnu: use -isystem to include system header files.

[Richard Earnshaw]

Nick Clifton wrote:
        I am going to check in the attached patch which imports a fix from
        the mainline dejagnu sources.  This fix is to use the -isystem
        switch to include system header files rather than -I.  This fixes
        several unexpected failures in the GCC and G++ testsuites where the
        newlib system header file <limits.h> is included in strict ANSI
        mode, and the compiler barfs on the #include_next directive.
        
Unfortunately this patch causes regressions on the gcc builtins tests. 
These tests rely on detecting newlib by looking for the definition of
_NEWLIB_VERSION being added by including limits.h; but the change in the
search order means that we now pick up a dummy version of newlib.h from
the gcc include directory.  

With your patch the search path has now become

 /work/rearnsha/gnu/egcs/gcc/include
 /work/rearnsha/gnu/egcs/arm-elf/./newlib/targ-include
 /home/rearnsha/gnusrc/egcs-cross/newlib/libc/include

Whereas previously the gcc/include directory came later in the search.

R.

Re: Dejagnu: use -isystem to include system header files.

[Nick Clifton]

Hi Richard,

> Nick Clifton wrote:
>         I am going to check in the attached patch which imports a fix from
>         the mainline dejagnu sources.  This fix is to use the -isystem
>         switch to include system header files rather than -I.  This fixes
>         several unexpected failures in the GCC and G++ testsuites where the
>         newlib system header file <limits.h> is included in strict ANSI
>         mode, and the compiler barfs on the #include_next directive.
>         
> Unfortunately this patch causes regressions on the gcc builtins tests. 
> These tests rely on detecting newlib by looking for the definition of
> _NEWLIB_VERSION being added by including limits.h; but the change in the
> search order means that we now pick up a dummy version of newlib.h from
> the gcc include directory.  
> 
> With your patch the search path has now become
> 
>  /work/rearnsha/gnu/egcs/gcc/include
>  /work/rearnsha/gnu/egcs/arm-elf/./newlib/targ-include
>  /home/rearnsha/gnusrc/egcs-cross/newlib/libc/include
> 
> Whereas previously the gcc/include directory came later in the search.

Hmmm, maybe newlib could provide the "l" variants of the builtin 
functions ?  What are these functions anyway ?  Or maybe 
builtins-config.h could include say <stdio.h> rather than <limits.h> so 
that it would pickup the newlib version and not the gcc version ?

Alternatively - can you think of another way of solving the problem that 
my patch was originally fixing ?  Namely that several GCC and G++ tests 
fail because they include <limits.h> whilst in strict ANSI mode and this 
fails because the newlib limits.h uses the GNU extension #include_next 
directive.  My first patch to solve this -  by undefining __GNUC__ if 
__STRICT_ANSI__ was defined was rejected on the gcc lists.

Cheers
   Nick

Re: Dejagnu: use -isystem to include system header files.

[Richard Earnshaw]

On Thu, 2004-11-18 at 09:19, Nick Clifton wrote:
> Hi Richard,
> 
> > Nick Clifton wrote:
> >         I am going to check in the attached patch which imports a fix from
> >         the mainline dejagnu sources.  This fix is to use the -isystem
> >         switch to include system header files rather than -I.  This fixes
> >         several unexpected failures in the GCC and G++ testsuites where the
> >         newlib system header file <limits.h> is included in strict ANSI
> >         mode, and the compiler barfs on the #include_next directive.
> >         
> > Unfortunately this patch causes regressions on the gcc builtins tests. 
> > These tests rely on detecting newlib by looking for the definition of
> > _NEWLIB_VERSION being added by including limits.h; but the change in the
> > search order means that we now pick up a dummy version of newlib.h from
> > the gcc include directory.  
> > 
> > With your patch the search path has now become
> > 
> >  /work/rearnsha/gnu/egcs/gcc/include
> >  /work/rearnsha/gnu/egcs/arm-elf/./newlib/targ-include
> >  /home/rearnsha/gnusrc/egcs-cross/newlib/libc/include
> > 
> > Whereas previously the gcc/include directory came later in the search.
> 
> Hmmm, maybe newlib could provide the "l" variants of the builtin 
> functions ?  What are these functions anyway ? 

Long double.  I'm not sure if newlib wants to go that way, but if it
does it's probably a fair amount of work, especially since long double
means different things on different targets.

>
>  Or maybe 
> builtins-config.h could include say <stdio.h> rather than <limits.h> so 
> that it would pickup the newlib version and not the gcc version ?
> 

That might be OK for this case, but I'm not sure if will solve the
problem generally.

> Alternatively - can you think of another way of solving the problem that 
> my patch was originally fixing ?  Namely that several GCC and G++ tests 
> fail because they include <limits.h> whilst in strict ANSI mode and this 
> fails because the newlib limits.h uses the GNU extension #include_next 
> directive.  My first patch to solve this -  by undefining __GNUC__ if 
> __STRICT_ANSI__ was defined was rejected on the gcc lists.

I think the gcc/include directory must be added implicitly from the -B
option.  It would appear that these add -isystem type include
directories, so it might be just a matter of ordering the -B and
-isystem options appropriately.

R.

Re: Dejagnu: use -isystem to include system header files.

[Nick Clifton]

Hi Richard,

>> Or maybe 
>>builtins-config.h could include say <stdio.h> rather than <limits.h> so 
>>that it would pickup the newlib version and not the gcc version ?

> That might be OK for this case, but I'm not sure if will solve the
> problem generally.

I have confirmed that using <stdio.h> in place of <limits.h> does fix 
the unexpected failures.

Is there actually a general problem ?  The issue seems to be which 
version of <limits.h> is included by a test case using the dejagnu test 
harness.  If we include the newlib version then tests that use strict 
ANSI parsing will fail.  If we include the version in the gcc build 
directory then tests that assume that <limits.h> has come from newlib 
will fail.

It seems to me that using -isystem to include header files in the newlib 
include directories is the correct thing to do.  They are system header 
files after all.  Assuming that you can determine whether you are going 
to link with a C-library created by newlib by #include'ing <limits.h> 
seems dodgy to me.  Using <stdio.h> instead of <limits.h> is a 
workaround, but really the testcases ought to provide some weak aliases 
for the missing functions and then the _NEWLIB_VERSION check would be 
unnecessary.  (It would limit the tests to only being run by targets 
which support weak aliases but I do not consider this to be a serious 
restriction).

> I think the gcc/include directory must be added implicitly from the -B
> option.  It would appear that these add -isystem type include
> directories, so it might be just a matter of ordering the -B and
> -isystem options appropriately.

But - how would this help in the situation where -ansi and -pedantic 
have been specified as well.  In those cases we do not want to get the 
limits.h file from newlib.

Cheers
   Nick

RE: Dejagnu: use -isystem to include system header files.

[Richard Earnshaw]

> > I think the gcc/include directory must be added implicitly 
> from the -B
> > option.  It would appear that these add -isystem type include
> > directories, so it might be just a matter of ordering the -B and
> > -isystem options appropriately.
> 
> But - how would this help in the situation where -ansi and -pedantic 
> have been specified as well.  In those cases we do not want 
> to get the 
> limits.h file from newlib.

The issue wasn't really about picking up gcc/include/limits.h over
newlib/include/limits.h, it was about how we processed the newlib
version when in strict ANSI mode.  That should be handled correctly when
we use -isystem (because that relaxes the rules).  So I think we should
try and get the search order back to what it was before, but when still
using -isystem.

R.

Re: Dejagnu: use -isystem to include system header files.

[Nick Clifton]

Hi Richard,

> The issue wasn't really about picking up gcc/include/limits.h over
> newlib/include/limits.h, it was about how we processed the newlib
> version when in strict ANSI mode.  That should be handled correctly when
> we use -isystem (because that relaxes the rules).  So I think we should
> try and get the search order back to what it was before, but when still
> using -isystem.

I am not sure that this is possible.  The <build-dir>/gcc/include 
directory is added via an -isystem switch that is synthesised by the gcc 
(or xgcc) compiler driver.  It is processed before any-isystem switch 
specified by the user on the command line so that gcc's fixed versions 
of system header files will be included before the systems.  Thus if the 
dejagnu test harness uses the -isystem switch to add the 
newlib/libc/include directory it is always going to be after 
<build-dir>/gcc/include in the list of directories searched.

This sounds correct to me.  The intention is that the fixed version of 
limits.h should be included before the system version of limits.h.  Thus 
builtins-config.h should not assume that #include <limits.h> will return 
the system limits.h.  It might well return a fixed version instead.

Cheers
   Nick

Re: Dejagnu: use -isystem to include system header files.

[Daniel Jacobowitz]

On Thu, Nov 11, 2004 at 11:58:15AM +0000, Nick Clifton wrote:
> Hi Guys,
> 
>   I am going to check in the attached patch which imports a fix from
>   the mainline dejagnu sources.  This fix is to use the -isystem
>   switch to include system header files rather than -I.  This fixes
>   several unexpected failures in the GCC and G++ testsuites where the
>   newlib system header file <limits.h> is included in strict ANSI
>   mode, and the compiler barfs on the #include_next directive.

This patch will break in-tree testing for yet other targets.  I believe
arm-elf was affected - anything which does not set
NO_IMPLICIT_EXTERN_C.  I discussed this with H-P on the dejagnu list
but never figured out a solution, but...

>         * lib/libgloss.exp (newlib_include_flags): Use -isystem, not -I.
>         (libio_include_flags, g++_include_flags, libstdc++_include_flags,
>         winsup_include_flags): Ditto.

... I strongly suspect that g++ and winsup should be left out.

-- 
Daniel Jacobowitz

Re: Dejagnu: use -isystem to include system header files.

[Nick Clifton]

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

Hi Daniel,

> This patch will break in-tree testing for yet other targets.  I believe
> arm-elf was affected - anything which does not set
> NO_IMPLICIT_EXTERN_C.  I discussed this with H-P on the dejagnu list
> but never figured out a solution, but...

>>        * lib/libgloss.exp (newlib_include_flags): Use -isystem, not -I.
>>        (libio_include_flags, g++_include_flags, libstdc++_include_flags,
>>        winsup_include_flags): Ditto.

> ... I strongly suspect that g++ and winsup should be left out.


Ok - what about this revision to the patched sources based upon Rob 
Savoye's suggestion that -isystem is only needed for newlib includes ?

It appears to work for the xstormy16 port and I did not detect any 
regression for the arm-elf port, so I think that it should be OK.

Assuming that you like this patch, shall I submit it to Rob for 
inclusion in the official sources as well ?

Cheers
   Nick

dejagnu/ChangeLog
2004-11-11  Nick Clifton  <nickc@redhat.com>

	* lib/libgloss.exp (libio_include_flags, g++_include_flags,
	winsup_include_flags): Revert previous patch, restoring the use of
	-I, for all libraries except newlib.  Newlib needs -isystem to
	avoid the problems with <limits.h> but the C++ and winsup
	libraries need -I because -isystem generates an implicit 'extern
	"C"' which may not be appropriate for certain targets.




[-- Attachment #2: libgloss.exp.patch --]
[-- Type: text/plain, Size: 4167 bytes --]

Index: dejagnu/lib/libgloss.exp
===================================================================
RCS file: /cvs/src/src/dejagnu/lib/libgloss.exp,v
retrieving revision 1.11
diff -c -3 -p -r1.11 libgloss.exp
*** dejagnu/lib/libgloss.exp	11 Nov 2004 11:55:11 -0000	1.11
--- dejagnu/lib/libgloss.exp	11 Nov 2004 15:46:03 -0000
*************** proc libio_include_flags { args } {
*** 191,197 ****
      if { $libio_bin_dir != "" && $libio_src_dir != "" } {
  	set libio_src_dir [file dirname ${libio_src_dir}]
  	set libio_bin_dir [file dirname ${libio_bin_dir}];
! 	return " -isystem ${libio_src_dir} -isystem ${libio_bin_dir}"
      } else {
  	return ""
      }
--- 191,197 ----
      if { $libio_bin_dir != "" && $libio_src_dir != "" } {
  	set libio_src_dir [file dirname ${libio_src_dir}]
  	set libio_bin_dir [file dirname ${libio_bin_dir}];
! 	return " -I${libio_src_dir} -I${libio_bin_dir}"
      } else {
  	return ""
      }
*************** proc g++_include_flags { args } {
*** 226,247 ****
  
      set dir [lookfor_file ${srcdir} libg++]
      if { ${dir} != "" } {
! 	append flags " -isystem ${dir} -isystem ${dir}/src"
      }
  
      set dir [lookfor_file ${srcdir} libstdc++-v3]
      if { ${dir} != "" } {
! 	append flags " -isystem ${dir}/include -isystem ${dir}/include/std -isystem ${dir}/include/c_std -isystem ${dir}/libsupc++"
      }
  
      set dir [lookfor_file ${gccpath} libstdc++-v3]
      if { ${dir} != "" } {
! 	append flags " -isystem ${dir}/include -isystem ${dir}/include/${target_alias}"
      }
  
      set dir [lookfor_file ${srcdir} libstdc++]
      if { ${dir} != "" } {
! 	append flags " -isystem ${dir} -isystem ${dir}/stl"
      }
  
      return "$flags"
--- 226,247 ----
  
      set dir [lookfor_file ${srcdir} libg++]
      if { ${dir} != "" } {
! 	append flags " -I${dir} I${dir}/src"
      }
  
      set dir [lookfor_file ${srcdir} libstdc++-v3]
      if { ${dir} != "" } {
! 	append flags " -I${dir}/include -I${dir}/include/std -I${dir}/include/c_std -I${dir}/libsupc++"
      }
  
      set dir [lookfor_file ${gccpath} libstdc++-v3]
      if { ${dir} != "" } {
! 	append flags " -I${dir}/include -I${dir}/include/${target_alias}"
      }
  
      set dir [lookfor_file ${srcdir} libstdc++]
      if { ${dir} != "" } {
! 	append flags " -I${dir} -I${dir}/stl"
      }
  
      return "$flags"
*************** proc libstdc++_include_flags { args } {
*** 317,335 ****
  
      set dir [lookfor_file ${srcdir} libstdc++-v3]
      if { ${dir} != "" } {
! 	append flags " -isystem ${dir}/include -isystem ${dir}/include/std -isystem ${dir}/include/c_std -isystem ${dir}/libsupc++"
      }
  
      set gccpath [get_multilibs]
  
      set dir [lookfor_file ${gccpath} libstdc++-v3]
      if { ${dir} != "" } {
! 	append flags " -isystem ${dir}/include -isystem ${dir}/include/${target_alias}"
      }
  
      set dir [lookfor_file ${srcdir} libstdc++]
      if { ${dir} != "" } {
! 	append flags " -isystem ${dir} -isystem ${dir}/stl"
      }
  
      return "$flags"
--- 317,335 ----
  
      set dir [lookfor_file ${srcdir} libstdc++-v3]
      if { ${dir} != "" } {
! 	append flags " -I${dir}/include -I${dir}/include/std -I${dir}/include/c_std -I${dir}/libsupc++"
      }
  
      set gccpath [get_multilibs]
  
      set dir [lookfor_file ${gccpath} libstdc++-v3]
      if { ${dir} != "" } {
! 	append flags " -I${dir}/include -I${dir}/include/${target_alias}"
      }
  
      set dir [lookfor_file ${srcdir} libstdc++]
      if { ${dir} != "" } {
! 	append flags " -I${dir} -I${dir}/stl"
      }
  
      return "$flags"
*************** proc winsup_include_flags { args } {
*** 867,873 ****
  	set winsup_dir [lookfor_file ${srcdir} winsup/include/windows.h]
  	if { ${winsup_dir} != "" } {
  	    set winsup_dir [file dirname ${winsup_dir}]
! 	    return " -isystem ${winsup_dir}"
  	}
      }
      verbose "No winsup support for this target"
--- 867,873 ----
  	set winsup_dir [lookfor_file ${srcdir} winsup/include/windows.h]
  	if { ${winsup_dir} != "" } {
  	    set winsup_dir [file dirname ${winsup_dir}]
! 	    return " -I${winsup_dir}"
  	}
      }
      verbose "No winsup support for this target"

Re: Dejagnu: use -isystem to include system header files.

[Daniel Jacobowitz]

On Thu, Nov 11, 2004 at 03:57:19PM +0000, Nick Clifton wrote:
> Hi Daniel,
> 
> >This patch will break in-tree testing for yet other targets.  I believe
> >arm-elf was affected - anything which does not set
> >NO_IMPLICIT_EXTERN_C.  I discussed this with H-P on the dejagnu list
> >but never figured out a solution, but...
> 
> >>       * lib/libgloss.exp (newlib_include_flags): Use -isystem, not -I.
> >>       (libio_include_flags, g++_include_flags, libstdc++_include_flags,
> >>       winsup_include_flags): Ditto.
> 
> >... I strongly suspect that g++ and winsup should be left out.
> 
> 
> Ok - what about this revision to the patched sources based upon Rob 
> Savoye's suggestion that -isystem is only needed for newlib includes ?
> 
> It appears to work for the xstormy16 port and I did not detect any 
> regression for the arm-elf port, so I think that it should be OK.
> 
> Assuming that you like this patch, shall I submit it to Rob for 
> inclusion in the official sources as well ?

I do like it, and I'd appreciate that!


-- 
Daniel Jacobowitz

Re: Dejagnu: use -isystem to include system header files.

[Hans-Peter Nilsson]

Yay cross-posting!  I helped by adding a list!

On Thu, 11 Nov 2004, Daniel Jacobowitz wrote:

> On Thu, Nov 11, 2004 at 11:58:15AM +0000, Nick Clifton wrote:
> > Hi Guys,
> >
> >   I am going to check in the attached patch which imports a fix from
> >   the mainline dejagnu sources.  This fix is to use the -isystem
> >   switch to include system header files rather than -I.  This fixes
> >   several unexpected failures in the GCC and G++ testsuites where the
> >   newlib system header file <limits.h> is included in strict ANSI
> >   mode, and the compiler barfs on the #include_next directive.
>
> This patch will break in-tree testing for yet other targets.

But presumably only for old broken systems: no non-obsolete
system should need to fake an extern "C" as is done when *not*
defining NO_IMPLICIT_EXTERN_C; they should all be C++-aware.

>  I believe
> arm-elf was affected - anything which does not set
> NO_IMPLICIT_EXTERN_C.

Bug in the arm-elf port: it should define NO_IMPLICIT_EXTERN_C.

Sounds like there's a bug in the -isystem interaction with
NO_IMPLICIT_EXTERN_C too.  (Like, always assume
NO_IMPLICIT_EXTERN_C for all passed -isystem options aka.
never fake 'extern "C"' for path-options given on the command
line.)

>  I discussed this with H-P on the dejagnu list
> but never figured out a solution, but...

Wot, there was no closure?  For the record, the original email:
<URL:http://lists.gnu.org/archive/html/bug-dejagnu/2002-10/msg00009.html>
(the reply was in 2002-11).  Our later conversation is at
<URL:http://lists.gnu.org/archive/html/dejagnu/2004-05/msg00003.html>.

I thought we had closure at
<URL:http://lists.gnu.org/archive/html/dejagnu/2004-05/msg00007.html>
or at least at
<URL:http://lists.gnu.org/archive/html/dejagnu/2004-05/msg00011.html>
but now that you bring it up, I just think it's more of a bug in
the arm-elf port and non-NO_IMPLICIT_EXTERN_C targets should be
xfailed for applicable tests. ;-)

> >         * lib/libgloss.exp (newlib_include_flags): Use -isystem, not -I.
> >         (libio_include_flags, g++_include_flags, libstdc++_include_flags,
> >         winsup_include_flags): Ditto.
>
> ... I strongly suspect that g++ and winsup should be left out.

Mh, a bit spurious.  Only actual C++ include directories could
be badly affected by non-NO_IMPLICIT_EXTERN_C-ness.

brgds, H-P

Re: Dejagnu: use -isystem to include system header files.

[Daniel Jacobowitz]

On Thu, Nov 11, 2004 at 07:25:19PM -0500, Hans-Peter Nilsson wrote:
> Bug in the arm-elf port: it should define NO_IMPLICIT_EXTERN_C.

Zack's been wanting to flip the default for NO_IMPLICIT_EXTERN_C for
ages.  But until that's been done - and done for a couple of years
- let's try to accomodate this in DejaGNU, OK :-)

> Wot, there was no closure?  For the record, the original email:
> <URL:http://lists.gnu.org/archive/html/bug-dejagnu/2002-10/msg00009.html>
> (the reply was in 2002-11).  Our later conversation is at
> <URL:http://lists.gnu.org/archive/html/dejagnu/2004-05/msg00003.html>.
> 
> I thought we had closure at
> <URL:http://lists.gnu.org/archive/html/dejagnu/2004-05/msg00007.html>
> or at least at
> <URL:http://lists.gnu.org/archive/html/dejagnu/2004-05/msg00011.html>
> but now that you bring it up, I just think it's more of a bug in
> the arm-elf port and non-NO_IMPLICIT_EXTERN_C targets should be
> xfailed for applicable tests. ;-)

There was closure that I needed to go try something, which I never got
around to :-)

-- 
Daniel Jacobowitz

Re: Dejagnu: use -isystem to include system header files.

[Zack Weinberg]

Daniel Jacobowitz <drow@false.org> writes:

> On Thu, Nov 11, 2004 at 07:25:19PM -0500, Hans-Peter Nilsson wrote:
>> Bug in the arm-elf port: it should define NO_IMPLICIT_EXTERN_C.
>
> Zack's been wanting to flip the default for NO_IMPLICIT_EXTERN_C for
> ages.  But until that's been done - and done for a couple of years
> - let's try to accomodate this in DejaGNU, OK :-)

I agree with Dan, but would like to point out that any newlib target
(I'm assuming arm-elf is such) ought to be able to take
NO_IMPLICIT_EXTERN_C right now.

zw