classification
Title: Fixed timemodule compile warnings (gmtoff).
Type: Stage: resolved
Components: Build, Interpreter Core, Windows Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Decorater, belopolsky, paul.moore, steve.dower, tim.golden, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2016-12-24 10:41 by Decorater, last changed 2017-05-17 21:48 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
timemodule.patch-1.patch Decorater, 2016-12-24 10:41 patches possible loss of data warnings. review
gmtoff_time_t.patch vstinner, 2017-01-03 12:39 review
Pull Requests
URL Status Linked Edit
PR 1276 vstinner, 2017-05-17 18:55
PR 1635 merged vstinner, 2017-05-17 19:14
Messages (8)
msg283937 - (view) Author: Decorater (Decorater) * Date: 2016-12-24 10:41
I fixed some Possible Loss of Data warnings. but only the ones for timemodule. There are still some others when building the 64 bit version however.

I added some comments that can be removed in another patch if desired otherwise it should look good for now for this module.
msg284460 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-01-02 04:44
Any reason we can't make gmtoff a time_t instead of an int?

If we're going to truncate values to get rid of the warnings, I'd like to also see either proof that it will never exceed the size of an int (which may be a simple comment, but it's not obvious that this is the case from what appears in the patch), or an assertion when it does overflow.

But since we're presumably passing the value back into Python as an int, expanding the destination variable to fit all possible values is best.
msg284553 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-03 12:39
Downcasting to int doesn't seem good to me.

gmtoff_time_t.patch uses time_t instead of an int to store gmtoff. It raises an OverflowError if the value doesn't fit into a C long (at least max is at least 2^31).

I guess that the issue only impacts Windows, I expect that most platforms have a tm_zone field in the "tm" structure?

/* Define to 1 if `tm_zone' is a member of `struct tm'. */
#define HAVE_STRUCT_TM_TM_ZONE 1
msg284554 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-03 12:41
The warning was introduced by a recent change:

changeset:   103680:a96101dd105c
user:        Alexander Belopolsky <alexander.belopolsky@gmail.com>
date:        Sun Sep 11 22:55:16 2016 -0400
files:       Doc/library/time.rst Misc/NEWS Modules/timemodule.c
description:
Closes #25283: Make tm_gmtoff and tm_zone available on all platforms.
msg284555 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-03 12:47
Not checking for integer overflow is more likely to hide bugs. Handling time is an hard problem, see a recent funny (or not) story from Cloudfare with the latest leap second added at midnight the 2016-12-31:
https://blog.cloudflare.com/how-and-why-the-leap-second-affected-cloudflare-dns/

Extract: "RRDNS is written in Go and uses Go’s time.Now() function to get the time. Unfortunately, this function does not guarantee monotonicity. Go currently doesn’t offer a monotonic time source (see issue 12914 for discussion)."

Hopefully, Python has time.monotonic() since Python 3.3 ;-)
msg293861 - (view) Author: Decorater (Decorater) * Date: 2017-05-17 17:36
I think the patch that haypo added should help take care of the warning. It does however only warned when building for 64 bit.
msg293874 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-05-17 18:55
commit 0d659e5614cad512a1940125135b443b3eecb5d7
Author: Victor Stinner <victor.stinner@gmail.com>
Date:   Tue Apr 25 01:22:42 2017 +0200
msg293889 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-05-17 21:48
I backported the fix to 3.6. So all warnings on timemodule.c should now be fixed. Thanks for the bug report and the patch Decorater! 

commit 69f3a5ac28041fac86897e0c90d98ad9fd6fa3f7
Author: Victor Stinner <victor.stinner@gmail.com>
Date:   Wed May 17 14:45:45 2017 -0700
History
Date User Action Args
2017-05-17 21:48:22vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg293889

stage: resolved
2017-05-17 19:14:38vstinnersetpull_requests: + pull_request1731
2017-05-17 19:14:15vstinnersettitle: Fixed timemodule compile warnings. -> Fixed timemodule compile warnings (gmtoff).
2017-05-17 18:55:17vstinnersetmessages: + msg293874
pull_requests: + pull_request1726
2017-05-17 17:36:32Decoratersetmessages: + msg293861
2017-01-03 12:47:46vstinnersetmessages: + msg284555
2017-01-03 12:41:02vstinnersetnosy: + belopolsky

messages: + msg284554
versions: + Python 3.6
2017-01-03 12:39:04vstinnersetfiles: + gmtoff_time_t.patch
nosy: + vstinner
messages: + msg284553

2017-01-02 04:44:42steve.dowersetmessages: + msg284460
2016-12-24 10:42:03Decoratersettitle: Fix timemodule compile warnings. -> Fixed timemodule compile warnings.
2016-12-24 10:41:49Decoratercreate