Do not build libsanitizer also for powerpc*-*-linux*

Looks like now function does not return anything for ARM case? I'd say 
we should replace this pc = ... with return like all other cases, the 
code is just asking for trouble.

On Mon, Jun 02, 2014 at 10:22:11AM +0400, Yury Gribov wrote:
> Looks like now function does not return anything for ARM case? I'd
> say we should replace this pc = ... with return like all other
> cases, the code is just asking for trouble.

But it should be
  return (pc & ~(uptr)1) - 1;
right?

> --- libsanitizer/sanitizer_common/sanitizer_stacktrace.cc	(revision 209878)
> +++ libsanitizer/sanitizer_common/sanitizer_stacktrace.cc	(revision 209879)
> @@ -18,11 +18,13 @@
>   namespace __sanitizer {
> 
>   uptr StackTrace::GetPreviousInstructionPc(uptr pc) {
> -#ifdef __arm__
> +#if defined(__arm__)
>     // Cancel Thumb bit.
>     pc = pc & (~1);
> -#endif
> -#if defined(__sparc__)
> +#elif defined(__powerpc__) || defined(__powerpc64__)
> +  // PCs are always 4 byte aligned.
> +  return pc - 4;
> +#elif defined(__sparc__)
>     return pc - 8;
>   #else
>     return pc - 1;

	Jakub

On 06/02/2014 11:56 AM, Jakub Jelinek wrote:
>> Looks like now function does not return anything for ARM case? I'd
>> say we should replace this pc = ... with return like all other
>> cases, the code is just asking for trouble.
>
> But it should be
>    return (pc & ~(uptr)1) - 1;
> right?

Yes, I had something like this in mind.

-Y

On Mon, Jun 2, 2014 at 8:56 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Jun 02, 2014 at 10:22:11AM +0400, Yury Gribov wrote:
>> Looks like now function does not return anything for ARM case? I'd
>> say we should replace this pc = ... with return like all other
>> cases, the code is just asking for trouble.
>
> But it should be
>   return (pc & ~(uptr)1) - 1;

Why the  -1 ? No ARM or Thumb instruction is 1 byte long. Instructions
are 4 bytes long if in ARM state and could be 2 or 4 bytes if Thumb
state.

regards
Ramana




> right?
>
>> --- libsanitizer/sanitizer_common/sanitizer_stacktrace.cc     (revision 209878)
>> +++ libsanitizer/sanitizer_common/sanitizer_stacktrace.cc     (revision 209879)
>> @@ -18,11 +18,13 @@
>>   namespace __sanitizer {
>>
>>   uptr StackTrace::GetPreviousInstructionPc(uptr pc) {
>> -#ifdef __arm__
>> +#if defined(__arm__)
>>     // Cancel Thumb bit.
>>     pc = pc & (~1);
>> -#endif
>> -#if defined(__sparc__)
>> +#elif defined(__powerpc__) || defined(__powerpc64__)
>> +  // PCs are always 4 byte aligned.
>> +  return pc - 4;
>> +#elif defined(__sparc__)
>>     return pc - 8;
>>   #else
>>     return pc - 1;
>
>         Jakub

On Mon, Jun 02, 2014 at 10:24:32AM +0100, Ramana Radhakrishnan wrote:
> On Mon, Jun 2, 2014 at 8:56 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Mon, Jun 02, 2014 at 10:22:11AM +0400, Yury Gribov wrote:
> >> Looks like now function does not return anything for ARM case? I'd
> >> say we should replace this pc = ... with return like all other
> >> cases, the code is just asking for trouble.
> >
> > But it should be
> >   return (pc & ~(uptr)1) - 1;
> 
> Why the  -1 ? No ARM or Thumb instruction is 1 byte long. Instructions
> are 4 bytes long if in ARM state and could be 2 or 4 bytes if Thumb
> state.

Well, that is what the code did before this patch.
The -1 just points to the middle of previous instruction, so that supposedly
it can be looked up in debug info etc.
Now, if all insns are at least 2 bytes long, perhaps you could subtract 2.

	Jakub

>> Why the  -1 ? No ARM or Thumb instruction is 1 byte long. Instructions
>> are 4 bytes long if in ARM state and could be 2 or 4 bytes if Thumb
>> state.
>
> The -1 just points to the middle of previous instruction,
> so that supposedly it can be looked up in debug info etc.

Right, that works quite well with gdb, addr2line, etc. I once tried 
adding proper handling of ARM/Thumb but this complicated code with no 
real benefit.

-Y

On Mon, Jun 2, 2014 at 10:44 AM, Yury Gribov <y.gribov@samsung.com> wrote:
>>> Why the  -1 ? No ARM or Thumb instruction is 1 byte long. Instructions
>>> are 4 bytes long if in ARM state and could be 2 or 4 bytes if Thumb
>>> state.
>>
>>
>> The -1 just points to the middle of previous instruction,
>> so that supposedly it can be looked up in debug info etc.
>
>
> Right, that works quite well with gdb, addr2line, etc. I once tried adding
> proper handling of ARM/Thumb but this complicated code with no real benefit.

Well, then put a comment in . A casual reader will have the same
reaction that I just did.

Ramana

>
> -Y