Export stack_used as __stack_used

  Hi all,

This commit renames "stack_used" as "__stack_used" and changes it from
a static variable in allocatestack.c to an internally exported symbol
available to all nptl source files.  This fixes bug 17629 (and thus
GDB PR 9635).

Is this ok to commit?

Cheers,
Gary

---

	[BZ #17629]
	* nptl/allocatestack.c (stack_used): Rename as...
	(__stack_used): Renamed from stack_used and made non-static.
	All uses updated.
	* nptl/nptl-init.c: Initialize __stack_used.
	* nptl/pthreadP.h (__stack_used): New declaration.
	* nptl_db/td_ta_thr_iter.c (iterate_thread_list): Cope with
	__stack_used == NULL.
---
 ChangeLog                 |   11 +++++++++++
 nptl/allocatestack.c      |   33 +++++++++++++++++----------------
 nptl/descr.h              |    2 +-
 nptl/nptl-init.c          |    1 +
 nptl/pthreadP.h           |    2 ++
 nptl/pthread_create.c     |    2 +-
 nptl_db/structs.def       |    2 +-
 nptl_db/td_ta_thr_iter.c  |   15 +++++++++++----
 nptl_db/td_thr_validate.c |    2 +-
 9 files changed, 46 insertions(+), 24 deletions(-)
  

On 06/17/2016 12:37 PM, Gary Benson wrote:
> Hi all,
> 
> This commit renames "stack_used" as "__stack_used" and changes it from
> a static variable in allocatestack.c to an internally exported symbol
> available to all nptl source files.  This fixes bug 17629 (and thus
> GDB PR 9635).

Thanks!

> 
> Is this ok to commit?

I'd have preferred to split the renaming bits out, alone, as that's
the part that actually fixes the bug, and leave the (internally) exporting
the symbol and the iterate_thread_list hunk as a patch of the Infinity
series, where it's needed.
  

On 06/17/2016 01:37 PM, Gary Benson wrote:
> Hi all,
>
> This commit renames "stack_used" as "__stack_used" and changes it from
> a static variable in allocatestack.c to an internally exported symbol
> available to all nptl source files.  This fixes bug 17629 (and thus
> GDB PR 9635).

What do you mean by “export”?  The __ prefix suggests that you want to 
export it as a symbol (probably GLIBC_PRIVATE), but I think you'd need 
to update nptl/Versions in this case.

If the variable remains nptl-internal, I expect that you could turn it 
into a hidden symbol, and keep its current name.

Thanks,
Florian
  

Florian Weimer wrote:
> On 06/17/2016 01:37 PM, Gary Benson wrote:
> > This commit renames "stack_used" as "__stack_used" and changes it
> > from a static variable in allocatestack.c to an internally
> > exported symbol available to all nptl source files.  This fixes
> > bug 17629 (and thus GDB PR 9635).
> 
> What do you mean by “export”?  The __ prefix suggests that you want
> to export it as a symbol (probably GLIBC_PRIVATE), but I think you'd
> need to update nptl/Versions in this case.

No, I don't need that, it needs to be visible outside allocatestack.c
but it doesn't need to be visible in, e.g., the output of "objdump -T".

> If the variable remains nptl-internal, I expect that you could turn
> it into a hidden symbol, and keep its current name.

I may be wrong but I think its the rename that fixes the GDB bug.

If it's visible outside of allocatestack.c (i.e. it's not "static")
then it should be named with a double-underscore, no?

Thanks,
Gary
  

On 06/17/2016 09:50 PM, Gary Benson wrote:
> Florian Weimer wrote:
>> On 06/17/2016 01:37 PM, Gary Benson wrote:
>>> This commit renames "stack_used" as "__stack_used" and changes it
>>> from a static variable in allocatestack.c to an internally
>>> exported symbol available to all nptl source files.  This fixes
>>> bug 17629 (and thus GDB PR 9635).
>>
>> What do you mean by “export”?  The __ prefix suggests that you want
>> to export it as a symbol (probably GLIBC_PRIVATE), but I think you'd
>> need to update nptl/Versions in this case.
>
> No, I don't need that, it needs to be visible outside allocatestack.c
> but it doesn't need to be visible in, e.g., the output of "objdump -T".

Okay, then please declare it attribute_hidden.

>> If the variable remains nptl-internal, I expect that you could turn
>> it into a hidden symbol, and keep its current name.
>
> I may be wrong but I think its the rename that fixes the GDB bug.

Only if GDB explicitly searches for __stack_used (and that with the help 
of debugging information because it's not exported).

There isn't even a reference from libthread_db.

> If it's visible outside of allocatestack.c (i.e. it's not "static")
> then it should be named with a double-underscore, no?

Hmm, yes for the benefit of static linking and namespace cleanliness. 
So perhaps keep __stack_used, but make it hidden as well.  (I don't 
understand the nocommon business and what this addresses.)

Florian
  

Gary Benson wrote:
> Florian Weimer wrote:
> > On 06/17/2016 01:37 PM, Gary Benson wrote:
> > > This commit renames "stack_used" as "__stack_used" and changes it
> > > from a static variable in allocatestack.c to an internally
> > > exported symbol available to all nptl source files.  This fixes
> > > bug 17629 (and thus GDB PR 9635).
> > 
> > What do you mean by “export”?  The __ prefix suggests that you want
> > to export it as a symbol (probably GLIBC_PRIVATE), but I think you'd
> > need to update nptl/Versions in this case.
> 
> No, I don't need that, it needs to be visible outside allocatestack.c
> but it doesn't need to be visible in, e.g., the output of "objdump -T".
> 
> > If the variable remains nptl-internal, I expect that you could turn
> > it into a hidden symbol, and keep its current name.
> 
> I may be wrong but I think its the rename that fixes the GDB bug.
> 
> If it's visible outside of allocatestack.c (i.e. it's not "static")
> then it should be named with a double-underscore, no?

Oh, hang on, libthread_db looks it up _by_name_.  How is that working
currently?

Cheers,
Gary
  

On 06/17/2016 10:17 PM, Gary Benson wrote:
> Gary Benson wrote:
>> Florian Weimer wrote:
>>> On 06/17/2016 01:37 PM, Gary Benson wrote:
>>>> This commit renames "stack_used" as "__stack_used" and changes it
>>>> from a static variable in allocatestack.c to an internally
>>>> exported symbol available to all nptl source files.  This fixes
>>>> bug 17629 (and thus GDB PR 9635).
>>>
>>> What do you mean by “export”?  The __ prefix suggests that you want
>>> to export it as a symbol (probably GLIBC_PRIVATE), but I think you'd
>>> need to update nptl/Versions in this case.
>>
>> No, I don't need that, it needs to be visible outside allocatestack.c
>> but it doesn't need to be visible in, e.g., the output of "objdump -T".
>>
>>> If the variable remains nptl-internal, I expect that you could turn
>>> it into a hidden symbol, and keep its current name.
>>
>> I may be wrong but I think its the rename that fixes the GDB bug.
>>
>> If it's visible outside of allocatestack.c (i.e. it's not "static")
>> then it should be named with a double-underscore, no?
>
> Oh, hang on, libthread_db looks it up _by_name_.  How is that working
> currently?

It seems that libpthread_db calls ps_pglobal_lookup, which is provided 
by the “application” (so GDB).  The mechanism is a bit weird: it almost 
looks as if someone thought about caching the name-based lookups in an 
array (with an index known at compile-time), but then never wrote the 
code to use a cache.

We check that the symbols can be found in the .symtab section (so via 
debugging information, I assume, not .dynsym).

Florian
  

On 06/17/2016 09:31 PM, Florian Weimer wrote:
> On 06/17/2016 10:17 PM, Gary Benson wrote:
>> Gary Benson wrote:
>>> Florian Weimer wrote:
>>>> On 06/17/2016 01:37 PM, Gary Benson wrote:
>>>>> This commit renames "stack_used" as "__stack_used" and changes it
>>>>> from a static variable in allocatestack.c to an internally
>>>>> exported symbol available to all nptl source files.  This fixes
>>>>> bug 17629 (and thus GDB PR 9635).
>>>>
>>>> What do you mean by “export”?  The __ prefix suggests that you want
>>>> to export it as a symbol (probably GLIBC_PRIVATE), but I think you'd
>>>> need to update nptl/Versions in this case.
>>>
>>> No, I don't need that, it needs to be visible outside allocatestack.c
>>> but it doesn't need to be visible in, e.g., the output of "objdump -T".
>>>
>>>> If the variable remains nptl-internal, I expect that you could turn
>>>> it into a hidden symbol, and keep its current name.
>>>
>>> I may be wrong but I think its the rename that fixes the GDB bug.

Yes it is.

Should be trivial to reproduce.  Simply write a test program that has
a global named "stack_used".  The program is totally free to do that,
because that is a symbol name not in the implementation namespace.

See:
 https://sourceware.org/bugzilla/show_bug.cgi?id=9635
And the small reproducer:
 https://sourceware.org/bugzilla/attachment.cgi?id=5756

>>>
>>> If it's visible outside of allocatestack.c (i.e. it's not "static")
>>> then it should be named with a double-underscore, no?
>>
>> Oh, hang on, libthread_db looks it up _by_name_.  How is that working
>> currently?
> 
> It seems that libpthread_db calls ps_pglobal_lookup, which is provided
> by the “application” (so GDB).  

Right.  libthread_db.so is loaded as a plugin inside gdb's address
space.  That library knows the layouts of the target's libpthread's
internals, but doesn't know where the addresses of the inferior's globals
are.  So when libthread_db.so needs to know the address of
the target inferior's libpthread's "stack_used" symbol, it asks gdb,
via the proc-service interface.

The problem is that if the program being debugged defines its own
unrelated "stack_used" symbol, gdb can well return the address of
that unrelated global to libthread_db.so.

My initial attempt was to try to fix this in gdb, by restricting gdb's global
lookup to symbols defined in libpthread.so.  However, it turned out that
that doesn't work when the inferior being debugged has been statically
linked, because in that case, libpthread is really linked inside 
the main program, and thus there's no separate "libpthread.so" gdb can
restrict the symbol lookup in.  So we're back to the same problem.

The solution I suggested in the PR then, is to rename the libpthread's
"stack_used" symbol, giving it a name that is in the implementation namespace.

I think it'd be reasonable if libthread_db's DB_GET_SYMBOL macro
had an assert that ensured that only symbols in the implementation
name space are looked up, even.

(Note that most of the libpthread symbols that libthread_db looks up
are prefixed "__nptl_"; such a prefix makes it even less likely to trip
on a conflict -- there's a remote but non-zero change that libgcc or
some other library part of the implementation has another symbol
named "__stack_used".)


> The mechanism is a bit weird: it almost
> looks as if someone thought about caching the name-based lookups in an
> array (with an index known at compile-time), but then never wrote the
> code to use a cache.
> 
> We check that the symbols can be found in the .symtab section (so via
> debugging information, I assume, not .dynsym).

Here I'm afraid you lost me.

Thanks,
Pedro Alves
  

On 06/17/2016 09:17 PM, Florian Weimer wrote:
> 
>>> If the variable remains nptl-internal, I expect that you could turn
>>> it into a hidden symbol, and keep its current name.
>>
>> I may be wrong but I think its the rename that fixes the GDB bug.
> 
> Only if GDB explicitly searches for __stack_used (and that with the help
> of debugging information because it's not exported).

Note that it's not GDB that _wants_ to search for the symbol, it's
libthread_db.so itself.  GDB just serves libthread_db.so's request,
through the proc_service [1] interface (nptl_db/proc_service.h).  That is,
through callbacks that serve as interface between libthread_db and
the debugger that loads it.

[1] based on Solaris's interface of the same name:
 http://docs.oracle.com/cd/E26505_01/html/816-5171/proc-service-3proc.html

> 
> There isn't even a reference from libthread_db.

AFAICS, the look ups in question are here:

 nptl_db/td_thr_validate.c:73:      err = DB_GET_SYMBOL (list, th->th_ta_p, stack_used);
 nptl_db/td_ta_thr_iter.c:164:    err = DB_GET_SYMBOL (list, ta, stack_used);

I think that nptl_version is another symbol that can potentially
cause the same problem:

 nptl_db/structs.def:51:DB_SYMBOL (stack_used)
 nptl_db/structs.def:52:DB_SYMBOL (__stack_user)
 nptl_db/structs.def:53:DB_SYMBOL (nptl_version)
 nptl_db/structs.def:56:DB_SYMBOL (__nptl_threads_events)

(Obviously it's less likely, given the "nptl" prefix, but still...)

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

> I think that nptl_version is another symbol that can potentially
> cause the same problem:
>
>  nptl_db/structs.def:51:DB_SYMBOL (stack_used)
>  nptl_db/structs.def:52:DB_SYMBOL (__stack_user)
>  nptl_db/structs.def:53:DB_SYMBOL (nptl_version)
>  nptl_db/structs.def:56:DB_SYMBOL (__nptl_threads_events)

I think that DB_SYMBOL should use aliases with a unique prefix instead
of the variable names directly, and those aliases should be put in the
dynamic symbol table so that lookup still works if libthread is
stripped.

Andreas.
  

On 06/18/2016 07:52 AM, Andreas Schwab wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> I think that nptl_version is another symbol that can potentially
>> cause the same problem:
>>
>>  nptl_db/structs.def:51:DB_SYMBOL (stack_used)
>>  nptl_db/structs.def:52:DB_SYMBOL (__stack_user)
>>  nptl_db/structs.def:53:DB_SYMBOL (nptl_version)
>>  nptl_db/structs.def:56:DB_SYMBOL (__nptl_threads_events)
> 
> I think that DB_SYMBOL should use aliases with a unique prefix instead
> of the variable names directly, and those aliases should be put in the
> dynamic symbol table so that lookup still works if libthread is
> stripped.

Agreed.  Requiring not-stripped libpthread used to
be a constant source of trouble for integrators, enough that gdb has a
FAQ entry about it:
 https://sourceware.org/gdb/wiki/FAQ#GDB_does_not_see_any_threads_besides_the_one_in_which_crash_occurred.3B_or_SIGTRAP_kills_my_program_when_I_set_a_breakpoint
We don't hear about this problem a lot anymore though, I believe because
gdb nowadays does not rely on libthread_db for so many things as it used to.

I seem to remember this having been proposed in the past, but there having
been pushback against putting these symbols in the dynamic symbol table,
because that increases the dynamic symbol table size just for debugging...
If this is no longer a concern, I'm all for it.

Thanks,
Pedro Alves
  

On 06/18/2016 01:08 PM, Pedro Alves wrote:

> Agreed.  Requiring not-stripped libpthread used to
> be a constant source of trouble for integrators, enough that gdb has a
> FAQ entry about it:
>  https://sourceware.org/gdb/wiki/FAQ#GDB_does_not_see_any_threads_besides_the_one_in_which_crash_occurred.3B_or_SIGTRAP_kills_my_program_when_I_set_a_breakpoint

Uh-oh.

> I seem to remember this having been proposed in the past, but there having
> been pushback against putting these symbols in the dynamic symbol table,
> because that increases the dynamic symbol table size just for debugging...
> If this is no longer a concern, I'm all for it.

Could we export a single symbol which is a struct and contains all the 
data we need?  Then the impact on dynamic symbol table size would be 
minimal.

Florian
  

Florian Weimer <fweimer@redhat.com> writes:

> Could we export a single symbol which is a struct and contains all the
> data we need?  Then the impact on dynamic symbol table size would be
> minimal.

That would raise the issue of compatibility. Probing each symbol
separately is much easier than trying to extend the struct.  What
exactly _is_ the impact of making the dynamic symtab a few entries
bigger?

Andreas.
  

On 06/20/2016 10:04 AM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> 
>> Could we export a single symbol which is a struct and contains all the
>> data we need?  Then the impact on dynamic symbol table size would be
>> minimal.
> 
> That would raise the issue of compatibility. Probing each symbol
> separately is much easier than trying to extend the struct.  What
> exactly _is_ the impact of making the dynamic symtab a few entries
> bigger?

I think this is what I was remembering:

 https://sourceware.org/ml/libc-alpha/2003-11/msg00007.html

Thanks,
Pedro Alves
  

On 06/20/2016 11:04 AM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>
>> Could we export a single symbol which is a struct and contains all the
>> data we need?  Then the impact on dynamic symbol table size would be
>> minimal.
>
> That would raise the issue of compatibility. Probing each symbol
> separately is much easier than trying to extend the struct.  What
> exactly _is_ the impact of making the dynamic symtab a few entries
> bigger?

Does compatibility really matter?  I thought you need exactly matching 
libpthread and libthread_db versions anyway.  I suppose we could even 
compile the offsets into libthread_db.

I assume that for compatibility across multiple versions, we will need 
something like Infinity.

Thanks,
Florian
  

Florian Weimer <fweimer@redhat.com> writes:

> Does compatibility really matter?  I thought you need exactly matching
> libpthread and libthread_db versions anyway.  I suppose we could even
> compile the offsets into libthread_db.

The thread_db interface appears to be generic enough to be able to cope
with different versions.

Andreas.
  

On 21 Jun 2016 19:31, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
> > Does compatibility really matter?  I thought you need exactly matching
> > libpthread and libthread_db versions anyway.  I suppose we could even
> > compile the offsets into libthread_db.
> 
> The thread_db interface appears to be generic enough to be able to cope
> with different versions.

while true, i don't think that means you can mix & match.  we've always
said glibc libs must be coherent (can't mix 2.15 libpthread with a 2.17
libc), so i'm not sure why thread_db is any different.  they're shipped
together by distros too.
-mike
  

On 06/22/2016 11:57 AM, Mike Frysinger wrote:
> On 21 Jun 2016 19:31, Andreas Schwab wrote:
>> Florian Weimer <fweimer@redhat.com> writes:
>>> Does compatibility really matter?  I thought you need exactly matching
>>> libpthread and libthread_db versions anyway.  I suppose we could even
>>> compile the offsets into libthread_db.
>>
>> The thread_db interface appears to be generic enough to be able to cope
>> with different versions.
> 
> while true, i don't think that means you can mix & match.  we've always
> said glibc libs must be coherent (can't mix 2.15 libpthread with a 2.17
> libc), so i'm not sure why thread_db is any different.  

Yeah, I don't think it's any different.

Making libthread_db cope with multiple versions would be a bad design decision, IMO.
It'd be easy-ish to make it cope if just some constant offsets in data structures
change (e.g., to cope with new fields in structures) between glibc versions, but
if the data structures themselves change, like moving from linked lists to vectors
to hash tables, etc. things get complicated.

Sorting all that out, along with debugger x debuggee arch- and ABI-independence,
is the whole point of Infinity.

Thanks,
Pedro Alves