-
Notifications
You must be signed in to change notification settings - Fork 81
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
Reevaluate debug sequence points after shortening jumps #3412
Reevaluate debug sequence points after shortening jumps #3412
Conversation
question: should debug points be removed if they were replaced with NOP, panic or keep code as is? |
Likely we'll just have several points at the same offset and then hi, #2428. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you, please, squash the commits and add signoff-by to the commit message?
The fix itself looks legit, let me test it a bit.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3412 +/- ##
==========================================
+ Coverage 86.10% 86.17% +0.06%
==========================================
Files 331 331
Lines 38107 38114 +7
==========================================
+ Hits 32813 32845 +32
+ Misses 3780 3751 -29
- Partials 1514 1518 +4 ☔ View full report in Codecov by Sentry. |
e55268c
to
cce2b8e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please, add compiler:
prefix to the commit message. Like the following: compiler: reevaluate debug sequence points after shortening jumps
. Ref. https://github.com/nspcc-dev/.github/blob/master/git.md#commit-messages.
Signed-off-by: Slava0135 <[email protected]>
cce2b8e
to
9b688a9
Compare
@roman-khimov, agree to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to be a nice fix.
Problem
Debug sequence points offsets were not updated after removing NOP during jump shortening
Solution
Count NOPs before seqpoint and offset opcodes on this amount
Added tests to check that reevaluation was correct