Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests.test_contextvars.ContextVarsTests.test_break_ctxvars on 32-bit Linux PowerPC #422

Closed
glaubitz opened this issue Sep 10, 2024 · 17 comments

Comments

@glaubitz
Copy link

Running the testsuite on 32-bit Linux PowerPC fails immediately with a segmentation fault:

(sid_powerpc-dchroot)glaubitz@perotto:~/greenlet/build/lib.linux-ppc-cpython-312/greenlet$ PYTHONPATH=/home/glaubitz/greenlet/build/lib.linux-ppc-cpython-312 python3.12 -m
 unittest discover -v
test_break_ctxvars (tests.test_contextvars.ContextVarsTests.test_break_ctxvars) ... Segmentation fault (core dumped)
(sid_powerpc-dchroot)glaubitz@perotto:~/greenlet/build/lib.linux-ppc-cpython-312/greenlet$

Backtrace in GDB shows:

(sid_powerpc-dchroot)glaubitz@perotto:~/greenlet/build/lib.linux-ppc-cpython-312/greenlet$ gdb /usr/bin/python3.12 core.1669529
GNU gdb (Debian 15.1-1) 15.1
Copyright (C) 2024 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "powerpc-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<https://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /usr/bin/python3.12...
Reading symbols from /usr/lib/debug/.build-id/a6/4f9ab5e6f2702d682cd21b2f9f8c0273be0ac6.debug...
[New LWP 1669529]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/powerpc-linux-gnu/libthread_db.so.1".
Core was generated by `python3.12 -m unittest discover -v'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0fa9855c in green_init (self=..., args=<error reading variable: Cannot access memory at address 0x0>,
    kwargs=<error reading variable: Cannot access memory at address 0x0>) at src/greenlet/greenlet.cpp:253
253         if (!PyArg_ParseTupleAndKeywords(
(gdb) bt
#0  0x0fa9855c in green_init (self=..., args=<error reading variable: Cannot access memory at address 0x0>, kwargs=<error reading variable: Cannot access memory at address 0x0>) at src/greenlet/greenlet.cpp:253
#1  0x10161ebc in type_call (type=<optimized out>, type@entry=0xfac027c <PyGreenlet_Type>, args=args@entry=(<built-in method run of _contextvars.Context object at remote 0xf740ed68>,), kwds=kwds@entry=0x0)
    at ../Objects/typeobject.c:1677
#2  0x100da1b4 in _PyObject_MakeTpCall (tstate=0x107c1f08 <_PyRuntime+235672>, callable=callable@entry=<type at remote 0xfac027c>, args=args@entry=0xf78cd664, nargs=1, keywords=<optimized out>, keywords@entry=0x0)
    at ../Objects/call.c:240
#3  0x100db0a0 in _PyObject_VectorcallTstate (kwnames=0x0, nargsf=<optimized out>, args=0xf78cd664, callable=<type at remote 0xfac027c>, tstate=<optimized out>) at ../Include/internal/pycore_call.h:90
#4  0x101bc314 in _PyEval_EvalFrameDefault (tstate=0x107c1f08 <_PyRuntime+235672>, frame=0xf78cd628, throwflag=<optimized out>) at Python/bytecodes.c:2715
#5  0x100dc1c8 in _PyFunction_Vectorcall (kwnames=0x0, nargsf=3, stack=0xffd0730c, func=<function at remote 0xf74a4e38>) at ../Objects/call.c:419
#6  _PyObject_FastCallDictTstate (kwargs=0x0, nargsf=3, args=0xffd0730c, callable=<function at remote 0xf74a4e38>, tstate=0x107c1f08 <_PyRuntime+235672>) at ../Objects/call.c:133
#7  _PyObject_Call_Prepend (tstate=tstate@entry=0x107c1f08 <_PyRuntime+235672>, callable=callable@entry=<function at remote 0xf74a4e38>,
    obj=obj@entry=<_RefCountChecker(testcase=<ContextVarsTests(_testMethodName='test_break_ctxvars', _outcome=<_Outcome(expecting_failure=False, result=<TextTestResult(failfast=False, failures=[], errors=[], testsRun=1, skipped=[], expectedFailures=[], unexpectedSuccesses=[], collectedDurations=[], shouldStop=False, buffer=False, tb_locals=False, _stdout_buffer=None, _stderr_buffer=None, _original_stdout=<_io.TextIOWrapper(mode='w') at remote 0xf77da7a8>, _original_stderr=<_io.TextIOWrapper(mode='w') at remote 0xf77da820>, _mirrorOutput=False, stream=<_WritelnDecorator(stream=<...>) at remote 0xf7451c90>, showAll=True, dots=False, descriptions=True, _newline=False, durations=None, _testRunEntered=True, _moduleSetUpFailed=False, _previousTestClass=<TestCaseMetaClass(__module__='tests.test_contextvars', _new_ctx_run=<function at remote 0xf74a65c8>, _increment=<function at remote 0xf74a6618>, _test_context=<function at remote 0xf74a6668>, test_context_propagated_by_context_run=<function at remote 0xf74a6988>, test_context_propaga...(truncated), args=args@entry=((), {}), kwargs=0x0) at ../Objects/call.c:508
#8  0x1016fb8c in slot_tp_call (
    self=self@entry=<_RefCountChecker(testcase=<ContextVarsTests(_testMethodName='test_break_ctxvars', _outcome=<_Outcome(expecting_failure=False, result=<TextTestResult(failfast=False, failures=[], errors=[], testsRun=1, skipped=[], expectedFailures=[], unexpectedSuccesses=[], collectedDurations=[], shouldStop=False, buffer=False, tb_locals=False, _stdout_buffer=None, _stderr_buffer=None, _original_stdout=<_io.TextIOWrapper(mode='w') at remote 0xf77da7a8>, _original_stderr=<_io.TextIOWrapper(mode='w') at remote 0xf77da820>, _mirrorOutput=False, stream=<_WritelnDecorator(stream=<...>) at remote 0xf7451c90>, showAll=True, dots=False, descriptions=True, _newline=False, durations=None, _testRunEntered=True, _moduleSetUpFailed=False, _previousTestClass=<TestCaseMetaClass(__module__='tests.test_contextvars', _new_ctx_run=<function at remote 0xf74a65c8>, _increment=<function at remote 0xf74a6618>, _test_context=<function at remote 0xf74a6668>, test_context_propagated_by_context_run=<function at remote 0xf74a6988>, test_context_propaga...(truncated), args=args@entry=((), {}), kwds=kwds@entry=0x0) at ../Objects/typeobject.c:8779

This issue was first seen in Debian with version 2.0.1: https://buildd.debian.org/status/logs.php?pkg=python-greenlet&arch=powerpc

Related bug report in Gentoo is: https://bugs.gentoo.org/923062

jamadden added a commit that referenced this issue Sep 10, 2024
…yArg_ParseTupleAndKeywords; this eliminates a cast.

Seems unlikely, but it might have something to do with #422
@jamadden
Copy link
Contributor

Commit a97d81f might fix this. It reverts a minor change to be more like greenlet 1.x.

@glaubitz
Copy link
Author

Thanks for the quick reply. Unfortunately, that change makes no difference and the bug still persists.

I wonder whether whether @he32 has run the testsuite on 32-bit NetBSD PowerPC.

@he32 Can you comment?

jamadden added a commit that referenced this issue Sep 10, 2024
…ny assumption about binary compatible layouts.

Refs #422
@jamadden
Copy link
Contributor

@glaubitz Could you please try db5c781 ? Hopefully, that will at least move the crash to a different location if not fix it entirely. (If it does crash again in a different location, the traceback would be helpful again.)

@glaubitz
Copy link
Author

No difference, unfortunately.

GDB yields the following including disassembly:

#0  0x0fa98a6c in greenlet::refs::_BorrowedGreenlet<_greenlet, &greenlet::refs::GreenletChecker>::operator-> (this=<synthetic pointer>)
    at src/greenlet/greenlet_internal.hpp:79
79          return reinterpret_cast<PyGreenlet*>(this->p)->pimpl;
(gdb) x/5i $pc
=> 0xfa98a6c <green_getrun(greenlet::refs::BorrowedGreenlet, void*)+16>:        lwz     r3,16(r9)
   0xfa98a70 <green_getrun(greenlet::refs::BorrowedGreenlet, void*)+20>:        lwz     r9,0(r3)
   0xfa98a74 <green_getrun(greenlet::refs::BorrowedGreenlet, void*)+24>:        stmw    r30,8(r1)
   0xfa98a78 <green_getrun(greenlet::refs::BorrowedGreenlet, void*)+28>:        lwz     r9,40(r9)
   0xfa98a7c <green_getrun(greenlet::refs::BorrowedGreenlet, void*)+32>:        mflr    r30
(gdb)

Btw, you can apply for an account with the GCC Compile Farm here and try it yourself on ssh gcc203.fsffrance.org. I have admin rights on this machine and could help set up the environment.

@jamadden
Copy link
Contributor

Ah. That's a completely different backtrace ( at src/greenlet/greenlet_internal.hpp:79, which is called from green_getrun). I was afraid of this.

For some reason, on this platform, plain-vanilla C++ objects have a layout that's different than on any other platform. This breaks some assumptions. It's fixable, but tedious, and it makes the code harder to read and reason about from a static analysis.

I wonder why the layout is different there. Any chance you're using unusual compiler arguments?

@glaubitz
Copy link
Author

No special compiler arguments are being used, it's built the same way as for all the other targets.

@jamadden
Copy link
Contributor

Ok, that brings us to the arguments that greenlet itself supplies.

In #94 from 2015, a workaround that added -Os for ppc linux was committed.

Perhaps that's no longer necessary. Or perhaps it should just be -O0.

@glaubitz
Copy link
Author

Ah, that could be it. @karcherm is currently looking into it and his suspicion was it could be "by value" vs. "by reference".

@karcherm
Copy link

It seems gcc on powerpc always passes structures by reference, even if their only member is a simple pointer:

#include <stdio.h>

struct X {
        int *ptr;
};

void printptr1(struct X arg)
{
        printf("%p\n", arg.ptr);
}

void printptr2(int *ptr)
{
        printf("%p\n", ptr);
}

typedef void pp(int *);
pp* p[2] = {(pp*)printptr1, printptr2};

int main(void)
{
        int j = 0x11234;
        int* iptr = &j;
        p[0](iptr);
        p[1](iptr);
}

prints the follwing output (no matter whether I use -O0 or -Os:

$ rm testme.o ; make testme.o CFLAGS="-Os -g"; make testme ; ./testme
cc -Os -g   -c -o testme.o testme.c
cc   testme.o   -o testme
0x11234
0xfff70808

@jamadden
Copy link
Contributor

jamadden commented Sep 11, 2024

I can confirm, I just found it in Power Architecture 32-bit Application Binary Interface Supplement 1.0 - Linux® & Embedded, section "3.2.3.1. Parameter Passing Register Selection Algorithm", where it says registers can contain:

A struct or union that shall be treated as a pointer to the data object, ...

This is backed up by "3.2.3.2. Parameter Passing Examples". Given the setup code:

typedef struct {
  int    a;
  double dd;
} sparm;
sparm   s, t;
int     c, d, e;
long double ld;
double  ff, gg, hh;
x = func(c, ff, d, ld, s, gg, t, e, hh);

the example states that in the call to x, register r5 winds up containing a "ptr to s." There are no exceptions for single member structs. The rule that the address of a struct is the address of its first member only allows for that optimization but does not require it.

That's unique across Win/Linux/Mac x86 and x86_64, Mac/Linux aarch64, S390x, and ppc64.

@glaubitz
Copy link
Author

If the struct just contains one pointer as a member, why not pass the pointer itself?

@jamadden
Copy link
Contributor

Type safety and automatic reference counting. It caught a number of memory leaks when I added it.

@karcherm
Copy link

If the struct just contains one pointer as a member, why not pass the pointer itself?

The functions we are talking about are called by more generic code, and they get in fact called using plain pointers. Greenlet attaches semantics to the pointers it receives from the generic code by using C++ smart pointers. There are different structs for pointers that you need to free when you are done, and pointers that are managed by some other code entity, and you don't need to worry about freeing - but you also must not store copies of such pointers that live beyong the current invocation of a function, because they might get freed by the owning code any time after the call is finished. This is called a "borrowed pointer".

The code in greenlet is non-conformant to the C/C++ standard by assuming that a caller that expects to call a function that receives a plain pointer (maybe even a simple void * ) can call functions that have a "struct wrapper type" instead. The struct wrapper makes memory handling easier. Using this non-conformant way instead of adding an extra layer of indirection by writing wrapper functions that convert raw pointers to wrapped pointers and pass them to the actual worker functions simplifies the source code.

@jamadden
Copy link
Contributor

Well said @karcherm , that's all true. I knew it was technically non-conformant, but across 10+ (very) different platforms, I'd never seen an ABI where it actually made a difference. Now I have, and must rework accordingly.

@jamadden
Copy link
Contributor

I think I updated all the slot functions in the issue422 branch. If someone has the opportunity to test that, I'd appreciate it!

If there are still errors, I may have to take @glaubitz up on accessing the GCC compiler farm.

@glaubitz
Copy link
Author

I think I updated all the slot functions in the issue422 branch. If someone has the opportunity to test that, I'd appreciate it!

I can confirm that the changes from the branch issue422 fix the testsuite for me on 32-bit PowerPC.

There is just one unrelated failure left which is probably a result of how I ran the testsuite.

test_version (tests.test_version.VersionTests.test_version) ... Traceback (most recent call last):
  File "/home/glaubitz/greenlet/setup.py", line 212, in <module>
    version=get_greenlet_version(),
            ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/glaubitz/greenlet/setup.py", line 201, in get_greenlet_version
    with open('src/greenlet/__init__.py') as f: # pylint:disable=unspecified-encoding
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: 'src/greenlet/__init__.py'
FAIL

Thanks a lot for fixing this!

@jamadden
Copy link
Contributor

That's great, thanks! And yes, the greenlet test suite is sensitive to being run from the directory it is checked out in because of that specific test. But its failure can be ignored.

shaldengeki referenced this issue in shaldengeki/monorepo Sep 24, 2024
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
&lt;https://github.com/python-greenlet/greenlet/issues/422&gt;</code>_.</li>
<li>Remove unnecessary logging sometimes during interpreter shutdown.
See <code>issue 426
&lt;https://github.com/python-greenlet/greenlet/issues/426&gt;</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
&lt;https://github.com/python-greenlet/greenlet/pull/419&gt;</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
&lt;https://github.com/python-greenlet/greenlet/issues/411&gt;</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
&lt;https://github.com/python-greenlet/greenlet/issues/166&gt;</code>_.</li>
<li><code>NetBSD on PowerPC.
&lt;https://github.com/python-greenlet/greenlet/pull/402&gt;</code>_</li>
<li>RiscV 64 with <code>-fno-omit-frame-pointer
&lt;https://github.com/python-greenlet/greenlet/pull/404&gt;</code><em>.
Note that
there are <code>known test failures
&lt;https://github.com/python-greenlet/greenlet/issues/403&gt;</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 />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=greenlet&package-manager=pip&previous-version=3.0.3&new-version=3.1.1)](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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants