Another HP-UX IA64 Build patch

Another HP-UX IA64 Build patch

[Steve Ellcey]

Here is the second of my IA64 HP-UX build patches.  This one involves
the declaration of basename() in include/libiberty.h.  What do people
think about using the prototype version whenever we are compiling with
GCC?

I did that rather than checking for HP-UX because some HP-UX systems
(old PA ones) cannot handle the prototype but it seems like GCC should
always be able to deal with it and it is when using GCC that "-Wall
-Werror" will be set so I thought it might be better to check for GCC
rather than HP-UX.  Does this seem reasonable to people?  Maybe we could
remove some of the OS checks if we checked for GCC instead.

Tested on IA64 HP-UX.

Steve Ellcey
sje@cup.hp.com

include/ChangeLog:

2005-05-05  Steve Ellcey  <sje@cup.hp.com>

	libiberty.h: Use prototype for basename if compiling with GCC.


*** src.orig/include/libiberty.h	Thu May  5 10:04:07 2005
--- src/include/libiberty.h	Thu May  5 10:07:44 2005
*************** extern char **dupargv (char **) ATTRIBUT
*** 94,100 ****
     to find the declaration so provide a fully prototyped one.  If it
     is 1, we found it so don't provide any declaration at all.  */
  #if !HAVE_DECL_BASENAME
! #if defined (__GNU_LIBRARY__ ) || defined (__linux__) || defined (__FreeBSD__) || defined (__OpenBSD__) || defined(__NetBSD__) || defined (__CYGWIN__) || defined (__CYGWIN32__) || defined (__MINGW32__) || defined (HAVE_DECL_BASENAME)
  extern char *basename (const char *);
  #else
  extern char *basename ();
--- 94,100 ----
     to find the declaration so provide a fully prototyped one.  If it
     is 1, we found it so don't provide any declaration at all.  */
  #if !HAVE_DECL_BASENAME
! #if defined (__GNU_LIBRARY__ ) || defined (__GNUC__) || defined (__linux__) || defined (__FreeBSD__) || defined (__OpenBSD__) || defined(__NetBSD__) || defined (__CYGWIN__) || defined (__CYGWIN32__) || defined (__MINGW32__) || defined (HAVE_DECL_BASENAME)
  extern char *basename (const char *);
  #else
  extern char *basename ();

Re: Another HP-UX IA64 Build patch

[Daniel Jacobowitz]

On Thu, May 05, 2005 at 10:14:37AM -0700, Steve Ellcey wrote:
> Here is the second of my IA64 HP-UX build patches.  This one involves
> the declaration of basename() in include/libiberty.h.  What do people
> think about using the prototype version whenever we are compiling with
> GCC?
> 
> I did that rather than checking for HP-UX because some HP-UX systems
> (old PA ones) cannot handle the prototype but it seems like GCC should
> always be able to deal with it and it is when using GCC that "-Wall
> -Werror" will be set so I thought it might be better to check for GCC
> rather than HP-UX.  Does this seem reasonable to people?  Maybe we could
> remove some of the OS checks if we checked for GCC instead.
> 
> Tested on IA64 HP-UX.

This is wrong.  If your system headers have a conflicting prototype,
you will lose, regardless of what compiler you're using.

Do you really not have a prototype for basename?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: Another HP-UX IA64 Build patch

[Steve Ellcey]

> This is wrong.  If your system headers have a conflicting prototype,
> you will lose, regardless of what compiler you're using.
> 
> Do you really not have a prototype for basename?
> 
> -- 
> Daniel Jacobowitz
> CodeSourcery, LLC

Yes, I have a prototype for basename on IA64 HP-UX.  It is in
/usr/include/libgen.h:

extern char *basename(char *);

I see it doesn't have the const modifier that libiberty has.  I didn't
get a warning or error presumbably because the build didn't include
libgen.h.

The problem I was trying to avoid is with PA HP-UX.  It has the same
prototype but if someone is trying to build binutils with the bundled
HP-UX compiler, that compiler doesn't handle prototypes.  It is an old
K&R compiler that is really just intended to be used to rebuild the
kernel.  The real HP compiler that people can buy is, of course, a full
ANSI C compiler.  But it looks like we require a full ANSI compiler to
build binutils now, I see all the uses of PARAM are gone now so I guess
I don't need to worry about that scenario anymore.

So should I just add __hpux__ to the list of OS's or is the difference
between 'char *' and 'const char *' going to bite me somewhere that I
haven't seen yet?

Steve Ellcey
sje@cup.hp.com

Re: Another HP-UX IA64 Build patch

[James E Wilson]

On Thu, 2005-05-05 at 10:51, Steve Ellcey wrote:
> I see it doesn't have the const modifier that libiberty has.  I didn't
> get a warning or error presumbably because the build didn't include
> libgen.h.

This is a libiberty bug.  Try "man basename" on a linux system, and it
will tell you that there are two versions of basename.  The posix one
which takes a char * argument, and a glibc _GNU_SOURCE one which takes a
const char * argument.  So this probably should be something like
#if defined(_GNU_SOURCE)
extern char *basename (const char *);
#elif defined...
extern char *basename (char *);
#else
extern char *basename ();
#endif

But it appears that nobody is including libgen.h anywhere, or we would
have caught this before.

For now, I think just adding __hpux__ to the list is fine, and we can
worry about the wrong prototype later, when and if it causes a problem.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: Another HP-UX IA64 Build patch

[Daniel Jacobowitz]

On Thu, May 05, 2005 at 11:10:30AM -0700, James E Wilson wrote:
> On Thu, 2005-05-05 at 10:51, Steve Ellcey wrote:
> > I see it doesn't have the const modifier that libiberty has.  I didn't
> > get a warning or error presumbably because the build didn't include
> > libgen.h.
> 
> This is a libiberty bug.  Try "man basename" on a linux system, and it
> will tell you that there are two versions of basename.  The posix one
> which takes a char * argument, and a glibc _GNU_SOURCE one which takes a
> const char * argument.  So this probably should be something like
> #if defined(_GNU_SOURCE)
> extern char *basename (const char *);
> #elif defined...
> extern char *basename (char *);
> #else
> extern char *basename ();
> #endif
> 
> But it appears that nobody is including libgen.h anywhere, or we would
> have caught this before.

Wait, huh?  What do glibc's quirks have to do with HP/UX's libgen.h?

If you use <libgen.h> on a glibc system you're going to get it without
the const, no matter what _GNU_SOURCE says.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: Another HP-UX IA64 Build patch

[James E Wilson]

On Thu, 2005-05-05 at 11:12, Daniel Jacobowitz wrote:
> Wait, huh?  What do glibc's quirks have to do with HP/UX's libgen.h?
> If you use <libgen.h> on a glibc system you're going to get it without
> the const, no matter what _GNU_SOURCE says.

The prototype in libiberty.h disagrees with POSIX, which means it
disagrees with all systems other than a glibc system.  Thus confusion. 
The confusion is solved by pointing out the glibc quirk.  That was all I
was trying to do.

However, I don't think think this matters much.  As I said in the
earlier message, I think we should just use this for hpux anyways.
We change libiberty only when and if a problem arises, and I don't
expect one to arise.  The declarations in libiberty shouldn't be used if
there is a system declaration, so there shouldn't be a possibility of a
mismatch here unless there are configure problems.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: Another HP-UX IA64 Build patch

[DJ Delorie]

> > I see it doesn't have the const modifier that libiberty has.  I didn't
> > get a warning or error presumbably because the build didn't include
> > libgen.h.
> 
> This is a libiberty bug.

It's not supposed to be.  Applications using libiberty.h need to
define HAVE_DECL_BASENAME properly.  If a prototype is found in the
system headers, libiberty.h should not provide a prototype *at all*.

If the application checks and does not find any prototype, libiberty.h
should provide a full prototype.

If the application does not check, libiberty.h will provide a
K&R-style empty prototype.

All the os-specific exceptions are there because a lot of application
developers are too lazy to add the checks in all over the place
(understandable) so they special cased their OS instead.  Adding to
the list is, in general, the wrong thing to do.  If anything, the list
should be shrinking as applications learn to properly test for the
prototype.

Re: Another HP-UX IA64 Build patch

[James E Wilson]

On Thu, 2005-05-05 at 12:28, DJ Delorie wrote:
> > This is a libiberty bug.
> It's not supposed to be.

Yes, it isn't a bug.  I over reached a bit while trying to explain the
basename prototype confusion to Steve.

The important part of my message was the bit at the end where I said we
fix libiberty when and if it causes a problem.  And since there is
apparently no problem, I'm not recommending any fix.  Other than perhaps
adding __hpux__ to the list if Steve needs it.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: Another HP-UX IA64 Build patch

[Andreas Schwab]

James E Wilson <wilson@specifixinc.com> writes:

> On Thu, 2005-05-05 at 10:51, Steve Ellcey wrote:
>> I see it doesn't have the const modifier that libiberty has.  I didn't
>> get a warning or error presumbably because the build didn't include
>> libgen.h.
>
> This is a libiberty bug.  Try "man basename" on a linux system, and it
> will tell you that there are two versions of basename.  The posix one
> which takes a char * argument, and a glibc _GNU_SOURCE one which takes a
> const char * argument.  So this probably should be something like
> #if defined(_GNU_SOURCE)
> extern char *basename (const char *);
> #elif defined...
> extern char *basename (char *);
> #else
> extern char *basename ();
> #endif

Note that the POSIX basename function may modify the string in place (when
removing trailing slashes).  The GNU (and libiberty) version (naturally)
never does this.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Another HP-UX IA64 Build patch

[DJ Delorie]

> Note that the POSIX basename function may modify the string in place (when
> removing trailing slashes).  The GNU (and libiberty) version (naturally)
> never does this.

If you care, use lbasename() instead.

Re: Another HP-UX IA64 Build patch

[James E Wilson]

On Thu, 2005-05-05 at 10:14, Steve Ellcey wrote:
> I did that rather than checking for HP-UX because some HP-UX systems
> (old PA ones) cannot handle the prototype

Is this because they ship with K&R C compilers?  If so, then we don't
care.  Binutils no longer supports compilation by K&R C compilers.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: Another HP-UX IA64 Build patch

[James E Wilson]

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

On Thu, 2005-05-05 at 10:14, Steve Ellcey wrote:
> Here is the second of my IA64 HP-UX build patches.  This one involves
> the declaration of basename() in include/libiberty.h.  What do people
> think about using the prototype version whenever we are compiling with
> GCC?

Never mind, I see the problem.  The question about the prototype
confused me, and put me on the wrong track.  Try this patch.  We need to
include sysdep.h before libiberty.h, so that libiberty.h gets the proper
HAVE_DECL_BASENAME definition.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com


[-- Attachment #2: patch.include.order --]
[-- Type: text/plain, Size: 735 bytes --]

2005-05-05  James E Wilson  <wilson@specifixinc.com>

	* ia64-opc.c: Include sysdep.h before libiberty.h.

Index: ia64-opc.c
===================================================================
RCS file: /cvs/src/src/opcodes/ia64-opc.c,v
retrieving revision 1.8
diff -p -p -r1.8 ia64-opc.c
*** ia64-opc.c	3 Mar 2005 11:49:49 -0000	1.8
--- ia64-opc.c	5 May 2005 20:05:54 -0000
***************
*** 20,27 ****
     02111-1307, USA.  */
  
  #include "ansidecl.h"
- #include "libiberty.h"
  #include "sysdep.h"
  #include "ia64-asmtab.h"
  #include "ia64-asmtab.c"
  
--- 20,27 ----
     02111-1307, USA.  */
  
  #include "ansidecl.h"
  #include "sysdep.h"
+ #include "libiberty.h"
  #include "ia64-asmtab.h"
  #include "ia64-asmtab.c"
  

Re: Another HP-UX IA64 Build patch

[Steve Ellcey]

> Never mind, I see the problem.  The question about the prototype
> confused me, and put me on the wrong track.  Try this patch.  We need to
> include sysdep.h before libiberty.h, so that libiberty.h gets the proper
> HAVE_DECL_BASENAME definition.
> -- 
> Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

This patch didn't seem to do anything for my build.  With or without
this change I need to use the patch I just sent for libiberty where I
use the prototype for basename if on hpux.

Steve Ellcey
sje@cup.hp.com

Re: Another HP-UX IA64 Build patch

[DJ Delorie]

> This patch didn't seem to do anything for my build.

Before considering a change to libiberty.h, please check the
following:

* Does configure attempt to discover if a prototype for basename is
  already available?

* If so, why is the logic failing in your case?

Re: Another HP-UX IA64 Build patch

[James E Wilson]

On Thu, 2005-05-05 at 13:42, DJ Delorie wrote:
> Before considering a change to libiberty.h, please check the
> following:
> * Does configure attempt to discover if a prototype for basename is
>   already available?
> * If so, why is the logic failing in your case?

This is somewhat complicated because I don't have access to ia64-hpux,
and I'm having to guess on some of this.  However, my understanding of
the problem is as follows.

libiberty.h uses HAVE_DECL_BASENAME.  It will provide a prototype only
if HAVE_DECL_BASENAME is defined (in which case it comes from the OS),
or if HAVE_DECL_BASENAME is defined to zero (in which case it comes from
libiberty.h itself).  If HAVE_DECL_BASENAME is not defined (i.e. no
configure test for it), then libiberty.h provides a non-prototype
declaration.

gas is compiled with -Wstrict-prototypes -Wmissing-prototypes -Werror,
which means a non-prototype declaration will result in a compiler error.

This means that if we want to use libiberty.h, we must always have a
configure test for basename, regardless of whether we use basename or
not.  If the configure test is missing, then we will get a compiler
error when libiberty.h is included.

In opcodes the configure test is there, but opc-ia64.c fails to include
config.h before libiberty.h.  My patch fixes this.  Steve Ellcey has
confirmed this in private mail.

Unfortunately, there are other files with similar problems in different
directories.  The list I just got from Steve Ellcey in private mail is
gas/as.h and binutils/size.c.  In these two cases, we will have to add
configure tests for basename, even though we don't call it, in order to
avoid the non-prototype definition in libiberty.h.

There is a way to hack around this mess.  We can modify libiberty.h to
define the prototype for hpux when HAVE_DECL_BASENAME is undefined (i.e.
the configure check is missing).  This hack has already been performed
for a number of different operating systems.  This hack has not yet been
performed for hpux.  This is why the problem only shows up on hpux.

I am willing to go either way on this if people want to express an
opinion here.  We can either add extra otherwise unnecessary configure
checks for basename, or we can hack libiberty.h for hpux.

I suppose there is a third option which is to simplify libiberty.h to
always emit the prototype for basename.  basename is the only function
in libiberty.h for which we can emit a non-prototype when HAVE_DECL_foo
is undefined.  The others always emit the prototype.  I don't know why
this is; I haven't tried looking yet.  This change is probably no where
near as safe as the previous two suggestions.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: Another HP-UX IA64 Build patch

[DJ Delorie]

One option is to not allow the use of basename() without the
HAVE_DECL_BASENAME test.  We can do this by #definining basename() to
you_forgot_the_have_decl_basename_check() in the non-prototype case in
libiberty.h.

I suspect that would break a *lot* of unsuspecting applications.  But,
we can't have a basename that's unprototyped on, for example, amd64
machines where the return type must be known (pointer vs int).

But how does gcc deal with system headers that have empty prototypes
for functions?  If we can hook into that, that would help.  But I
wouldn't recommend using <libiberty.h> where "libiberty.h" is
appropriate.

> I suppose there is a third option which is to simplify libiberty.h
> to always emit the prototype for basename.

basename is different because we *can't* just emit a prototype, as it
sometimes conflicts with the OS's prototype.

> basename is the only function in libiberty.h for which we can emit a
> non-prototype when HAVE_DECL_foo is undefined.

It's the only one that doesn't return "int" so we must, or we corrupt
pointers on 64-bit-pointer machines.

Re: Another HP-UX IA64 Build patch

[James E Wilson]

On Thu, 2005-05-05 at 15:24, DJ Delorie wrote:
> One option is to not allow the use of basename() without the
> HAVE_DECL_BASENAME test.  We can do this by #definining basename() to
> you_forgot_the_have_decl_basename_check() in the non-prototype case in
> libiberty.h.

This makes the solution more obvious, but does not seem to fix anything.

The problem here is that libiberty.h declares basename always, even if
you aren't using it.  Thus we must always have a configure test for
basename even if we aren't using it.  This change just makes it more
obvious that the otherwise unnecessary configure test is missing.

Also, as you mention, this is likely to break lots of code using
libiberty.h that isn't already using -Wstrict-prototypes and -Werror, as
the current libiberty.h code fails only in this case.

> But how does gcc deal with system headers that have empty prototypes
> for functions?  If we can hook into that, that would help.  But I
> wouldn't recommend using <libiberty.h> where "libiberty.h" is
> appropriate.

I think the main answer to this is that now we require an ISO C
compiler, everybody has prototypes for everything that matters.

-Wstrict-prototypes does not warn for declarations in system header
files.  -Wmissing-prototypes requires a prototype before use.  So we
only need to ensure that a prototype exists in the local gcc system.h
(or whatever) if there is no prototype in the system header files. 
However, I don't think this is a problem, because I think everybody we
care about has prototypes in the system header files nowadays.

This fails in the libiberty.h case, because libiberty.h is not a system
header file, and contains a non-prototype declaration for basename. 
Using <libiberty.h> would fix this, but as you say, might cause other
problems, like getting the wrong version of it.

The use of libiberty.h in gcc itself is not a problem, because gcc has
the basename configure check.
 
> basename is different because we *can't* just emit a prototype, as it
> sometimes conflicts with the OS's prototype.

The prototype is only emitted if there is a configure check for
basename, and configure failed to find it.  In this case, there should
be no conflict unless the configure check is broken

> It's the only one that doesn't return "int" so we must, or we corrupt
> pointers on 64-bit-pointer machines.

So what you are saying here is that we declare basename even if there is
no configure check for basename, just in case someone needed the
configure check but forgot to perform it.  However, in this case, I
would argue that the libiberty.h user is clearly broken (because of the
missing configure check), and it isn't libiberty's responsibility to
make it work.

Changing this might break some programs that have relied on this feature
though.

This brings us back to the previously suggested solutions, hack in
__hpux__ like is done for other targets, or add otherwise unnecessary
basename configure checks in the binutils and gas directories.

I suppose if we want a more involved fix, we could modify gcc to define
some macros when -Wstrict-prototypes -Wmissing-prototypes and -Werror
are used.  We can then modify libiberty.h to check for these macros.  If
all three are defined, then we don't emit the non-prototype declaration
of basename.  In this case, if someone did use it by accident without
the configure check, and it was necessary, then they would get an error
from gcc for the missing prototype.  This seems safe, but requires a new
gcc release to build binutils, which is impractical.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: Another HP-UX IA64 Build patch

[DJ Delorie]

> The problem here is that libiberty.h declares basename always, even if
> you aren't using it.

No, I'm proposing that we do NOT declare basename if the application
didn't run the HAVE_DECL test.  However, to prevent applications from
using basename() without the HAVE_DECL test (the char * return
problem), we then add a #define to effectively poison it.

Thus, we need the HAVE_DECL test if we use basename(), but if we don't
use basename() we don't need the HAVE_DECL test.

Re: Another HP-UX IA64 Build patch

[Steve Ellcey]

> From: DJ Delorie <dj@redhat.com>
>
> No, I'm proposing that we do NOT declare basename if the application
> didn't run the HAVE_DECL test.  However, to prevent applications from
> using basename() without the HAVE_DECL test (the char * return
> problem), we then add a #define to effectively poison it.
> 
> Thus, we need the HAVE_DECL test if we use basename(), but if we don't
> use basename() we don't need the HAVE_DECL test.

So how about something like the following (tested on IA64 HP-UX):


include/ChangeLog:

2005-05-09  Steve Ellcey  <sje@cup.hp.com>

	libiberty.h: Do not define empty basename prototype.


*** src.orig/include/libiberty.h	Thu May  5 10:04:07 2005
--- src/include/libiberty.h	Mon May  9 16:03:12 2005
*************** extern char **dupargv (char **) ATTRIBUT
*** 97,103 ****
  #if defined (__GNU_LIBRARY__ ) || defined (__linux__) || defined (__FreeBSD__) || defined (__OpenBSD__) || defined(__NetBSD__) || defined (__CYGWIN__) || defined (__CYGWIN32__) || defined (__MINGW32__) || defined (HAVE_DECL_BASENAME)
  extern char *basename (const char *);
  #else
! extern char *basename ();
  #endif
  #endif
  
--- 97,106 ----
  #if defined (__GNU_LIBRARY__ ) || defined (__linux__) || defined (__FreeBSD__) || defined (__OpenBSD__) || defined(__NetBSD__) || defined (__CYGWIN__) || defined (__CYGWIN32__) || defined (__MINGW32__) || defined (HAVE_DECL_BASENAME)
  extern char *basename (const char *);
  #else
! /* Do not allow basename to be used if there is no prototype seen.  We
!    either need to use the above prototype or have one from
!    autoconf which would result in HAVE_DECL_BASENAME being set.  */
! #define basename basename_cannot_be_used_without_a_prototype
  #endif
  #endif
  

Re: Another HP-UX IA64 Build patch

[DJ Delorie]

Yes, that was what I was thinking.  We'll have to be careful to check
for all the potential places where basename() is used and make sure
they do the HAVE_DECL thing, else they'll break if/when this is
committed.

Re: Another HP-UX IA64 Build patch

[Steve Ellcey]

> From: DJ Delorie <dj@redhat.com>
>
> Yes, that was what I was thinking.  We'll have to be careful to check
> for all the potential places where basename() is used and make sure
> they do the HAVE_DECL thing, else they'll break if/when this is
> committed.

OK, it looks like the official include/libiberty.h is kept in the GCC
archive so I will take my patch over there and propose it.

I'm still looking for approval/checkin for two other patches, an
obvious IA64 testsuite fix:

	http://sourceware.org/ml/binutils/2005-05/msg00288.html

and a patch to not use getc_unlocked if you don't have a declaration for
it:

	http://sourceware.org/ml/binutils/2005-05/msg00218.html

Steve Ellcey
sje@cup.hp.com

Re: Another HP-UX IA64 Build patch

[James E Wilson]

On Thu, 2005-05-05 at 13:10, James E Wilson wrote:
>       * ia64-opc.c: Include sysdep.h before libiberty.h.

I've checked this in, now that I got confirmation that it does solve the
problem for this one file.

Unfortunately, other files have similar problems.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com