Bug 1540316
| Summary: | python2: Source code implies 16-byte alignment for PyObject_GC_New allocator, provides only 8 | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Florian Weimer <fweimer> |
| Component: | python2 | Assignee: | Petr Viktorin (pviktori) <pviktori> |
| Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | unspecified | Docs Contact: | |
| Priority: | unspecified | ||
| Version: | rawhide | CC: | bkabrda, cstratak, dmalcolm, mcyprian, mhroncok, pviktori, rkuska, tomspur, torsava |
| Target Milestone: | --- | ||
| Target Release: | --- | ||
| Hardware: | x86_64 | ||
| OS: | Unspecified | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | If docs needed, set a value | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2018-02-15 15:03:58 UTC | Type: | Bug |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
| Bug Depends On: | |||
| Bug Blocks: | 1537244, 1537255 | ||
Raised on python-dev: https://mail.python.org/pipermail/python-dev/2018-January/152000.html This has been around since 3.4: https://github.com/python/cpython/commit/e348c8d154cf6342c79d627ebfe89dfe9de23817 I wonder how dangerous would it be to backport it. Will try to in a scratch build anyway. https://koji.fedoraproject.org/koji/taskinfo?taskID=24578130 with the above patch applied. (In reply to Miro Hrončok from comment #2) > This has been around since 3.4: > > https://github.com/python/cpython/commit/ > e348c8d154cf6342c79d627ebfe89dfe9de23817 > > I wonder how dangerous would it be to backport it. Will try to in a scratch > build anyway. The patch is definitely a bit on the scary side. You could perhaps add additional padding to PyGC_Head to keep its size unchanged. But I looked on codesearch.debian.net and couldn't find any external extensions using GC tracking, but perhaps I didn't search thoroughly enough. $ abipkgdiff --debug-info-pkg1 ./python2-debuginfo-2.7.14-8.fc28.x86_64.rpm --debug-info-pkg2 ./python2-debuginfo-2.7.14-9.fc28.x86_64.rpm --devel-pkg1 ./python2-devel-2.7.14-8.fc28.x86_64.rpm --devel-pkg2 ./python2-devel-2.7.14-9.fc28.x86_64.rpm ./python2-libs-2.7.14-8.fc28.x86_64.rpm ./python2-libs-2.7.14-9.fc28.x86_64.rpm
================ changes of 'libpython2.7.so.1.0'===============
Functions changes summary: 0 Removed, 1 Changed, 0 Added function
Variables changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added variable
1 function with some indirect sub-type change:
[C]'function void _PyGC_Dump(PyGC_Head*)' at gcmodule.c:1523:1 has some indirect sub-type changes:
parameter 1 of type 'PyGC_Head*' has sub-type changes:
in pointed to type 'typedef PyGC_Head' at objimpl.h:259:1:
underlying type 'union _gc_head' at objimpl.h:252:1 changed:
type size changed from 256 to 192 bits
1 data member changes (1 filtered):
type of 'long double _gc_head::dummy' changed:
type name changed from 'long double' to 'double'
type size changed from 128 to 64 bits
================ end of changes of 'libpython2.7.so.1.0'===============
(In reply to Florian Weimer from comment #4) > (In reply to Miro Hrončok from comment #2) > > This has been around since 3.4: > > > > https://github.com/python/cpython/commit/ > > e348c8d154cf6342c79d627ebfe89dfe9de23817 > > > > I wonder how dangerous would it be to backport it. Will try to in a scratch > > build anyway. > > The patch is definitely a bit on the scary side. You could perhaps add > additional padding to PyGC_Head to keep its size unchanged. See: https://mail.python.org/pipermail/python-dev/2018-January/152011.html Thank you! Scratch build in progress: https://koji.fedoraproject.org/koji/taskinfo?taskID=24592559 Built.
Thanks again for the patch, Florian! It saved us a lot of head-scratching.
If you want I can take care of pushing the patch upstream. Two things I'd need to do that properly, though:
- could you sign PSF's old-school contributor agreement (which confirms your contributions are open-source):
https://www.python.org/psf/contrib/contrib-form/
- and fill in your GitHub username on bugs.python.org -- that's used in some automated checks.
(In reply to Petr Viktorin from comment #8) > Built. > Thanks again for the patch, Florian! It saved us a lot of head-scratching. Do you know if this is the direction upstream wants to follow? The discussion was more favorable to changing the allocator parameters. Not sure. I'll let you know if upstream wants to use your patch. |
The immediate consequence is a FTBFS bug: /bin/sh: line 5: 16396 Segmentation fault (core dumped) LD_LIBRARY_PATH=/builddir/build/BUILD/Python-2.7.14/build/optimized ./python -E -S -m sysconfig --generate-posix-vars generate-posix-vars failed PyInstance_NewRaw contains this code: inst = PyObject_GC_New(PyInstanceObject, &PyInstance_Type); if (inst == NULL) { Py_DECREF(dict); return NULL; } inst->in_weakreflist = NULL; Py_INCREF(klass); inst->in_class = (PyClassObject *)klass; inst->in_dict = dict; _PyObject_GC_TRACK(inst); _PyObject_GC_TRACK expands to: #define _PyObject_GC_TRACK(o) do { \ PyGC_Head *g = _Py_AS_GC(o); \ if (g->gc.gc_refs != _PyGC_REFS_UNTRACKED) \ Py_FatalError("GC object already tracked"); \ … Via: #define _Py_AS_GC(o) ((PyGC_Head *)(o)-1) We get to this: /* GC information is stored BEFORE the object structure. */ typedef union _gc_head { struct { union _gc_head *gc_next; union _gc_head *gc_prev; Py_ssize_t gc_refs; } gc; long double dummy; /* force worst-case alignment */ } PyGC_Head; PyGC_Head has 16-byte alignment. The net result is that _PyObject_GC_TRACK(inst); promises to the compiler that inst is properly aligned for the PyGC_Head type, but it is not: PyObject_GC_New returns a pointer which is only 8-byte-aligned. Objects/obmalloc.c contains this: /* * Alignment of addresses returned to the user. 8-bytes alignment works * on most current architectures (with 32-bit or 64-bit address busses). * The alignment value is also used for grouping small requests in size * classes spaced ALIGNMENT bytes apart. * * You shouldn't change this unless you know what you are doing. */ #define ALIGNMENT 8 /* must be 2^N */ #define ALIGNMENT_SHIFT 3 #define ALIGNMENT_MASK (ALIGNMENT - 1) So either the allocator alignment needs to be increased, or the PyGC_Head alignment needs to be decreased.