Compiler warnings in _zoneinfo.c on Windows build in 64-bit

@vstinner

BPO	40686
Nosy	@vstinner, @ammaraskar, @pganssle, @corona10, @pablogsal, @miss-islington
PRs	bpo-40686: Fix compiler warnings in _zoneinfo.c #20619 bpo-40686: Replace error suppression with static assertion #20624 bpo-40686: Fix compiler warnings on _zoneinfo.c #23614 [3.9] bpo-40686: Fix compiler warnings on _zoneinfo.c (GH-23614) #23804

^{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-12-16.15:31:43.746>
created_at = <Date 2020-05-19.16:41:28.319>
labels = ['build', '3.9', '3.10']
title = 'Compiler warnings in _zoneinfo.c on Windows build in 64-bit'
updated_at = <Date 2020-12-16.16:33:27.053>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2020-12-16.16:33:27.053>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-12-16.15:31:43.746>
closer = 'vstinner'
components = ['Build']
creation = <Date 2020-05-19.16:41:28.319>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 40686
keywords = ['patch']
message_count = 12.0
messages = ['369374', '370415', '370495', '370551', '370597', '370598', '383175', '383177', '383178', '383179', '383184', '383185']
nosy_count = 6.0
nosy_names = ['vstinner', 'ammar2', 'p-ganssle', 'corona10', 'pablogsal', 'miss-islington']
pr_nums = ['20619', '20624', '23614', '23804']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue40686'
versions = ['Python 3.9', 'Python 3.10']

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]

@pablogsal

@pablogsal
#64541 looks like related to this issue.
Can you update the current status?

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?

Two things.

Endianness not detected correctly due to AC_RUN_IFELSE macros #64541 solved this issue?
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

Endianness not detected correctly due to AC_RUN_IFELSE macros #64541 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 :)

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:

cpython/Modules/_ssl.c

Line 46 in a871f69

/* Don't warn about deprecated functions */

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.

New changeset aefb69b by Victor Stinner in branch 'master':
bpo-40686: Fix compiler warnings on _zoneinfo.c (GH-23614)
aefb69b

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.

Fixing these compiler warnings helps PR 18532 "[workflow] Use MSVC problem matcher for Windows action build".

New changeset 7492b55 by Miss Islington (bot) in branch '3.9':
bpo-40686: Fix compiler warnings on _zoneinfo.c (GH-23614) (GH-23804)
7492b55

Paul created a follow-up issue: bpo-42660.

Navigation Menu

Repository navigation

Compiler warnings in _zoneinfo.c on Windows build in 64-bit #84863

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Compiler warnings in _zoneinfo.c on Windows build in 64-bit #84863

Description

Activity

vstinner commented on May 19, 2020

corona10 commented on May 31, 2020

pablogsal commented on May 31, 2020

corona10 commented on Jun 1, 2020

pablogsal commented on Jun 2, 2020

ammaraskar commented on Jun 2, 2020

vstinner commented on Dec 16, 2020

vstinner commented on Dec 16, 2020

vstinner commented on Dec 16, 2020

vstinner commented on Dec 16, 2020

vstinner commented on Dec 16, 2020

vstinner commented on Dec 16, 2020

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions