Page MenuHomePhabricator

[PowerPC] Fix sanitizer frame unwind on 32-bit ABIs
Needs ReviewPublic

Authored by seurer on Mar 20 2017, 1:09 PM.

Details

Reviewers
kbarton
Summary

This fixes many sanitizer problems with -m32. It is really intended
for gcc but patches to the sanitizers make their way through llvm
first.

ref: https://gcc.gnu.org/ml/gcc-patches/2017-02/msg00855.html

(Note: patch being submitted by me on behalf of Segher Boessenkool
who works on gcc. See ref: above.)

Diff Detail

Event Timeline

seurer created this revision.Mar 20 2017, 1:09 PM
kcc added a subscriber: kcc.Mar 21 2017, 3:30 PM

This adds more ifdefs. Please don't. Instead please eliminate all ifdefs from inside the function body.
Or, better, remove all ifdefs completely
like this:

uhwptr GetPC1(uhwptr *frame) {

if (SANITIZER_PPC) return WHATEVER;
if (SANITIZER_S390) return WHATEVER;
return frame[1];

}

Please note that the current code was approved w/o me (https://reviews.llvm.org/D18895): I would never ever allow ifdefs like this.

kcc added a comment.Mar 21 2017, 3:36 PM

Also, please make sure to CC llvm-commits to all code reviews in sanitizer run-time.

segher added a subscriber: segher.Mar 21 2017, 3:54 PM
In D31149#706988, @kcc wrote:

This adds more ifdefs. Please don't. Instead please eliminate all ifdefs from inside the function body.
Or, better, remove all ifdefs completely
like this:

uhwptr GetPC1(uhwptr *frame) {

if (SANITIZER_PPC) return WHATEVER;
if (SANITIZER_S390) return WHATEVER;
return frame[1];

}

Please note that the current code was approved w/o me (https://reviews.llvm.org/D18895): I would never ever allow ifdefs like this.

Hi Kostya,

The #ifdef this adds is for _CALL_SYSV, which is a preprocessor symbol that is defined if and only if
you are compiling for the SVR4 ABI (on PowerPC anyway). I cannot change the ABI. Do you want this
hidden in some other macro somewhere? If so, where?

+llvm-commits (please don't forget it)
The only place where the C preprocessor is welcome is lib/sanitizer_common/sanitizer_platform.h

Ah, okay, that'll work fine. I'll make a new patch then (tomorrow).

Someone who can test it will have to handle the s390 part, sorry.