[3.7] bpo-27987: align PyGC_Head to alignof(long double) by methane · Pull Request #13335 · python/cpython

methane · 2019-05-15T09:30:54Z

https://bugs.python.org/issue27987

vstinner · 2019-05-15T14:11:21Z

Include/objimpl.h

        Py_ssize_t gc_refs;
    } gc;
-    double dummy;  /* force worst-case alignment */
+    long double dummy;  /* force worst-case alignment (16 byte for amd64) */


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?

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

I'm not sure this is really x86-64 ABI requirement...
C99 requires malloc returns memory aligned to be used for any built-in types.

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

I don't think so. long double may be aligned to 8 byte boundary.
But I am not sure. I don't have 32bit development environment for now.

vstinner · 2019-05-16T12:33:37Z

C code:

union {
    struct {
        void *next;
        void *prev;
    } _gc;
    long double alignment;
} PyGC_Head;

int main()
{
    return sizeof(PyGC_Head);
}

With long double:

vstinner@apu$ gcc x.c -o x && ./x; echo $? # 64-bit void*
16
vstinner@apu$ gcc -m32 x.c -o x && ./x; echo $? # 32-bit void*
12

Without long double:

vstinner@apu$ gcc x.c -o x && ./x; echo $? # 64-bit void*
16
vstinner@apu$ gcc -m32 x.c -o x && ./x; echo $? # 32-bit void*
8

I'm not sure that 12 bytes is great for alignment. 32-bit CPU may be more efficient on SIMD instructions with 8 bytes alignment, no?

vstinner · 2019-05-16T12:34:32Z

@methane also proposed PR #13336 for master, but he rejected it.

methane · 2019-05-16T12:39:02Z

PyGC_Head in master branch is two uintptr_t. It is 8 bytes (not 12 bytes) on x86.

methane · 2019-05-21T05:38:16Z

I'm not sure that 12 bytes is great for alignment. 32-bit CPU may be more efficient on SIMD instructions with 8 bytes alignment, no?

This pull request changes from 12 bytes to 12 or 16 bytes, no?

vstinner · 2019-05-21T09:41:23Z

Sorry @methane, I don't understand this change :-( What is the effect of PyListObject for example? Is the address of ob_item aligned to 16 bytes on x86 (32-bit)? Is it on x86-64 (64 bit)?

methane · 2019-05-23T06:35:10Z

@vstinner You wrote:

    struct {
        void *next;
        void *prev;
    } _gc;

But it is actually:

    struct {
        void *next;
        void *prev;
        ssize_t gc_refs; /* only in Python < 3.8 */
    } _gc;

This is very big difference between Python 3.7 and 3.8. That's why I closed PR for 3.8 but don't close this.

sizeof(PyGC_Head) is 12 byte on x86, regardless alignment is double or long double. (I can now "gcc -m32" on Ubuntu, thank you)
On amd64, sizeof(PyGC_Head) is 24 byte with double alignment, and 32byte with long double alignment.

What is the effect of PyListObject for example? Is the address of ob_item aligned to 16 bytes on x86 (32-bit)? Is it on x86-64 (64 bit)?

This pull request affects only where PyObject* ((PyObject*)(gc + 1)) is aligned.
This pull request doesn't affect alignment in PyListObject. offsetof(PyListObject, ob_item) is 24.

vstinner

LGTM. I think that I understood the issue and that the fix is correct :-)

bedevere-bot · 2019-05-25T12:17:28Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian PGO 3.7 has failed when building commit ea2b76b.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/128/builds/1189) and take a look at the build logs.
Check if the failure is related to this commit (ea2b76b) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/128/builds/1189

Click to see traceback logs

From https://github.com/python/cpython
 * branch                  3.7        -> FETCH_HEAD
Reset branch '3.7'

find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
make[2]: [clean] Error 1 (ignored)
Segmentation fault
make[3]: *** [pybuilddir.txt] Error 1
make[2]: *** [build_all_generate_profile] Error 2
make[1]: *** [profile-gen-stamp] Error 2
make: *** [profile-run-stamp] Error 2

find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
make: [clean] Error 1 (ignored)

bedevere-bot · 2019-05-25T15:56:55Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian PGO 3.7 has failed when building commit ea2b76b.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/128/builds/1190) and take a look at the build logs.
Check if the failure is related to this commit (ea2b76b) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/128/builds/1190

Click to see traceback logs

Reset branch '3.7'

find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
make[2]: [clean] Error 1 (ignored)
Segmentation fault
make[3]: *** [pybuilddir.txt] Error 1
make[2]: *** [build_all_generate_profile] Error 2
make[1]: *** [profile-gen-stamp] Error 2
make: *** [profile-run-stamp] Error 2

find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
make: [clean] Error 1 (ignored)

gpshead · 2019-05-25T16:00:59Z

this change needs to be undone. followup on the bug.

…13335)" This reverts commit ea2b76b.

…-13569) This reverts commit ea2b76b. See the bug for discussion. https://bugs.python.org/issue27987

methane · 2019-05-26T02:35:32Z

Do you think missing "build" directory is happened by this change?

I thought it just a trouble on the build bot environment.

gpshead · 2019-05-26T06:05:22Z

The resulting python interpreter was crashing on the the PGO bot and a pile of new undefined behavior was happening on the undefined behavior sanitizer (ubsan) bot.

PGO bot:

./python -E -S -m sysconfig --generate-posix-vars ;\
if test $? -ne 0 ; then \
	echo "generate-posix-vars failed" ; \
	rm -f ./pybuilddir.txt ; \
	exit 1 ; \
fi
Segmentation fault
generate-posix-vars failed
Makefile:605: recipe for target 'pybuilddir.txt' failed

undefined behavior sanitizer millions of times:

Objects/funcobject.c:663:5: runtime error: member access within misaligned address 0x7fb4b5a03a08 for type 'union _gc_head', which requires 16 byte alignment
0x7fb4b5a03a08: note: pointer points here
 b4 7f 00 00  70 8d c6 b5 b4 7f 00 00  a0 13 a0 b5 b4 7f 00 00  00 00 00 00 00 00 00 00  00 00 00 00
              ^ 
Objects/funcobject.c:663:5: runtime error: member access within misaligned address 0x7fb4b5a03a08 for type 'struct (anonymous struct at ./Include/objimpl.h:253:5)', which requires 16 byte alignment
0x7fb4b5a03a08: note: pointer points here
 b4 7f 00 00  70 8d c6 b5 b4 7f 00 00  a0 13 a0 b5 b4 7f 00 00  00 00 00 00 00 00 00 00  00 00 00 00
              ^ 
Objects/funcobject.c:663:5: runtime error: store to misaligned address 0x7fb4b5a03a08 for type 'union _gc_head *', which requires 16 byte alignment
0x7fb4b5a03a08: note: pointer points here
 b4 7f 00 00  70 8d c6 b5 b4 7f 00 00  a0 13 a0 b5 b4 7f 00 00  00 00 00 00 00 00 00 00  00 00 00 00
              ^

…)" (GH-13569)" This reverts commit 2156fec.

methane · 2019-05-26T06:20:46Z

#13318 fixed it already.

…H-13581) This reverts commit 2156fec. Now that 1b85f4e is in, this change makes sense.

methane added 2 commits May 15, 2019 18:25

bpo-27987: align sizeof(PyGC_Head) to alignof(long double)

e3eea5f

add news entry

8c88866

methane added the type-bug An unexpected behavior, bug, or error label May 15, 2019

methane requested a review from vstinner May 15, 2019 09:30

the-knights-who-say-ni added the CLA signed label May 15, 2019

bedevere-bot added the awaiting core review label May 15, 2019

vstinner reviewed May 15, 2019

View reviewed changes

more comment

3c6affc

vstinner approved these changes May 24, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels May 24, 2019

methane merged commit ea2b76b into python:3.7 May 25, 2019

bedevere-bot removed the awaiting merge label May 25, 2019

methane deleted the gc-align branch May 25, 2019 12:13

gpshead added a commit to gpshead/cpython that referenced this pull request May 25, 2019

Revert "bpo-27987: align PyGC_Head to alignof(long double) (pythonGH-…

8f14a55

…13335)" This reverts commit ea2b76b.

gpshead added a commit that referenced this pull request May 25, 2019

[3.7] Revert "align PyGC_Head to alignof(long double) (GH-13335)" (GH…

2156fec

…-13569) This reverts commit ea2b76b. See the bug for discussion. https://bugs.python.org/issue27987

methane added a commit that referenced this pull request May 26, 2019

Revert "[3.7] Revert "align PyGC_Head to alignof(long double) (GH-13335…

f099710

…)" (GH-13569)" This reverts commit 2156fec.

methane mentioned this pull request May 26, 2019

[3.7] bpo-27987: align PyGC_Head to alignof(long double) (GH-13335) #13581

Merged

gpshead pushed a commit that referenced this pull request Jun 3, 2019

[3.7] bpo-27987: align PyGC_Head to alignof(long double) (GH-13335) (G…

8766cb7

…H-13581) This reverts commit 2156fec. Now that 1b85f4e is in, this change makes sense.

Uh oh!

Conversation

methane commented May 15, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner May 15, 2019

Choose a reason for hiding this comment

Uh oh!

methane May 16, 2019

Choose a reason for hiding this comment

Uh oh!

vstinner commented May 16, 2019

Uh oh!

vstinner commented May 16, 2019

Uh oh!

methane commented May 16, 2019

Uh oh!

methane commented May 21, 2019

Uh oh!

vstinner commented May 21, 2019

Uh oh!

methane commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 25, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

bedevere-bot commented May 25, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

gpshead commented May 25, 2019

Uh oh!

methane commented May 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gpshead commented May 26, 2019

Uh oh!

methane commented May 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

methane commented May 15, 2019 •

edited by bedevere-bot

Loading

methane commented May 23, 2019 •

edited

Loading

methane commented May 26, 2019 •

edited

Loading