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

8263790: C2: new igv_print_immediately() for debugging purpose #3071

Closed

Conversation

@kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented Mar 18, 2021

Add a new igv_print_immediately, it prints the current method immediately. This differs from other igv_print* methods, it creates a well-formed and complete ideal graph xml immediately, while others accomplish their ideal graph xml when VM exists (i.e. destructor of IdealGraphPrinter::_xx_printer). If VM crashes before VM exit, the ideal graph xml will be ill-formed, this is fairly a common case when debugging another crash.

Test manually!

Cheers,
Yang


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263790: C2: new igv_print_immediately() for debugging purpose

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3071/head:pull/3071
$ git checkout pull/3071

To update a local copy of the PR:
$ git checkout pull/3071
$ git pull https://git.openjdk.java.net/jdk pull/3071/head

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 18, 2021

👋 Welcome back yyang! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the rfr label Mar 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 18, 2021

@kelthuzadx The following label will be automatically applied to this pull request:

  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 18, 2021

Webrevs

Loading

@chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Mar 18, 2021

I'm not sure if I understand this correctly. Is it just about having an ill-formatted xml file or are you actually having problems opening the graph in the IGV? I've just played around with calling igv_print() in gdb from a few places but could open the incomplete xml files in the IGV and have a look at the graphs. Are you seeing this when calling igv_print() from certain breakpoints while debugging?

If it's the IGV that has problems opening it, could you maybe provide more information how to reproduce this and/or share the xml file that causes problems?

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Mar 18, 2021

I'm not sure if I understand this correctly. Is it just about having an ill-formatted xml file or are you actually having problems opening the graph in the IGV? I've just played around with calling igv_print() in gdb from a few places but could open the incomplete xml files in the IGV and have a look at the graphs. Are you seeing this when calling igv_print() from certain breakpoints while debugging?

If it's the IGV that has problems opening it, could you maybe provide more information how to reproduce this and/or share the xml file that causes problems?

Let me add more information. When encountered a breakpoint, I call igv_print() from debugger, it produces a custom_debug.xml:

$head -10 custom_debug.xml  && echo '---' && tail -10 custom_debug.xml
<graphDocument>
<group>
<properties>
<p name='name'>
virtual void java.lang.Object.&lt;init&gt;()</p>
<p name='public'>
true</p>
</properties>
<graph name='Debug'>
<nodes>
---
<edge from='1' to='11' index='1'/>
<edge from='7' to='11' index='2'/>
<edge from='1' to='11' index='3'/>
<edge from='3' to='6' index='0'/>
<edge from='3' to='5' index='0'/>
<edge from='3' to='3' index='0'/>
<edge from='0' to='3' index='1'/>
<edge from='0' to='1' index='0'/>
</edges>
</graph>

There is a lack of closed tags(</graphDocument> and </group>) at the end of the xml. These tags will be generated only when the VM exits normally, but the problem is that we sometimes cannot exit the VM normally during debugging (for example, when we are debugging another problem, which will crash the VM after breakpoint.), so if the VM exits abnormally, we get a problematic xml, it can not be opened by idealgraphvisualizer due to the following reason:
image
image

When I add missing tags(</graphDocument> and </group>) manually, this file can be opened by idealgraphvisualizer correctly. So I think we can add igv_print_immediately to print a complete ideal graph XML as soon as it is called.

Loading

@chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Mar 18, 2021

Okay, that is strange that the closing tags are the only problem. I cannot reproduce this. What version of IGV are you using? Looking at your filters in the screenshot, I assume you are not using one of the most recent versions (there were some fixes/improvements recently done by @robcasloz including new default filters). You could try to build the latest version of IGV and try it again. Could you also share the failing custom_debug.xml file in the JBS issue?

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Mar 19, 2021

Sure, I have uploaded custom_debug.xml in JBS issue. I don't find an option like '-version' for idealgraphvisualizer. But from GUI->Preference->About, it shows Version 1.0 (1.0).

Loading

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Mar 19, 2021

Please try the latest version from https://github.com/openjdk/jdk/tree/master/src/utils/IdealGraphVisualizer (you might need Java 8 to build).

Loading

@chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Mar 19, 2021

Sure, I have uploaded custom_debug.xml in JBS issue. I don't find an option like '-version' for idealgraphvisualizer. But from GUI->Preference->About, it shows Version 1.0 (1.0).

Thanks! I tried it out with a recently built version and I could open it without any problems in the IGV.

Loading

@robcasloz
Copy link
Contributor

@robcasloz robcasloz commented Mar 19, 2021

Hi @kelthuzadx, I cannot reproduce the problem running the latest IGV on JDK 8 and linux-x64 either.

Note that IGV is designed to tolerate XML files without closing </graphDocument> and </group> tags. The parser does not use an error handler, which leads to ignoring all parsing errors [1], and on top of that certain parsing exceptions are ignored:

try {
XMLReader reader = createReader();
reader.setContentHandler(new XMLParser(xmlDocument, monitor));
reader.parse(new InputSource(Channels.newInputStream(channel)));
} catch (SAXException ex) {
if (!(ex instanceof SAXParseException) || !"XML document structures must start and end within the same entity.".equals(ex.getMessage())) {
throw new IOException(ex);
}
}

[1] https://docs.oracle.com/javase/8/docs/api/org/xml/sax/XMLReader.html#setErrorHandler-org.xml.sax.ErrorHandler-

Perhaps the problem you report should be addressed by making sure IGV also accepts missing closing </graphDocument> and </group> tags in your case? As you mention, this is a common debugging scenario.

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Mar 19, 2021

Perhaps the problem you report should be addressed by making sure IGV also accepts missing closing and tags in your case?

Yes, absolutely.

Thank you all for pointing out this. It seems that my IGV is too old to parse incomplete xml documents:-( . I will update it to let it work.

The original intention of this patch is to generate complete xml, but it seems that even incomplete xml can be opened by the latest IGV, so this patch seems to be of little significance(Maybe the only use is to make the old version IGV happy).

Thanks!
Yang

Loading

@chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Mar 19, 2021

By not adding the closing tags to the xml file after igv_print(), subsequent calls can immediately append a graph to the current method which makes it easier to look at them in the IGV. I think that is wanted most of the time. If not then we need something like in your patch. However, if I see this correctly, the Debug.xml file in your patch would be overridden each time calling igv_print_immediately(), only allowing to have one graph at a time which is probably not desired. But either way, I'm not sure if it is worth offering such flexibility.

I will update it to let it work.

Sounds good.

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Apr 11, 2021

The latest IGV can not open incomplete XML as well. When I changed the system locale to en-US, it works for incomplete XML as expected. I guess this is yet another an i18n problem. As @robcasloz pointed out, the parser does not use an error handler, which leads to ignoring all parsing errors, and on top of that certain parsing exceptions are ignored

 try { 
     XMLReader reader = createReader(); 
     reader.setContentHandler(new XMLParser(xmlDocument, monitor)); 
     reader.parse(new InputSource(Channels.newInputStream(channel))); 
 } catch (SAXException ex) { 
     if (!(ex instanceof SAXParseException) || !"XML document structures must start and end within the same entity.".equals(ex.getMessage())) { 
         throw new IOException(ex); 
     } 
 } 

In the catch clause, ex.getMessage() compares with ASCII characters, but ex.getMessage()gets characters that corresponding to their system locale settings. To support non-English system locale settings(if needed), we could code something like this:

     if (!(ex instanceof SAXParseException) || !"XML document structures must start and end within the same entity.".equals(disable_i18n(ex.getMessage())))

fig1
FIG2

Loading

@robcasloz
Copy link
Contributor

@robcasloz robcasloz commented Apr 12, 2021

In the catch clause, ex.getMessage() compares with ASCII characters, but ex.getMessage()gets characters that corresponding to their system locale settings. To support non-English system locale settings(if needed), we could code something like this:

     if (!(ex instanceof SAXParseException) || !"XML document structures must start and end within the same entity.".equals(disable_i18n(ex.getMessage())))

That makes sense, can you please try if this fix works for you? robcasloz@702e763

If it does, please feel free to re-purpose this issue with the proposed fix (once #3361 is integrated), and with a test case that parses an incomplete XML document in src/utils/IdealGraphVisualizer/Data/src/test/java/com/sun/hotspot/igv/data/serialization/ParserTest.java. Please make sure to test in on different JDK versions (I suggest JDK 8, 11, and 15).

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Apr 13, 2021

In the catch clause, ex.getMessage() compares with ASCII characters, but ex.getMessage()gets characters that corresponding to their system locale settings. To support non-English system locale settings(if needed), we could code something like this:

     if (!(ex instanceof SAXParseException) || !"XML document structures must start and end within the same entity.".equals(disable_i18n(ex.getMessage())))

That makes sense, can you please try if this fix works for you? robcasloz@702e763

If it does, please feel free to re-purpose this issue with the proposed fix (once #3361 is integrated), and with a test case that parses an incomplete XML document in src/utils/IdealGraphVisualizer/Data/src/test/java/com/sun/hotspot/igv/data/serialization/ParserTest.java. Please make sure to test in on different JDK versions (I suggest JDK 8, 11, and 15).

Thank you @robcasloz, I can verify it on the weekend. In order to not disturb others, I'd like to close this PR and I have filed this as https://bugs.openjdk.java.net/browse/JDK-8265106.

Loading

@kelthuzadx kelthuzadx closed this Apr 13, 2021
@kelthuzadx kelthuzadx deleted the new_igv_print_immediately branch Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants