Re: [PATCH v2] libgcc: Add a backchain fallback to _Unwind_Backtrace() on PowerPC

[Raphael M Zinsly <rzinsly@linux.ibm.com>]

On 28/09/2021 16:50, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Aug 26, 2021 at 11:53:24AM -0300, Raphael Moreira Zinsly wrote:
>> Without dwarf2 unwind tables available _Unwind_Backtrace() is not
>> able to return the full backtrace.
>> This patch adds a fallback function on powerpc to get the backtrace
>> by doing a backchain, this code was originally at glibc.
> 
> Okay, the backchain as fallback if other (better!) methods cannot work.
> 
>> 	* config/rs6000/linux-unwind.h (struct rt_sigframe): Move it to
>> 	outside of get_regs() in order to use it in another function,
>> 	this is done twice: for __powerpc64__ and for !__powerpc64__.
>> 	(struct trace_arg): New struct.
>> 	(struct layout): New struct.
>> 	(ppc_backchain_fallback): New function.
>> 	* unwind.inc (_Unwind_Backtrace): Look for _URC_NORMAL_STOP
>> 	code state and call MD_BACKCHAIN_FALLBACK.
> 
> Changelog lines wrap at 80 chars, not 70 or so.  The emails from commits
> (to bugzilla) are a bit malformed (it counts the number of columns for
> leading tabs wrong it seems), but the actual commits are just fine.
> 
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/unwind-backchain.c
>> @@ -0,0 +1,22 @@
>> +/* { dg-do run { target { powerpc*-*-linux* } } } */
> 
> Don't say such targets in gcc.target/powerpc/ tests please.  Everything
> in gcc.target is for powerpc*-*-* already, so if you really want to
> limit to powerpc*-*-linux* just write *-*-linux*.  But there are better
> ways to get what you want, like, testing for the actual feature you want
> (which is if backtrace() works?)  But such an improvement can be done
> later (and needs more testing etc).
> 

Ok, I'll change to *-*-linux*.
I used powerpc*-*-linux* because most of the tests in this directory do 
that way.

> But please write some simple comment saying why you need -linux* in the
> test.
> 
>> +void
>> +test_backtrace()
>> +{
>> +  int addresses;
>> +  void *buffer[10];
>> +
>> +  addresses = backtrace(buffer, 10);
>> +  if(addresses != 4)
>> +    __builtin_abort();
>> +}
> 
> Does that work?!  Has this been tested on all powerpc*-linux configs?
> Importantly also BE and 32-bit.
> 

I haven't tested on BE, will do.

> Okay for trunk with the testcase fix, if all testing works out.  Thanks!
> 
> 
> Segher
> 

Thanks for the review,
-- 
Raphael Moreira Zinsly