Hi!
Note, the changes aren't acceptable for 5.1 at this point (release is
tomorrow), and for 5.2 backport they should be tested for a while on the
trunk, so there is no rush now.
> --- a/libsanitizer/ChangeLog
> +++ b/libsanitizer/ChangeLog
> @@ -1,3 +1,15 @@
> +2015-04-19 Martin Sebor <mse...@redhat.com>
> +
> + PR sanitizer/65479
> + * libsanitizer/sanitizer_common/sanitizer_stacktrace.h
> + (StackTrace::signaled, StackTrace::min_insn_bytes): New data members.
> + (StackTrace::StackTrace): Initialize signaled.
> + * libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
> + (StackTrace::GetPreviousInstructionPc): Rewrite.
> + * libsanitizer/sanitizer_common/sanitizer_stacktrace_libcdep.cc
> + (StackTrace::Print): Use min_insn_bytes to adjust PC value.
> + (BufferedStackTrace::Unwind): Set signaled.
libsanitizer/ should not show up in the ChangeLog entry.
But as somebody said earlier, the libsanitizer changes really should go
to LLVM compiler-rt repo first and then be just backported, either
cherry-picked (probably the case for the 5 branch backport later on) or go in
full merge from compiler-rt.
> --- a/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
> +++ b/libsanitizer/sanitizer_common/sanitizer_stacktrace.cc
> @@ -15,19 +15,33 @@
>
> namespace __sanitizer {
>
> -uptr StackTrace::GetPreviousInstructionPc(uptr pc) {
> -#if defined(__arm__)
> - // Cancel Thumb bit.
> - pc = pc & (~1);
> -#endif
Your code loses this, which is undesirable.
> -#if defined(__powerpc__) || defined(__powerpc64__)
> - // PCs are always 4 byte aligned.
> - return pc - 4;
> -#elif defined(__sparc__) || defined(__mips__)
> - return pc - 8;
The SPARC/MIPS case is of course needed, because on these architectures
the call is followed by a delay slot. But I wonder why you need anything
special on any other architecture, why pc - 1 isn't good enough for those.
The point isn't to find a PC of the call instruction, on some targets that
is very hard and you need to disassemble, but to just find some byte in the
call instruction.
> +const unsigned StackTrace::min_insn_bytes =
> +#if defined __ia64__
> + // Intel Itanium has 5 byte instructions.
> + 5
E.g. this is wrong, ia64 doesn't have 5 byte instructions, but has VLIW
bundles, where in the 16 byte bundle there are up to 3 41-bit instructions
plus template. But, ia64 isn't supported by libsanitizer and I doubt there
are enough users that would be interested in writing support for a dead
architecture.
Jakub
