__builtin_bswap16 is used without checking it is supported

[jmroot]

BPO	41617
Nosy	@vstinner, @jmroot
PRs	bpo-41617: check for bswap builtins before using #21942 [3.9] bpo-41617: check for bswap builtins before using (GH-21942) #21943 bpo-41617: Fix pycore_bitutils.h to support clang 3.0 #21949 bpo-41617: Fix pycore_bitutils.h to support clang 3.0 #22042 [3.9] bpo-41617: Fix pycore_byteswap.h to support clang 3.0 (GH-22042) #22044 bpo-41617: Add _Py__has_builtin() macro #23260 [3.9] bpo-41617: Add _Py__has_builtin() macro (GH-23260) #23262

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-11-13.15:38:39.158>
created_at = <Date 2020-08-23.01:02:46.962>
labels = ['build', '3.9', '3.10']
title = '__builtin_bswap16 is used without checking it is supported'
updated_at = <Date 2020-12-08.13:31:40.621>
user = 'https://github.com/jmroot'

bugs.python.org fields:

activity = <Date 2020-12-08.13:31:40.621>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-11-13.15:38:39.158>
closer = 'vstinner'
components = ['Build']
creation = <Date 2020-08-23.01:02:46.962>
creator = 'jmr'
dependencies = []
files = []
hgrepos = []
issue_num = 41617
keywords = ['patch']
message_count = 13.0
messages = ['375806', '375856', '376193', '376199', '376204', '376205', '377149', '380889', '380894', '380900', '380902', '382731', '382733']
nosy_count = 2.0
nosy_names = ['vstinner', 'jmr']
pr_nums = ['21942', '21943', '21949', '22042', '22044', '23260', '23262']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue41617'
versions = ['Python 3.9', 'Python 3.10']

[jmroot]

Older clang versions don't have __builtin_bswap16, but it's always used when compiling with clang, which means the build fails with those older versions. The code should use __has_builtin to check.

[vstinner]

Older clang versions don't have __builtin_bswap16,

According to nodejs/node#7644 it's available in clang 3.2 but not in clang 3.0.

I wrote PR 21949 to fix the issue: it only uses __has_builtin() if __clang__ is defined, it's different than PR 21942.

[vstinner]

Joshua Root: I'm curious. Can I ask you on which platform do you use clang older than 3.2? It has been released 8 years ago, December 2012. The LLVM project is known to evolve very quickly!

[vstinner]

New changeset e6905e4 by Victor Stinner in branch 'master':
bpo-41617: Fix pycore_bitutils.h to support clang 3.0 (GH-22042)
e6905e4

[vstinner]

New changeset 4217b3c by Victor Stinner in branch '3.9':
bpo-41617: Fix pycore_byteswap.h to support clang 3.0 (GH-22042) (GH-22044)
4217b3c

[vstinner]

Thank you Joshua Root for your bug report! Your PR 21942 was good, but I chose to avoid __has_builtin() since it is not supported by all compilers and only available since GCC 10 for GCC.

[jmroot]

I'm curious. Can I ask you on which platform do you use clang older than 3.2?

Mac OS X 10.7 / Xcode 4.6.3. I'm not using it personally, but we have automated builds on that platform.

Unfortunately the patch ultimately committed did not fix the build there. Clang reports its version as "Apple LLVM version 4.2 (clang-425.0.28) (based on LLVM 3.2svn)". __clang_major__ is 4 and __clang_minor__ is 2.

Apple's versioning scheme is different to that of LLVM upstream, which is one reason why I preferred detecting features directly rather than inserting externally-derived knowledge about which versions provide which features.

Apologies for not getting back to you about this sooner; the notifications appear to have gotten lost.

[vstinner]

Unfortunately the patch ultimately committed did not fix the build there. Clang reports its version as "Apple LLVM version 4.2 (clang-425.0.28) (based on LLVM 3.2svn)". __clang_major__ is 4 and __clang_minor__ is 2.

Oh... I wrote PR 23260 which is based on your original PR 21942. I added a new _Py__has_builtin() macro rather than defining a __has_builtin() which always returns 0.

[vstinner]

New changeset b3b9808 by Victor Stinner in branch 'master':
bpo-41617: Add _Py__has_builtin() macro (GH-23260)
b3b9808

[vstinner]

New changeset ec306a2 by Victor Stinner in branch '3.9':
bpo-41617: Add _Py__has_builtin() macro (GH-23260) (GH-23262)
ec306a2

[vstinner]

Ok, I hope that this issue it's really fixed ;-) Thanks again Joshua Root.

[jmroot]

Confirmed fixed in 3.9.1 and 3.10.0a3. Thanks Victor!

[vstinner]

Confirmed fixed in 3.9.1 and 3.10.0a3. Thanks Victor!

Woooot!