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

Fix wrong line numbers in some cases #492

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

kuznet1
Copy link
Contributor

@kuznet1 kuznet1 commented Sep 28, 2024

Current logic allowed to make multiple entries in line number table for one pc. Usually it works fine as in most cases hashmap iterate by increasing key and only the last entry in line number table for each PC is taken into account. But when one line is <16, and second line is >= 16 (default hashmap capacity) then it will iterated in reverse order, so jvm reports wrong line and test fails. I swapped key & value of map in LineNumberAttributeBuilder, so for one pc can match only one lineNumber.

@chibash
Copy link
Member

chibash commented Sep 29, 2024

Can you add description?

@kuznet1
Copy link
Contributor Author

kuznet1 commented Sep 30, 2024

I added a description

@chibash
Copy link
Member

chibash commented Oct 1, 2024

Thanks. But does this patch really fix this?

it will iterated in reverse order

I don't think Map#entrySet preserves an order.

@kuznet1
Copy link
Contributor Author

kuznet1 commented Oct 1, 2024

Before this patch map was lineNum -> pc, now is pc -> lineNum (swap key & value), so for one pc can be only one lineNumber

@chibash
Copy link
Member

chibash commented Oct 1, 2024

Before this patch map was lineNum -> pc, now is pc -> lineNum (swap key & value), so for one pc can be only one lineNumber

I understood this. If so, can you revise the description (and title?) of this pull req? Furthermore, does the test case check this problem?

@kuznet1 kuznet1 changed the title Add test for 16th line & fix Fix wrong line numbers in some cases Oct 1, 2024
@kuznet1
Copy link
Contributor Author

kuznet1 commented Oct 1, 2024

Yes, of course. Without fix it reports that exception is thrown from 4th line

@chibash chibash merged commit 7013f30 into jboss-javassist:master Oct 3, 2024
2 checks passed
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.

2 participants