diff mbox

backtrace for hppa

Message ID alpine.DEB.2.20.1801161611370.17210@digraph.polyomino.org.uk
State New
Headers show

Commit Message

Joseph Myers Jan. 16, 2018, 4:20 p.m. UTC
As far as I can tell, the only architecture in glibc that uses the generic 
debug/backtrace.c is hppa.  
<https://sourceware.org/glibc/wiki/Release/2.26#HPPA> also shows the 
debug/tst-backtrace* tests as failing for hppa.  That suggests the generic 
debug/backtrace.c is not in fact functional anywhere and so the x86_64 
backtrace implementation (which many other architectures include) would be 
a better generic version.

(a) Could someone confirm that those backtrace tests are indeed still 
failing for hppa with current master?  If so, a bug should be filed in 
Bugzilla for this.

(b) Does the following patch change the results for hppa?  This is simply 
the completely mechanical use of the x86_64 version as if it were the 
default (adjusted to avoid hardcoding libgcc_s.so.1, since hppa uses 
libgcc_s.so.4); I haven't tested it at all, even to see if it compiles.

(Even if this patch doesn't help backtrace results for hppa, that only 
indicates an hppa-specific version of backtrace is needed, not that the 
generic one is a useful default.)

Comments

Zack Weinberg Jan. 16, 2018, 4:30 p.m. UTC | #1
On Tue, Jan 16, 2018 at 11:20 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> As far as I can tell, the only architecture in glibc that uses the generic
> debug/backtrace.c is hppa.
> <https://sourceware.org/glibc/wiki/Release/2.26#HPPA> also shows the
> debug/tst-backtrace* tests as failing for hppa.  That suggests the generic
> debug/backtrace.c is not in fact functional anywhere and so the x86_64
> backtrace implementation (which many other architectures include) would be
> a better generic version.

I observe that sysdeps/hppa/frame.h has received no substantive
changes since 2000, and contains

| /* FIXME: will verify this later */
| struct layout
| {
|   void *next;
|   void *return_address;
| };

which suggests that the use of debug/backtrace.c for HPPA was never
properly validated.

> --- a/sysdeps/x86_64/backtrace.c
> +++ b/sysdeps/x86_64/backtrace.c
> @@ -20,6 +20,7 @@
>  #include <libc-lock.h>
>  #include <dlfcn.h>
>  #include <execinfo.h>
> +#include <gnu/lib-names.h>
>  #include <stdlib.h>
>  #include <unwind.h>
>
> @@ -49,7 +50,7 @@ dummy_getcfa (struct _Unwind_Context *ctx __attribute__ ((unused)))
>  static void
>  init (void)
>  {
> -  libgcc_handle = __libc_dlopen ("libgcc_s.so.1");
> +  libgcc_handle = __libc_dlopen (LIBGCC_S_SO);
>
>    if (libgcc_handle == NULL)
>      return;

This change should probably be applied independently of whether it
helps hppa (and I don't see any reason to wait, either).

zw
Dave Anglin Jan. 16, 2018, 4:48 p.m. UTC | #2
On 2018-01-16 11:20 AM, Joseph Myers wrote:
> (a) Could someone confirm that those backtrace tests are indeed still
> failing for hppa with current master?  If so, a bug should be filed in
> Bugzilla for this.
They still fail.  I will file a bug as soon as I get a chance (maybe 
tonight).

Dave
Joseph Myers Jan. 16, 2018, 4:53 p.m. UTC | #3
On Tue, 16 Jan 2018, Zack Weinberg wrote:

> > --- a/sysdeps/x86_64/backtrace.c
> > +++ b/sysdeps/x86_64/backtrace.c
> > @@ -20,6 +20,7 @@
> >  #include <libc-lock.h>
> >  #include <dlfcn.h>
> >  #include <execinfo.h>
> > +#include <gnu/lib-names.h>
> >  #include <stdlib.h>
> >  #include <unwind.h>
> >
> > @@ -49,7 +50,7 @@ dummy_getcfa (struct _Unwind_Context *ctx __attribute__ ((unused)))
> >  static void
> >  init (void)
> >  {
> > -  libgcc_handle = __libc_dlopen ("libgcc_s.so.1");
> > +  libgcc_handle = __libc_dlopen (LIBGCC_S_SO);
> >
> >    if (libgcc_handle == NULL)
> >      return;
> 
> This change should probably be applied independently of whether it
> helps hppa (and I don't see any reason to wait, either).

Without the hppa change, it should be a no-op (m68k uses libgcc_s.so.2, 
but has its own backtrace.c).  So should be harmless, but purely a cleanup 
in the absence of the hppa change, with no specific justification for 
including it during the freeze.

Logically, there are at least three changes.

1. Make sysdeps/x86_64/backtrace.c use LIBGCC_S_SO.

2. Make hppa use sysdeps/x86_64/backtrace.c (if that helps).

3. Move sysdeps/x86_64/backtrace.c to debug/backtrace.c, remove all the 
backtrace.c files that included it (seven right now, potentially hppa if 
my patch helps, potentially riscv), and remove all frame.h files as unused 
(pure cleanup, should leave installed stripped shared libraries 
unchanged).
Andreas Schwab Jan. 16, 2018, 4:57 p.m. UTC | #4
On Jan 16 2018, Joseph Myers <joseph@codesourcery.com> wrote:

> As far as I can tell, the only architecture in glibc that uses the generic 
> debug/backtrace.c is hppa.  
> <https://sourceware.org/glibc/wiki/Release/2.26#HPPA> also shows the 
> debug/tst-backtrace* tests as failing for hppa.  That suggests the generic 
> debug/backtrace.c is not in fact functional anywhere and so the x86_64 
> backtrace implementation (which many other architectures include) would be 
> a better generic version.

Perhaps merged with the m68k version.

Andreas.
Joseph Myers Jan. 16, 2018, 5:06 p.m. UTC | #5
On Tue, 16 Jan 2018, Andreas Schwab wrote:

> On Jan 16 2018, Joseph Myers <joseph@codesourcery.com> wrote:
> 
> > As far as I can tell, the only architecture in glibc that uses the generic 
> > debug/backtrace.c is hppa.  
> > <https://sourceware.org/glibc/wiki/Release/2.26#HPPA> also shows the 
> > debug/tst-backtrace* tests as failing for hppa.  That suggests the generic 
> > debug/backtrace.c is not in fact functional anywhere and so the x86_64 
> > backtrace implementation (which many other architectures include) would be 
> > a better generic version.
> 
> Perhaps merged with the m68k version.

There are several versions that use libgcc_s but have 
architecture-specific differences from x86_64 (arm i386 m68k s390-32 
s390-64 sparc; microblaze and powerpc don't use libgcc_s at all).  It may 
well be possible to factor things to reduce the duplication between those 
variants, but there are enough differences that I think it makes sense to 
make put the x86_64 version (used for x86_64 and seven other architectures 
at present) in debug/ before any further refactoring.
Adhemerval Zanella Jan. 16, 2018, 6:06 p.m. UTC | #6
On 16/01/2018 14:48, John David Anglin wrote:
> On 2018-01-16 11:20 AM, Joseph Myers wrote:
>> (a) Could someone confirm that those backtrace tests are indeed still
>> failing for hppa with current master?  If so, a bug should be filed in
>> Bugzilla for this.
> They still fail.  I will file a bug as soon as I get a chance (maybe tonight).
> 
> Dave
> 

And your patch does fix the backtrace issues on hppa.
Palmer Dabbelt Jan. 18, 2018, 6:24 p.m. UTC | #7
On Tue, 16 Jan 2018 08:53:48 PST (-0800), joseph@codesourcery.com wrote:
> On Tue, 16 Jan 2018, Zack Weinberg wrote:
>
>> > --- a/sysdeps/x86_64/backtrace.c
>> > +++ b/sysdeps/x86_64/backtrace.c
>> > @@ -20,6 +20,7 @@
>> >  #include <libc-lock.h>
>> >  #include <dlfcn.h>
>> >  #include <execinfo.h>
>> > +#include <gnu/lib-names.h>
>> >  #include <stdlib.h>
>> >  #include <unwind.h>
>> >
>> > @@ -49,7 +50,7 @@ dummy_getcfa (struct _Unwind_Context *ctx __attribute__ ((unused)))
>> >  static void
>> >  init (void)
>> >  {
>> > -  libgcc_handle = __libc_dlopen ("libgcc_s.so.1");
>> > +  libgcc_handle = __libc_dlopen (LIBGCC_S_SO);
>> >
>> >    if (libgcc_handle == NULL)
>> >      return;
>>
>> This change should probably be applied independently of whether it
>> helps hppa (and I don't see any reason to wait, either).
>
> Without the hppa change, it should be a no-op (m68k uses libgcc_s.so.2,
> but has its own backtrace.c).  So should be harmless, but purely a cleanup
> in the absence of the hppa change, with no specific justification for
> including it during the freeze.
>
> Logically, there are at least three changes.
>
> 1. Make sysdeps/x86_64/backtrace.c use LIBGCC_S_SO.
>
> 2. Make hppa use sysdeps/x86_64/backtrace.c (if that helps).
>
> 3. Move sysdeps/x86_64/backtrace.c to debug/backtrace.c, remove all the
> backtrace.c files that included it (seven right now, potentially hppa if
> my patch helps, potentially riscv), and remove all frame.h files as unused
> (pure cleanup, should leave installed stripped shared libraries
> unchanged).

Thanks for the heads up, I think this fixed some of our bugs!
Joseph Myers Jan. 18, 2018, 6:26 p.m. UTC | #8
On Thu, 18 Jan 2018, Palmer Dabbelt wrote:

> Thanks for the heads up, I think this fixed some of our bugs!

To be clear, if it works for RISC-V my recommendation is a #include of the 
x86_64 version of backtrace.  Not a copy, and not the larger refactoring 
which can wait for 2.28 as it doesn't fix any user-visible bugs.
Adhemerval Zanella Jan. 18, 2018, 6:37 p.m. UTC | #9
On 18/01/2018 16:26, Joseph Myers wrote:
> On Thu, 18 Jan 2018, Palmer Dabbelt wrote:
> 
>> Thanks for the heads up, I think this fixed some of our bugs!
> 
> To be clear, if it works for RISC-V my recommendation is a #include of the 
> x86_64 version of backtrace.  Not a copy, and not the larger refactoring 
> which can wait for 2.28 as it doesn't fix any user-visible bugs.
> 

Yes, I intend to refactor this for 2.28. Although this could simpler to
this release point for RISC-V, I think these cross-arch references 
bring more issues than solutions we should avoid pushing these further.
Dave Anglin Jan. 18, 2018, 7:47 p.m. UTC | #10
On 2018-01-18 1:26 PM, Joseph Myers wrote:
> On Thu, 18 Jan 2018, Palmer Dabbelt wrote:
>
>> Thanks for the heads up, I think this fixed some of our bugs!
> To be clear, if it works for RISC-V my recommendation is a #include of the
> x86_64 version of backtrace.  Not a copy, and not the larger refactoring
> which can wait for 2.28 as it doesn't fix any user-visible bugs.
>
Just want to add my thanks for fixing these tests on hppa.

Dave
diff mbox

Patch

diff --git a/sysdeps/hppa/backtrace.c b/sysdeps/hppa/backtrace.c
new file mode 100644
index 0000000..27ce597
--- /dev/null
+++ b/sysdeps/hppa/backtrace.c
@@ -0,0 +1 @@ 
+#include <sysdeps/x86_64/backtrace.c>
diff --git a/sysdeps/x86_64/backtrace.c b/sysdeps/x86_64/backtrace.c
index 2706b50..d423cc0 100644
--- a/sysdeps/x86_64/backtrace.c
+++ b/sysdeps/x86_64/backtrace.c
@@ -20,6 +20,7 @@ 
 #include <libc-lock.h>
 #include <dlfcn.h>
 #include <execinfo.h>
+#include <gnu/lib-names.h>
 #include <stdlib.h>
 #include <unwind.h>
 
@@ -49,7 +50,7 @@  dummy_getcfa (struct _Unwind_Context *ctx __attribute__ ((unused)))
 static void
 init (void)
 {
-  libgcc_handle = __libc_dlopen ("libgcc_s.so.1");
+  libgcc_handle = __libc_dlopen (LIBGCC_S_SO);
 
   if (libgcc_handle == NULL)
     return;