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

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

[Norbert Lange]

Compiling GDB with '-fvisibility=hidden' will remove the
symbols that should be exported.
This patch explicitly marks them as visible.

gdb/ChangeLog

2020-01-04 Norbert Lange  <nolange79@gmail.com>

	PR build/24805
	* gdbsupport/gdb_proc_service.h: push/pop visibility
	attribute for exported functions.

[PATCH] always export the symbols for the proc_service interface

[Norbert Lange]

Compiling GDB with '-fvisibility=hidden' will remove the
symbols that should be exported.
This patch explicitly marks them as visible.

gdb/ChangeLog

	PR build/24805
        * gdbsupport/gdb_proc_service.h: push/pop visibility
        attribute for exported functions.
---
 gdb/gdbsupport/gdb_proc_service.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/gdb/gdbsupport/gdb_proc_service.h b/gdb/gdbsupport/gdb_proc_service.h
index 8565da3286..72abb5c1c0 100644
--- a/gdb/gdbsupport/gdb_proc_service.h
+++ b/gdb/gdbsupport/gdb_proc_service.h
@@ -23,6 +23,11 @@
 
 #ifdef HAVE_PROC_SERVICE_H
 
+/* ensure the symbols are public, even if the default is hidden */
+#if __GNUC__ >= 4
+#pragma GCC visibility push(default)
+#endif
+
 /* glibc's proc_service.h doesn't wrap itself with extern "C".  Need
    to do it ourselves.  */
 EXTERN_C_PUSH
@@ -31,6 +36,10 @@ EXTERN_C_PUSH
 
 EXTERN_C_POP
 
+#if __GNUC__ >= 4
+#pragma GCC visibility pop
+#endif
+
 #else /* HAVE_PROC_SERVICE_H */
 
 /* The following fallback definitions have been imported and adjusted
@@ -69,6 +78,10 @@ EXTERN_C_POP
 # endif
 #endif
 
+#if __GNUC__ >= 4
+#pragma GCC visibility push(default)
+#endif
+
 EXTERN_C_PUSH
 
 /* Functions in this interface return one of these status codes.  */
@@ -168,6 +181,10 @@ extern void ps_plog (const char *fmt, ...);
 
 EXTERN_C_POP
 
+#if __GNUC__ >= 4
+#pragma GCC visibility pop
+#endif
+
 #endif /* HAVE_PROC_SERVICE_H */
 
 #endif /* COMMON_GDB_PROC_SERVICE_H */
-- 
2.24.1

Re: [PATCH] always export the symbols for the proc_service interface

[Tom Tromey]

>>>>> "Norbert" == Norbert Lange <nolange79@gmail.com> writes:

Norbert> Compiling GDB with '-fvisibility=hidden' will remove the
Norbert> symbols that should be exported.
Norbert> This patch explicitly marks them as visible.

Norbert> gdb/ChangeLog

Norbert> 	PR build/24805
Norbert>         * gdbsupport/gdb_proc_service.h: push/pop visibility
Norbert>         attribute for exported functions.

Thanks for the patch.

I have one question and one comment.

Norbert> +++ b/gdb/gdbsupport/gdb_proc_service.h
Norbert> @@ -23,6 +23,11 @@
 
Norbert>  #ifdef HAVE_PROC_SERVICE_H
 
Norbert> +/* ensure the symbols are public, even if the default is hidden */

In the gdb style, comments should be complete sentences, which end in a
period followed by 2 spaces.  So this should read:

/* Ensure the symbols are public, even if the default is hidden.  */

Norbert> +#if __GNUC__ >= 4
Norbert> +#pragma GCC visibility pop
Norbert> +#endif

Would it be better to just have this done a single time at the top and
bottom of the file?

thanks,
Tom

Re: [PATCH] always export the symbols for the proc_service interface

[Norbert Lange]

Am Mo., 6. Jan. 2020 um 17:07 Uhr schrieb Tom Tromey <tom@tromey.com>:
>
> >>>>> "Norbert" == Norbert Lange <nolange79@gmail.com> writes:
>
> Norbert> Compiling GDB with '-fvisibility=hidden' will remove the
> Norbert> symbols that should be exported.
> Norbert> This patch explicitly marks them as visible.
>
> Norbert> gdb/ChangeLog
>
> Norbert>        PR build/24805
> Norbert>         * gdbsupport/gdb_proc_service.h: push/pop visibility
> Norbert>         attribute for exported functions.
>
> Thanks for the patch.
>
> I have one question and one comment.
>
> Norbert> +++ b/gdb/gdbsupport/gdb_proc_service.h
> Norbert> @@ -23,6 +23,11 @@
>
> Norbert>  #ifdef HAVE_PROC_SERVICE_H
>
> Norbert> +/* ensure the symbols are public, even if the default is hidden */
>
> In the gdb style, comments should be complete sentences, which end in a
> period followed by 2 spaces.  So this should read:
>
> /* Ensure the symbols are public, even if the default is hidden.  */

Noted, am I required to redo the patch if that's the only change?

>
> Norbert> +#if __GNUC__ >= 4
> Norbert> +#pragma GCC visibility pop
> Norbert> +#endif
>
> Would it be better to just have this done a single time at the top and
> bottom of the file?

there are other headers included in between, so that's a bad idea
(and you could do attach function attributes instead for the second case,
see patch in bug report.)

I still dont get why the external header is supported at all, if the
implementation
needs to match 100% anyway (unlike where you just call functions from a header).

Norbert

Re: [PATCH][PR build/24805] Explicitly export symbols from gdb_proc_service

[Pedro Alves]

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?

Thanks,
Pedro Alves

Re: [PATCH][PR build/24805] Explicitly export symbols from gdb_proc_service

[Norbert Lange]

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?
>
> Thanks,
> Pedro Alves
>

Obviously it doesn't, else I would not have spent time figuring out
why libthread_db wont load.

Unfortunately you need to pass the correct visibility to the
compilation, a hidden symbol can
be optimized globally almost like a static function locally.
A visible symbol needs to be accessed though a layer of indirection.
Wish you could disable
that logic for micro controllers where that means you don't get simple
relative jumps.
Would love if the linker would be the only one controlling the type of symbol,
that would make some subset of LTO required. But thats offtopic.

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

While we are at it, I see alot stuff exported from gdb, besides the
proc_service.
I am not sure where they come from or intentional

# debian gdb
# nm -D '/usr/bin/gdb'  | grep ' [TVB] '
0000000000491170 T _obstack_allocated_p
0000000000491040 T _obstack_begin
0000000000491060 T _obstack_begin_1
00000000004911b0 T _obstack_free
0000000000491220 T _obstack_memory_used
0000000000491080 T _obstack_newchunk
00000000002daf80 T ps_getpid
00000000000f4f10 T ps_get_thread_area
00000000002daea0 T ps_lgetfpregs
00000000002dadc0 T ps_lgetregs
00000000002daf10 T ps_lsetfpregs
00000000002dae30 T ps_lsetregs
00000000002dada0 T ps_pdread
00000000002dadb0 T ps_pdwrite
00000000002dace0 T ps_pglobal_lookup
0000000000163770 T xmalloc
00000000001637a0 T xrealloc
0000000000167790 T _Znam
00000000001677a0 T _ZnamRKSt9nothrow_t
0000000000167720 T _Znwm
0000000000167770 T _ZnwmRKSt9nothrow_t

# gdb master built with -fvisibility=hidden
nm -D '/tmp/devsupport_gdb/usr/local/bin/gdb'  | grep ' [TVB] '
0000000000a7e1a0 B __environ
0000000000a7e1a0 V environ
0000000000a7e1a8 B optarg
0000000000a7e1c8 B optind
00000000005e4f40 T ps_getpid
00000000007913c0 T ps_get_thread_area
00000000005e4fc0 T ps_lgetfpregs
00000000005e50a0 T ps_lgetregs
00000000005e4f50 T ps_lsetfpregs
00000000005e5030 T ps_lsetregs
00000000005e5120 T ps_pdread
00000000005e5110 T ps_pdwrite
00000000005e5130 T ps_pglobal_lookup
0000000000a7e1c0 B stderr
0000000000a7e1b0 B stdin
0000000000a7e180 B stdout
0000000000695510 T _Znam
0000000000691530 T _ZnamRKSt9nothrow_t
0000000000693ae0 T _Znwm
0000000000691550 T _ZnwmRKSt9nothrow_t
0000000000a6e040 V _ZTIN10__cxxabiv115__forced_unwindE
0000000000a6e1a8 V _ZTINSt13__future_base12_Result_baseE
0000000000a6dfc0 V _ZTINSt6thread6_StateE
0000000000a6dfd0 V _ZTISt12future_error
0000000000a6e0a0 V _ZTISt9bad_alloc
0000000000a6dfe8 V _ZTVN10__cxxabiv117__class_type_infoE
0000000000a6e050 V _ZTVN10__cxxabiv119__pointer_type_infoE
0000000000a6e110 V _ZTVN10__cxxabiv120__function_type_infoE
0000000000a6e0b8 V _ZTVN10__cxxabiv120__si_class_type_infoE
0000000000a6e150 V _ZTVN10__cxxabiv121__vmi_class_type_infoE
0000000000a6e1c0 V _ZTVSt12future_error

Re: [PATCH][PR build/24805] Explicitly export symbols from gdb_proc_service

[Pedro Alves]

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(+)

diff --git a/gdb/gdbsupport/gdb_proc_service.h b/gdb/gdbsupport/gdb_proc_service.h
index 3ce2ee80fa9..9872ab2bca4 100644
--- a/gdb/gdbsupport/gdb_proc_service.h
+++ b/gdb/gdbsupport/gdb_proc_service.h
@@ -170,4 +170,33 @@ EXTERN_C_POP
 
 #endif /* HAVE_PROC_SERVICE_H */
 
+/* Make sure we export the needed symbols, in case GDB is built with
+   -fvisibility=hidden.  */
+
+#define PS_EXPORT(SYM)						\
+  __attribute__((visibility ("default"))) typeof (SYM) SYM
+
+PS_EXPORT (ps_get_thread_area);
+PS_EXPORT (ps_getpid);
+PS_EXPORT (ps_lcontinue);
+PS_EXPORT (ps_lgetfpregs);
+PS_EXPORT (ps_lgetregs);
+PS_EXPORT (ps_lsetfpregs);
+PS_EXPORT (ps_lsetregs);
+PS_EXPORT (ps_lstop);
+PS_EXPORT (ps_pcontinue);
+PS_EXPORT (ps_pdread);
+PS_EXPORT (ps_pdwrite);
+PS_EXPORT (ps_pglobal_lookup);
+PS_EXPORT (ps_pstop);
+PS_EXPORT (ps_ptread);
+PS_EXPORT (ps_ptwrite);
+
+#ifdef __sun__
+PS_EXPORT (ps_lgetxregs);
+PS_EXPORT (ps_lgetxregsize);
+PS_EXPORT (ps_lsetxregs);
+PS_EXPORT (ps_plog);
+#endif
+
 #endif /* COMMON_GDB_PROC_SERVICE_H */

base-commit: 7da6a5b938b426379f61e56e259a925bedfe242b
-- 
2.14.5

Re: [PATCH][PR build/24805] Explicitly export symbols from gdb_proc_service

[Pedro Alves]

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

Re: [PATCH][PR build/24805] Explicitly export symbols from gdb_proc_service

[Norbert Lange]

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.

Re: [PATCH][PR build/24805] Explicitly export symbols from gdb_proc_service

[Pedro Alves]

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