classification
Title: Compiler warnings in _zoneinfo.c on Windows build in 64-bit
Type: Stage: resolved
Components: Build Versions: Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ammar2, corona10, miss-islington, p-ganssle, pablogsal, vstinner
Priority: normal Keywords: patch

Created on 2020-05-19 16:41 by vstinner, last changed 2020-12-16 16:33 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 20619 closed corona10, 2020-06-03 16:48
PR 20624 closed p-ganssle, 2020-06-04 14:12
PR 23614 merged vstinner, 2020-12-02 10:11
PR 23804 merged miss-islington, 2020-12-16 15:26
Messages (12)
msg369374 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-19 16:41
c:\vstinner\python\3.9\modules\_zoneinfo.c(903): warning C4267: '=': conversion from 'size_t' to 'unsigned int', possible loss of data [C:\vstinner\python\3.9\PCbuild\_ 
zoneinfo.vcxproj]
c:\vstinner\python\3.9\modules\_zoneinfo.c(904): warning C4267: '=': conversion from 'size_t' to 'unsigned int', possible loss of data [C:\vstinner\python\3.9\PCbuild\_ 
zoneinfo.vcxproj]
c:\vstinner\python\3.9\modules\_zoneinfo.c(1224): warning C4068: unknown pragma [C:\vstinner\python\3.9\PCbuild\_zoneinfo.vcxproj]
c:\vstinner\python\3.9\modules\_zoneinfo.c(1225): warning C4068: unknown pragma [C:\vstinner\python\3.9\PCbuild\_zoneinfo.vcxproj]
c:\vstinner\python\3.9\modules\_zoneinfo.c(1227): warning C4068: unknown pragma [C:\vstinner\python\3.9\PCbuild\_zoneinfo.vcxproj]
c:\vstinner\python\3.9\modules\_zoneinfo.c(1770): warning C4244: '=': conversion from 'ssize_t' to 'uint8_t', possible loss of data [C:\vstinner\python\3.9\PCbuild\_zon 
einfo.vcxproj]
c:\vstinner\python\3.9\modules\_zoneinfo.c(2408): warning C4028: formal parameter 2 different from declaration [C:\vstinner\python\3.9\PCbuild\_zoneinfo.vcxproj]
msg370415 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-05-31 07:14
@pablogsal
GH-20342 looks like related to this issue.
Can you update the current status?
msg370495 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-05-31 18:43
> Can you update the current status?

Sorry, what do you mean by "current status"? Do you meant this issue or the status of that PR?
msg370551 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-06-01 13:51
Two things.

1. GH-20342 solved this issue?
2. If not what's left for this issue? :)

Sorry, Normally I should check the current status.
But I don't have Windows machine so I can not test it.
However the update will help contributors who want to deal with :)

Thanks Pablo
msg370597 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2020-06-02 08:21
> 1. GH-20342 solved this issue?

As far as I understand, yes. Although I did not check if all the warnings here are solved in that PR, as the PR eliminates all the ones that we found, it should be resolved.

I think we can close this issue

> Thanks Pablo

Thanks to you for all the help :)
msg370598 - (view) Author: Ammar Askar (ammar2) * (Python committer) Date: 2020-06-02 08:23
There's still the "unknown pragma" warnings left, I pinged p-ganssle about it in the zoneinfo commit but it should probably be guarded like the ones in the ssl module:

https://github.com/python/cpython/blob/a871f692b4a2e6c7d45579693e787edc0af1a02c/Modules/_ssl.c#L46
msg383175 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-16 15:25
The problem can be simplified to this x.c file:
---
static int invalid_day(unsigned int day)
{
    return (day < 0 || day > 6);
}

int main()
{
    invalid_day(3);
    return 0;
}
---

GCC emits the warning:

$ gcc x.c -o x -O3 -Wall -Wextra
x.c: In function 'invalid_day':
x.c:3:17: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
    3 |     return (day < 0 || day > 6);
      |                 ^

There are different options to avoid the warning:


(A) Remove "day < 0" test

Easiest option, portable, simple: my PR 23614.


(B) Disable compiler warnings on the test

Solution currently implemented with pragma + PR 20619 to fix pragmas.


(C) Cast the 'day' variable to a signed type

I understand that Paul wants the code to be as generic as possible, and not depending on the "day" parameter type. For example, casting to "int8_t" may introduce a risk of integer overflow if day type is larger than 8 bits. Not my favorite option.


(D) Make "day < 0" conditional depending if day type is signed or not
(E) Check that day type is unsigned to ensure indirectly that "day >= 0"

Checking if *a type* is signed or not is easy using the C preprocessor:

#define _Py_IS_TYPE_UNSIGNED(type) (((type)-1) > (type)0) 

The problem is that there is no standard function to get a variable type. GCC and clang provide the __typeof__(var) extension, C++ provides decltype(var) (but CPython code base cannot be built with a C++ compiler if I recall correctly).

Paul's PR 20624 introduces Py_ASSERT_VAR_UNSIGNED(var) macro which fails during compilation if the variable is unsigned, or does nothing if the compiler doesn't provide a way to get a variable type (ex: MSC on Windows).


--


Most answers about "comparison of unsigned expression always false" question on the Internet are (A): remove the check which emits the warning.

My worry is also that outside _zoneinfo.c, they are tons of functions which rely on the fact that an unsigned type cannot be negativ. I don't want to start adding Py_ASSERT_VAR_UNSIGNED(). For me, it's part of the C language and there is no need to be explicit about it. If a developer changes a variable type, they have to check the type bounds and check of the variable is used.

I would prefer to be consistent and never check for "< 0" if the type is unsigned, nor ensure with an assertion that the type is unsigned.

Paul is in disagreement with that.
msg383177 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-16 15:26
New changeset aefb69b23f056c61e82ad228d950f348de090c70 by Victor Stinner in branch 'master':
bpo-40686: Fix compiler warnings on _zoneinfo.c (GH-23614)
https://github.com/python/cpython/commit/aefb69b23f056c61e82ad228d950f348de090c70
msg383178 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-16 15:31
The 3 compiler warnings are now fixed in master, and a 3.9 backport will land as soon as the CI tests pass. Thanks everyone who helped to fix these warnings.

I know that the solution is not perfect, but we have to deal with C language limitations.
msg383179 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-16 15:56
Fixing these compiler warnings helps PR 18532 "[workflow] Use MSVC problem matcher for Windows action build".
msg383184 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-16 16:33
New changeset 7492b55ea0ca993c353b6373341b92e40faa9c4d by Miss Islington (bot) in branch '3.9':
bpo-40686: Fix compiler warnings on _zoneinfo.c (GH-23614) (GH-23804)
https://github.com/python/cpython/commit/7492b55ea0ca993c353b6373341b92e40faa9c4d
msg383185 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-16 16:33
Paul created a follow-up issue: bpo-42660.
History
Date User Action Args
2020-12-16 16:33:27vstinnersetmessages: + msg383185
2020-12-16 16:33:15vstinnersetmessages: + msg383184
2020-12-16 15:56:27vstinnersetmessages: + msg383179
2020-12-16 15:31:43vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg383178

stage: patch review -> resolved
2020-12-16 15:26:32miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request22664
2020-12-16 15:26:26vstinnersetmessages: + msg383177
2020-12-16 15:25:07vstinnersetmessages: + msg383175
2020-12-02 10:11:44vstinnersetpull_requests: + pull_request22482
2020-06-04 14:12:58p-gansslesetpull_requests: + pull_request19849
2020-06-03 16:48:43corona10setkeywords: + patch
stage: patch review
pull_requests: + pull_request19845
2020-06-02 08:23:32ammar2setnosy: + ammar2
messages: + msg370598
2020-06-02 08:21:05pablogsalsetmessages: + msg370597
2020-06-01 13:51:08corona10setmessages: + msg370551
2020-05-31 18:43:44pablogsalsetmessages: + msg370495
2020-05-31 07:14:11corona10setnosy: + pablogsal, corona10
messages: + msg370415
2020-05-19 16:41:28vstinnercreate