[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

STINNER Victor added the comment: Objects/memoryview.c uses memcpy() on _Bool which leads to undefined behavior with GCC 11: see bpo-42587. -- ___ Python tracker ___

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Change by Petr Viktorin : -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker ___

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

miss-islington added the comment: New changeset 572ef747692055a270a9fbf8eeaf5c4a15c8e332 by Miss Islington (bot) in branch '3.8': bpo-39689: Do not use native packing for format "?" with standard size (GH-18969)

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

miss-islington added the comment: New changeset 0f9e889cd94c1e86f8ff28733b207c99803ca8a4 by Miss Islington (bot) in branch '3.7': bpo-39689: Do not use native packing for format "?" with standard size (GH-18969)

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Change by miss-islington : -- pull_requests: +18515 pull_request: https://github.com/python/cpython/pull/19154 ___ Python tracker ___

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Change by miss-islington : -- pull_requests: +18516 pull_request: https://github.com/python/cpython/pull/19155 ___ Python tracker ___

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Petr Viktorin added the comment: Moved to Discourse, IMO that's a better place for maintainers of other PEP-3118-compatible libraries to chime in: https://discuss.python.org/t/behavior-of-struct-format-native-bool/3774 -- ___ Python tracker

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Petr Viktorin added the comment: I see. Thanks for your patience explaining this to me! I will merge and continue in a different issue. -- ___ Python tracker ___

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

miss-islington added the comment: New changeset 472fc843ca816d65c12f9508ac762ca492165c45 by Stefan Krah in branch 'master': bpo-39689: Do not use native packing for format "?" with standard size (GH-18969) https://github.com/python/cpython/commit/472fc843ca816d65c12f9508ac762ca492165c45

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Stefan Krah added the comment: I think this issue should be about fixing the tests so that people looking at the sanitizer buildbots can move on. GH-18969 fixes "?" and "!?", which clearly used wrong semantics with the new compiler behavior. This should be an uncontroversial fix that also

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Petr Viktorin added the comment: I think we are speaking past each other. In my (current) view, the semantics are spelled out in the documentation: "any non-zero value will be True when unpacking". There's also a mention that this corresponds to the _Bool type in C. While this was the case

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Ronald Oussoren added the comment: Sigh... never mind, I misread the code. Please ignore msg364472 -- ___ Python tracker ___ ___

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Ronald Oussoren added the comment: Note that the implementation of np_bool in _struct.c [1] is incorrect because this is supposed to access a boolean of a standard size, but uses _Bool. The size of _Bool is not prescribed, and IIRC sizeof(_Bool) was 4 with the compilers used for macOS/PPC.

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Stefan Krah added the comment: > So it's clear that something has to change. IMO, preserving (2) and relaxing > (1) is the more useful choice. But not in this issue I think. GH-18969 is a minimal change that *removes* UB for the standard sizes. UB for the native type is a direct

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Petr Viktorin added the comment: > You are the one who wanted to *introduce* a hack by dereferencing as char and then cast to _Bool. :-) Yes, I did change my mind after reading the documentation. The docs say two contradicting things: 1. The '?' conversion code corresponds to the _Bool type

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Change by Stefan Krah : -- pull_requests: +18317 pull_request: https://github.com/python/cpython/pull/18969 ___ Python tracker ___

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Stefan Krah added the comment: memoryview only supports the native format, so I've disabled the (wrong) test that casts arrays with arbitrary values to _Bool. So memoryview is done. IMO the problem in _struct is that it swaps the x->unpack function for the native one, which does not seem

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Stefan Krah added the comment: New changeset f8ce3e2dae277baa2ef92e8a3e935953dc6c3f39 by Miss Islington (bot) in branch '3.8': bpo-39689: Do not test undefined casts to _Bool (GH-18964) (#18966) https://github.com/python/cpython/commit/f8ce3e2dae277baa2ef92e8a3e935953dc6c3f39 --

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Stefan Krah added the comment: New changeset 636eecc16229432ec8e26e6da287c52f3ca3 by Miss Islington (bot) in branch '3.7': bpo-39689: Do not test undefined casts to _Bool (GH-18964) (#18965) https://github.com/python/cpython/commit/636eecc16229432ec8e26e6da287c52f3ca3 --

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Change by miss-islington : -- nosy: +miss-islington nosy_count: 9.0 -> 10.0 pull_requests: +18313 pull_request: https://github.com/python/cpython/pull/18965 ___ Python tracker

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Stefan Krah added the comment: New changeset 1ae9cde4b2323235b5f9ff4bc76e4175a2257172 by Stefan Krah in branch 'master': bpo-39689: Do not test undefined casts to _Bool (GH-18964) https://github.com/python/cpython/commit/1ae9cde4b2323235b5f9ff4bc76e4175a2257172 --

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Change by miss-islington : -- pull_requests: +18314 pull_request: https://github.com/python/cpython/pull/18966 ___ Python tracker ___

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Change by Stefan Krah : -- pull_requests: +18312 pull_request: https://github.com/python/cpython/pull/18964 ___ Python tracker ___

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Stefan Krah added the comment: The memcpy() is NOT a hack and performs exactly the same operation as casting the pointer and then dereferencing, only in a manner that avoids unaligned accesses. On platforms like x86 the memcpy() is optimized to a simple assignment. Casting the pointer and

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Petr Viktorin added the comment: > The docs for native mode (we now always assume C99): > > "The '?' conversion code corresponds to the _Bool type defined by C99." But, nowhere is it suggested that conversion is "do memcpy and then interpret the bit pattern". That is pretty weird way to

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Stefan Krah added the comment: The docs for native mode (we now always assume C99): "The '?' conversion code corresponds to the _Bool type defined by C99." The memoryview tests that fail are essentially auto-generated and not prescriptive. They just happened to work without UBSan: >>> x =

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Petr Viktorin added the comment: > So I vote for not handling incorrectly packed values and removing "and any non-zero value will be True when unpacking" from the docs, which does not seem to make any sense for _Bool. I disagree. I don't think struct module's job is to be faithful to _Bool

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Stefan Krah added the comment: I checked that NumPy also packs correctly: >>> import numpy as np >>> x = np.array([0,1,2,3], dtype=np.bool) >>> x.tobytes() b'\x00\x01\x01\x01' So I vote for not handling incorrectly packed values and removing "and any non-zero value will be True when

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Stefan Krah added the comment: Okay, in memoryview the cast tests can trigger UB checks. memoryview assumes that bool is packed correctly, so just casting does not work. Casting anything to bool is of course a bit silly anyway. diff --git a/Lib/test/test_buffer.py b/Lib/test/test_buffer.py

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Stefan Krah added the comment: So which memoryview test (unless it is using the struct module) relies on UB? -- ___ Python tracker ___

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Stefan Krah added the comment: Concerning memoryview, I've looked at this very briefly: memoryview does not pack values directly, it casts first, which is legal: #define PACK_SINGLE(ptr, src, type) \ do { \ type x;

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Petr Viktorin added the comment: Oh! I just reead the docs more carefully; they say: For the '?' format character, the return value is either True or False. When packing, the truth value of the argument object is used. Either 0 or 1 in the native or standard bool representation will be

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Petr Viktorin added the comment: > > > maybe we should be raising an error if the bytes are not a valid platform > > > _Bool pattern? > > > > That's quite hard to test for. > > How so? We just make the same assumption you're making that true = b'\x01' > and false = NUL. Right, with that

[issue39689] struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

Petr Viktorin added the comment: The memoryview module does the same thing as struct, and its tests expect the same behavior as with struct. So, whatever we do in struct should be also done in memoryview. -- title: test_struct failure on s390x Fedora Clang buildbot -> struct and