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 range for debug symbols of method parameters #1246

Merged
merged 1 commit into from Nov 8, 2021

Conversation

marchof
Copy link
Member

@marchof marchof commented Nov 7, 2021

By inserting the probe array access code at the beginning of the method
we accidently moved the range of the debug symbols of the method
parameters after the probe array access code. Actually the method
parameters are still valid from the very beginning of the method.

This offset in the method parameter's debug symbols may confuse tools
like Jandex.

@marchof marchof added type: bug 🐛 Something isn't working component: core labels Nov 7, 2021
@marchof marchof requested a review from Godin November 7, 2021 20:31
@marchof marchof self-assigned this Nov 7, 2021
By inserting the probe array access code at the beginning of the method
we accidently moved the range of the debug symbols of the method
parameters after the probe array access code. Actually the method
parameters are still valid from the very beginning of the method.

This offset in the method parameter's debug symbols may confuse tools
like Jandex.
@marchof
Copy link
Member Author

marchof commented Nov 8, 2021

@aloubyansky
Copy link

I confirm this change fixes the issue I originally reported. Great work @marchof!

@aloubyansky
Copy link

I verified it by running two tests:

  • https://github.com/aloubyansky/playground/tree/jacoco-jandex which loads a class (compiled w/o the -parameters arg) before JaCoCo instrumentation and reads method parameter names using Jandex API and then applies JaCoCo instrumentation to it and reads method parameter names again in the same way. This test fails with the 0.8.7 release and passes with this change;
  • I also upgraded Quarkus locally to use the 0.8.8-SNAPSHOT version built from this branch and ran the original reproducers provided by the Quarkus user. They worked as well.

@Godin
Copy link
Member

Godin commented Nov 8, 2021

@marchof nice fix 👍 and impressive that this was unnoticed for many years 😄

@aloubyansky thank you for reporting this and testing! ❤️

@Godin Godin merged commit 70d5b98 into master Nov 8, 2021
@Godin Godin deleted the method-parameter-debug-symbols branch November 8, 2021 16:08
@Godin Godin added this to Implementation in Current work items via automation Nov 8, 2021
@Godin Godin added this to the 0.8.8 milestone Nov 8, 2021
@Godin Godin moved this from Implementation to Done in Current work items Nov 8, 2021
eformat added a commit to petbattle/pet-battle-api that referenced this pull request Nov 15, 2021
eformat added a commit to rht-labs/pet-battle-api that referenced this pull request Nov 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: core type: bug 🐛 Something isn't working
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants