Skip to content
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.

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

Jump to bottom

__builtin_bswap16 is used without checking it is supported #85783

Closed
jmroot mannequin opened this issue Aug 23, 2020 · 13 comments
Closed

__builtin_bswap16 is used without checking it is supported #85783

jmroot mannequin opened this issue Aug 23, 2020 · 13 comments
Labels
3.9 only security fixes 3.10 only security fixes build The build process and cross-build

Comments

@jmroot
Copy link
Mannequin

jmroot mannequin commented Aug 23, 2020

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
    Copy link
    Mannequin Author

    jmroot mannequin commented Aug 23, 2020

    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.

    @jmroot jmroot mannequin added 3.9 only security fixes build The build process and cross-build 3.10 only security fixes labels Aug 23, 2020
    @vstinner
    Copy link
    Member

    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
    Copy link
    Member

    vstinner commented Sep 1, 2020

    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
    Copy link
    Member

    vstinner commented Sep 1, 2020

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

    @vstinner
    Copy link
    Member

    vstinner commented Sep 1, 2020

    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
    Copy link
    Member

    vstinner commented Sep 1, 2020

    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.

    @vstinner vstinner closed this as completed Sep 1, 2020
    @vstinner vstinner closed this as completed Sep 1, 2020
    @jmroot
    Copy link
    Mannequin Author

    jmroot mannequin commented Sep 19, 2020

    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.

    @jmroot jmroot mannequin reopened this Sep 19, 2020
    @jmroot jmroot mannequin reopened this Sep 19, 2020
    @vstinner
    Copy link
    Member

    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
    Copy link
    Member

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

    @vstinner
    Copy link
    Member

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

    @vstinner
    Copy link
    Member

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

    @jmroot
    Copy link
    Mannequin Author

    jmroot mannequin commented Dec 8, 2020

    Confirmed fixed in 3.9.1 and 3.10.0a3. Thanks Victor!

    @vstinner
    Copy link
    Member

    vstinner commented Dec 8, 2020

    Confirmed fixed in 3.9.1 and 3.10.0a3. Thanks Victor!

    Woooot!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.9 only security fixes 3.10 only security fixes build The build process and cross-build
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @vstinner