bpo-40686: Replace error suppression with static assertion #20624

pganssle · 2020-06-04T14:12:56Z

Rather than suppressing the unnecessary compiler warning, we will avoid the question by removing the lower bound and introducing a static assertion about the type of day.

In some ways this is still worse than the original version, since it only does the assertion when the __typeof__ GCC extension is available, and if the type of day is changed to be signed it will actually require code changes to the boundary check, but for practical purposes (since, at least at the moment, there's very little chance that we will remove GCC-based compilations from our CI checks) this will at least prevent the introduction of this particular bug.

This supercedes #20619. There's a corresponding patch to the backport here.

CC @corona10 @vstinner @pablogsal

https://bugs.python.org/issue40686

pganssle · 2020-06-04T14:18:26Z

Modules/_zoneinfo.c

+// This is not perfect, but it's better than nothing.
+#ifdef __GNUC__
+#define IS_TYPE_UNSIGNED(type) (((type)-1) > (type)0)
+#define Py_ASSERT_VAR_UNSIGNED(var) \


We could, theoretically, move this into pymacro.h (or some private variant thereof), and potentially we could make it more portable, there might be an MSVC version of __typeof__ that I'm unaware of.

If we ever start allowing C11 (under flags or whatever so that it doesn't break non-C11 compilers), I think there are ways to accomplish the same thing using just the standard.

vstinner

I prefer this approach than PR #20619, but I would prefer to move things to pymacros.h to ease future maintenance and move the complexity out of _zoneinfo.c.

vstinner · 2020-06-04T15:56:25Z

Modules/_zoneinfo.c

+// variable, so we'll make Py_ASSERT_VAR_UNSIGNED a noop on non-GCC platforms.
+// This is not perfect, but it's better than nothing.
+#ifdef __GNUC__
+#define IS_TYPE_UNSIGNED(type) (((type)-1) > (type)0)


Can you move IS_TYPE_UNSIGNED() to pymacro.h as PY_IS_TYPE_UNSIGNED()?

Py_ASSERT_VAR_UNSIGNED() can stay here :-)

vstinner · 2020-06-04T16:01:13Z

Modules/_zoneinfo.c

+// This takes advantage of GCC compiler extensions to determine the type of the
+// variable, so we'll make Py_ASSERT_VAR_UNSIGNED a noop on non-GCC platforms.
+// This is not perfect, but it's better than nothing.
+#ifdef __GNUC__


Hum, I would prefer to test a "HAVE_TYPEOF" rather than "hardcoding" GCC here. I suggest to define it in pymacros.h. Something like:

#if defined(__GNUC__) || defined(__clang__) // typeof() function # define HAVE_TYPEOF 1 #endif

clang also supports typeof().

Note: I prefer typeof() over __typeof__() :-)

typeof() is already used in pymacros.h. typeof() is used in pycore_atomic.h.

Oh, clang manual says:

The parser recognizes “asm” and “typeof” as keywords in gnu* modes; the variants “__asm__” and “__typeof__” are recognized in all modes.

Yeah, I think __typeof__ is the "compatibility mode" version of this. The standard way you build extensions seems to use one of the modes where typeof doesn't work as far as I can tell, so that's how I did it there. If we're already unconditionally using typeof() I guess it's fine to switch to that here.

pganssle · 2020-06-04T17:56:21Z

Include/pyport.h

+#   if defined(__cplusplus)
+#       define _Py_TYPEOF(x) decltype(x)
+#   else
+#       define _Py_TYPEOF(x) __typeof__(x)


I am not entirely sure if maybe this should be __extension__ ( __typeof__(x) ) to allow its use in -pedantic mode or something.

vstinner

If you have troubles with -pedantic, you might try to test defined(STRICT_ANSI): see Py_ARRAY_LENGTH() crazy macro.

vstinner · 2020-06-04T21:29:20Z

Include/pyport.h

+#if defined(__GNUC__) || defined(__clang__) || defined(__cplusplus)
+#   define _Py_HAVE_TYPEOF 1
+#   if defined(__cplusplus)
+#       define _Py_TYPEOF(x) decltype(x)


I'm not sure that it's worth it to add _Py_TYPEOF() just to support C++. C extensions are usually built in C. All stdlib C extensions are built in C mode.

Since it's a private macro, I suggest to only support C and documented _Py_HAVE_TYPEOF as Define if __typeof__() builtin function is available.

I think the purpose here was not so much to support C++ but to give us some compiler-independent behavior. In C++ mode, there's an alternative to typeof that works in all the scenarios we use __typeof__ for, so this extends the scenarios where we can do __typeof__-like operations.

I also think that maybe we want _Py_TYPEOF() to be defined as __extension__ ( __typeof__(x) ) , in which case it's better to have our own macro defined for it (and at that point the __cplusplus definition is only a cost in that we may not be testing it properly if we never build anything in C++ mode).

The other benefit to this is that if other compilers add support for typeof-like functionality, we'll be able to seamlessly add them.

vstinner · 2020-06-04T21:30:42Z

Include/pyport.h

+        "_Py_TYPEOF is not available in all supported compilation modes on " \
+        "all supported compilers.  Use the _Py_HAVE_TYPEOF macro to guard " \
+        "any statements using _Py_TYPEOF." \
+        )


I'm not sure of the purpose of defining _Py_TYPEOF() if it's not supported. If _Py_HAVE_TYPEOF is not defined, _Py_TYPEOF is not defined and typeof() must not be used. No?

I suppose that's true. I was mainly trying to give a clean error message, because C error messages are messy.

Another option is to drop _Py_HAVE_TYPEOF and have people use #ifdef _Py_TYPEOF as the guard, if they want to.

vstinner

_Py_IS_TYPE_UNSIGNED() is simple and makes sense, but I still fail to see the benefit of Py_ASSERT_VAR_UNSIGNED(day);. IMO the preprocessor magic to define _Py_TYPEOF() is not worth it.

If you are scared that one day, someone changes the type to make it signed, just add a comment somewhere. But I don't see why anyone would do that without thinking twice about bounds checking.

pganssle · 2020-06-23T14:27:41Z

If you are scared that one day, someone changes the type to make it signed, just add a comment somewhere. But I don't see why anyone would do that without thinking twice about bounds checking.

Even in Python I think it's better to have the semantics of your code enforce your assumptions wherever possible, but this is C — I 100% don't trust people to understand the full implications of everything they are doing and to read all the comments on everything that might be affected by changing the type in one struct or another.

If there were a real trade-off here, I might be happy to compromise, but this is just working around a bad compiler warning.

How about we just move this back to how I had it originally, with the macro defined in zoneinfo.c, so that we can get rid of the compiler warnings, and then we can address making a more generalized version of this macro in a separate enhancement?

vstinner · 2020-06-23T14:47:05Z

Yeah, it's maybe better to move _Py_TYPEOF in _zoneinfo.c.

Rather than suppressing the unnecessary compiler warning, we will avoid the question by removing the lower bound and introducing a static assertion about the type of `day`. In some ways this is still worse than the original version, since it only does the assertion when the __typeof__ GCC extension is available, and if the type of `day` is changed to be signed it will actually require code changes to the boundary check, but for practical purposes (since, at least at the moment, there's very little chance that we will remove GCC-based compilations from our CI checks) this will at least prevent the introduction of this particular bug. https://bugs.python.org/issue40686

This reverts commit 93a13c3.

pganssle · 2020-10-20T19:30:14Z

@vstinner I've restored the patch to its original version, before we tried to get more general with it. What do you think?

vstinner · 2020-10-27T02:27:02Z

@vstinner I've restored the patch to its original version, before we tried to get more general with it. What do you think?

I don't think that it's worth it and I would suggest to remove all pragmas which produce annoying compiler warnings on Windows.

pganssle · 2020-10-27T02:55:46Z

Ok, we'll just skip the whole thing if you don't think it's worth fixing these erroneous warnings.

the-knights-who-say-ni added the CLA signed label Jun 4, 2020

bedevere-bot added the awaiting core review label Jun 4, 2020

pganssle added the skip news label Jun 4, 2020

pganssle commented Jun 4, 2020

View reviewed changes

vstinner reviewed Jun 4, 2020

View reviewed changes

pganssle commented Jun 4, 2020

View reviewed changes

vstinner reviewed Jun 4, 2020

View reviewed changes

vstinner reviewed Jun 19, 2020

View reviewed changes

pganssle added 4 commits October 20, 2020 14:10

Add macros to get type a variable's type

cec55ef

Use _Py_TYPEOF in _zoneinfo.c

93a13c3

Revert "Use _Py_TYPEOF in _zoneinfo.c"

b948cb2

This reverts commit 93a13c3.

pganssle force-pushed the zoneinfo_static_type_assertion branch from 2b0ced5 to b948cb2 Compare October 20, 2020 18:11

pganssle closed this Oct 27, 2020

pganssle mentioned this pull request Dec 1, 2020

bpo-40686: Fix compiler warnings in _zoneinfo.c #20619

Closed

vstinner mentioned this pull request Dec 2, 2020

bpo-40686: Fix compiler warnings on _zoneinfo.c #23614

Merged

Uh oh!

bpo-40686: Replace error suppression with static assertion #20624

bpo-40686: Replace error suppression with static assertion #20624

Uh oh!

Conversation

pganssle commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pganssle Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

pganssle commented Jun 23, 2020

Uh oh!

vstinner commented Jun 23, 2020

Uh oh!

pganssle commented Oct 20, 2020

Uh oh!

vstinner commented Oct 27, 2020

Uh oh!

pganssle commented Oct 27, 2020

Uh oh!

Uh oh!

pganssle commented Jun 4, 2020 •

edited

Loading

pganssle Jun 4, 2020 •

edited

Loading

vstinner Jun 4, 2020 •

edited

Loading