classification
Title: tp_flags: Undefined behaviour with 32 bits long
Type: Stage:
Components: Interpreter Core Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ezio.melotti, haypo, jcea, loewis, mark.dickinson, pitrou, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2012-09-29 16:04 by haypo, last changed 2013-01-15 09:44 by jcea. This issue is now closed.

Files
File name Uploaded Description Edit
unsigned_tp_flags.patch haypo, 2012-09-29 16:04 review
unsigned_tp_flags-2.patch haypo, 2012-10-01 23:43 review
Messages (14)
msg171574 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-09-29 16:04
"1L << 31" is undefined if long is 32 bits long. We should use unsigned long for PyTypeObject.tp_flags.

Attached patch implements this fix. I don't know if it breaks somehow the backward compatibility.

See also issues #1621 and #7406.
msg171577 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-09-29 16:58
Victor:  you missed one. :-)

#define Py_TPFLAGS_HAVE_STACKLESS_EXTENSION (3L<<15)
msg171578 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-09-29 17:14
Actually, I think it'll be messy to make this work:  PyType_GetFlags is part of the stable ABI, so that's got to continue to return a long rather than an unsigned long.  And then we've got, in object.h:

#ifdef Py_LIMITED_API
#define PyType_HasFeature(t,f)  ((PyType_GetFlags(t) & (f)) != 0)
#else
#define PyType_HasFeature(t,f)  (((t)->tp_flags & (f)) != 0)
#endif

So we'd need an extra cast from long to unsigned long in the first branch.

I suggest instead just replacing that one occurrence of (1L << 31) with (long)(1UL << 31) to get around the undefined behaviour.


P.S. The docs would also need to be updated in these two files:

   Doc/c-api/typeobj.rst
   Doc/includes/typestruct.h
msg171579 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2012-09-29 17:25
Martin, some things I'm not clear on w.r.t. the stable ABI.

(1) Do defined constants like Py_TPFLAGS_INT_SUBCLASS form part of the stable ABI?

(2) Would changing the type of Py_TPFLAGS_INT_SUBCLASS from long to unsigned long break the stable ABI?
msg171655 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-09-30 18:28
Ok, I'm not Martin, but I'll try to answer. An ABI pertains to the interfaces exposed by compiled object code, not source code. With C, function signatures in compiled code don't keep any type information (*), and they obviously doesn't know about macros either. So I'd answer "No" to both questions :-)

(*) it would be different with C++, because of name mangling
msg171720 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-10-01 14:42
What matters is that precompiled stay compatible; in addition, existing source code should continue to compile unmodified.

In the specific case, the flags type also shows up in PyType_Spec. As a consequence, the actual TPFLAGS_ values *do* constitute a part of the API.

OTOH, a number of the flags are not considered part of the API at all (unfortunately, they aren't explicitly excluded, either). Before we make such a change, we should really declare what flags are meant to be by an extension module, and what flags are implementation details only to be used by the object runtime itself.

Wrt. the proposed change: changing tp_flags to unsigned int is fine. I cannot see any real problem with changing PyType_Spec.flags to unsigned int - changing it to unsigned long would be incompatible on some systems.

Wrt. changing the existing flags: I'd prefer some deprecation procedure that just bans them from being used in an extension module (ultimately wrapping them within Py_BUILD_CORE). Once they are deprecated, changing their type is clearly fine.
msg171723 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-10-01 14:51
tp_flags type is long, not int.
Le 1 oct. 2012 16:42, "Martin v. Löwis" <report@bugs.python.org> a écrit :

>
> Martin v. Löwis added the comment:
>
> What matters is that precompiled stay compatible; in addition, existing
> source code should continue to compile unmodified.
>
> In the specific case, the flags type also shows up in PyType_Spec. As a
> consequence, the actual TPFLAGS_ values *do* constitute a part of the API.
>
> OTOH, a number of the flags are not considered part of the API at all
> (unfortunately, they aren't explicitly excluded, either). Before we make
> such a change, we should really declare what flags are meant to be by an
> extension module, and what flags are implementation details only to be used
> by the object runtime itself.
>
> Wrt. the proposed change: changing tp_flags to unsigned int is fine. I
> cannot see any real problem with changing PyType_Spec.flags to unsigned int
> - changing it to unsigned long would be incompatible on some systems.
>
> Wrt. changing the existing flags: I'd prefer some deprecation procedure
> that just bans them from being used in an extension module (ultimately
> wrapping them within Py_BUILD_CORE). Once they are deprecated, changing
> their type is clearly fine.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue16086>
> _______________________________________
>
msg171735 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2012-10-01 17:16
> tp_flags type is long, not int.

Indeed, and PyType_Spec.flags is int, not long.
msg171763 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-10-01 23:43
> tp_flags type is long, not int.

Oh, I misunderstood what MvL wrote, sorry. I missed PyType_Spec structure.

Here is an updated and more complete patch. I changed the return code of PyType_GetFlags(), instead of changing PyType_HasFeature() macro.

> OTOH, a number of the flags are not considered part of the API at all (unfortunately, they aren't explicitly excluded, either). Before we make such a change, we should really declare what flags are meant to be by an extension module, and what flags are implementation details only to be used by the object runtime itself.

Can't we decide that later? (in other issue?)
msg172788 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-10-12 22:15
Can anyone review my new patch, unsigned_tp_flags-2.patch?
msg174235 - (view) Author: Roundup Robot (python-dev) Date: 2012-10-30 22:42
New changeset 10795aac6df7 by Victor Stinner in branch 'default':
Issue #16086: PyTypeObject.tp_flags and PyType_Spec.flags are now unsigned
http://hg.python.org/cpython/rev/10795aac6df7
msg174240 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-30 23:28
Note that Py_TPFLAGS_INT_SUBCLASS already not used in Python 3.x. One bit of tp_flags can be freed.
msg174241 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-10-30 23:30
> Note that Py_TPFLAGS_INT_SUBCLASS already not used in Python 3.x. One bit of tp_flags can be freed.

Please open a new issue if you consider this issue important enough.
msg174242 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-30 23:31
See also issue9307.
History
Date User Action Args
2013-01-15 09:44:01jceasetnosy: + jcea
2012-10-30 23:31:56serhiy.storchakasetmessages: + msg174242
2012-10-30 23:30:51hayposetmessages: + msg174241
2012-10-30 23:28:30serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg174240
2012-10-30 23:13:14hayposetstatus: open -> closed
resolution: fixed
2012-10-30 22:42:26python-devsetnosy: + python-dev
messages: + msg174235
2012-10-12 22:15:42hayposetmessages: + msg172788
2012-10-01 23:43:35hayposetfiles: + unsigned_tp_flags-2.patch

messages: + msg171763
2012-10-01 17:16:58loewissetmessages: + msg171735
2012-10-01 14:51:41hayposetmessages: + msg171723
2012-10-01 14:42:02loewissetmessages: + msg171720
2012-09-30 18:28:31pitrousetnosy: + pitrou
messages: + msg171655
2012-09-29 17:25:14mark.dickinsonsetnosy: + loewis
messages: + msg171579
2012-09-29 17:14:01mark.dickinsonsetmessages: + msg171578
2012-09-29 16:58:10mark.dickinsonsetmessages: + msg171577
2012-09-29 16:04:15haypocreate