Re: [PATCH] gdb: use libtool in GDB_AC_CHECK_BFD

Re: [PATCH] gdb: use libtool in GDB_AC_CHECK_BFD

[Nick Alcock]

[Tried to Cc: gdb-patches, but I'm not yet subscribed to it from this
 address. Not sure if this is going to work... and libctf is part of
 binutils anyway, so I'm Cc:ing binutils@ in so that at least my
 response goes somewhere public.]

On 10 Nov 2022, Jose E. Marchesi said:

>>>>>>> "Jose" == Jose E Marchesi via Gdb-patches <gdb-patches@sourceware.org> writes:
>> Jose> 1) The calls to AM_ZLIB and AC_ZSTD from gdb/configure.ac.
>> Jose> 2) The definition of ZLIB and ZLIBINC from gdb/Makefile.in
>> Jose> 3) ../config/zlib.m4 from aclocal_m4_deps in gdb/Makefile.in
>>
>> ... however the use in ctf-api.h seems like it would prevent removing
>> some of the configury.  gdb includes this header and so it needs the
>> correct -I options to find the in-tree zlib.h.
>>
>> I don't see why ctf-api.h needs this include, but I essentially don't
>> know anything about CTF.
>
> Nick, would it be possible to remove the include of zlib.h from
> ctf-api.h?  Even if libctf uses zlib, it would be good to not expose the
> dependency in the API header...

Yeah, this is an interestingly painful one. I completely agree that it
needs fixing, because it causes problems if you don't have a system
zlib: you end up with a basically unusable ctf-api.h after installation,
which isn't good at all.

But... this is because of this function in the exported API:

extern int ctf_gzwrite (ctf_dict_t *fp, gzFile fd);

Now this is actually a typedef, described as "semi-opaque":

typedef struct gzFile_s *gzFile;    /* semi-opaque gzip file descriptor */

IF the name of gzfile_s is guaranteed unchanging across zlib releases we
could do this instead of the #include:

/* Note to users: this is a 'gzFile' from <zlib.h>.  */
struct gzFile_s;
extern int ctf_gzwrite (ctf_dict_t *fp, gzFile_s *fd);

The problem is that zlib apparently considers this structure name an
implementation detail (given that what it gives you in the prototype is
a typedef to it), but if that name ever changes then including
<ctf-api.h> and <zlib.h> in the same translation unit would fail. Indeed
this function used to take a void * (with the same typedef name) in
1.2.5 and before :(

Maybe I could just make it take a void *, but that's not very clear at
all! That type name is actually useful documentation... and I can't
introduce the typedef myself because double-declaring a typedef is an
error. (And a configure test won't help here, because the problem is the
zlib version installed when *users* include <ctf-api.h>...)

So... suggestions? Do people think gzFile_s is unlikely to be renamed
again or changed away from a structure in future, so that doing this is
safe? (Even when it was a void *, it was a structure internally, albeit
a totally different one.)

-- 
NULL && (void)

Re: [PATCH] gdb: use libtool in GDB_AC_CHECK_BFD

[Simon Marchi]

On 11/10/22 12:11, Nick Alcock via Gdb-patches wrote:
> [Tried to Cc: gdb-patches, but I'm not yet subscribed to it from this
>  address. Not sure if this is going to work... and libctf is part of
>  binutils anyway, so I'm Cc:ing binutils@ in so that at least my
>  response goes somewhere public.]
> 
> On 10 Nov 2022, Jose E. Marchesi said:
> 
>>>>>>>> "Jose" == Jose E Marchesi via Gdb-patches <gdb-patches@sourceware.org> writes:
>>> Jose> 1) The calls to AM_ZLIB and AC_ZSTD from gdb/configure.ac.
>>> Jose> 2) The definition of ZLIB and ZLIBINC from gdb/Makefile.in
>>> Jose> 3) ../config/zlib.m4 from aclocal_m4_deps in gdb/Makefile.in
>>>
>>> ... however the use in ctf-api.h seems like it would prevent removing
>>> some of the configury.  gdb includes this header and so it needs the
>>> correct -I options to find the in-tree zlib.h.
>>>
>>> I don't see why ctf-api.h needs this include, but I essentially don't
>>> know anything about CTF.
>>
>> Nick, would it be possible to remove the include of zlib.h from
>> ctf-api.h?  Even if libctf uses zlib, it would be good to not expose the
>> dependency in the API header...
> 
> Yeah, this is an interestingly painful one. I completely agree that it
> needs fixing, because it causes problems if you don't have a system
> zlib: you end up with a basically unusable ctf-api.h after installation,
> which isn't good at all.
> 
> But... this is because of this function in the exported API:
> 
> extern int ctf_gzwrite (ctf_dict_t *fp, gzFile fd);

I don't know if this is relevant, maybe this is an API that can't
change, but taking a step back, it seems strange in the first place to
leak the dependency on zlib through the API.  Imagine you wanted to
switch to another gz compression library without changing your API.  It
would be nicer if zlib was just an implementation detail here.  For
instance, you could take a plain fd and use gzdopen internally to open a
gzFile around it.

Simon

Re: [PATCH] gdb: use libtool in GDB_AC_CHECK_BFD

[Nix]

On 10 Nov 2022, Simon Marchi via Gdb-patches spake thusly:

> On 11/10/22 12:11, Nick Alcock via Gdb-patches wrote:
>> [Tried to Cc: gdb-patches, but I'm not yet subscribed to it from this
>>  address. Not sure if this is going to work... and libctf is part of
>>  binutils anyway, so I'm Cc:ing binutils@ in so that at least my
>>  response goes somewhere public.]
>> 
>> On 10 Nov 2022, Jose E. Marchesi said:
>> 
>>>>>>>>> "Jose" == Jose E Marchesi via Gdb-patches <gdb-patches@sourceware.org> writes:
>>>> Jose> 1) The calls to AM_ZLIB and AC_ZSTD from gdb/configure.ac.
>>>> Jose> 2) The definition of ZLIB and ZLIBINC from gdb/Makefile.in
>>>> Jose> 3) ../config/zlib.m4 from aclocal_m4_deps in gdb/Makefile.in
>>>>
>>>> ... however the use in ctf-api.h seems like it would prevent removing
>>>> some of the configury.  gdb includes this header and so it needs the
>>>> correct -I options to find the in-tree zlib.h.
>>>>
>>>> I don't see why ctf-api.h needs this include, but I essentially don't
>>>> know anything about CTF.
>>>
>>> Nick, would it be possible to remove the include of zlib.h from
>>> ctf-api.h?  Even if libctf uses zlib, it would be good to not expose the
>>> dependency in the API header...
>> 
>> Yeah, this is an interestingly painful one. I completely agree that it
>> needs fixing, because it causes problems if you don't have a system
>> zlib: you end up with a basically unusable ctf-api.h after installation,
>> which isn't good at all.
>> 
>> But... this is because of this function in the exported API:
>> 
>> extern int ctf_gzwrite (ctf_dict_t *fp, gzFile fd);
>
> I don't know if this is relevant, maybe this is an API that can't
> change, but taking a step back, it seems strange in the first place to
> leak the dependency on zlib through the API.

It's historical (it was there in the Solaris days, IIRC) and alas it has
users, and it has a use case (see below). We *could* rip it out and fix
the users, but that does mean breaking the API and bumping soname and
this seems like a rather petty reason to do so to me. (But then I know
I'm a stable-API fetishist.)

>                                               Imagine you wanted to
> switch to another gz compression library without changing your API.  It
> would be nicer if zlib was just an implementation detail here.  For
> instance, you could take a plain fd and use gzdopen internally to open a
> gzFile around it.

I think the intent here was that people who were already writing stuff
through a gzFile could pass it into this to write CTF in as well. That
doesn't work at all well if you pass in the underlying fd and do a
gzdopen (you end up with zlib thinking there are two gzip streams
underway simultaneously on the same fd, and a total mess results).

So really you'd need *both* :)


But really this whole class of API made more sense back in the past when
zlib was the only compression library worth speaking of. These days
you'd probably prefer to hand back the memory buffer and ask the caller
to do the compression themselves using whatever mechanism they prefer --
but the fact remains that this thing is in the API and I'd rather not
break it for such a minor reason if there is a non-totally-horrible
alternative.

-- 
NULL && (void)

Re: [PATCH] gdb: use libtool in GDB_AC_CHECK_BFD

[Simon Marchi]

> It's historical (it was there in the Solaris days, IIRC) and alas it has
> users, and it has a use case (see below). We *could* rip it out and fix
> the users, but that does mean breaking the API and bumping soname and
> this seems like a rather petty reason to do so to me. (But then I know
> I'm a stable-API fetishist.)
> 
>>                                               Imagine you wanted to
>> switch to another gz compression library without changing your API.  It
>> would be nicer if zlib was just an implementation detail here.  For
>> instance, you could take a plain fd and use gzdopen internally to open a
>> gzFile around it.
> 
> I think the intent here was that people who were already writing stuff
> through a gzFile could pass it into this to write CTF in as well. That
> doesn't work at all well if you pass in the underlying fd and do a
> gzdopen (you end up with zlib thinking there are two gzip streams
> underway simultaneously on the same fd, and a total mess results).
> 
> So really you'd need *both* :)
> 
> 
> But really this whole class of API made more sense back in the past when
> zlib was the only compression library worth speaking of. These days
> you'd probably prefer to hand back the memory buffer and ask the caller
> to do the compression themselves using whatever mechanism they prefer --
> but the fact remains that this thing is in the API and I'd rather not
> break it for such a minor reason if there is a non-totally-horrible
> alternative.

Thanks for the details.

Another idea I thought of, which would still break the API (in that
callers would need to include a new source file) but not the ABI would
be to put that declaration in a separate header, say ctf-gzip-api.h.
Users of ctf-api.h would not have to deal with zlib.  Users of
ctf-gzip-api.h would have to deal with zlib, but that's fine because
they are consciously using it.

Simon

Re: [PATCH] gdb: use libtool in GDB_AC_CHECK_BFD

[Nick Alcock]

On 11 Nov 2022, Simon Marchi stated:

> Another idea I thought of, which would still break the API (in that
> callers would need to include a new source file) but not the ABI would
> be to put that declaration in a separate header, say ctf-gzip-api.h.
> Users of ctf-api.h would not have to deal with zlib.  Users of
> ctf-gzip-api.h would have to deal with zlib, but that's fine because
> they are consciously using it.

This thing is distinctly rarely used, so I'd say this is the best
suggestion so far: it unbreaks gdb and all known existing users can
definitely adapt easily, and it avoids depending on nasty guts of zlib.
If ctf-gzip-api.h #included ctf-api.h itself, the needed change to users
would be even smaller.

I'm trying to get a new libctf patch series into shape and can add this
change to it if you like: alternatively, consider a suitable patch
pre-approved. (You'll need to add a suitable #include to
libctf/ctf-serialize.c.)