GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-149202: Fix frame pointer unwinding on ppc64le and armv7/clang by encukou · Pull Request #149409 · python/cpython Conversation
|
Documentation build overview
21 files changed ·
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🤖 New build scheduled with the buildbot fleet by @encukou for commit efb829e 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F149409%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
!buildbot ARM |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🤖 New build scheduled with the buildbot fleet by @encukou for commit 955261c 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F149409%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
!buildbot S390x |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🤖 New build scheduled with the buildbot fleet by @encukou for commit 955261c 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F149409%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Just read the PEP today, so I'll gently nudge for an extra fix here, or maybe I could send another PR :) ppc64le doesn't need any frame pointer compiler flags, so I'd exclude it entirely through configure. The power abi specification requires that compilers maintain a back chain by default, so unwinding already works without a dedicated frame pointer. Adding however Also following up on Diego's comment, the current unwinding model here won't work for ppc64le (stack frame layout is on page 34 of the ABI spec pdf). The LR is saved in the caller's frame (SP+16), not the current frame, so frame_pointer[RETURN_OFFSET] can't reach it. I can provide access to a power buildbot for testing it out or try to come up with a solution myself. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@diegorusso, does this look good to you? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I tested manually the change on s390x: test_frame_pointer_unwind fails on the main branch, and pass with this change: the PR works as expected.
Only -mbackchain is used (-fno-omit-frame-pointer is not used):
$ grep backchain Makefile
BASECFLAGS= -mbackchain -fno-strict-overflow -Wsign-compare
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| toolchains, CPython builds itself with ``-fno-omit-frame-pointer`` and, when | ||
| available, ``-mno-omit-leaf-frame-pointer`` by default. These flags allow | ||
| available, ``-mno-omit-leaf-frame-pointer`` by default. On 32-bit ARM, | ||
| CPython also adds ``-marm`` when supported. On s390 platforms, CPython also |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
s390 refers to the old 32-bit platform which is no longer in use. You should write s390x to refer to the current 64-bit Linux platform. Same remark for the whole PR.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| ``-mno-omit-leaf-frame-pointer``, ``-marm`` on 32-bit ARM, and ``-mbackchain`` | ||
| on s390 platforms when the compiler supports them, so profilers and debuggers | ||
| can unwind native interpreter frames more reliably. Users can pass | ||
| ``--without-frame-pointers`` to opt out. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| ``--without-frame-pointers`` to opt out. | |
| :option:`--without-frame-pointers` to ``./configure`` to opt out. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| .. _whatsnew315-frame-pointers: | ||
|
|
||
| * CPython is now built with frame pointers enabled by default | ||
| (:pep:`831`). Pass :option:`--without-frame-pointers` to opt out. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| (:pep:`831`). Pass :option:`--without-frame-pointers` to opt out. | |
| (:pep:`831`). Pass :option:`--without-frame-pointers` to ``./configure`` to opt out. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| By default, the build appends ``-fno-omit-frame-pointer``, | ||
| ``-mno-omit-leaf-frame-pointer`` when the compiler supports it, | ||
| ``-marm`` on 32-bit ARM when supported, and ``-mbackchain`` on s390 | ||
| platforms when supported, to ``BASECFLAGS`` so |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It may be useful to explain that -fno-omit-frame-pointer and -mno-omit-leaf-frame-pointer are not used (and should not be used) on s390x, only -mbackchain is used. Same remark for other places documenting these compiler flags.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for the review! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| # define FRAME_POINTER_NEXT_OFFSET 0 | ||
| # define FRAME_POINTER_RETURN_OFFSET \ | ||
| (S390X_FRAME_RETURN_ADDRESS_OFFSET / (Py_ssize_t)sizeof(uintptr_t)) | ||
| #elif defined(__powerpc64__) || defined(__ppc64__) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@vstinner as the powerpc expert: Are these the right macros to detect ppc64le?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
#if defined(__powerpc64__) || defined(__ppc64__) looks to good to me according to existing CPython code. __powerpc64__ macro is defined on Linux. The __ppc64__ macro is maybe needed on macOS or Windows, I'm not sure, but it's good to check for the two macros :-)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Well, the Linux stack layout might not apply to Mac... but then again, the last ppc Mac came out 20 years ago.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
!buildbot PPC64LE |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🤖 New build scheduled with the buildbot fleet by @encukou for commit 4ad01d6 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F149409%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There is also the PR gh-149362. I don't understand why PR should be used. If you only want to develop/debug in this PR, maybe mark it as a draft? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🤖 New build scheduled with the buildbot fleet by @encukou for commit b7cb2b3 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F149409%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I tested this change manually on Linux ppc64le: test_frame_pointer_unwind does pass on Python built with ./configure --with-pydebug (gcc -Og) and on Python built with ./configure (gcc -O3).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
ARM Raspbian still fails the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
The GNU backtrace failure is independent from the FP. Backtrace uses eh_frame information and on Arm (32bit) we should build Python with https://stackoverflow.com/questions/24700150/on-raspberry-pi-backtrace-returns-0-frames |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I confirm that this is the case. I'm going to raise a new PR to skip the GNU backtrace test on Arm Raspbian. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Wait this doesn't check out Python should be build with eh_frame in all architectures no? Why is this deactivated by default on arm 32 bit?! How does gdb work there? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
I had to investigate this a little bit but I'll reply to you in the PR I'm going to create. TL;DR: .eh_frame is not the runtime unwind format on every architecture.
GDB works on the 32-bit ARM binary because it can read debugger-only .debug_frame from the ELF file. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I wrote PR gh-149485 to remove |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Raised here: #149493 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.