Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
abd0a7c
Replace error suppression with static assertion
pganssle Jun 4, 2020
cec55ef
Add macros to get type a variable's type
pganssle Jun 4, 2020
93a13c3
Use _Py_TYPEOF in _zoneinfo.c
pganssle Jun 4, 2020
b948cb2
Revert "Use _Py_TYPEOF in _zoneinfo.c"
pganssle Oct 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Include/pymacro.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
(uintptr_t)((a) - 1)) & ~(uintptr_t)((a) - 1)))
/* Check if pointer "p" is aligned to "a"-bytes boundary. */
#define _Py_IS_ALIGNED(p, a) (!((uintptr_t)(p) & (uintptr_t)((a) - 1)))
#define _Py_IS_TYPE_UNSIGNED(type) (((type)-1) > (type)0)

/* Use this for unused arguments in a function definition to silence compiler
* warnings. Example:
Expand Down
21 changes: 21 additions & 0 deletions Include/pyport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,27 @@ typedef int Py_ssize_clean_t;
# define PY_FORMAT_SIZE_T "z"
#endif


/* _Py_HAVE_TYPEOF and _Py_TYPEOF can opportunistically support an equivalent
* of GCC's typeof extension where possible. It is not possible to get
* equivalent behavior on all platforms, so all uses of _Py_TYPEOF should be
* guarded by _Py_HAVE_TYPEOF.
*/
#if defined(__GNUC__) || defined(__clang__) || defined(__cplusplus)
# define _Py_HAVE_TYPEOF 1
# if defined(__cplusplus)
# define _Py_TYPEOF(x) decltype(x)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# else
# define _Py_TYPEOF(x) __typeof__(x)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# endif
#else
# define _Py_TYPEOF(x) Py_FatalError( \
"_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." \
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#endif

/* Py_LOCAL can be used instead of static to get the fastest possible calling
* convention for functions that are local to a given module.
*
Expand Down
26 changes: 17 additions & 9 deletions Modules/_zoneinfo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,17 @@

#include "datetime.h"

// 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__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@vstinner vstinner Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#define IS_TYPE_UNSIGNED(type) (((type)-1) > (type)0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Py_ASSERT_VAR_UNSIGNED() can stay here :-)

#define Py_ASSERT_VAR_UNSIGNED(var) \
Copy link
Member Author

@pganssle pganssle Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Py_BUILD_ASSERT(IS_TYPE_UNSIGNED(__typeof__(var)))
#else
#define Py_ASSERT_VAR_UNSIGNED(var)
#endif

// Imports
static PyObject *io_open = NULL;
static PyObject *_tzpath_find_tzfile = NULL;
Expand Down Expand Up @@ -1217,15 +1228,12 @@ calendarrule_new(uint8_t month, uint8_t week, uint8_t day, int8_t hour,
return -1;
}

// day is an unsigned integer, so day < 0 should always return false, but
// if day's type changes to a signed integer *without* changing this value,
// it may create a bug. Considering that the compiler should be able to
// optimize out the first comparison if day is an unsigned integer anyway,
// we will leave this comparison in place and disable the compiler warning.
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wtype-limits"
if (day < 0 || day > 6) {
#pragma GCC diagnostic pop
// The actual bounds of day are (day >= 0 && day <= 6), but since day is an
// unsigned variable, day >= 0 is always true. To ensure that a bug is not
// introduced in the event that someone changes day to a signed type, we
// will assert that it is an unsigned type.
Py_ASSERT_VAR_UNSIGNED(day);
if (day > 6) {
PyErr_Format(PyExc_ValueError, "Day must be in [0, 6]");
return -1;
}
Expand Down