Skip to content

[3.10] gh-91924: Fix __ltrace__ for non-UTF-8 stdout encoding #93214

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

Merged
merged 2 commits into from
May 25, 2022
Merged

[3.10] gh-91924: Fix __ltrace__ for non-UTF-8 stdout encoding #93214

merged 2 commits into from
May 25, 2022

Conversation

vstinner
Copy link
Member

Fix ltrace debug feature if the stdout encoding is not UTF-8.

If the stdout encoding is not UTF-8, the first call to
lltrace_resume_frame() indirectly sets lltrace to 0 when calling
unicode_check_encoding_errors() which calls
encodings.search_function().

Add test_lltrace.test_lltrace() test.

Fix __ltrace__ debug feature if the stdout encoding is not UTF-8.

If the stdout encoding is not UTF-8, the first call to
lltrace_resume_frame() indirectly sets lltrace to 0 when calling
unicode_check_encoding_errors() which calls
encodings.search_function().

Add test_lltrace.test_lltrace() test.
@vstinner
Copy link
Member Author

@sweeneyde @serhiy-storchaka: Would you mind to review this fix for Python 3.10? The fix is different and I adapted the test from the main branch for 3.10.

Without the fix but with the new test, LC_ALL=en_US.iso88591 ./python -m test test_lltrace fails. With the fix, the test pass.

@sweeneyde
Copy link
Member

I'm not sure why Windows CI is failing, the tests pass on my Windows machine.

Maybe it has something to do with not calling fflush(stdout) at the end of prtrace? Or maybe it's an encoding issue?

@vstinner
Copy link
Member Author

I'm not sure why Windows CI is failing, the tests pass on my Windows machine.

In the 3.10 branch, the Windows CI builds Python in release mode. I fixed the test.

@vstinner
Copy link
Member Author

@sweeneyde: the CI tests now pass.

Copy link
Member

@sweeneyde sweeneyde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. The local variable is nicer, but I agree that minimizing changes (EXT_POP and call_function) is more important.

@vstinner
Copy link
Member Author

The local variable is nicer, but I agree that minimizing changes (EXT_POP and call_function) is more important.

Adding an argument to call_function() can make the performance worse, and the code more complicated depending if LLTRACE macro is defined or not. I really want to minimize the risk of regression in 3.10 stable branch. __ltrace__ is a niche debug tool, it's mostly used by core developers to hack Python.

@vstinner vstinner merged commit 9369942 into python:3.10 May 25, 2022
@vstinner vstinner deleted the fix_lltrace_310 branch May 25, 2022 22:16
@vstinner
Copy link
Member Author

I hesitated to fix __ltrace__ typo: add a second L, but Python 3.10.0 was released last year, I prefer to not change it in a minor 3.10.x release. It's also documented at: https://docs.python.org/3.10/using/configure.html#python-debug-build

@vstinner
Copy link
Member Author

It reminds me the weird typo of float.__set_format__(): it was previously known as float.__setformat__() in Python 3.7. I removed the method in Python 3.11.

@vstinner
Copy link
Member Author

Oh, for the float method, I backported my change fixing the typo: #31558

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.

3 participants