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.

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

Jump to bottom

Fix compiler warnings in pythoncore for Win64 #62607

Closed
jkloth opened this issue Jul 8, 2013 · 13 comments
Closed

Fix compiler warnings in pythoncore for Win64 #62607

jkloth opened this issue Jul 8, 2013 · 13 comments
Labels
build The build process and cross-build OS-windows

Comments

@jkloth
Copy link
Contributor

jkloth commented Jul 8, 2013

BPO 18407
Nosy @loewis, @vstinner, @tiran, @tjguk, @jkloth, @zware, @zooba
PRs
  • bpo-18407: ast.c now uses Py_ssize_t for asdl_seq_LEN() #10655
  • bpo-18407: win32_urandom() uses PY_DWORD_MAX #10656
    		•
    Files
  • issue18407.diff
  • issue18407-2.diff
    		•

    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 2018-11-22.13:51:56.630>
created_at = <Date 2013-07-08.19:09:35.189>
labels = ['build', 'OS-windows']
title = 'Fix compiler warnings in pythoncore for Win64'
updated_at = <Date 2018-11-22.13:51:56.629>
user = 'https://github.com/jkloth'

    bugs.python.org fields:

    activity = <Date 2018-11-22.13:51:56.629>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-11-22.13:51:56.630>
closer = 'vstinner'
components = ['Build', 'Windows']
creation = <Date 2013-07-08.19:09:35.189>
creator = 'jkloth'
dependencies = []
files = ['30864', '30883']
hgrepos = []
issue_num = 18407
keywords = ['patch']
message_count = 13.0
messages = ['192686', '192687', '192688', '192695', '192696', '192697', '192806', '224356', '238900', '238904', '330255', '330256', '330257']
nosy_count = 8.0
nosy_names = ['loewis', 'vstinner', 'christian.heimes', 'tim.golden', 'jkloth', 'jeremy.kloth', 'zach.ware', 'steve.dower']
pr_nums = ['10655', '10656']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue18407'
versions = ['Python 3.4', 'Python 3.5']
    @jkloth
    Copy link
    Contributor Author

    jkloth commented Jul 8, 2013

    The attached patch fixes compiler warnings for the pythoncore project when building on 64-bit Windows.

    Fixes for built-in modules are not included, however.

    @jkloth jkloth added build The build process and cross-build OS-windows labels Jul 8, 2013
    @vstinner
    Copy link
    Member

    vstinner commented Jul 8, 2013 

    •  \<AdditionalOptions\>/USECL:MS_OPTERON /GS- %(AdditionalOptions)\</AdditionalOptions\>

    + <BufferSecurityCheck>false</BufferSecurityCheck>

    Please don't change too much things in the same patch. The issue bpo-15792 is a better place for such change.

    @vstinner
    Copy link
    Member

    vstinner commented Jul 8, 2013

    This issue duplicates the isuse bpo-9566, but your patch is interesting. I created other more specific issues like bpo-18295 and bpo-18294.

    • return lb - ll->ll_label;
      + return Py_SAFE_DOWNCAST(lb - ll->ll_label, Py_intptr_t, int);

    I don't think that such change is correct, IMO the right fix is to change the result type to Py_intptr_t.

    @jeremykloth
    Copy link
    Mannequin

    jeremykloth mannequin commented Jul 8, 2013

    Yeah, sorry. This made it in by mistake. It was in the tree just to
    eliminate warning noise.

    On Mon, Jul 8, 2013 at 1:20 PM, STINNER Victor <report@bugs.python.org> wrote:

    STINNER Victor added the comment:

    •  \<AdditionalOptions\>/USECL:MS_OPTERON /GS- %(AdditionalOptions)\</AdditionalOptions\>

    •  \<BufferSecurityCheck\>false\</BufferSecurityCheck\>

    Please don't change too much things in the same patch. The issue bpo-15792 is a better place for such change.

    ----------

    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue18407\>

    @jeremykloth
    Copy link
    Mannequin

    jeremykloth mannequin commented Jul 8, 2013

    The change in grammar.c:addlabel() is correct. The return value is an
    index into the ll->ll_label array, thus an int. The code could be
    rewritten to avoid the pointer addition by saving the value of
    ll->ll_nlabels before it is incremented and return that instead,

    On Mon, Jul 8, 2013 at 1:23 PM, STINNER Victor <report@bugs.python.org> wrote:

    STINNER Victor added the comment:

    This issue duplicates the isuse bpo-9566, but your patch is interesting. I created other more specific issues like bpo-18295 and bpo-18294.

    • return lb - ll->ll_label;
    • return Py_SAFE_DOWNCAST(lb - ll->ll_label, Py_intptr_t, int);

    I don't think that such change is correct, IMO the right fix is to change the result type to Py_intptr_t.

    ----------

    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue18407\>

    @jkloth
    Copy link
    Contributor Author

    jkloth commented Jul 8, 2013

    Ugh, sorry for the bad quoting (silly GMail).

    @jkloth
    Copy link
    Contributor Author

    jkloth commented Jul 10, 2013

    Added new patch that removes the duplicate changes from issue bpo-15792 and comments the lone explicit cast.

    These changes fix 116 of the 216 warnings (54%!) for Win64. Together with issue bpo-15792 brings the remaining count to 72.

    I am unsure if splitting the changes further makes much sense as guidance I have received on IRC and as others have noted in various bug reports about "spamming" the tracker with issues per file.

    With that and MvL's comment in issue bpo-18295, I do not think that it is advisable to separate out changes for that issue.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jul 30, 2014

    Can we have the stage set to patch review please.

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Mar 22, 2015

    Would someone like to review the patch please, being aware that there are other open issues relating to compiler warnings on Windows.

    @zooba
    Copy link
    Member

    zooba commented Mar 22, 2015

    Does the patch still apply cleanly?

    @vstinner
    Copy link
    Member

    New changeset c48ff73 by Victor Stinner in branch 'master':
    bpo-18407: win32_urandom() uses PY_DWORD_MAX (GH-10656)
    c48ff73

    @vstinner
    Copy link
    Member

    New changeset 4d73ae7 by Victor Stinner in branch 'master':
    bpo-18407: ast.c uses Py_ssize_t for asdl_seq_LEN() iterator (GH-10655)
    4d73ae7

    @vstinner
    Copy link
    Member

    Almost all compiler warnings on 64-bit Windows have been fixed: bpo-9566 tracked most of them.

    I extract the CryptGenRandom() fix from bpo-18407-2.diff. I rewrote the ast.c fix differently (use larger type, don't downcast).

    The patch may still contains relevant changes on Python/dtoa.c and Python/getargs.c, but again, the most critical bugs and warnings have already been fixed. So I suggest to abandon bpo-18407-2.diff which has been written 5 years ago.

    Thanks Jeremy Kloth for the report and for the patch! I think that it's now time to close the issue. See bpo-9566 for the few remaining issues.

    @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
    build The build process and cross-build OS-windows
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @vstinner @zooba @jkloth