[PR,build/24805] Explicitly export symbols from gdb_proc_service

  On 1/6/20 11:39 PM, Norbert Lange wrote:
> Am Mo., 6. Jan. 2020 um 20:21 Uhr schrieb Pedro Alves <palves@redhat.com>:
>>
>> On 1/4/20 8:20 PM, Norbert Lange wrote:
>>> Compiling GDB with '-fvisibility=hidden' will remove the
>>> symbols that should be exported.
>>> This patch explicitly marks them as visible.
>>
>> Curious.  We have gdb/proc-service.list supposedly for this,
>> doesn't -Wl,--dynamic-list work with -fvisibility=hidden then?
>>
> 
> Obviously it doesn't, else I would not have spent time figuring out
> why libthread_db wont load.

OK.  Haven't looked at visibility issues in years.  It also wasn't clear
to me whether the issue could be that -Wl,--dynamic-list wasn't used
in your build for some reason, maybe related to how you're configuring GDB.

> -Wl,--dynamic-list merely filters the visible symbols, it does not see
> "hidden" ones.

BTW, you didn't post an actual patch to the list:
 https://sourceware.org/ml/gdb-patches/2020-01/msg00083.html

I found it in bugzilla, though.

We can't drop support for the glibc header, since that would mean
to also drop support for the Solaris version of the header, and
also for whatever other libcs that people build gdb with (e.g.,
does musl have its own version of the header with different types?).
Up until not so long ago, glibc didn't use to install the header,
that's why we keep a local copy, IIRC.

How about something like this?  It's similar to your #2 at
<https://sourceware.org/bugzilla/show_bug.cgi?id=24805#c3>, but
I'm using typeof to avoid issues with different systems using
different prototypes.

From 224b74b0fab67cdf25119f16be98a80ada071a09 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 14 Jan 2020 15:30:49 +0000
Subject: [PATCH] Ensure proc-service symbols have default visibility

---
 gdb/gdbsupport/gdb_proc_service.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)


base-commit: 7da6a5b938b426379f61e56e259a925bedfe242b
  

On 1/14/20 3:36 PM, Pedro Alves wrote:
> On 1/6/20 11:39 PM, Norbert Lange wrote:
>> Am Mo., 6. Jan. 2020 um 20:21 Uhr schrieb Pedro Alves <palves@redhat.com>:
>>>
>>> On 1/4/20 8:20 PM, Norbert Lange wrote:
>>>> Compiling GDB with '-fvisibility=hidden' will remove the
>>>> symbols that should be exported.
>>>> This patch explicitly marks them as visible.
>>>
>>> Curious.  We have gdb/proc-service.list supposedly for this,
>>> doesn't -Wl,--dynamic-list work with -fvisibility=hidden then?
>>>
>>
>> Obviously it doesn't, else I would not have spent time figuring out
>> why libthread_db wont load.
> 
> OK.  Haven't looked at visibility issues in years.  It also wasn't clear
> to me whether the issue could be that -Wl,--dynamic-list wasn't used
> in your build for some reason, maybe related to how you're configuring GDB.
> 
>> -Wl,--dynamic-list merely filters the visible symbols, it does not see
>> "hidden" ones.
> 
> BTW, you didn't post an actual patch to the list:
>  https://sourceware.org/ml/gdb-patches/2020-01/msg00083.html

I knew I must have been confused.  I found your patch on the list,
as a reply to that...

> 
> I found it in bugzilla, though.
> 


> How about something like this?  It's similar to your #2 at
> <https://sourceware.org/bugzilla/show_bug.cgi?id=24805#c3>, but
> I'm using typeof to avoid issues with different systems using
> different prototypes.

BTW, I forgot to mention why I suggested this as alternative
to the push/pop approach.  It was that the push/pop approach
makes everything indirectly included by <proc_service.h>
have default visibility too.  I don't know whether that ends
up being any function in practice, though.

Thanks,
Pedro Alves
  

Am Di., 14. Jan. 2020 um 16:36 Uhr schrieb Pedro Alves <palves@redhat.com>:
>
> We can't drop support for the glibc header, since that would mean
> to also drop support for the Solaris version of the header, and
> also for whatever other libcs that people build gdb with (e.g.,
> does musl have its own version of the header with different types?).
> Up until not so long ago, glibc didn't use to install the header,
> that's why we keep a local copy, IIRC.

No, you don't get what I am trying to say.
Commonly you only use external headers for function calls,
and the exact definition can vary a bit.

What you are doing, for ex. in proc-service.c is implementing the function.

like:
> ps_err_e
> ps_pdread (struct ps_prochandle *ph, psaddr_t addr, void *buf, size_t size)

you don't get much, if any, information from the header that's not replicated
in the source. The only upside would be to be able to detect mismatches
(but that could be done in a separate test_external_proc_header.c,
potentially even during configuration).

Whatever other libc's (musl doesnt provide/use this AFAIK) or OS's do,
including their header does not help at all?

You get no mismatch: Don't need it.
You get a mismatch: you need to fix that in your c file aswell.

>
> How about something like this?  It's similar to your #2 at
> <https://sourceware.org/bugzilla/show_bug.cgi?id=24805#c3>, but
> I'm using typeof to avoid issues with different systems using
> different prototypes.

Yeah, that's better, unless typeof is not supported by the compiler
(or its C++ mode).

Am Di., 14. Jan. 2020 um 16:47 Uhr schrieb Pedro Alves <palves@redhat.com>:
>
> On 1/14/20 3:36 PM, Pedro Alves wrote:
> > On 1/6/20 11:39 PM, Norbert Lange wrote:
> >> Am Mo., 6. Jan. 2020 um 20:21 Uhr schrieb Pedro Alves <palves@redhat.com>:
> >>>
> >>> On 1/4/20 8:20 PM, Norbert Lange wrote:
> >>>> Compiling GDB with '-fvisibility=hidden' will remove the
> >>>> symbols that should be exported.
> >>>> This patch explicitly marks them as visible.
> >>>
> >>> Curious.  We have gdb/proc-service.list supposedly for this,
> >>> doesn't -Wl,--dynamic-list work with -fvisibility=hidden then?
> >>>
> >>
> >> Obviously it doesn't, else I would not have spent time figuring out
> >> why libthread_db wont load.
> >
> > OK.  Haven't looked at visibility issues in years.  It also wasn't clear
> > to me whether the issue could be that -Wl,--dynamic-list wasn't used
> > in your build for some reason, maybe related to how you're configuring GDB.
> >
> >> -Wl,--dynamic-list merely filters the visible symbols, it does not see
> >> "hidden" ones.
> >
> > BTW, you didn't post an actual patch to the list:
> >  https://sourceware.org/ml/gdb-patches/2020-01/msg00083.html
>
> I knew I must have been confused.  I found your patch on the list,
> as a reply to that...

NP

>
> > How about something like this?  It's similar to your #2 at
> > <https://sourceware.org/bugzilla/show_bug.cgi?id=24805#c3>, but
> > I'm using typeof to avoid issues with different systems using
> > different prototypes.
>
> BTW, I forgot to mention why I suggested this as alternative
> to the push/pop approach.  It was that the push/pop approach
> makes everything indirectly included by <proc_service.h>
> have default visibility too.  I don't know whether that ends
> up being any function in practice, though.

Yes I am aware of this issue, and its a latent problem I guess.
Normally you don't implement header definitions, so you might be ok.

Adding tests whether the functions are exported would be nice.

Norbert.
  

On 1/14/20 4:56 PM, Norbert Lange wrote:
> Am Di., 14. Jan. 2020 um 16:36 Uhr schrieb Pedro Alves <palves@redhat.com>:
>>
>> We can't drop support for the glibc header, since that would mean
>> to also drop support for the Solaris version of the header, and
>> also for whatever other libcs that people build gdb with (e.g.,
>> does musl have its own version of the header with different types?).
>> Up until not so long ago, glibc didn't use to install the header,
>> that's why we keep a local copy, IIRC.
> 
> No, you don't get what I am trying to say.
> Commonly you only use external headers for function calls,
> and the exact definition can vary a bit.
> 
> What you are doing, for ex. in proc-service.c is implementing the function.
> 
> like:
>> ps_err_e
>> ps_pdread (struct ps_prochandle *ph, psaddr_t addr, void *buf, size_t size)
> 
> you don't get much, if any, information from the header that's not replicated
> in the source. The only upside would be to be able to detect mismatches
> (but that could be done in a separate test_external_proc_header.c,
> potentially even during configuration).
> 
> Whatever other libc's (musl doesnt provide/use this AFAIK) or OS's do,
> including their header does not help at all?

Well, there's also typedefs, enums, structures.  ps_err_e, for example.

> 
> You get no mismatch: Don't need it.
> You get a mismatch: you need to fix that in your c file aswell.

Or in the header, if the mismatch is in the (non-function) types.

> 
>>
>> How about something like this?  It's similar to your #2 at
>> <https://sourceware.org/bugzilla/show_bug.cgi?id=24805#c3>, but
>> I'm using typeof to avoid issues with different systems using
>> different prototypes.
> 
> Yeah, that's better, unless typeof is not supported by the compiler
> (or its C++ mode).

AFAIK, all relevant compilers support it.

>>> How about something like this?  It's similar to your #2 at
>>> <https://sourceware.org/bugzilla/show_bug.cgi?id=24805#c3>, but
>>> I'm using typeof to avoid issues with different systems using
>>> different prototypes.
>>
>> BTW, I forgot to mention why I suggested this as alternative
>> to the push/pop approach.  It was that the push/pop approach
>> makes everything indirectly included by <proc_service.h>
>> have default visibility too.  I don't know whether that ends
>> up being any function in practice, though.
> 
> Yes I am aware of this issue, and its a latent problem I guess.
> Normally you don't implement header definitions, so you might be ok.
> 
> Adding tests whether the functions are exported would be nice.

Agreed.

I've merged this to master now.

Thanks,
Pedro Alves