[GR-34179] Upgrade CE debug info feature to provide information about Java types for Windows/PECOFF #3732

stooke · 2021-08-27T11:36:12Z

This PR adds type information to the CodeView 4 debug information (See PR #2396). It is related to issue #2986, but is targeted to Windows and Visual Studio.

This PR is based off (and highly dependant on) the work in PR #3046.

With this patch, memory can be examined from the Visual Studio or WinDbg command line and cast to Java types as laid out by the Graal compiler. Static member variables can be referenced directly.

Caveats:

Pointer types for instance members in the presence of isolates are not yet implemented, as there appears to be no way to express a pointer that is an offset from r14 in CodeView. I will continue to research this, but at this time, this implementation works best if isolates are disabled.
As there is no local variable information added by this PR, users must manually cast registers to pointers; for example (java_lang_String*)(rdx).

I recommend @adinn as a first reviewer for this PR, as it is based on his debug type PR.

olpaw · 2021-08-31T14:23:37Z

@adinn looks like github does not allow me to put you on this one as reviewer 😞

adinn · 2021-08-31T15:21:40Z

@olpaw No worry. I'll review it anyway :-)

adinn

This looks ok to me modulo the 3 inline comments I made. Not that I can really comment on the intricacies of the PeCOFF records and their detailed layout.

I believe this is going to need to be modified slightly when rebased over head in order to take account of the changes made to support inline info. Specifically PrimaryEntry.getSubranges() is still being called from CVLinerecordBuilder. That has been dropped in head in favour of calling leafRangeIterator(). I don't think there us anything else needs changing though.

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVDebugInfo.java

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeRecord.java

...tevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeSectionBuilder.java

adinn · 2021-09-14T13:24:10Z

I believe this is going to need to be modified slightly when rebased over head in order to take account of the changes made to support inline info. Specifically PrimaryEntry.getSubranges() is still being called from CVLinerecordBuilder. That has been dropped in head in favour of calling leafRangeIterator(). I don't think there us anything else needs changing though.

@stooke The above tweak turns out to be unneeded. The upstream code was updated to use the correct iterator when the gdb inline methods patch went in. Once this PR is rebased it all fits together without any conflict. So, it appears to be ready for Oracle review and merging.

@pejovica Could you please review this and help get it through the Oracle gate?

pejovica · 2021-09-16T08:47:48Z

@adinn Sure, I should be able to start with that after next week.

adinn · 2021-10-04T11:47:57Z

@pejovica This PR is still waiting to be reviewed. will you be able to do that soon or do we need to find someone else to review it?

olpaw

Hi @stooke
Great to see progress on the Windows side of CE debuginfo generation!

Please consolidate the commits. E.g. combine subsequent style related commits into one (or even better, fold them into the commit that is the origin of the style issue). Also start commit messages with uppercase. A more thorough review from @pejovica will follow.

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVDebugInfo.java

...tratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubrecord.java

...src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubsectionBuilder.java

olpaw · 2021-10-04T15:57:18Z

Please consolidate the commits. E.g. combine subsequent style related commits into one (or even better, fold them into the commit that is the origin of the style issue). Also start commit messages with uppercase.

stooke · 2021-10-04T16:09:48Z

Please consolidate the commits. E.g. combine subsequent style related commits into one (or even better, fold them into the commit that is the origin of the style issue). Also start commit messages with uppercase.

Hello @olpaw , and thank you for your review! I've started to address your concerns, and squashed many of the commits, changing the messages to start with uppercase. Please let me know if I should squash further.

olpaw · 2021-10-04T16:28:44Z

I've started to address your concerns, and squashed many of the commits, changing the messages to start with uppercase. Please let me know if I should squash further.

Looks much better now. Thanks!

adinn · 2021-10-07T14:30:08Z

@olpaw is this still waiting on a review from @pejovica or is it ok for it to be pushed through the gate?

olpaw · 2021-10-07T14:36:05Z

@olpaw is this still waiting on a review from @pejovica or is it ok for it to be pushed through the gate?

@pejovica is currently reviewing the PR (and testing it thoroughly on his Windows machine). Since the PR is quite big this might take some more time. Sorry for the inconvenience.

adinn · 2021-10-07T14:53:14Z

@olpaw Sure, thanks for the update and no inconvenience or problem about @pejovica taking his time. I'm very glad to hear he is giving it a proper workout.

pejovica

@stooke Thanks for working on this!

Overall, the PR looks good, but there are a few more things to do. Apart from what is stated in the comments, my main concern is the design of CVTypeSectionBuilder. In particular, I think there is no need to replace forwarding records with complete records on the fly as it only makes things harder to reason about.

Instead, I suggest establishing a few invariants that we can easily check:

Type records should only use forwarding records for cross-referencing.
Symbol records should only use complete records for cross-referencing (e.g., for S_UDT records).

This would mean that CVTypeSectionBuilder.buildClass will still make a complete record, but while doing so it will only be able to reference forwarding records. As a result, there will be no need to track whether the construction of a complete record is in progress -- it will no longer matter as only forwarding records will be referenced, and we can make them as needed.

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/PECoffObjectFile.java

...tratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubrecord.java

...src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubsectionBuilder.java

pejovica · 2021-10-19T17:40:35Z

...src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubsectionBuilder.java

+            String externName = memberNameToCodeViewName(classEntry, f);
+            if (cvDebugInfo.useHeapBase()) {
+                /* REL32 offset from heap base register. */
+                addToSymbolSubsection(new CVSymbolSubrecord.CVSymbolRegRel32Record(cvDebugInfo, externName, typeIndex, f.getOffset(), cvDebugInfo.getHeapbaseRegister()));


Well, I don't think this can work... So I'd suggest that we drop this for now and document that we only support debugging with -H:-SpawnIsolates.

Also note that the register must be specified using the appropriate CV constant, such as CV_AMD64_R14.

I propose we definitely say we don't support debugging with isolates (should we emit a warning?), but I'd like to keep this code in; using pointer arithmetic (and a notepad) it is possible to make your way around even with isolates.

Fixed the register constants properly.

...tratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubrecord.java

...tevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeSectionBuilder.java

stooke · 2022-07-26T20:21:49Z

Feel free to ping me here or on Slack if any public gates need to be restarted. If you add new commits, I need to sync them with the internal PR and that means I need to re-run all internal gates, too.

Sorry, I just added a dummy commit to restart the public gates; should I squash that and push again? Next time I'll ping you instead.

fniephaus · 2022-07-26T20:27:47Z

It's all good... just wanted you to be aware that I need to sync all commits and that there are other ways to restart gates. :)

stooke · 2022-07-26T20:29:38Z

It's all good... just wanted you to be aware that I need to sync all commits and that there are other ways to restart gates. :)

Duly noted. :) Should I squash my dummy commit? The PR is good ether way.

fniephaus · 2022-07-26T20:31:24Z

Good! :) No need to do anything, I'll just drop the dummy commit so I don't need to restart the internal gates.

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeRecord.java

pejovica · 2022-08-01T18:03:38Z

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeRecord.java

+            /* Add in size of record type and length fields. */
+            return estimatedSize + Short.BYTES * 2;


Well, this is not entirely true. The record size does not include the length field, only the type field. However, we really do lose 4 bytes due to the alignment requirement.

I think this could be better expressed using availableSize (instead of estimatedSize) as the initial value can be clearly defined

int availableSize = MAX_LF_FIELDLIST_RECORD_SIZE - CVUtil.align4(Short.BYTES) - CVUtil.align4(LF_INDEX_RECORD_SIZE);

and the rest of the code would only require a slight adjustment.

@pejovica, I've refactored this to correct the calculation and alignment calculations, but I haven't moved to using 'availableSize' as I think the concept of limits is best kept to the FieldListBuilder.
If this is a showstopper, please let me know and we can discuss it.

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeRecord.java

...tevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeSectionBuilder.java

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeRecord.java

stooke · 2022-08-18T16:04:29Z

@pejovica, thank you again for your comments. I have posted a set of changes to address them (but haven't adopted 'availableSize' for reasons given above).

pejovica

@stooke I left you a few more comments, but other than that the PR looks good.

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVNames.java

...tratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubrecord.java

...src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubsectionBuilder.java

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeRecord.java

...tevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeSectionBuilder.java

...tratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeSectionImpl.java

stooke · 2022-10-26T19:49:08Z

@pejovica thank you for your review, and I'm sorry circumstances forced me to take this long to get back to this PR. I've incorporated your suggestions. Should I rebase on head?

pejovica · 2022-11-03T12:34:02Z

@stooke No worries, glad you're back!

Should I rebase on head?

Yes, that would be great.

stooke · 2022-11-07T21:36:41Z

@stooke No worries, glad you're back!

Should I rebase on head?

Yes, that would be great.

@pejovica , I've rebased the PR. Thank you for your patience.

...tevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeSectionBuilder.java

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeRecord.java

...src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubsectionBuilder.java

pejovica

@stooke One last thing, can you please squash everything into one commit before I proceed with the integration.

stooke · 2022-12-12T14:32:10Z

@stooke One last thing, can you please squash everything into one commit before I proceed with the integration.

@pejovica I have pushed the PR as a single commit.

pejovica · 2022-12-13T11:17:32Z

Excellent, thanks!

graalvmbot added the oca-signed label Aug 27, 2021

oubidar-Abderrahim self-assigned this Aug 31, 2021

olpaw added this to To do in Native Image via automation Aug 31, 2021

olpaw assigned olpaw and unassigned oubidar-Abderrahim Aug 31, 2021

olpaw requested a review from pejovica August 31, 2021 14:24

olpaw assigned pejovica Aug 31, 2021

adinn approved these changes Sep 10, 2021

View reviewed changes

olpaw self-requested a review October 4, 2021 12:00

olpaw requested changes Oct 4, 2021

View reviewed changes

Native Image automation moved this from To do to In progress Oct 4, 2021

olpaw reviewed Oct 4, 2021

View reviewed changes

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVDebugInfo.java Show resolved Hide resolved

olpaw reviewed Oct 4, 2021

View reviewed changes

...tratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubrecord.java Outdated Show resolved Hide resolved

olpaw reviewed Oct 4, 2021

View reviewed changes

...tratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubrecord.java Show resolved Hide resolved

olpaw reviewed Oct 4, 2021

View reviewed changes

...src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubsectionBuilder.java Outdated Show resolved Hide resolved

stooke force-pushed the pr-windows-typeinfo branch from f435b9a to e24f4e9 Compare October 4, 2021 15:28

stooke force-pushed the pr-windows-typeinfo branch from 2eb268c to 4096fab Compare October 4, 2021 16:06

olpaw approved these changes Oct 4, 2021

View reviewed changes

pejovica requested changes Nov 1, 2021

View reviewed changes

stooke force-pushed the pr-windows-typeinfo branch from 3431d8c to ba6b72d Compare August 9, 2022 18:38

pejovica reviewed Aug 11, 2022

View reviewed changes

stooke force-pushed the pr-windows-typeinfo branch from 2138991 to 9aa5b5b Compare August 18, 2022 14:31

pejovica reviewed Aug 30, 2022

View reviewed changes

jerboaa mentioned this pull request Sep 29, 2022

Planned Upgrades to Debug Info Support #5047

Open

stooke force-pushed the pr-windows-typeinfo branch from d70d1ac to 7c10644 Compare November 7, 2022 19:03

pejovica reviewed Dec 8, 2022

View reviewed changes

...tevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeSectionBuilder.java Outdated Show resolved Hide resolved

pejovica reviewed Dec 9, 2022

View reviewed changes

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVTypeRecord.java Outdated Show resolved Hide resolved

...src/com.oracle.objectfile/src/com/oracle/objectfile/pecoff/cv/CVSymbolSubsectionBuilder.java Outdated Show resolved Hide resolved

pejovica approved these changes Dec 9, 2022

View reviewed changes

stooke force-pushed the pr-windows-typeinfo branch from 65c365f to 5f289af Compare December 9, 2022 17:42

Implement PR oracle#3732 Java type information for Windows

77e3a74

stooke force-pushed the pr-windows-typeinfo branch from a51643a to 77e3a74 Compare December 12, 2022 14:31

pejovica changed the title ~~[GR-39210] Upgrade CE debug info feature to provide information about Java types for Windows/PECOFF~~ [GR-34179] Upgrade CE debug info feature to provide information about Java types for Windows/PECOFF Dec 13, 2022

graalvmbot merged commit 77e3a74 into oracle:master Dec 16, 2022

Native Image automation moved this from In progress to Done Dec 16, 2022

Native Image Debugging automation moved this from In progress to Done Dec 16, 2022

stooke mentioned this pull request Dec 16, 2022

Windows Debug Information for local variables. #5635

Open

sophie-kaleba pushed a commit to sophie-kaleba/truffle that referenced this pull request Jan 16, 2023

Implement PR oracle#3732 Java type information for Windows

621965c

zakkak added native-image-debuginfo redhat-interest labels Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GR-34179] Upgrade CE debug info feature to provide information about Java types for Windows/PECOFF #3732

[GR-34179] Upgrade CE debug info feature to provide information about Java types for Windows/PECOFF #3732

stooke commented Aug 27, 2021 •

edited

olpaw commented Aug 31, 2021

adinn commented Aug 31, 2021

adinn left a comment

adinn commented Sep 14, 2021

pejovica commented Sep 16, 2021

adinn commented Oct 4, 2021

olpaw left a comment •

edited

olpaw commented Oct 4, 2021

stooke commented Oct 4, 2021 •

edited by olpaw

olpaw commented Oct 4, 2021

adinn commented Oct 7, 2021

olpaw commented Oct 7, 2021 •

edited

adinn commented Oct 7, 2021

pejovica left a comment

pejovica Oct 19, 2021

stooke Nov 1, 2021

stooke Nov 15, 2021

stooke commented Jul 26, 2022

fniephaus commented Jul 26, 2022

stooke commented Jul 26, 2022

fniephaus commented Jul 26, 2022

pejovica Aug 1, 2022

stooke Aug 16, 2022

stooke commented Aug 18, 2022

pejovica left a comment

stooke commented Oct 26, 2022

pejovica commented Nov 3, 2022

stooke commented Nov 7, 2022

pejovica left a comment

stooke commented Dec 12, 2022

pejovica commented Dec 13, 2022

		/* Add in size of record type and length fields. */
		return estimatedSize + Short.BYTES * 2;

[GR-34179] Upgrade CE debug info feature to provide information about Java types for Windows/PECOFF #3732

[GR-34179] Upgrade CE debug info feature to provide information about Java types for Windows/PECOFF #3732

Conversation

stooke commented Aug 27, 2021 • edited

olpaw commented Aug 31, 2021

adinn commented Aug 31, 2021

adinn left a comment

Choose a reason for hiding this comment

adinn commented Sep 14, 2021

pejovica commented Sep 16, 2021

adinn commented Oct 4, 2021

olpaw left a comment • edited

Choose a reason for hiding this comment

olpaw commented Oct 4, 2021

stooke commented Oct 4, 2021 • edited by olpaw

olpaw commented Oct 4, 2021

adinn commented Oct 7, 2021

olpaw commented Oct 7, 2021 • edited

adinn commented Oct 7, 2021

pejovica left a comment

Choose a reason for hiding this comment

pejovica Oct 19, 2021

Choose a reason for hiding this comment

stooke Nov 1, 2021

Choose a reason for hiding this comment

stooke Nov 15, 2021

Choose a reason for hiding this comment

stooke commented Jul 26, 2022

fniephaus commented Jul 26, 2022

stooke commented Jul 26, 2022

fniephaus commented Jul 26, 2022

pejovica Aug 1, 2022

Choose a reason for hiding this comment

stooke Aug 16, 2022

Choose a reason for hiding this comment

stooke commented Aug 18, 2022

pejovica left a comment

Choose a reason for hiding this comment

stooke commented Oct 26, 2022

pejovica commented Nov 3, 2022

stooke commented Nov 7, 2022

pejovica left a comment

Choose a reason for hiding this comment

stooke commented Dec 12, 2022

pejovica commented Dec 13, 2022

stooke commented Aug 27, 2021 •

edited

olpaw left a comment •

edited

stooke commented Oct 4, 2021 •

edited by olpaw

olpaw commented Oct 7, 2021 •

edited