py/nlr: In MP_NLR_JUMP_HEAD make top and top_ptr volatile. #5008
Conversation
Can you elaborate? I know better than to make any confident sounding assertion about the behavior of Anyway, the real issue here is that GCC doesn't understand that the following Either add: before the Or, as that page suggests, instead add |
The question is where to put the volatile's, what else needs to be volatile? It may be that a future compiler finds another subtle optimisation that the code didn't guard against and it breaks again. Because the NLR mechanism is not strict C (except with the setjmp/longjmp implementation) It's very compiler dependent. See 34c04d2 for another case of compiler dependence with NLR. Also grep for volatile in py/vm.c. Completely removing NLR is a full, robust solution to all these issues, although perhaps overkill. It could be that the right use of volatile is enough here. |
dpgeorge
force-pushed
the
py-nlr-volatile-top-ptr
branch
from
e07f118
to
f432bbf
Compare
And use it to replace the same pattern at the end of nlrthumb.c:nlr_jump.
Recent versions of gcc perform optimisations which can lead to the
following code from the MP_NLR_JUMP_HEAD macro being omitted:
top->ret_val = val; \
MP_NLR_RESTORE_PYSTACK(top); \
*_top_ptr = top->prev; \
This is noticeable (at least) in the unix coverage on x86-64 built with gcc
9.1.0. This is because the nlr_jump function is marked as no-return, so
gcc deduces that the above code has no effect.
Adding MP_UNREACHABLE tells the compiler that the asm code may branch
elsewhere, and so it cannot optimise away the code.
dpgeorge
force-pushed
the
py-nlr-volatile-top-ptr
branch
from
f432bbf
to
11ecdf2
Compare
|
Ok, the builtins_unreachable is a nice solution that works well. So I made that change, see 11ecdf2 |
The unix coverage port currently crashes with the toolchain and set up that I'm using (gcc 9.1.0 on Arch Linux x86-64). This is due to gcc doing further optimisations which leads to the following code being omitted (ie no corresponding machine code generated) in
MP_NLR_JUMP_HEAD:This is because the nlr_jump function is marked as no-return, so gcc deduces that the above code has no effect (rightfully so, I guess).
I'm not very happy with the solution presented here, rather it's more for discussion how to resolve the problem properly (and as an workaround if necessary).
There could be other variables that need to be declared volatile (eg to do with PYSTACK), and other platforms (non x86-64) may have different behaviour and require other fixes.
This really points to the importance of #4131, removal of NLR completely.