Skip to content

bpo-27987: align PyGC_Head to alignof(long double) explicitly #13336

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.

Sign up for GitHub

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

Closed
methane wants to merge 1 commit into from
Closed

bpo-27987: align PyGC_Head to alignof(long double) explicitly #13336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 21 additions & 18 deletions Include/cpython/objimpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,20 +43,23 @@ PyAPI_FUNC(Py_ssize_t) _PyGC_CollectIfEnabled(void);
&& (Py_TYPE(o)->tp_is_gc == NULL || Py_TYPE(o)->tp_is_gc(o)))

/* GC information is stored BEFORE the object structure. */
typedef struct {
// Pointer to next object in the list.
// 0 means the object is not tracked
uintptr_t _gc_next;

// Pointer to previous object in the list.
// Lowest two bits are used for flags documented later.
uintptr_t _gc_prev;
typedef union {
struct {
// Pointer to next object in the list.
// 0 means the object is not tracked
uintptr_t next;

// Pointer to previous object in the list.
// Lowest two bits are used for flags documented later.
uintptr_t prev;
} _gc;
long double _dummy; /* for worst alignment */

@vstinner vstinner May 15, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please elaborate the comment? Add "bpo-27987" and explain that x64-64 ABI requires to align on 16 bytes.

Do you have to align to 16 bytes on x86 (32-bit) as well?

@methane methane May 16, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! I forgot about 32bit platform.
I thought this PR does nothing, but it makes PyGC_Head to 16 bytes from 8 bytes on x86 platform.

Maybe, we can use __attribute__ ((aligned)) if compiler is GCC or clang.

} PyGC_Head;

#define _Py_AS_GC(o) ((PyGC_Head *)(o)-1)

/* True if the object is currently tracked by the GC. */
#define _PyObject_GC_IS_TRACKED(o) (_Py_AS_GC(o)->_gc_next != 0)
#define _PyObject_GC_IS_TRACKED(o) (_Py_AS_GC(o)->_gc.next != 0)

/* True if the object may be tracked by the GC in the future, or already is.
This can be useful to implement some optimizations. */
Expand All @@ -65,7 +68,7 @@ typedef struct {
(!PyTuple_CheckExact(obj) || _PyObject_GC_IS_TRACKED(obj)))


/* Bit flags for _gc_prev */
/* Bit flags for _gc.prev */
/* Bit 0 is set when tp_finalize is called */
#define _PyGC_PREV_MASK_FINALIZED (1)
/* Bit 1 is set when the object is in generation which is GCed currently. */
Expand All @@ -74,23 +77,23 @@ typedef struct {
#define _PyGC_PREV_SHIFT (2)
#define _PyGC_PREV_MASK (((uintptr_t) -1) << _PyGC_PREV_SHIFT)

// Lowest bit of _gc_next is used for flags only in GC.
// Lowest bit of _gc.next is used for flags only in GC.
// But it is always 0 for normal code.
#define _PyGCHead_NEXT(g) ((PyGC_Head*)(g)->_gc_next)
#define _PyGCHead_SET_NEXT(g, p) ((g)->_gc_next = (uintptr_t)(p))
#define _PyGCHead_NEXT(g) ((PyGC_Head*)(g)->_gc.next)
#define _PyGCHead_SET_NEXT(g, p) ((g)->_gc.next = (uintptr_t)(p))

// Lowest two bits of _gc_prev is used for _PyGC_PREV_MASK_* flags.
#define _PyGCHead_PREV(g) ((PyGC_Head*)((g)->_gc_prev & _PyGC_PREV_MASK))
// Lowest two bits of _gc.prev is used for _PyGC_PREV_MASK_* flags.
#define _PyGCHead_PREV(g) ((PyGC_Head*)((g)->_gc.prev & _PyGC_PREV_MASK))
#define _PyGCHead_SET_PREV(g, p) do { \
assert(((uintptr_t)p & ~_PyGC_PREV_MASK) == 0); \
(g)->_gc_prev = ((g)->_gc_prev & ~_PyGC_PREV_MASK) \
(g)->_gc.prev = ((g)->_gc.prev & ~_PyGC_PREV_MASK) \
| ((uintptr_t)(p)); \
} while (0)

#define _PyGCHead_FINALIZED(g) \
(((g)->_gc_prev & _PyGC_PREV_MASK_FINALIZED) != 0)
(((g)->_gc.prev & _PyGC_PREV_MASK_FINALIZED) != 0)
#define _PyGCHead_SET_FINALIZED(g) \
((g)->_gc_prev |= _PyGC_PREV_MASK_FINALIZED)
((g)->_gc.prev |= _PyGC_PREV_MASK_FINALIZED)

#define _PyGC_FINALIZED(o) \
_PyGCHead_FINALIZED(_Py_AS_GC(o))
Expand Down
12 changes: 6 additions & 6 deletions Include/internal/pycore_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ PyAPI_FUNC(int) _PyDict_CheckConsistency(PyObject *mp, int check_content);
* NB: While the object is tracked by the collector, it must be safe to call the
* ob_traverse method.
*
* Internal note: _PyRuntime.gc.generation0->_gc_prev doesn't have any bit flags
* Internal note: _PyRuntime.gc.generation0->_gc.prev doesn't have any bit flags
* because it's not object header. So we don't use _PyGCHead_PREV() and
* _PyGCHead_SET_PREV() for it to avoid unnecessary bitwise operations.
*
Expand All @@ -34,15 +34,15 @@ static inline void _PyObject_GC_TRACK_impl(const char *filename, int lineno,

PyGC_Head *gc = _Py_AS_GC(op);
_PyObject_ASSERT_FROM(op,
(gc->_gc_prev & _PyGC_PREV_MASK_COLLECTING) == 0,
(gc->_gc.prev & _PyGC_PREV_MASK_COLLECTING) == 0,
"object is in generation which is garbage collected",
filename, lineno, "_PyObject_GC_TRACK");

PyGC_Head *last = (PyGC_Head*)(_PyRuntime.gc.generation0->_gc_prev);
PyGC_Head *last = (PyGC_Head*)(_PyRuntime.gc.generation0->_gc.prev);
_PyGCHead_SET_NEXT(last, gc);
_PyGCHead_SET_PREV(gc, last);
_PyGCHead_SET_NEXT(gc, _PyRuntime.gc.generation0);
_PyRuntime.gc.generation0->_gc_prev = (uintptr_t)gc;
_PyRuntime.gc.generation0->_gc.prev = (uintptr_t)gc;
}

#define _PyObject_GC_TRACK(op) \
Expand All @@ -69,8 +69,8 @@ static inline void _PyObject_GC_UNTRACK_impl(const char *filename, int lineno,
PyGC_Head *next = _PyGCHead_NEXT(gc);
_PyGCHead_SET_NEXT(prev, next);
_PyGCHead_SET_PREV(next, prev);
gc->_gc_next = 0;
gc->_gc_prev &= _PyGC_PREV_MASK_FINALIZED;
gc->_gc.next = 0;
gc->_gc.prev &= _PyGC_PREV_MASK_FINALIZED;
}

#define _PyObject_GC_UNTRACK(op) \
Expand Down
Loading