Unbreak PowerPC build on macOS #419
Conversation
|
CI error has obviously nothing to do with my PR: Python 3.7 does not build on macOS arm64, AFAIK, someone should fix a broken CI setup :) |
|
barracuda156
force-pushed
the
powerpc
branch
from
ff0d597 to
14c1890
Compare
|
@cclauss Thank you, rebased. |
|
Seems fine to me. I don't have merge permission, hopefully someone who does will see this. |
|
@barracuda156 Does the testsuite actually pass for you on 32-bit PowerPC with these changes? I'm asking, because I'm getting a segmentation fault on 32-bit PowerPC on Linux. Looking at the assembly code for 32-bit PowerPC on Linux and Unix and macOS, the former looks quite different. |
|
@glaubitz How do I run it? I tried now Perhaps |
|
If this is relevant from then I need to add |
|
I'm running it like this and it crashes with a segmentation fault: |
|
@glaubitz Unless I miss something (on mobile now), this looks similar/identical to what MacPorts does, save for |
|
So, the crash looks like this: Let me know if you're seeing this as well. |
|
That's probably the same thing as #422. If so, it should be fixed in trunk. |
Not 100% sure. There could still be syntax errors in the actual assembly code. @barracuda156 needs to test-build and run the testsuite on MacOS X on PowerPC. |
Sorry, I should have been more clear. I meant this branch, with the patch, rebased on master to pull in the fix for 422. The backtrace was clearly the same problem as LinuxPPC-32 was having. But now that I look more closely, I see that it's the same problem, but possibly from a different source. I'll have to make another change. |
|
@jamadden So should I wait for the fix before trying to make tests run? |
OK, I misunderstood.
But @barracuda156 never ran the testsuite because he didn't know why, did he? I don't think there are other changes necessary, the SysV ABI should be the same across Linux, NetBSD and MacOS X. @jamadden Before making any more changes, please let @barracuda156 run the tests with on master with #419 applied. |
|
@glaubitz I think macOS should not be using SysV ABI, though a given code can be identical between SysV and PowerOpen. I will try running tests today and update here. |
barracuda156
force-pushed
the
powerpc
branch
from
14c1890 to
473ff40
Compare
|
Is there a way to run tests really manually, test-by-test? This unitframework does not seem to do anything. |
Argh. Reading from the top of the thread down and not bottom up, I see why the backtrace looked like the Linux problem: Because the backtrace you posted actually was the Linux crash. Reading comprehension fail.
(One tests depends on being run from a project directory, so expect a version test to fail.) That said, most test files will be runnable directly (they'll have a call to |
|
@jamadden Thank you, that indeed works, however it wants a package which we do not have: I will try adding it. |
|
To run the tests, you must install greenlet with the dependencies listed in the 'test' extra. Normally this is done with If you're not using pip or running BTW, I'm a huge MacPorts fan, and have been since at least 2010. Thanks for your involvement! |
|
Ok, I made a port for (Sorry if it is something silly on my side, I know nothing about how Python stuff should work.) |
Greenlet has a package (directory) called One way to confirm would be to use the same interpreter from the same working directory in the same shell as you used to report the graphviz bug, and execute this: $ /opt/local/bin/python3.11 -c 'import platform; print(platform.__file__)'
/opt/local/Library/Frameworks/Python.framework/Versions/3.11/lib/python3.11/platform.pyIf you don't get that output, but instead get something like the following, it means one of those possibilities I mentioned above is true. $ pwd
/.../GithubSources/greenlet/src/greenlet
$ /opt/local/bin/python3.11 -c 'import platform; print(platform.__file__)'
/.../GithubSources/greenlet/src/greenlet/platform/__init__.py(Setting the |
|
@jamadden Hmm, I can get the correct directory upon setting env variable, but Python test fails to run like before: |
Got it, thanks! It may be awhile before I can get to this.
It shouldn't be. greenlet doesn't directly use that API, nor does it call
Thank you, that is different and helps localize a bit. Any luck with |
Oh, never mind. It was scrolled off to the right. (Why does github insist on limiting the width of the comment column?) |
Yeah, I added that in the end.
Sorry that I can't do much for 10.5, but 10.6 ppc already takes all of the spare time. If only I had another Quad to run both in parallel. |
Huh? Snow Leopard on PowerPC? How is that possible? |
There are developer builds which run on PowerPC. (Recently we got 10.6.8 running, however for a reason yet unknown DNS/DHCP are broken on it, so it is of little utility at the moment.) |
|
@glaubitz This: |
|
These are the top three frames (lightly edited for clarity): Starting from the bottom, The next frame up is...jumping into the Python binary image!? There's nothing there that should make that possible; at no time does this execute any CPython functions. Now, Finally, the top frame is where we're actually executing The First, Python itself could be doing something at thread exit time and freeing memory allocated by the thread. It doesn't do that, so we can rule that out. Second, something is broken with thread local variables (and/or thread local variable cleanup) such that it's running more than once, or has already deallocated the memory (holding the pointer itself) before running the cleanup code. Since this is a pre-release of an ancient platform (2009) running code that depends on C++ 11 features, that second possibility seems the more likely. I can only think of three possible ways forward. First, you can try compiling with optimizations disabled ( Second, you could try experimenting with the Failing that, we can disable thread-local cleanup for this platform and just let thread data leak. Since nobody should be using this platform for anything serious, that's unlikely to be a major problem. @barracuda156 Could you try with optimizations disabled? If that's a no-go, I'll disable cleanup, merge this patch, and call it good. |
|
@jamadden Sure, I will rebuild with optimizations off and update on the results. P. S. I am not aware of anything in C++11 not working on this platform. Of course, for C++11 and up it uses modern |
I'm not sure that's true. Here are the bottom three frames; the first column is the shared library that the code is from, the second is the address of the function being executed: Obviously the actual threading exit and thread local cleanup (frames 6 and 7) is initiated by Frame 5 is the C++ cleanup, from There are THREE |
|
The good question is whether the right one is being used. I will look into this. ( |
It's mapped to two different address ranges. Hence there are two copies loaded. |
|
Thanks for bringing this C++ runtime issue, it is a well-known thing, but I did not consider that tests are compiling stuff and therefore need this issue taken care of. After |
|
There are at least two expected crashes of subprocesses during the test run, but I don't think that's one of them. Could you say which test was running? Assuming it's not an expected crash, that's a failure in |
|
@jamadden Let me rerun this cleanly. I will also rebuild with |
|
Yes, I copied a wrong crash earlier. This is the third one: Re optimizations: I passed |
So long as
The only way I know of doing this is to actually edit the I wish I knew a better way. I maintain two copies of that file, one being the original file containing the right flags for clang, and the other being modified to work with gcc. |
|
I'm unable to make further progress on this. Despite the excellent instructions provided by @barracuda156 I'm simply not going to have the time to set up a pre-release development environment on one of my 32-bit machines. Therefore, I will go with the fallback plan of merging this patch and disabling thread cleanup on this platform. If we get more information or someone comes forward with a patch, I will happily revisit the issue. |
I would suggest pinging @he32 before merging a patch that touches BSD code. |
|
This doesn't touch BSD code. It's strictly 32-bit PPC on MacOSX. |
|
@jamadden The setup from scratch will probably take about 2 hrs all in, but debugging may take a lot more, of course. Time is a scarce resource. I support your suggestion for now, but I hope we can improve on that. (Sorry, I can‘t really be useful for Python debugging myself beyond running tests, I consistently avoided Python so far, LOL.) |
|
@jamadden And that you very much for caring to fix this. This is really appreciated. |
This commit touched BSD code for PowerPC. |
|
True, it did, and it seems correct to me. If there's a regression on NetBSD, the only platform using that file, I await the bug report and patch. I need to close this out and move on. I've spent far too much time on it as is. |
|
@glaubitz I have a strong impression the earlier commit changing that code was off. Correct me if I am wrong, but AFAIK the only platform using r-prefixed registers is Darwin, not even AIX (though otherwise they are supposed to be at least close). |
|
@glaubitz UPD. In fact commits here do not touch that, I confused another recent issue with ppc assembler elsewhere with this. |
|
My point is that there was a change to a slightly unrelated platform without actually testing the change. You should have installed NetBSD for macppc on your PowerMac and verified the change before submitting it. |
|
Well, honestly I’d be surprised if the correct assembler for NetBSD is identical to a wrong one on macOS and Linux on the same architecture, but I can try requesting someone with BSD to verify this. (I do get your point, but I think this case is fairly trivial.) |
Bumps [greenlet](https://github.com/python-greenlet/greenlet) from 3.0.3 to 3.1.1. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/python-greenlet/greenlet/blob/master/CHANGES.rst">greenlet's changelog</a>.</em></p> <blockquote> <h1>3.1.1 (2024-09-20)</h1> <ul> <li>Fix crashes on 32-bit PPC Linux. Note that there is no CI for this, and support is best effort; there may be other issues lurking. See <code>issue 422 <https://github.com/python-greenlet/greenlet/issues/422></code>_.</li> <li>Remove unnecessary logging sometimes during interpreter shutdown. See <code>issue 426 <https://github.com/python-greenlet/greenlet/issues/426></code>_.</li> <li>Fix some crashes on 32-bit PPC MacOS. This is a very old platform, and is only known to be tested on beta versions of an operating system that was never released, using the GCC 14 only provided by MacPorts; it may or may not work on the final MacOS X release that supported 32-bit PowerPC. It has the known issue of leaking memory when greenlets are used in multiple threads. Help debugging this would be appreciated. See <code>PR 419 <https://github.com/python-greenlet/greenlet/pull/419></code>_.</li> </ul> <h1>3.1.0 (2024-09-10)</h1> <p>.. note::</p> <pre><code>This will be the last release to support Python 3.7 and 3.8. </code></pre> <ul> <li>Adds support for Python 3.13.</li> </ul> <p>.. note::</p> <p>greenlet will not work in no-gil (free threaded) builds of CPython. Internally, greenlet heavily depends on the GIL.</p> <ul> <li>Greatly reduce the chances for crashes during interpreter shutdown. See <code>issue 411 <https://github.com/python-greenlet/greenlet/issues/411></code>_.</li> </ul> <h2>Platform Support</h2> <p>Support for the following platforms was contributed by the community. Note that they are untested by this project's continuous integration services.</p> <ul> <li>Hitachi's <code>SuperH CPU <https://github.com/python-greenlet/greenlet/issues/166></code>_.</li> <li><code>NetBSD on PowerPC. <https://github.com/python-greenlet/greenlet/pull/402></code>_</li> <li>RiscV 64 with <code>-fno-omit-frame-pointer <https://github.com/python-greenlet/greenlet/pull/404></code><em>. Note that there are <code>known test failures <https://github.com/python-greenlet/greenlet/issues/403></code></em>, so this</li> </ul> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/python-greenlet/greenlet/commit/dd0a948da112a574864b518e5739bd90d068a1b0"><code>dd0a948</code></a> Preparing release 3.1.1</li> <li><a href="https://github.com/python-greenlet/greenlet/commit/ab8d3bc1245f0e454cd3865009f6332a8c0090a0"><code>ab8d3bc</code></a> Disable thread-local cleanup on 32-bit MacOS PPC with GCC. This will result i...</li> <li><a href="https://github.com/python-greenlet/greenlet/commit/e9db22a94101e10909829c198b6d693ccf11f48f"><code>e9db22a</code></a> Merge pull request <a href="https://github.com/python-greenlet/greenlet/issues/429">#429</a> from python-greenlet/issue419redux</li> <li><a href="https://github.com/python-greenlet/greenlet/commit/6081a16bb1c560262b34cf112896a6982756d02c"><code>6081a16</code></a> Merge pull request <a href="https://github.com/python-greenlet/greenlet/issues/419">#419</a> from barracuda156/powerpc</li> <li><a href="https://github.com/python-greenlet/greenlet/commit/dbf311a021d80aecb38033a9f44fcf13be9e7a6f"><code>dbf311a</code></a> Greater safety and fewer assumptions doing cross-thread cleanup.</li> <li><a href="https://github.com/python-greenlet/greenlet/commit/9e8a90b1a47e1254df65057af444671e22ca61e0"><code>9e8a90b</code></a> Set back greenlet_thread_state.hpp file</li> <li><a href="https://github.com/python-greenlet/greenlet/commit/1bf374f4d8aaa8039012f70f93249726d23bdf59"><code>1bf374f</code></a> Duplicate greenlet_thread_state.hpp history.</li> <li><a href="https://github.com/python-greenlet/greenlet/commit/64e0b4f6ceaa5cc2debafe551caea1276aa84aa9"><code>64e0b4f</code></a> Copy greenlet_thread_state.hpp into TThreadStateCreator.hpp</li> <li><a href="https://github.com/python-greenlet/greenlet/commit/358a2e8f20816a68e65703b5488dd0337722a9a9"><code>358a2e8</code></a> Keep greenlet_thread_state.hpp</li> <li><a href="https://github.com/python-greenlet/greenlet/commit/5144f7070cd7882f643f6b16a3dbc9138dbbf483"><code>5144f70</code></a> Sigh. Pip hides compiler output which is, you know, important, and the only w...</li> <li>Additional commits viewable in <a href="https://github.com/python-greenlet/greenlet/compare/3.0.3...3.1.1">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Current code is broken, see #418
Looks like this is a silly copy-paste error – compare macOS file (which was likely made blindly) vs Linux one (which hopefully was actually tested).
Also, fix PowerPC macro for Darwin, so that a meaningful implementation is picked when building for
ppc64too. I cannot test it now, but given that Darwin does not use TOC and insns in the header are supported both in 32- and 64-bit ABI, it should work fine.