Likely incorrect patch: remove the faulty basename() prototype from libiberty

Likely incorrect patch: remove the faulty basename() prototype from libiberty

[Ed Schouten]

Hi there,

Libiberty has the prototype of basename() declared in its libiberty.h.
Unfortunately, it has it declared with a different prototype as
standardized by POSIX. POSIX requires that its argument is of type
"char *', while the one in libiberty.h uses "char *". Normally this
problem is not exposed at all, because none of the GDB code includes
the header file with the actual prototype (libgen.h).

I'm currently working on patching up FreeBSD to also use the prototype
as standardized by POSIX, as opposed to the BSD-specific one that has
"const char *'. In FreeBSD we have some utilities built against GDB
(e.g., our kernel debugger kgdb) that ends up including both
libiberty.h and libgen.h. meaning we get compiler errors due to
different declarations for the same function.

For now I'm solving this by simply patching up libiberty to pull in
the declaration from libgen.h. This seems to make everything build
again.

diff --git a/include/libiberty.h b/include/libiberty.h
index 8e096a0..403e690 100644
--- a/include/libiberty.h
+++ b/include/libiberty.h
@@ -109,7 +109,7 @@ extern int countargv (char**);
  || defined (__FreeBSD__) || defined (__OpenBSD__) || defined (__NetBSD__) \
  || defined (__CYGWIN__) || defined (__CYGWIN32__) || defined (__MINGW32__) \
  || defined (__DragonFly__) || defined (HAVE_DECL_BASENAME)
-extern char *basename (const char *) ATTRIBUTE_RETURNS_NONNULL
ATTRIBUTE_NONNULL(1);
+#include <libgen.h>
 #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

I guess that this is not the right way of solving this. I'd guess that
we should just remove all of the basename() specific bits from all of
libiberty and just include libgen.h in the source files that actually
need it.

What do you folks think?

-- 
Ed Schouten <ed@nuxi.nl>
Nuxi, 's-Hertogenbosch, the Netherlands
KvK-nr.: 62051717

Re: Likely incorrect patch: remove the faulty basename() prototype from libiberty

[DJ Delorie]

Ed Schouten <ed@nuxi.nl> writes:
> For now I'm solving this by simply patching up libiberty to pull in
> the declaration from libgen.h. This seems to make everything build
> again.

Unless you check *every* one of those operating systems, and put in a
configure test for libgen and test for it, this patch is wrong.

Most likely, you're not calling configure right, else it would have seen
the right prototype and skipped this part of the header.  Or libiberty's
configure and tests need to check for libgen.h as well as the other
headers it's checking for.

Either way, you can't blindly depend on a header being present and
useful.  "Seems to" is insufficient evidence that a patch is correct.

Re: Likely incorrect patch: remove the faulty basename() prototype from libiberty

[Ed Schouten]

Hi there,

2016-05-29 18:32 GMT+02:00 DJ Delorie <dj@redhat.com>:
> Most likely, you're not calling configure right, else it would have seen
> the right prototype and skipped this part of the header.  Or libiberty's
> configure and tests need to check for libgen.h as well as the other
> headers it's checking for.

Exactly. ./configure does not check for the existence of libgen.h, nor
does it try to include it anywhere in its codebase.

> Either way, you can't blindly depend on a header being present and
> useful.  "Seems to" is insufficient evidence that a patch is correct.

For the local change I'm going to make to the FreeBSD base system and
ports collection, this works for me. I'm merely forwarding the patch
that I'm currently using.

That said, we can easily validate whether <libgen.h> is always
present. What's the official list on which GDB is supposed to build?
It's merely a matter of going down that list and looking at
documentation/source files.

-- 
Ed Schouten <ed@nuxi.nl>
Nuxi, 's-Hertogenbosch, the Netherlands
KvK-nr.: 62051717

Re: Likely incorrect patch: remove the faulty basename() prototype from libiberty

[DJ Delorie]

I think the right path to go down is to add libgen.h to the list in
AC_CHECK_HEADERS for every project that uses basename() (including
libiberty itself, and binutils, gdb, and gcc), and then everyone who
uses basename() from libiberty.h would need to include libgen.h *if*
it's found by configure.

Re: Likely incorrect patch: remove the faulty basename() prototype from libiberty

[Ed Schouten]

Hi DJ,

2016-05-29 19:06 GMT+02:00 DJ Delorie <dj@redhat.com>:
> I think the right path to go down is to add libgen.h to the list in
> AC_CHECK_HEADERS for every project that uses basename() (including
> libiberty itself, and binutils, gdb, and gcc), and then everyone who
> uses basename() from libiberty.h would need to include libgen.h *if*
> it's found by configure.

If the users of libiberty have to be explicitly patched up to include
<libgen.h> to get basename(),  what's the use of having any of it part
of libiberty in the first place? Wouldn't that practically be the same
as removing basename() from libiberty altogether?

That approach also sounds fine by me, but then again, I'm just an
outsider to this project.

-- 
Ed Schouten <ed@nuxi.nl>
Nuxi, 's-Hertogenbosch, the Netherlands
KvK-nr.: 62051717

Re: Likely incorrect patch: remove the faulty basename() prototype from libiberty

[DJ Delorie]

Libiberty provides basename() for those platforms that do not have one.

On platforms that *do* have one, you need to tell libiberty that you
already have one.  In your case, including some other header that
prototypes it, and *telling* libiberty that you have done so, is the
right solution.

Including a header that prototypes it, and *not* telling libiberty that
you have done so, leads to the error.

So each project must decide if it has a basename, and if not, it relies
on libiberty to provide one.  If you decide you have a basename and
*also* ask libiberty to provide one, you end up with a conflict.

Most of this difficulty comes from the fact that autoconf is run by gdb,
not libiberty, to control the macros that libiberty's includes see when
included by gdb.  There is no way for libiberty's autoconf to tell gdb
what to do.  So, we can't have a solution that's 100% controlled by
libiberty alone.

Re: Likely incorrect patch: remove the faulty basename() prototype from libiberty

[Joseph Myers]

On Sun, 29 May 2016, DJ Delorie wrote:

> I think the right path to go down is to add libgen.h to the list in
> AC_CHECK_HEADERS for every project that uses basename() (including
> libiberty itself, and binutils, gdb, and gcc), and then everyone who
> uses basename() from libiberty.h would need to include libgen.h *if*
> it's found by configure.

Note that there are two different and incompatible basename functions.  
libgen.h has the POSIX one.  libiberty has the GNU one (which glibc 
declares in <string.h> with _GNU_SOURCE).  If you include libgen.h with 
glibc, you get basename defined to __xpg_basename, which presumably is not 
what is wanted by applications using libiberty.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Likely incorrect patch: remove the faulty basename() prototype from libiberty

[Ed Schouten]

Hi Joseph,

2016-06-02 23:27 GMT+02:00 Joseph Myers <joseph@codesourcery.com>:
> Note that there are two different and incompatible basename functions.

... when using glibc.

On systems with other C libraries, there is typically only one variant
of basename(). In some cases it tends to be the BSD one, in others it
tends to be the one from POSIX (which glibc calls __xpg_basename).

On systems that do provide the POSIX variant, consumers of libiberty
already use it, regardless of what they'd want.

-- 
Ed Schouten <ed@nuxi.nl>
Nuxi, 's-Hertogenbosch, the Netherlands
KvK-nr.: 62051717