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

Message ID CABh_MKn1yUfKjtZO4nZ+XyvVXHUgtD7QE-hAM-AP6FNH=zKooQ@mail.gmail.com
State New, archived
Headers

Commit Message

Ed Schouten May 29, 2016, 2:58 p.m. UTC
  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.

    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?
  

Comments

Ed Schouten May 29, 2016, 4:36 p.m. UTC | #1
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 May 29, 2016, 5:24 p.m. UTC | #2
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.
  

Patch

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