search

MatthieuDartiailh / bytecode

Python module to modify bytecode
https://bytecode.readthedocs.io/
MIT License
302 stars 38 forks source link

fix(cfg): ensure correct TryBegin management #147

Closed P403n1x87 closed 2 weeks ago

P403n1x87 commented 3 weeks ago

We make sure that the TryBegin blocks are managed correctly in CFGs

P403n1x87 commented 3 weeks ago

I'm not quite sure this is the right fix. If there is no current TryBegin block set but we have a TryEnd, then perhaps we should have a TryBegin too?

codecov-commenter commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.63%. Comparing base (c2d97b6) to head (15f9cf8).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #147 +/- ## ======================================= Coverage 95.63% 95.63% ======================================= Files 6 6 Lines 1971 1971 Branches 475 443 -32 ======================================= Hits 1885 1885 Misses 52 52 Partials 34 34 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

P403n1x87 commented 3 weeks ago

Framework tests for CPython 3.11 are passing on #143 with this fix https://github.com/MatthieuDartiailh/bytecode/actions/runs/11385325044/job/31674923987?pr=143

MatthieuDartiailh commented 3 weeks ago

This is a bit odd. Can you make a small test for this case. To me such a situation should not occur or may occur if the TryBegin itself is after the jump. A different fix could be to ignore a TryEnd if there is no known TryBegin.

P403n1x87 commented 3 weeks ago

This is a bit odd. Can you make a small test for this case. To me such a situation should not occur or may occur if the TryBegin itself is after the jump. A different fix could be to ignore a TryEnd if there is no known TryBegin.

My gut feeling is that this is the same as #145 whereby the lookup for the entry would fail because the TryBegin was not created yet for small try blocks. I'll see if I can craft a reproducer for this.

P403n1x87 commented 3 weeks ago

I haven't had time to craft a reproducer or do more experiments.

A different fix could be to ignore a TryEnd if there is no known TryBegin.

Perhaps one can skip get_trailing_try_end if there is no current TryBegin if that's what you're implying. Or perhaps one should ensure that get_trailing_try_end doesn't find a TryBegin before a TryEnd?

MatthieuDartiailh commented 3 weeks ago

My idea was to skip. Maybe it's worth trying to see what the framework test finds.

P403n1x87 commented 3 weeks ago

Skipping seems to work too https://github.com/MatthieuDartiailh/bytecode/actions/runs/11416651416/job/31768093575?pr=143

MatthieuDartiailh commented 3 weeks ago

Thanks for testing. Could you check that when we skip there is indeed a TryBegin after the end of the block ?

I feel like all of this means something is broken in the generation of those TryBegin/TryEnd and that we should find the source rather than just addressing the symptoms.

MatthieuDartiailh commented 2 weeks ago

Superseded by #149