msg352669 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-09-17 22:12 |
With the following change, Python no longer compiles. I'm sure that it compiled successfully 2 weeks ago.
diff --git a/Objects/longobject.c b/Objects/longobject.c
index 4cf2b0726e..0ad2609882 100644
--- a/Objects/longobject.c
+++ b/Objects/longobject.c
@@ -16,10 +16,10 @@ class int "PyObject *" "&PyLong_Type"
/*[clinic end generated code: output=da39a3ee5e6b4b0d input=ec0275e3422a36e3]*/
#ifndef NSMALLPOSINTS
-#define NSMALLPOSINTS 257
+#define NSMALLPOSINTS 0
#endif
#ifndef NSMALLNEGINTS
-#define NSMALLNEGINTS 5
+#define NSMALLNEGINTS 0
#endif
_Py_IDENTIFIER(little);
Main GCC error:
Objects/longobject.c: In function '_PyLong_Copy':
./Include/pymacro.h:119:26: error: expected expression before 'do'
119 | #define Py_UNREACHABLE() do { \
| ^~
Objects/longobject.c:83:30: note: in expansion of macro 'Py_UNREACHABLE'
83 | #define get_small_int(ival) (Py_UNREACHABLE(), NULL)
| ^~~~~~~~~~~~~~
It seems to be a regression introduced by:
commit 6b519985d23bd0f0bd072b5d5d5f2c60a81a19f2
Author: animalize <animalize@users.noreply.github.com>
Date: Fri Sep 6 14:00:56 2019 +0800
replace inline function `is_small_int` with a macro version (GH-15710)
|
msg352680 - (view) |
Author: Ma Lin (malin) * |
Date: 2019-09-18 00:03 |
This commit changed Py_UNREACHABLE() five days ago:
https://github.com/python/cpython/commit/3ab61473ba7f3dca32d779ec2766a4faa0657923
If remove this change, it can be compiled successfully.
|
msg352681 - (view) |
Author: Ma Lin (malin) * |
Date: 2019-09-18 00:21 |
We can change Py_UNREACHABLE() to assert(0) in longobject.c
Or remove the article in Py_UNREACHABLE()
|
msg352682 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-09-18 00:24 |
See https://bugs.python.org/issue37812#msg352670 and https://bugs.python.org/issue38015
|
msg352700 - (view) |
Author: Sergey Fedoseev (sir-sigurd) * |
Date: 2019-09-18 06:29 |
I believe that the problem is caused by the change in Py_UNREACHABLE() (https://github.com/python/cpython/commit/3ab61473ba7f3dca32d779ec2766a4faa0657923).
Before the mentioned commit Py_UNREACHABLE() was an expression, now it's a block. Py_UNREACHABLE() macro is public (see https://docs.python.org/3/c-api/intro.html#c.Py_UNREACHABLE), so this change can cause similar problems outside of CPython (i.e. that change was breaking).
|
msg352703 - (view) |
Author: Sergey Fedoseev (sir-sigurd) * |
Date: 2019-09-18 06:54 |
Also quote from Py_UNREACHABLE() doc:
> Use this in places where you might be tempted to put an assert(0) or abort() call.
https://github.com/python/cpython/commit/6b519985d23bd0f0bd072b5d5d5f2c60a81a19f2 does exactly that, it replaces assert(0) with Py_UNREACHABLE().
|
msg352708 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-09-18 07:41 |
> Before the mentioned commit Py_UNREACHABLE() was an expression, now it's a block.
Right, it was changed by:
commit 3ab61473ba7f3dca32d779ec2766a4faa0657923
Author: Zachary Ware <zachary.ware@gmail.com>
Date: Thu Sep 12 13:35:48 2019 +0100
Enhance Py_UNREACHABLE macro (GH-16032)
I add Zachary Ware in the nosy list.
|
msg352735 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2019-09-18 15:56 |
Whoops :(. Fixing this is beyond my C skill. I'd rather not lose the easter egg, but if it can't be fixed properly then 3ab61473ba7f3dca32d779ec2766a4faa0657923 should just be reverted.
|
msg352757 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2019-09-18 21:41 |
While I don't like how that get_small_int macro is defined...
and I don't like that Py_UNREACHABLE() was usable as an expression in the past...
it is probably best to just revert 3ab61473ba7f3dca32d779ec2766a4faa0657923.
The choice to use a macro for this when it was created and the way the macro was defined means it is an expression forever unless we want to bother forcing code cleanups for uses of it.
|
msg352758 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2019-09-18 21:48 |
I have poked at this a bit and have an implementation that defines a `static inline void _Py_Unreachable()` in pymacro.h, see https://github.com/zware/cpython/commit/d07b4255dc1170155e18db0fea99ec1cb29c2f0a
This works on at least macOS and Windows, and we also have prior art for defining static inline functions in headers in GH-10079. If we agree that that's an acceptable solution, I'll open a PR for that change.
|
msg352763 - (view) |
Author: Ma Lin (malin) * |
Date: 2019-09-19 01:03 |
If use static inline function, and Py_UNREACHABLE() inside an if-else branch that should return a value, compiler may emit warning:
https://godbolt.org/z/YtcNSf
MSVC v19.14:
warning C4715: 'test': not all control paths return a value
clang 8.0.0:
warning: control may reach end of non-void function [-Wreturn-type]
Other compilers (gcc, icc) don't emit this warning.
This situation in real code:
https://github.com/python/cpython/blob/v3.8.0b4/Include/object.h#L600
https://github.com/python/cpython/blob/v3.8.0b4/Objects/longobject.c#L3088
|
msg352764 - (view) |
Author: Ma Lin (malin) * |
Date: 2019-09-19 01:11 |
PR 16270 use Py_UNREACHABLE() in a single line.
It solves this particular issue.
|
msg352765 - (view) |
Author: Ammar Askar (ammar2) * |
Date: 2019-09-19 01:34 |
For the control path warning, that can be fixed by just hinting to the compiler that the function doesn't return like so:
# ifdef __GNUC__
__attribute__ ((noreturn))
# elif defined(_MSC_VER)
__declspec(noreturn)
# endif
static inline void
_Py_UNREACHABLE()
{
(https://godbolt.org/z/AG9tx_)
|
msg352774 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-09-19 08:54 |
I like the idea of implementing Py_UNREACHABLE() as a function. Would it make sense to even declare it as a regular function rather than a static inline function? What is the benefit of inlining here? Inlining can make the code larger, rather than a function call at the machine code level is shorter.
|
msg352776 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-09-19 09:34 |
I wrote PR 16280: "Convert Py_UNREACHABLE() macro to a function". This change fix this issue but also enhance Py_UNREACHABLE() which now dumps the Python traceback where the bug occurs. See my example:
https://github.com/python/cpython/pull/16280#issuecomment-533049748
|
msg352780 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-09-19 09:48 |
I propose to restrict this issue to Py_UNREACHABLE() macro/function.
Please use bpo-37812 to discuss longobject.c.
|
msg352782 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2019-09-19 09:53 |
I prefer to keep it a macro. The compiler does not know that it is never executed, so it can generate a suboptimal code.
While it is a macro, it can be made a no-op, or even with compiler-specific instructions like __builtin_unreachable. This can help the compiler to generate more optimal code. For example, the popular idiom:
switch (kind) {
case PyUnicode_1BYTE_KIND: {
...
break;
}
case PyUnicode_2BYTE_KIND: {
...
break;
}
case PyUnicode_4BYTE_KIND: {
...
break;
}
default: Py_UNREACHABLE();
}
could be compiled to the code equivalent to:
if (kind == PyUnicode_1BYTE_KIND) {
...
break;
}
else if (kind == PyUnicode_2BYTE_KIND) {
...
break;
}
else { // assuming (kind == PyUnicode_4BYTE_KIND)
...
break;
}
|
msg352783 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-09-19 10:00 |
> While it is a macro, it can be made a no-op, or even with compiler-specific instructions like __builtin_unreachable.
My PR 16280 uses _Py_NO_RETURN which uses __attribute__((__noreturn__)) with GCC and clang.
I'm not sure how __builtin_unreachable could be used with Py_UNREACHABLE() macro.
> While it is a macro, it can be made a no-op
I understood that Py_UNREACHABLE() is used on purpose to prevent undefined behavior. For example, if a function accepts an enum, but is called with a value which is not part of the enum: what should happen? Should Python crash? Usually, we try to be nice and return an error. But sometimes, you cannot report an error and so Py_UNREACHABLE() is a good solution.
> I prefer to keep it a macro. The compiler does not know that it is never executed, so it can generate a suboptimal code.
I don't see how PR 16280 could have an effect on that. I don't see how the compiler can guess that the code is never executed with the current macro.
--
Using a function allows to put a breakpoint on it.
In fact, I can easily modify PR 16280 to keep the macro, since I only call Py_FatalError() with a string. The function body is simple.
|
msg352786 - (view) |
Author: Sergey Fedoseev (sir-sigurd) * |
Date: 2019-09-19 10:36 |
FWIW I proposed to add Py_ASSUME() macro that uses __builtin_unreachable() in bpo-38147.
|
msg352799 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2019-09-19 14:59 |
_Py_NO_RETURN is a promise that the code past the function is unreachable, not that the function call is unreachable.
> I'm not sure how __builtin_unreachable could be used with Py_UNREACHABLE() macro.
In the release mode Py_UNREACHABLE() can be replaced with __builtin_unreachable().
> For example, if a function accepts an enum, but is called with a value which is not part of the enum: what should happen? Should Python crash?
If the accessibility of the code depends on the call from Python code, we should raise normal exception. If it is not possible to pass it out of the function, we should use PyErr_WriteUnraisable().
If the accessibility of the code depends on the call from C code,
we can use PyErr_BadInternalCall() or raise a SystemError directly.
If we are in the situation where recovering from errors is not possible (for example if the memory manager is broken) we use Py_FatalError().
If we are absolutely sure that the code never can be executed (unless memory was corrupted or something like), we use Py_UNREACHABLE(). It will silence compiler complains, fail loudly in debug mode if our assumption is wrong (this is our programming bug), and allow the compiler to optimize out the code if substituted by __builtin_unreachable() in the release mode.
> I don't see how the compiler can guess that the code is never executed with the current macro.
But we can make the macro to expand to __builtin_unreachable in the release mode. Or just to no-op if there is other way to silence the compiler warning.
> Using a function allows to put a breakpoint on it.
You can put a brackpoint on Py_FatalError().
> In fact, I can easily modify PR 16280 to keep the macro, since I only call Py_FatalError() with a string.
It would be nice. We can consider using __builtin_unreachable() in different issue. I am also going to use Py_UNREACHABLE() more widely to write more uniform, but still optimal code. It could replace
if (cond1) { ...
} else if (cond2) { ...
} else if (cond3) { ...
} else {
assert (!cond4);
...
}
with
if (cond1) { ...
} else if (cond2) { ...
} else if (cond3) { ...
} else if (cond4) { ...
} else {
Py_UNREACHABLE();
}
|
msg352892 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-09-20 21:10 |
New changeset b1542583bee204130934c2b90684041e29378250 by Victor Stinner in branch 'master':
bpo-38205: Py_UNREACHABLE() calls Py_FatalError() (GH-16290)
https://github.com/python/cpython/commit/b1542583bee204130934c2b90684041e29378250
|
msg352898 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-09-20 21:36 |
New changeset 245d439320e0921459502883fe809213e3b93e35 by Victor Stinner in branch '3.8':
bpo-38205: Py_UNREACHABLE() calls Py_FatalError() (GH-16290) (GH-16306)
https://github.com/python/cpython/commit/245d439320e0921459502883fe809213e3b93e35
|
msg353028 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-09-23 14:58 |
Serhiy created bpo-38249: "Optimize out Py_UNREACHABLE in the release mode".
|
msg353659 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2019-10-01 10:28 |
The initial issue has been fixed in 3.8 and master. I close the issue.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:20 | admin | set | github: 82386 |
2019-10-01 10:28:14 | vstinner | set | status: open -> closed resolution: fixed messages:
+ msg353659
stage: patch review -> resolved |
2019-09-23 14:58:27 | vstinner | set | messages:
+ msg353028 |
2019-09-20 21:36:35 | vstinner | set | messages:
+ msg352898 |
2019-09-20 21:17:37 | vstinner | set | pull_requests:
+ pull_request15892 |
2019-09-20 21:10:19 | vstinner | set | messages:
+ msg352892 |
2019-09-19 16:25:52 | vstinner | set | pull_requests:
+ pull_request15875 |
2019-09-19 14:59:53 | serhiy.storchaka | set | messages:
+ msg352799 |
2019-09-19 10:36:41 | sir-sigurd | set | messages:
+ msg352786 |
2019-09-19 10:00:57 | vstinner | set | messages:
+ msg352783 |
2019-09-19 09:53:25 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg352782
|
2019-09-19 09:48:28 | vstinner | set | messages:
+ msg352780 |
2019-09-19 09:48:06 | vstinner | set | title: Python no longer compiles without small integer singletons -> Py_UNREACHABLE() no longer behaves as a function call |
2019-09-19 09:34:59 | vstinner | set | messages:
+ msg352776 |
2019-09-19 09:18:35 | vstinner | set | pull_requests:
+ pull_request15865 |
2019-09-19 08:54:51 | vstinner | set | messages:
+ msg352774 |
2019-09-19 01:34:35 | ammar2 | set | nosy:
+ ammar2 messages:
+ msg352765
|
2019-09-19 01:11:08 | malin | set | messages:
+ msg352764 |
2019-09-19 01:08:03 | malin | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request15860 |
2019-09-19 01:03:41 | malin | set | messages:
+ msg352763 |
2019-09-18 21:48:26 | zach.ware | set | messages:
+ msg352758 |
2019-09-18 21:41:46 | gregory.p.smith | set | messages:
+ msg352757 |
2019-09-18 15:56:32 | zach.ware | set | nosy:
+ gregory.p.smith messages:
+ msg352735
|
2019-09-18 07:41:13 | vstinner | set | nosy:
+ zach.ware messages:
+ msg352708
|
2019-09-18 06:54:17 | sir-sigurd | set | messages:
+ msg352703 |
2019-09-18 06:29:04 | sir-sigurd | set | nosy:
+ sir-sigurd messages:
+ msg352700
|
2019-09-18 00:24:01 | vstinner | set | messages:
+ msg352682 |
2019-09-18 00:21:30 | malin | set | messages:
+ msg352681 |
2019-09-18 00:03:52 | malin | set | nosy:
+ malin messages:
+ msg352680
|
2019-09-17 22:12:35 | vstinner | create | |