Bug 1540316 - python2: Source code implies 16-byte alignment for PyObject_GC_New allocator, provides only 8
Summary: python2: Source code implies 16-byte alignment for PyObject_GC_New allocator,...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: python2
Version: rawhide
Hardware: x86_64
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Petr Viktorin (pviktori)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1537244 1537255
TreeView+ depends on / blocked
 
Reported: 2018-01-30 18:41 UTC by Florian Weimer
Modified: 2018-02-15 15:03 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-02-15 15:03:58 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Florian Weimer 2018-01-30 18:41:06 UTC
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.

Comment 1 Florian Weimer 2018-01-30 18:58:32 UTC
Raised on python-dev: https://mail.python.org/pipermail/python-dev/2018-January/152000.html

Comment 2 Miro Hrončok 2018-01-30 21:29:45 UTC
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.

Comment 3 Miro Hrončok 2018-01-30 21:48:22 UTC
https://koji.fedoraproject.org/koji/taskinfo?taskID=24578130 with the above patch applied.

Comment 4 Florian Weimer 2018-01-30 22:23:07 UTC
(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.

Comment 5 Miro Hrončok 2018-01-30 22:39:21 UTC
$ 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'===============

Comment 6 Florian Weimer 2018-01-31 10:48:06 UTC
(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

Comment 7 Petr Viktorin (pviktori) 2018-01-31 10:54:14 UTC
Thank you!
Scratch build in progress: https://koji.fedoraproject.org/koji/taskinfo?taskID=24592559

Comment 8 Petr Viktorin (pviktori) 2018-01-31 17:19:56 UTC
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.

Comment 9 Florian Weimer 2018-02-01 16:42:07 UTC
(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.

Comment 10 Petr Viktorin (pviktori) 2018-02-06 12:52:18 UTC
Not sure. I'll let you know if upstream wants to use your patch.


Note You need to log in before you can comment on or make changes to this bug.