bpo-35081: Remove Py_BUILD_CORE from datetime.h #10238

vstinner · 2018-10-30T12:18:29Z

Datetime macros like PyDate_Check() are now reimplemented in
_datetimemodule.c, rather than having two implementations depending
on Py_BUILD_CORE in datetime.h.

These macros are not used outside _datetimemodule.c.

https://bugs.python.org/issue35081

Datetime macros like PyDate_Check() are now reimplemented in _datetimemodule.c, rather than having two implementations depending on Py_BUILD_CORE in datetime.h. These macros are not used outside _datetimemodule.c.

vstinner · 2018-10-30T12:21:45Z

These macros are part of the public C API:
https://docs.python.org/dev/c-api/datetime.html

But the "public C API" should use PyDateTimeAPI, not directly types like PyDateTime_DateType.

This PR has no effect on C extensions, since only Python itself is compiled using Py_BUILD_CORE.

vstinner · 2018-10-30T12:22:05Z

cc @pganssle

pganssle

I think the most likely "bug" this will cause is if someone was erroneously building with Py_BUILD_CORE enabled and they were using these macros (and pretty much only these macros) without importing the datetime.datetime_CAPI capsule, it can cause some segmentation faults. I think it's possible to use these without importing that capsule with Py_BUILD_CORE, but I haven't tried it out - maybe it's actually not a concern.

I imagine that according to Hyrum's Law, that a few people are relying on this, but I imagine the overlap between the people who are relying on this behavior and the people who would notice if we tried to add some sort of compile-time warning for 1 release is pretty small, so we might as well just go for it. It's not like it's a hard problem to fix.

pganssle · 2018-10-30T14:02:47Z

This brings up an interesting question - are the tests compiled with Py_BUILD_CORE defined?

If not, then to the extent that there are C API tests, I guess they've all been hitting the Py_BUILD_CORE paths, not the public paths? That seems less than ideal.

vstinner · 2018-10-30T14:22:16Z

Third party must not be compiled with Py_BUILD_CORE defined. It seems like it's not technically possible (Include/internal/ headers are not installed on Fedora for example).

_testcapimodule.c is not compiled with Py_BUILD_CORE defined. IMHO _testcapi must test the API consumed by 3rd party modules, so we are good :-)

Modules/_datetimemodule.c

serhiy-storchaka · 2018-10-30T14:43:20Z

Include/datetime.h

-#define PyTZInfo_CheckExact(op) (Py_TYPE(op) == &PyDateTime_TZInfoType)
-
-#else
-
 /* Define global variable for the C API and a macro for setting it. */
 static PyDateTime_CAPI *PyDateTimeAPI = NULL;


This variable will be created in every compilation unit that includes "datetime.h".

Was that already a problem for the C API? this was a pre-existing part of the public API.

At the moment, within CPython, this is only included in _datetimemodule.c and _testcapimodule.c. The latter was, according to Victor, already being compiled without Py_BUILD_CORE.

Right, it's a little bit strange to use static here. But my PR doesn't change that, and I don't see how to change the API to avoid it.

@serhiy-storchaka: Would you mind to open a new issue to track this issue?

@vstinner I think your PR does change one thing, which is that this static variable is now created even when Py_BUILD_CORE is on, right? I don't think that there are any major negative consequences, though, and I don't see a way around it without either changing the public interface or re-introducing Py_BUILD_CORE to this file, which I gather was the aim of this PR.

serhiy-storchaka · 2018-10-30T15:26:10Z

Is Py_BUILD_CORE always defined for _datetimemodule.c? Is it possible that custom builds compile it as an external module?

vstinner · 2018-10-30T15:39:16Z

This [PyDateTimeAPI] variable will be created in every compilation unit that includes "datetime.h".

Oh, but "datetime.h" is not include by Python.h. Only _testcapimodule.c and _datetimemodule.c include it. I guess that it's only included when it's needed.

I recompiled Python with this PR (I'm not using --enable-shared):

$ nm ./python|grep PyDateTimeAPI
$ gdb ./python
GNU gdb (GDB) Fedora 8.1.1-3.fc28

(gdb) p PyDateTimeAPI
No symbol "PyDateTimeAPI" in current context.

It seems like there is no PyDateTimeAPI variable in the python binary.

Only _datetime and _testcapi SO libraries have the symbol:

$ for OBJ in $(find -name "*.o"); do nm $OBJ|grep PyDateTimeAPI && echo $OBJ; done
0000000000000000 b PyDateTimeAPI
./build/temp.linux-x86_64-3.8-pydebug/home/vstinner/prog/python/master/Modules/_datetimemodule.o
0000000000000000 b PyDateTimeAPI
./build/temp.linux-x86_64-3.8-pydebug/home/vstinner/prog/python/master/Modules/_testcapimodule.o

I'm using ./configure --with-pydebug CFLAGS="-O0", so even without optimizations, no symbol is emited if it's not used (again, datetime.h is not included by default by Python.h, so it's expected).

--

But I looked more closely at warnings, and my PR introduces a warning:

In file included from /home/vstinner/prog/python/master/Modules/_datetimemodule.c:6:
./Include/datetime.h:184:25: warning: ‘PyDateTimeAPI’ defined but not used [-Wunused-variable]
 static PyDateTime_CAPI *PyDateTimeAPI = NULL;
                         ^~~~~~~~~~~~~

Is Py_BUILD_CORE always defined for _datetimemodule.c?

Py_BUILD_CORE is not defined when _datetimemodule.c is compiled, but the code currently works around the issue by defining Py_BUILD_CORE when datetime.h is included:

/* Differentiate between building the core module and building extension
 * modules.
 */
#ifndef Py_BUILD_CORE
#define Py_BUILD_CORE
#endif
#include "datetime.h"
#undef Py_BUILD_CORE

--

Oh, this issue is more complex than what I expected...

vstinner · 2018-10-31T23:13:08Z

I added a _PY_DATETIME_IMPL define only used in _datetimemodule.c, to not define macros of the C API and to avoid the warning about the unused static variable.

vstinner · 2018-11-01T02:24:09Z

I tried to compile _testcapi with Py_BUILD_CORE defined in my PR #10274, but datetime.h without this change creates a lot of troubles in that case.

vstinner · 2018-11-01T02:44:35Z

@pganssle, @serhiy-storchaka: I rewrote my PR using a new _PY_DATETIME_IMPL define. Would you mind to review it again? Thanks.

serhiy-storchaka · 2018-11-01T05:12:49Z

You replace one macro name with other macro name. What is the benefit?

vstinner · 2018-11-01T12:25:10Z

You replace one macro name with other macro name. What is the benefit?

This change allows to use datetime.h inside CPython with Py_BUILD_CORE defined.

Currently, if datetime.h is compiled with Py_BUILD_CORE, the API uses PyDateTime_DateType which comes from the external _datetime module, it's not part of libpython. The current headre file is wrong.

IMHO the code surrounded by Py_BUILD_CORE in datetime.h must really be moved to _datetimemodule.c.

pganssle

Just a minor change to the comment on this section.

I agree with the idea of moving the private type checking macros into _datetimemodule.c, though I was under the impression that this was part of a wider move to get rid of Py_BUILD_CORE, in which case this seems like a partial success, since like @serhiy-storchaka said, you are really just replacing one macro with another.

I think I don't have a sufficiently broad view to understand why Py_BUILD_CORE needs to be renamed in this file other than moving it to a macro that starts with _ to emphasize that it is private.

@vstinner Does this version accomplish the "remove Py_BUILD_CORE" goal to your satisfaction, or should we start thinking about a path forward that also removes _PY_DATETIME_IMPL`?

Include/datetime.h

pganssle · 2018-11-01T16:09:53Z

Oh I did just see this comment:

Currently, if datetime.h is compiled with Py_BUILD_CORE, the API uses PyDateTime_DateType which comes from the external _datetime module, it's not part of libpython. The current headre file is wrong.

So I do see why you would want to at least rename it. I'm still curious to know if you think that _PY_DATETIME_IMPL should eventually be removed as well.

Co-Authored-By: vstinner <[email protected]>

vstinner · 2018-11-01T17:18:37Z

I'm still curious to know if you think that _PY_DATETIME_IMPL should eventually be removed as well.

I would prefer to remove any #ifdef from datetime.h, but I didn't find any way to do that.

If anyone see a solution, please tell me :-)

The main blocker is "static PyDateTime_CAPI *PyDateTimeAPI = NULL;" in datetime.h. You really don't want to have that in _datetimemodule.c... But _datetimemodule.c has to include datetime.h...

serhiy-storchaka · 2018-11-01T17:29:45Z

Include/datetime.h

@@ -224,6 +207,8 @@ static PyDateTime_CAPI *PyDateTimeAPI = NULL;

 #define PyTZInfo_Check(op) PyObject_TypeCheck(op, PyDateTimeAPI->TZInfoType)


Why these macros are defined for non-core C API at all? PyDate_Check() and like are not defined.

I don't understand what you mean, PyDate_Check is defined in this block, these are all part of the documented C API.

I don't understand. In which case PyDate_Check() is not defined when you include datetime.h?

Ah, sorry, I was confused by changes in this PR. Now PyDate_Check is defined unconditionally, but PyTZInfo_Check only if _PY_DATETIME_IMPL is not defined, right? Then the question is why it is.

_PY_DATETIME_IMPL is only used when datetime.h is included from _datetimemodule.c. In this case, PyDate_Check() and PyTZInfo_Check() are defined in _datetimemodule.c.

Previously, I always defined these macros, and then used #undef in _datetimemodule.c. But this approach doesn't work if tomorrow these macros are converted to static inline functions.

@serhiy-storchaka: do you prefer the #undef approach, and only exclude "static PyDateTime_CAPI *PyDateTimeAPI = NULL;" using _PY_DATETIME_IMPL?

Before Victor's PR, all of these macros are defined here, twice - they have different definitions when built as part of CPython than when built as part of an extension module.

The reason they are different is that the datetime module exposes all of its functionality via a capsule (PyDateTimeAPI), but the datetime module itself doesn't need to use the capsule because it has direct access to the relevant symbols.

Victor's PR takes the "CPython only" builds and moves them out of datetime.h and into _datetimemodule.c, which is the only place they are (and can be) used. The public macros are under _PY_DATETIME_IMPL in this case so that he doesn't have to #undef them in _datetimemodules.c.

There's probably a case to be made in favor of dropping the macros defined in _datetimemodules.c entirely and having _datetimemodules.c just use the public macros, but I am not sure what the consequences there could be. I imagine a performance hit for starters (probably minor), and it would mean that _datetimemodules.c would need access to the PyDateTimeAPI singleton as well.

I think Victor's current proposed changes are an improvement over the current state of things and unless we missed something, should have no change in behavior for end users while making datetime easier to maintain. We can start with these changes and then do an analysis later of whether the code can be further improved by unifying the *_Check macros and having _datetimemodule.c generally use the public API where possible.

Also, hopefully I understood your comment correctly. If you meant that PyTZInfo_Check is different from the other ones, I don't think it is. PyTZInfo_Check is used in `_datetimemodule.c, so it has to be present in both the public and private APIs.

I would add that currently, including datetime.h in a C file which defines Py_BUILD_CORE doesn't work, since it uses types which are only exported by an external library... (_datetime is compared as a shared library by default on Linux)

pganssle · 2018-11-03T17:00:55Z

Modules/_datetimemodule.c

-#define Py_BUILD_CORE
-#endif
-#include "datetime.h"
-#undef Py_BUILD_CORE


I think it makes no difference in this case, but it's somewhat interesting that in the old version, Py_BUILD_CORE was un-defined regardless of whether it was originally defined.

I think this means that if Py_BUILD_CORE is defined when compiling _datetimemodules.c, it will now remain so for the rest of the file, as opposed to the pre-PR version, where it is unconditionally flipped off after importing datetime.h. I don't see any other references to it in the file, though, so it should be safe to remove this.

pganssle

This looks good to me, I think we should merge it and try to figure out a graceful way to remove the _Py_DATETIME_IMPL macro at a later time.

vstinner · 2018-11-04T23:44:30Z

I think we should merge it and try to figure out a graceful way to remove

the _Py_DATETIME_IMPL macro at a later time. In my experience, if it's merged, it is not going to change soon. If we cannot find a better solution today, I don't see how we are supposed to find a better solution tomorrow. Another option is to not include datetime.h from _datetimemodule.c. Or to move the subset outside the #ifdef into a new smaller header file included by datetime.h and by _datetimemodule.c, but it sounds overengineered for a simple issue... Honestly, I don't what is wrong with the #ifdef. As Serhiy wrote, my change just replaced a define by another. It's a simple change :-) But I am ok to merge this change, since datetime.h is currently broken if Py_BUILD_CORE is defined. Right now, it is never included in a C file defining Py_BUILD_CORE, but that can change later. ... so much bikeshedding for such simple header file :-)

serhiy-storchaka · 2018-11-05T05:37:18Z

... so much bikeshedding for such simple header file :-)

Just don't forget about Chesterton's Fence.

pganssle · 2018-11-05T15:12:36Z

@vstinner I was just going by your earlier statements that you wanted to remove this sort of conditional compilation if possible. Per Serhiy's comment about Chesterton's Fence, we should probably make sure the commit message is pretty clear about the history of this particular macro guard.

I'm guessing that this will only be removable in the event that there's either some backwards-incompatible changes happening in the C API (unlikely) or if there's a pretty significant refactoring of the include files. I am really not ready for or interested in advocating for such a refactoring at the moment, but in 2 years when we've all forgotten why this macro was added, it will probably be good to know that this was a compromise solution.

vstinner · 2018-11-06T14:18:20Z

This PR gives me headache. Sorry, I close it. If someone else wants to rewrite it, please go ahead.

pganssle · 2018-11-06T14:37:33Z

@vstinner I'm not sure if there's a communication issue here, I was saying that I think it should have been merged as is, with a note in the commit message about why the _Py_DATETIME_IMPL is necessary so that future reformers know it's not intended to be a permanent fixture. 😕

bpo-35081: Remove Py_BUILD_CORE from datetime.h

2f7832f

Datetime macros like PyDate_Check() are now reimplemented in _datetimemodule.c, rather than having two implementations depending on Py_BUILD_CORE in datetime.h. These macros are not used outside _datetimemodule.c.

the-knights-who-say-ni added the CLA signed label Oct 30, 2018

bedevere-bot added the awaiting merge label Oct 30, 2018

vstinner added the skip news label Oct 30, 2018

vstinner requested review from abalkin and benjaminp October 30, 2018 12:21

Add #undef PyDateTime_TimeZone_UTC to C file

1858252

pganssle approved these changes Oct 30, 2018

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting merge labels Oct 30, 2018

serhiy-storchaka reviewed Oct 30, 2018

View reviewed changes

pganssle mentioned this pull request Oct 30, 2018

Non-core review downgrades core dev PRs python/bedevere#139

Closed

Add a comment

22abaee

vstinner added 2 commits November 1, 2018 00:08

PyDateTimeAPI variable must not be defined in _datetimemodule.c

8a11d06

Extend the scope of _PY_DATETIME_IMPL

a2750c6

vstinner mentioned this pull request Nov 1, 2018

[WIP] bpo-35081: _testcapi is now compiled with Py_BUILD_CORE #10274

Closed

pganssle requested changes Nov 1, 2018

View reviewed changes

Include/datetime.h Outdated Show resolved Hide resolved

Update Include/datetime.h

69a455d

Co-Authored-By: vstinner <[email protected]>

serhiy-storchaka reviewed Nov 1, 2018

View reviewed changes

pganssle reviewed Nov 3, 2018

View reviewed changes

pganssle approved these changes Nov 3, 2018

View reviewed changes

vstinner closed this Nov 6, 2018

vstinner deleted the datetime_core branch November 6, 2018 14:18

pganssle mentioned this pull request Nov 8, 2018

bpo-35081: Remove Py_BUILD_CORE from datetime.h #10416

Merged

WillAyd mentioned this pull request Mar 19, 2020

Error on C Warnings pandas-dev/pandas#32163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-35081: Remove Py_BUILD_CORE from datetime.h #10238

bpo-35081: Remove Py_BUILD_CORE from datetime.h #10238

vstinner commented Oct 30, 2018 •

edited by bedevere-bot

Loading

vstinner commented Oct 30, 2018

vstinner commented Oct 30, 2018

pganssle left a comment

pganssle commented Oct 30, 2018

vstinner commented Oct 30, 2018

serhiy-storchaka Oct 30, 2018

pganssle Oct 30, 2018 •

edited

Loading

vstinner Oct 30, 2018

pganssle Oct 30, 2018 •

edited

Loading

serhiy-storchaka commented Oct 30, 2018

vstinner commented Oct 30, 2018

vstinner commented Oct 31, 2018

vstinner commented Nov 1, 2018

vstinner commented Nov 1, 2018

serhiy-storchaka commented Nov 1, 2018

vstinner commented Nov 1, 2018

pganssle left a comment

pganssle commented Nov 1, 2018

vstinner commented Nov 1, 2018 •

edited

Loading

serhiy-storchaka Nov 1, 2018

pganssle Nov 1, 2018

vstinner Nov 1, 2018

serhiy-storchaka Nov 1, 2018

vstinner Nov 1, 2018

pganssle Nov 1, 2018 •

edited

Loading

pganssle Nov 1, 2018

vstinner Nov 1, 2018

pganssle Nov 3, 2018

pganssle left a comment

vstinner commented Nov 4, 2018 via email

serhiy-storchaka commented Nov 5, 2018

pganssle commented Nov 5, 2018

vstinner commented Nov 6, 2018

pganssle commented Nov 6, 2018

		@@ -224,6 +207,8 @@ static PyDateTime_CAPI *PyDateTimeAPI = NULL;

		#define PyTZInfo_Check(op) PyObject_TypeCheck(op, PyDateTimeAPI->TZInfoType)

bpo-35081: Remove Py_BUILD_CORE from datetime.h #10238

bpo-35081: Remove Py_BUILD_CORE from datetime.h #10238

Conversation

vstinner commented Oct 30, 2018 • edited by bedevere-bot Loading

vstinner commented Oct 30, 2018

vstinner commented Oct 30, 2018

pganssle left a comment

Choose a reason for hiding this comment

pganssle commented Oct 30, 2018

vstinner commented Oct 30, 2018

Choose a reason for hiding this comment

pganssle Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pganssle Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

serhiy-storchaka commented Oct 30, 2018

vstinner commented Oct 30, 2018

vstinner commented Oct 31, 2018

vstinner commented Nov 1, 2018

vstinner commented Nov 1, 2018

serhiy-storchaka commented Nov 1, 2018

vstinner commented Nov 1, 2018

pganssle left a comment

Choose a reason for hiding this comment

pganssle commented Nov 1, 2018

vstinner commented Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pganssle Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pganssle left a comment

Choose a reason for hiding this comment

vstinner commented Nov 4, 2018 via email

serhiy-storchaka commented Nov 5, 2018

pganssle commented Nov 5, 2018

vstinner commented Nov 6, 2018

pganssle commented Nov 6, 2018

vstinner commented Oct 30, 2018 •

edited by bedevere-bot

Loading

pganssle Oct 30, 2018 •

edited

Loading

pganssle Oct 30, 2018 •

edited

Loading

vstinner commented Nov 1, 2018 •

edited

Loading

pganssle Nov 1, 2018 •

edited

Loading