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.

Sign up for GitHub

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

Jump to bottom

[GR-38651] Add -H:StripDebugInfo and other tweaks for Debug Info. #5691

Merged
merged 10 commits into from
Jan 30, 2023
Merged

[GR-38651] Add -H:StripDebugInfo and other tweaks for Debug Info. #5691

merged 10 commits into from
Jan 30, 2023

Conversation

graalvmbot
Copy link
Collaborator

No description provided.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jan 3, 2023
@fniephaus fniephaus requested a review from adinn January 3, 2023 14:00
@fniephaus fniephaus self-assigned this Jan 3, 2023
@graalvmbot graalvmbot force-pushed the fniephaus/GR-38651/ce-debuginfo branch from ab58edb to e82c7dd Compare January 5, 2023 13:33
adinn
adinn reviewed Jan 9, 2023
View reviewed changes
...vm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageBFDNameProvider.java
@@ -510,6 +521,16 @@ private void mangleType(ResolvedJavaType type) {
}
}

private void mangleType(UnresolvedJavaType type) {
if (type.isArray()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't have a class loader for this unresolved type so I am not sure that we can guarantee that its name will not clash with a name for some valid resolved type. It might be safer to put all unresolved types into their own namespace to ensure this does not happen. That would also involve modifying the debug type info generation code to adopt the same namespace convention.

I'll leave that change for a follow-up PR once we can identify what other changes are required to ensure that debug info generator does not break if an unresolved type pops up as an element of the declaration closure for the runtime type set.

adinn
adinn approved these changes Jan 9, 2023
View reviewed changes
Copy link
Collaborator

@adinn adinn left a comment

Choose a reason for hiding this comment

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

Looks ok to push. I think we will need to do more work to ensure that unresolved types don't cause any grief during debug info generation.

@fniephaus fniephaus changed the title [GR-38651] -H:StripDebugInfo and other tweaks for Debug Info. [GR-38651] Add -H:StripDebugInfo and other tweaks for Debug Info. Jan 9, 2023
@graalvmbot graalvmbot force-pushed the fniephaus/GR-38651/ce-debuginfo branch from c0ef533 to 15799f3 Compare January 9, 2023 15:31
adinn
adinn approved these changes Jan 9, 2023
View reviewed changes
Copy link
Collaborator

@adinn adinn left a comment

Choose a reason for hiding this comment

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

New changes still looking good

@graalvmbot graalvmbot force-pushed the fniephaus/GR-38651/ce-debuginfo branch from 15799f3 to 995db62 Compare January 16, 2023 10:52
@olpaw olpaw requested a review from christianhaeubl January 16, 2023 11:43
@olpaw
Copy link
Member

olpaw commented Jan 16, 2023

@christianhaeubl to switch our debuginfo implementation to the new improved one by RedHat @fniephaus found an issue when it is used in combination with G1 GC. Commit 058563c contains our attempt to fix the problem. Please let us know if this looks legit to you. The code is question is creating dwarf expressions (small dwarf sub-programms that get interpreted by gdb to resolve oop references) based on the object representation for the image that gets built.

@christianhaeubl
Copy link
Member

@olpaw thanks, looks good to me.

@graalvmbot graalvmbot force-pushed the fniephaus/GR-38651/ce-debuginfo branch 2 times, most recently from 41f2371 to bd50635 Compare January 26, 2023 09:54
@graalvmbot graalvmbot force-pushed the fniephaus/GR-38651/ce-debuginfo branch 2 times, most recently from 66d1486 to 6f85813 Compare January 26, 2023 18:45
@graalvmbot graalvmbot force-pushed the fniephaus/GR-38651/ce-debuginfo branch from 6f85813 to 4b240bd Compare January 27, 2023 08:35
@graalvmbot graalvmbot force-pushed the fniephaus/GR-38651/ce-debuginfo branch from 4b240bd to fc40db3 Compare January 27, 2023 13:15
@graalvmbot graalvmbot merged commit 7cdf79a into master Jan 30, 2023
@graalvmbot graalvmbot deleted the fniephaus/GR-38651/ce-debuginfo branch January 30, 2023 16:38
@fniephaus fniephaus added this to the 23.0.0 Release milestone Feb 1, 2023
This was referenced Feb 17, 2023
Closed
Closed
@zakkak zakkak mentioned this pull request Mar 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fniephaus fniephaus fniephaus left review comments

@adinn adinn adinn approved these changes

@zakkak zakkak zakkak requested changes

@olpaw olpaw olpaw approved these changes

@christianhaeubl christianhaeubl Awaiting requested review from christianhaeubl

Assignees

@fniephaus fniephaus

Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Milestone
GraalVM for JDK 17 / JDK 20 (June 13,...
Development

Successfully merging this pull request may close these issues.
6 participants
@graalvmbot @olpaw @christianhaeubl @adinn @zakkak @fniephaus