7439: Introduced IConstantPoolExtension for Constant Pools #333

jpbempel · 2021-10-29T14:28:24Z

Add extension methods when constants are read, referenced, resolved, constant pools fully resolved and parsing finished to be able for example:

track constant pool usage inside events
replace or translate symbols, method names, etc... like
de-obfuscations of stack traces

Progress

Commit message must refer to an issue
Change must be properly reviewed

Issue

JMC-7439: Introduced IConstantPoolExtension for Constant Pools

Reviewers

Marcus Hirt (@thegreystone - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jmc pull/333/head:pull/333
$ git checkout pull/333

Update a local copy of the PR:
$ git checkout pull/333
$ git pull https://git.openjdk.java.net/jmc pull/333/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 333

View PR using the GUI difftool:
$ git pr show -t 333

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jmc/pull/333.diff

bridgekeeper · 2021-10-29T14:28:31Z

👋 Welcome back jpbempel! 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.

mlbridge · 2021-10-29T14:32:29Z

Webrevs

Add extension methods when constants are read, referenced, resolved and constant pools fully resolved to be able for example: * track constant pool usage inside events * replace or translate symbols, method names, etc... like de-obfuscations of stack traces

thegreystone · 2021-10-29T18:51:49Z

...jmc.flightrecorder/src/main/java/org/openjdk/jmc/flightrecorder/parser/IParserExtension.java

+	 * 
+	 * @return an instance implementing IConstantPoolExtension
+	 */
+	default IConstantPoolExtension createConstantPoolExtension() {


Perhaps we might as well make the other methods default too?

default String getValueInterpretation(String eventTypeId, String fieldId) { return null; } default IEventSinkFactory getEventSinkFactory(IEventSinkFactory subFactory) { return subFactory; }

docwarems · 2021-10-30T09:43:12Z

Hi, I wanted to test this PR but are still unsure with Eclipse setup/usage. I managed to check out the PR in my fork (what a GitHub Desktop magic :-) ). The project builds and I can run JMC.
For testing the PR I wanted to start with the ConstantPoolExtensionTest Unittest and debug it. It's the first time I tried to run a JMC unittest fro Eclipse. As used to from IntelliJ IDE I tried "run as unittest" from the class context menu. This created a JUnit launch config.
But the launcher fails with a basic ClassNotFoundException for this this test class. So obviously Eclipse failed to automatically create a working launch config.

Can you help me with this basic setup question? And sorry for beeing OT here. I can later delete this message again.

thegreystone · 2021-10-30T14:45:17Z

Normally I just have separate clones and workspaces for various people that contribute, e.g.:

Directory of C:\Users\Marcus\git\jmcreviews
2021-10-30 16:26

.
2021-10-30 16:26 ..
2021-09-14 10:52 cimi
2020-11-13 22:19 guru
2021-03-17 22:51 jpbempel
2021-01-12 17:31 miro
2020-12-07 23:57 Suchitainf
and so on... (Yep, I'm on my Windows machine right now.)

I then copy my eclipse workspace and simply remove all the projects and re-import them, so that I have separate Eclipse workspaces for various people that I need to review, and separate clones to try their various PRs.

For running the tests:
cd core
mvn verify

To run the test from within Eclipse, I simply right clicked on the class and selected Run as JUnit Test:

This gave me:

docwarems · 2021-10-30T15:22:38Z

To run the test from within Eclipse, I simply right clicked on the class and selected Run as JUnit Test:
image

So it's just as simple as expected and I as I did too. But I get

This is how the automatically generated Launch Config looks like

So I will skip the test...

docwarems · 2021-10-30T17:17:11Z

Rather than starting with the unit test, I added the MyConstantPoolExtension from to test to the FlightRecordingLoader.loadStream() (only for testing of course). I added breakpoints to all methods of the MyConstantPoolExtension class and loaded a recording (recorded from our company developed app). All methods were executed a lot times as expected. I expected to see my own class or packages names sooner or later, but I only saw JDK internal names. So I probably didn't got the idea. Can you help?

I pushed the change to my fork FYI.
docwarems@c9669a7

thegreystone · 2021-10-31T14:14:22Z

I would avoid cherry picking changes to other forks when reviewing. Your commit in your fork shows one changed file with 80 additions and 1 deletion. This PR has 8 changed files.

It is much easier to simply clone JPs fork, and switch to the branch with his changes.

docwarems · 2021-11-08T19:20:15Z

I haven't CPed anything. Look at my commit history - it contains the changes from jpbempel - even if I did it differently than you propsed.
But OK, I just spent a another 2hrs to set up a another Eclipse project with jpbempels fork. The usual problems with the setup - I'll had to repeat project importing and platform reloading a lot of times until all dependencies where available. And Maven internal error from Eclipse all the time. It's a pain.
I'll continue tests within the next days.

jpbempel · 2021-11-09T08:20:48Z

FYI, I am reworking the PR to match better with my needs and the dependencies

- getId - eventsLoaded - getItemCollection to expose data gathered and exposed in IParserStats all ConstantPool extensions

docwarems · 2021-11-13T11:07:26Z

Back to the tests - this time with the JPs clone and branch "pool-extension". I realized that in my Eclipse setup the ConstantPoolExtensionTest class doesn't get compiled unless I do a

cd core
mvn package
on console. That's why I obiously got the ClassNotFoundException previously. I realized that when I finally found the "Show command line" option with the class path.
Now I can run the test but have to do "mvn package" always if I want to make changes, which is obviously very clumsy.
Sorry that I report more about my Eclipse setup problems here than regarding the PR...

Regarding the PR, I have the same test result as last time. I just can't seenany of my own classes in the Parsewr Extension callbacks when I replaced the test JFR file with my own. As I said, I probably didn't got the idea.

I would suggest that you provide a test JFR file with a custom class like fr.jpbempel.HelloWorld and demonstrate in the test how to get it.

jpbempel · 2021-11-13T20:58:38Z

hello @docwarems
My test is focusing on event Types that referencing constants.

when using the callback:

Object constantRead(long constantIndex, Object constant, String eventTypeId) {
	return constant;
}

you should look at constant argument that should be what you would like to de-obfuscate and return a translated value instead of the original one:

Object constantRead(long constantIndex, Object constant, String eventTypeId) {
	return deobfuscate(constant);
}

thegreystone · 2021-11-19T18:22:37Z

...ightrecorder/src/main/java/org/openjdk/jmc/flightrecorder/internal/parser/LoaderContext.java


 import org.openjdk.jmc.common.collection.FastAccessNumberMap;
 import org.openjdk.jmc.common.item.IAttribute;
 import org.openjdk.jmc.common.item.IItem;
+import org.openjdk.jmc.common.unit.ContentType;


Not needed, right?

thegreystone · 2021-11-19T18:25:34Z

...flightrecorder/src/main/java/org/openjdk/jmc/flightrecorder/internal/parser/ParserStats.java

@@ -39,6 +39,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;


Using Maps, not Sets - not needed.

docwarems · 2021-11-20T13:08:28Z

My test is focusing on event Types that referencing constants.

Yeah, that's more or less what I expected. Maybe I'm unable to find my constant of interest in the mass of other classes. Could you provide a breakpoint condition for the breakpoint condition shown below which would stop at a class "fr.jpbempel.HelloWorld"?

jpbempel · 2021-11-21T21:41:34Z

Hey, @docwarems

I have created a custom event:

package com.bempel.events;

import jdk.jfr.Event;
import jdk.jfr.Label;
import jdk.jfr.Name;

@Name("com.bempel.events.MyEvent")
@Label("MyEvent")
public class MyEvent extends Event {
	@Label("message")
	public String message;
}

I have used like this:

MyEvent myEvent = new MyEvent();
myEvent.message = "showVetList " + uuid;
myEvent.commit();

I have recording with this event in it:

You can look at constant pools :

in java.lang.Class

but also in jdk.Types.Symbol:

in debugging ConstantPoolExtensionTest:

But it's a little bit complex because:
a class constant is not having directly the name of the class but reference another symbol constant for that
see:
https://github.com/openjdk/jmc/blob/master/core/org.openjdk.jmc.flightrecorder/src/main/java/org/openjdk/jmc/flightrecorder/internal/parser/v1/StructTypes.java#L277-L287

That's why it may have more sense to look for the symbol constant instead of directly the class constant.

docwarems · 2021-11-22T20:15:08Z

Hello @jpbempel, the breakpoint condition was the breakthrough - I just wasn't able to catch the desired constant before. Even with the simplest HelloWork console program I ended up with a JFR file where I wasn't able to step through all constants in a reasonable time and find my desired constant.

Next I should be able to take my deobfuscator remapping code and build a true extension. I'll come back then. Regarding the PR review, I wonder if you want to wait for my further work, or if you prefer to merge.

jpbempel · 2021-11-22T21:19:42Z

It would be nice to have your feedback on this, because I came back a lot on the design to implement my needs. So depending on yours, we may revisit some parts...

thegreystone · 2021-11-26T13:04:16Z

One last nit for this PR - can you please check the files for unnecessary imports? I'll add an issue for ensuring that this is done automatically.

docwarems · 2021-11-27T09:23:31Z

@jpbempel I will not find enough time to continue before my holidays starting in mid December. It's better than one hour this day, one hour another day.

openjdk · 2021-11-29T12:00:15Z

@jpbempel This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

7439: Introduced IConstantPoolExtension for Constant Pools

Reviewed-by: hirt

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 24 new commits pushed to the master branch:

4d1c087: 7247: Alternate for Twitter4J to support twitter plug-in
3ce6483: 7447: Add serializers and writer modules to core test coverage
35c8e0e: 7446: Add Maven profile for enabling test coverage
24f4cb0: 5904: Merge org.openjdk.jmc.ui.celleditors with org.openjdk.jmc.rjmx.ui.celleditors
d218205: 7440: JMC fails on Apple M1
77580fe: 7418: Stop using Collections sort method
ef36b79: 7437: Fix require bundle error in Eclipse
1d17e0c: 7435: Update the maven enforcer plugin
cf4ceb3: 7434: Update the maven download plugin
9bb5799: 7433: Update the spotless maven plugin
... and 14 more: https://git.openjdk.java.net/jmc/compare/2844db9fab2164b1c6b675466bbd5a5e805e9560...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

thegreystone · 2021-12-01T09:23:21Z

Hi @jpbempel - you can now integrate this PR. :)

jpbempel · 2021-12-01T09:54:39Z

/integrate

openjdk · 2021-12-01T09:56:13Z

Going to push as commit d4449ce.
Since your change was applied there have been 25 commits pushed to the master branch:

ca20442: 7198: Add websocket server that pushes data on selection updates
4d1c087: 7247: Alternate for Twitter4J to support twitter plug-in
3ce6483: 7447: Add serializers and writer modules to core test coverage
35c8e0e: 7446: Add Maven profile for enabling test coverage
24f4cb0: 5904: Merge org.openjdk.jmc.ui.celleditors with org.openjdk.jmc.rjmx.ui.celleditors
d218205: 7440: JMC fails on Apple M1
77580fe: 7418: Stop using Collections sort method
ef36b79: 7437: Fix require bundle error in Eclipse
1d17e0c: 7435: Update the maven enforcer plugin
cf4ceb3: 7434: Update the maven download plugin
... and 15 more: https://git.openjdk.java.net/jmc/compare/2844db9fab2164b1c6b675466bbd5a5e805e9560...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-12-01T09:56:38Z

@jpbempel Pushed as commit d4449ce.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

docwarems · 2021-12-15T16:00:24Z

The PR is merged, but I've resumed my tests and are in the middle of adapting my de-obfuscation code to the new extension mechanism.
Question: How would you load such an extension in real life? By means of a plugin JAR? There are a lot of plugin JARs in the plugin folder of the build JMC, but I couldn't find a starting point for development. Could you point me to such a resource?

docwarems · 2021-12-17T09:51:44Z

@jpbempel as assumed doing the deobfuscation using the IConstantPoolExtension is much more complicated than as I did it previously because it modifies internal state. I'm facing follow-up exceptions and have been trying for a while to sort them out. But I am clueless now.
I overwrote constantRead() and look for constants with instanceof String. Then I look up these Strings in my obfuscation mapping and if I get a hit, I replace the strings or the relevant substring with the deobfuscated string and return it. But this seem to create wrong internal state. I'm logging to console original and replaced strings and what Is see look fine to me according to my understanding.

Do you have any more hint? Otherwise I would push my current code to my fork and post the link here. If you can help I will continue, otherwise I'll have to stay with my existing solution doing the deobfuscation on formatting level, which is just fine for our purpose.

docwarems · 2021-12-20T18:04:58Z

@jpbempel in this branch https://github.com/docwarems/jmc/tree/PR-333-Deobfuscate you'll find a demonstration of my current problem. The README describes how to run the demo. I will/can not further investigate unless you come with a hint what I'm doing wrong.

jpbempel · 2021-12-29T21:23:43Z

is your stacktrace for the error similar to:

java.lang.StringIndexOutOfBoundsException: String index out of range: -11
        at java.base/java.lang.String.substring(String.java:1841)
        at org.openjdk.jmc.flightrecorder.internal.parser.v1.StructTypes$JfrJavaClass.convertNames(StructTypes.java:331)
        at org.openjdk.jmc.flightrecorder.internal.parser.v1.StructTypes$JfrJavaClass.getFullName(StructTypes.java:315)
        at org.openjdk.jmc.flightrecorder.internal.parser.v1.StructTypes$JfrJavaClass.hashCode(StructTypes.java:341)
        at java.base/java.util.Objects.hashCode(Objects.java:116)
        at org.openjdk.jmc.flightrecorder.internal.parser.v1.StructTypes$JfrMethod.hashCode(StructTypes.java:625)
        at org.openjdk.jmc.flightrecorder.stacktrace.graph.AggregatableFrame.hashCode(AggregatableFrame.java:90)
        at java.base/java.util.HashMap.hash(HashMap.java:339)
        at java.base/java.util.HashMap.get(HashMap.java:552)

?
in that case it's because you have changed the class name but not the package name associated.
Then when referencing a class or a method, package name & class name are not the same, which is not expected.

in your example to fix the issue:

    private static Deobfuscator createFooDeobfuscator() {
    	Map<String, String> obfuscatorClassMap = new HashMap<String, String>();  // obfuscated class -> class
    	obfuscatorClassMap.put("se/hirt/jmc/tutorial/hotmethods/HotMethods", "de/docware/ms/foo/Foo");
		Map<String, String> obfuscatorPackageMap = new HashMap<String, String>();  // obfuscated package -> package
+++             obfuscatorPackageMap.put("se/hirt/jmc/tutorial/hotmethods", "de/docware/ms/foo");
		Map<String, Map<String, String>> obfuscatorClassMethodsMap = new HashMap<String, Map<String, String>>();  // (class, obfuscated method) -> method
    	return new Deobfuscator(obfuscatorClassMap, obfuscatorPackageMap, obfuscatorClassMethodsMap);
    }

docwarems · 2022-01-01T11:22:50Z

@jpbempel A happy new to year to you and everyone of the JMC team :-)

Yes, that's very obvious, once I replaced the package too, the error was gone and I could see the replacement effect inside JMC.
I should now be able to complete my code. I will provide a new PR for review in a while.

Thanks a lot!

docwarems · 2022-01-02T17:53:35Z

Back to my real deobfuscation mapping, I found that of course I DID handle the package remapping there, but using real Proguard obfuscation mapping the situation is more complex (or real) than the demo I provided.

When I debugged one of these StringIndexOutOfBoundsExceptions, I have this situation in StructTypes.convertNames()

name=de.docware.apps.etk.base.db.cache.EtkDbsCacheElems, getPackageName()=de.docware.apps.etk.base.db.AbstractRevisionChangeSet

And these are the relevant entries from the Proguard mapping file.

de.docware.apps.etk.base.db.cache.EtkDbsCacheElems -> de.docware.apps.etk.base.d.a.e:
de.docware.apps.etk.base.db.AbstractRevisionChangeSet -> de.docware.apps.etk.base.d.a:

So "de.docware.apps.etk.base.d.a" refers both to a class and a package name.

When in constantRead() the String constant ist de-obfuscated to a clear name I wonder if it's possible to tell whether the constant refers to a class or a package name. At least I don't see a difference in the debugger. The first map which gets a hit returns the deobfuscated string, which in my code is the class map. That's why getPackageName() delivers the class name rather than the package.

Is there a possibility in constantRead() to tell what kind of object (class, package, ...) the constant refers to?

openjdk bot added the rfr label Oct 29, 2021

thegreystone added the hacktoberfest-accepted label Oct 29, 2021

jpbempel force-pushed the pool-extension branch from 1ef4e5b to d2d44de Compare October 29, 2021 16:43

jpbempel changed the title ~~7439: Add methods in IParserExtension for Constant Pools~~ 7439: Introduced IConstantPoolExtension for Constant Pools Oct 29, 2021

thegreystone reviewed Oct 29, 2021

View reviewed changes

add default for existing IParserExtension methods

ff79807

added new extension methods

33732c9

- getId - eventsLoaded - getItemCollection to expose data gathered and exposed in IParserStats all ConstantPool extensions

jpbempel requested a review from thegreystone November 10, 2021 07:50

formatting

47c9b72

thegreystone reviewed Nov 19, 2021

View reviewed changes

remove unused import

5edf67d

thegreystone approved these changes Nov 29, 2021

View reviewed changes

openjdk bot added the ready label Nov 29, 2021

openjdk bot closed this Dec 1, 2021

openjdk bot added integrated and removed ready rfr labels Dec 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7439: Introduced IConstantPoolExtension for Constant Pools #333

7439: Introduced IConstantPoolExtension for Constant Pools #333

jpbempel commented Oct 29, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 29, 2021

mlbridge bot commented Oct 29, 2021 •

edited

Loading

thegreystone Oct 29, 2021

docwarems commented Oct 30, 2021

thegreystone commented Oct 30, 2021

docwarems commented Oct 30, 2021

docwarems commented Oct 30, 2021 •

edited

Loading

thegreystone commented Oct 31, 2021 •

edited

Loading

docwarems commented Nov 8, 2021

jpbempel commented Nov 9, 2021

docwarems commented Nov 13, 2021

jpbempel commented Nov 13, 2021

thegreystone Nov 19, 2021

thegreystone Nov 19, 2021

docwarems commented Nov 20, 2021

jpbempel commented Nov 21, 2021 •

edited

Loading

docwarems commented Nov 22, 2021

jpbempel commented Nov 22, 2021

thegreystone commented Nov 26, 2021 •

edited

Loading

docwarems commented Nov 27, 2021

openjdk bot commented Nov 29, 2021

thegreystone commented Dec 1, 2021 •

edited

Loading

jpbempel commented Dec 1, 2021

openjdk bot commented Dec 1, 2021

openjdk bot commented Dec 1, 2021

docwarems commented Dec 15, 2021

docwarems commented Dec 17, 2021

docwarems commented Dec 20, 2021 •

edited

Loading

jpbempel commented Dec 29, 2021

docwarems commented Jan 1, 2022

docwarems commented Jan 2, 2022 •

edited

Loading

7439: Introduced IConstantPoolExtension for Constant Pools #333

7439: Introduced IConstantPoolExtension for Constant Pools #333

Conversation

jpbempel commented Oct 29, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 29, 2021

mlbridge bot commented Oct 29, 2021 • edited Loading

Webrevs

thegreystone Oct 29, 2021

Choose a reason for hiding this comment

docwarems commented Oct 30, 2021

thegreystone commented Oct 30, 2021

docwarems commented Oct 30, 2021

docwarems commented Oct 30, 2021 • edited Loading

thegreystone commented Oct 31, 2021 • edited Loading

docwarems commented Nov 8, 2021

jpbempel commented Nov 9, 2021

docwarems commented Nov 13, 2021

jpbempel commented Nov 13, 2021

thegreystone Nov 19, 2021

Choose a reason for hiding this comment

thegreystone Nov 19, 2021

Choose a reason for hiding this comment

docwarems commented Nov 20, 2021

jpbempel commented Nov 21, 2021 • edited Loading

docwarems commented Nov 22, 2021

jpbempel commented Nov 22, 2021

thegreystone commented Nov 26, 2021 • edited Loading

docwarems commented Nov 27, 2021

openjdk bot commented Nov 29, 2021

thegreystone commented Dec 1, 2021 • edited Loading

jpbempel commented Dec 1, 2021

openjdk bot commented Dec 1, 2021

openjdk bot commented Dec 1, 2021

docwarems commented Dec 15, 2021

docwarems commented Dec 17, 2021

docwarems commented Dec 20, 2021 • edited Loading

jpbempel commented Dec 29, 2021

docwarems commented Jan 1, 2022

docwarems commented Jan 2, 2022 • edited Loading

jpbempel commented Oct 29, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 29, 2021 •

edited

Loading

docwarems commented Oct 30, 2021 •

edited

Loading

thegreystone commented Oct 31, 2021 •

edited

Loading

jpbempel commented Nov 21, 2021 •

edited

Loading

thegreystone commented Nov 26, 2021 •

edited

Loading

thegreystone commented Dec 1, 2021 •

edited

Loading

docwarems commented Dec 20, 2021 •

edited

Loading

docwarems commented Jan 2, 2022 •

edited

Loading