Skip to content
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.

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

gh-88226: Emit TARGET labels in Python/ceval.c when debugging, even if computed gotos aren't enabled #98265

Merged
merged 34 commits into from
Nov 22, 2022

Conversation

smontanaro
Copy link
Contributor

@smontanaro smontanaro commented Oct 14, 2022

This is an updated PR from the original one I submitted (in the before GitHub issues days).

Note: I am still acquainting myself with my new MacBook, so don't have quite the full suite of autoconf tools available. I was thus unable to generate configure from configure.ac. I manually editing configure instead.

@smontanaro smontanaro changed the title gh-25949: Emit TARGET labels in Python/ceval.c when debugging, even if computed gotos aren't enabled gh-88226: Emit TARGET labels in Python/ceval.c when debugging, even if computed gotos aren't enabled Oct 14, 2022
@smontanaro
Copy link
Contributor Author

I messed up the reference to the issue number when creating the NEWS blurb. I entered "25949" instead of "88226" (I think that's the right number). I don't know if the generated blurb can simply be git mv ...'d or not.

@smontanaro
Copy link
Contributor Author

I think I fixed this problem. I git rm'd the first blurb then ran blurb again.

@smontanaro
Copy link
Contributor Author

One other thing. I'm confident this does the right thing. I ran these steps to verify that no warnings were issued for unused labels when compiling Python/ceval.o:

#!/bin/bash

./configure --without-pydebug --with-computed-gotos 2>&1 | egrep -v '^checking'
echo "Computed goto, Py_DEBUG disabled"
rm -f Python/ceval.o
make Python/ceval.o

./configure --with-pydebug --with-computed-gotos 2>&1 | egrep -v '^checking'
echo "Computed goto, Py_DEBUG enabled"
rm -f Python/ceval.o
make Python/ceval.o

./configure --with-pydebug --without-computed-gotos 2>&1 | egrep -v '^checking'
echo "Computed goto disabled, Py_DEBUG enabled"
rm -f Python/ceval.o
make Python/ceval.o

./configure --without-pydebug --without-computed-gotos 2>&1 | egrep -v '^checking'
echo "Computed goto disabled, Py_DEBUG disabled"
rm -f Python/ceval.o
make Python/ceval.o

@smontanaro
Copy link
Contributor Author

Clang seems happy with the change as well.

@smontanaro
Copy link
Contributor Author

This still spits out warnings when compiling Python/ceval.c on Windows. I haven't the slightest idea how to control for that, but I assume it must be possible. If you know how this is done, can you make a suggestion?

@serhiy-storchaka
Copy link
Member

Disabling the warnings globally looks not good to me. It can lead to preserving unused labels in other parts of the code. Why not disable them locally, using compiler-specific pragmas? This is how we handle other types of warnings.

@smontanaro
Copy link
Contributor Author

smontanaro commented Nov 3, 2022 via email

@smontanaro
Copy link
Contributor Author

Update... Thanks to @zooba and @mattip, the latest variation performs a more surgical suppression of the unused label warning. configure is no longer affected, and the TARGET_* labels are always defined, not just when Py_DEBUG is enabled.

Python/ceval.c Outdated Show resolved Hide resolved
Python/ceval.c Outdated Show resolved Hide resolved
smontanaro and others added 2 commits November 3, 2022 15:32
configure.ac Outdated Show resolved Hide resolved
@smontanaro
Copy link
Contributor Author

@serhiy-storchaka Can you merge this when you have a moment? Thx...

@serhiy-storchaka
Copy link
Member

@smontanaro Aren't you a coredev? Don't you have the ability to do that yourself? I just don't promise that I will do it quickly, because we have regular blackouts here (5 minutes until the next blackout). But I will do it in 2 hours or tomorrow.

@smontanaro
Copy link
Contributor Author

@serhiy-storchaka No worries and no rush, just as long as it's on your radar screen. Whenever you have a chance. I gave up my commit privilege a few years ago (don't really want it back).

I wasn't aware you are in Ukraine. Stay well.

@serhiy-storchaka serhiy-storchaka merged commit d4cf192 into python:main Nov 22, 2022
@serhiy-storchaka
Copy link
Member

Done. Don't forget to remove the branch in your GitHub repository and in your local copy if you no longer need it.

@smontanaro smontanaro deleted the gh-25949 branch November 22, 2022 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants