classification
Title: Fix compiler warnings in pythoncore for Win64
Type: compile error Stage: resolved
Components: Build, Windows Versions: Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, jeremy.kloth, jkloth, loewis, steve.dower, tim.golden, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2013-07-08 19:09 by jkloth, last changed 2018-11-22 13:51 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
issue18407.diff jkloth, 2013-07-08 19:09 review
issue18407-2.diff jkloth, 2013-07-10 13:33 review
Pull Requests
URL Status Linked Edit
PR 10655 merged vstinner, 2018-11-22 12:52
PR 10656 merged vstinner, 2018-11-22 12:54
Messages (13)
msg192686 - (view) Author: Jeremy Kloth (jkloth) * Date: 2013-07-08 19:09
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.
msg192687 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-07-08 19:20
-      <AdditionalOptions>/USECL:MS_OPTERON /GS- %(AdditionalOptions)</AdditionalOptions>
+      <BufferSecurityCheck>false</BufferSecurityCheck>

Please don't change too much things in the same patch. The issue #15792 is a better place for such change.
msg192688 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-07-08 19:23
This issue duplicates the isuse #9566, but your patch is interesting. I created other more specific issues like #18295 and #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.
msg192695 - (view) Author: Jeremy Kloth (jeremy.kloth) Date: 2013-07-08 20:54
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 #15792 is a better place for such change.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue18407>
> _______________________________________
msg192696 - (view) Author: Jeremy Kloth (jeremy.kloth) Date: 2013-07-08 20:57
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 #9566, but your patch is interesting. I created other more specific issues like #18295 and #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>
> _______________________________________
msg192697 - (view) Author: Jeremy Kloth (jkloth) * Date: 2013-07-08 20:59
Ugh, sorry for the bad quoting (silly GMail).
msg192806 - (view) Author: Jeremy Kloth (jkloth) * Date: 2013-07-10 13:33
Added new patch that removes the duplicate changes from issue #15792 and comments the lone explicit cast.

These changes fix 116 of the 216 warnings (54%!) for Win64.  Together with issue #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 #18295, I do not think that it is advisable to separate out changes for that issue.
msg224356 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-07-30 22:24
Can we have the stage set to patch review please.
msg238900 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2015-03-22 14:21
Would someone like to review the patch please, being aware that there are other open issues relating to compiler warnings on Windows.
msg238904 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2015-03-22 14:34
Does the patch still apply cleanly?
msg330255 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-22 13:43
New changeset c48ff73dd60bec5dcbe64bedeff91e6db26d98bc by Victor Stinner in branch 'master':
bpo-18407: win32_urandom() uses PY_DWORD_MAX (GH-10656)
https://github.com/python/cpython/commit/c48ff73dd60bec5dcbe64bedeff91e6db26d98bc
msg330256 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-22 13:45
New changeset 4d73ae776140a583fdfe8f016d88cc767791e481 by Victor Stinner in branch 'master':
bpo-18407: ast.c uses Py_ssize_t for asdl_seq_LEN() iterator (GH-10655)
https://github.com/python/cpython/commit/4d73ae776140a583fdfe8f016d88cc767791e481
msg330257 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-11-22 13:51
Almost all compiler warnings on 64-bit Windows have been fixed: bpo-9566 tracked most of them.

I extract the CryptGenRandom() fix from issue18407-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 issue18407-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.
History
Date User Action Args
2018-11-22 13:51:56vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg330257

stage: patch review -> resolved
2018-11-22 13:45:19vstinnersetmessages: + msg330256
2018-11-22 13:43:13vstinnersetmessages: + msg330255
2018-11-22 12:54:44vstinnersetpull_requests: + pull_request9909
2018-11-22 12:52:52BreamoreBoysetnosy: - BreamoreBoy
2018-11-22 12:52:10vstinnersetstage: patch review
pull_requests: + pull_request9908
2015-03-22 14:34:56steve.dowersetmessages: + msg238904
2015-03-22 14:21:53BreamoreBoysetnosy: + zach.ware, steve.dower
messages: + msg238900
2014-07-30 22:24:20BreamoreBoysetnosy: + BreamoreBoy, - brian.curtin
messages: + msg224356
versions: + Python 3.5
2013-07-10 13:33:30jklothsetfiles: + issue18407-2.diff

messages: + msg192806
2013-07-08 20:59:22jklothsetmessages: + msg192697
2013-07-08 20:57:26jeremy.klothsetmessages: + msg192696
2013-07-08 20:54:03jeremy.klothsetmessages: + msg192695
2013-07-08 19:23:30vstinnersetmessages: + msg192688
2013-07-08 19:20:41vstinnersetmessages: + msg192687
2013-07-08 19:10:24jklothsettype: compile error
components: + Build, Windows
versions: + Python 3.4
2013-07-08 19:09:35jklothcreate