judovana / java-runtime-decompiler

GNU General Public License v3.0
68 stars 14 forks source link

CPLC: new dependency resolution model leads to bootstrap problems #168

Closed mkoncek closed 3 years ago

mkoncek commented 3 years ago

Since we decided to get rid of classpath listing and switch to obtaining imported types from the class file itself, we did not realize that this leads to a chicken-egg problem, i. e. we need to already have a .class file compiled (with all of its nested classes) in order to compile the source code. This may not be a huge problem for our use case but it something to think about.

Moreover I believe that if the user adds an import during the editation inside JRD, the current approach will only resolve imports that are already present in the last version of the class.

1) I believe this is quite a problem. Maybe we cannot get rid of the classpath listing after all.

2) Related to import resolution from the class files I have also noticed that the algorithms is actually a bit more complex than we thought: a. for each source file find all its nested classes b. for each source file + all the nested classes find all imported types c. if any of the type is an inner class, find its outermost class and import all nested classes of that outer class

You see there is a lot of class providing with no relation to the source contents themselves but to whatever you are currently holding as the compiled class already.

mkoncek commented 3 years ago

On the other hand, the limitations of retransform do not allow manipulating the inner classes too much? Especially removing them? Would need to check this.

judovana commented 3 years ago

Moreover I believe that if the user adds an import during the editation inside JRD, the current approach will only resolve imports that are already present in the last version of the class.

Nope. This was actually first thing I tried. And it works fine. The preloading from class is only making life easier. All is resolved by javac at the end.

judovana commented 3 years ago

However, if my staement is wrong, then the easiest thing whcih ccan be done, is to append the generated lst from bytecode with original CP listing....

mkoncek commented 3 years ago

That shouldn't have worked unless you used some java system libraries. I just tried to import a non-system package as well and it didn't work. (On the trunk CPLC, which doesn't use classpath listing.)

judovana commented 3 years ago

Moreover I believe that if the user adds an import during the editation inside JRD, the current approach will only resolve imports that are already present in the last version of the class.

Nope. This was actually first thing I tried. And it works fine. The preloading from class is only making life easier. All is resolved by javac at the end.

Ok. You are right.I had to check against older then final implementation. This is showstopper. solution a) append original listing solution b) iterative compilation, trying to init classes on demand and refreshing listing.

mkoncek commented 3 years ago

I believe we could read imports from the source file. The hard thing then is how to distinguish implicit imports of java.lang against classes of the same package. Or even nested classes which are not imported and so on... Isn't it possible for you to transitively init every class + nested classes at the beginning? I believe we either get glasspath listing to work properly or introduce several hacks to resolve imports, with most time spent on implicit imports.

mkoncek commented 3 years ago

Or maybe, we I can do it iteratively, collecting compiler errors and trying pull java.lang.Whatever / package.Whaterver from you and expose that.

mkoncek commented 3 years ago

I believe we could read imports from the source file. The hard thing then is how to distinguish implicit imports of java.lang against classes of the same package. Or even nested classes which are not imported and so on... Isn't it possible for you to transitively init every class + nested classes at the beginning? I believe we either get glasspath listing to work properly or introduce several hacks to resolve imports, with most time spent on implicit imports.

Reading imports won't work on Procyon.

judovana commented 3 years ago

For quick fix, just apend original listing

-- Mgr. Jiri Vanek @.***

---------- Původní e-mail ---------- Od: mkoncek @.> Komu: pmikova/java-runtime-decompiler @. github.com> Datum: 6. 10. 2021 10:02:05 Předmět: Re: [pmikova/java-runtime-decompiler] CPLC: new dependency resolution model leads to bootstrap problem (#168) "

I believe we could read imports from the source file. The hard thing then is how to distinguish implicit imports of java.lang against classes of the same package. Or even nested classes which are not imported and so on... Isn't it possible for you to transitively init every class + nested classes at the beginning? I believe we either get glasspath listing to work properly or introduce several hacks to resolve imports, with most time spent on implicit imports.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub (https://github.com/pmikova/java-runtime-decompiler/issues/168#issuecomment-935699218) , or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAWFCS7N4U2POI3CG6SGOATUFP67LANCNFSM5FM43ICQ) . Triage notifications on the go with GitHub Mobile for iOS (https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or Android (https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub) . "

judovana commented 3 years ago

To read imports form source is not good idea. To many issues. Leavce this work on javac

-- Mgr. Jiri Vanek @.***

---------- Původní e-mail ---------- Od: mkoncek @.> Komu: pmikova/java-runtime-decompiler @. github.com> Datum: 6. 10. 2021 10:02:05 Předmět: Re: [pmikova/java-runtime-decompiler] CPLC: new dependency resolution model leads to bootstrap problem (#168) "

I believe we could read imports from the source file. The hard thing then is how to distinguish implicit imports of java.lang against classes of the same package. Or even nested classes which are not imported and so on... Isn't it possible for you to transitively init every class + nested classes at the beginning? I believe we either get glasspath listing to work properly or introduce several hacks to resolve imports, with most time spent on implicit imports.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub (https://github.com/pmikova/java-runtime-decompiler/issues/168#issuecomment-935699218) , or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAWFCS7N4U2POI3CG6SGOATUFP67LANCNFSM5FM43ICQ) . Triage notifications on the go with GitHub Mobile for iOS (https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or Android (https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub) . "

judovana commented 3 years ago

Yes. Iterative complation was original idea. But was canceled, ecause javac compalins "can not found name" instead of FQN.

-- Mgr. Jiri Vanek @.***

---------- Původní e-mail ---------- Od: mkoncek @.> Komu: pmikova/java-runtime-decompiler @. github.com> Datum: 6. 10. 2021 10:10:02 Předmět: Re: [pmikova/java-runtime-decompiler] CPLC: new dependency resolution model leads to bootstrap problem (#168) "

Or maybe, we I can do it iteratively, collecting compiler errors and trying pull java.lang.Whatever / package.Whaterver from you and expose that.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub (https://github.com/pmikova/java-runtime-decompiler/issues/168#issuecomment-935710621) , or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAWFCS6JAYVDMYRUCJPFKZTUFP73PANCNFSM5FM43ICQ) . Triage notifications on the go with GitHub Mobile for iOS (https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or Android (https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub) . "

judovana commented 3 years ago

not only there. It is really bad idea.

-- Mgr. Jiri Vanek @.***

---------- Původní e-mail ---------- Od: mkoncek @.> Komu: pmikova/java-runtime-decompiler @. github.com> Datum: 6. 10. 2021 10:12:55 Předmět: Re: [pmikova/java-runtime-decompiler] CPLC: new dependency resolution model leads to bootstrap problem (#168) "

" I believe we could read imports from the source file. The hard thing then is how to distinguish implicit imports of java.lang against classes of the same package. Or even nested classes which are not imported and so on... Isn't it possible for you to transitively init every class + nested classes at the beginning? I believe we either get glasspath listing to work properly or introduce several hacks to resolve imports, with most time spent on implicit imports. " Reading imports won't work on Procyon.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub (https://github.com/pmikova/java-runtime-decompiler/issues/168#issuecomment-935715934) , or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAWFCS7QDUIRVHTIINTML3LUFQAIDANCNFSM5FM43ICQ) . Triage notifications on the go with GitHub Mobile for iOS (https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or Android (https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub) . "

mkoncek commented 3 years ago

Important question is: will this work when the user imports a class that contains not yet initialized inner classes?

judovana commented 3 years ago

Sure not. But shoudl we care? For example the agent.api I initialize in premain, so it will be available. If you declare something on your own - thats why there is --init and init button on gui.

-- Mgr. Jiri Vanek @.***

---------- Původní e-mail ---------- Od: mkoncek @.> Komu: pmikova/java-runtime-decompiler @. github.com> Datum: 6. 10. 2021 10:42:26 Předmět: Re: [pmikova/java-runtime-decompiler] CPLC: new dependency resolution model leads to bootstrap problem (#168) "

Important question is: will this work when the user imports a class that contains not yet initialized inner classes?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub (https://github.com/pmikova/java-runtime-decompiler/issues/168#issuecomment-935761486) , or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAWFCSZWO4ZYLM3ONMISXQDUFQDWVANCNFSM5FM43ICQ) . Triage notifications on the go with GitHub Mobile for iOS (https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or Android (https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub) . "

mkoncek commented 3 years ago

So, let's summarize the strategy. As i said, i believe the imports of the previous .class file corresponding to the current source file are largely irrelevant and we should not care about them. Imagine changing the method bodies to use a same-named class from a different package. You are already initializing the nested classes before you decompile them so CPLC should see the whole class with its inner classes. If i simply only use classpath listing as previously, what can go wrong? Would things like @Override still not be present in the listing without being explicitly requested? Do you want ot not want the iterative approach (import from compile errors)? I noticed that if there is an import statement in the sources, the compiler emits 2 reports, one of them containing that package name.

judovana commented 3 years ago

The previous - from bytecode - imports meter. I initialize only inner classes. You have to initialize ALL original deps. Not only inners. (where I THINK yuo already really read all deps, so we are done here? if you use only CPlisting, more classes will be missed. issues with override-like cases were there, are there and will be there, unless you include iterative compilation.

Why are you refusing to concatenate your generated listing (with initialize) +current CP listing? That is solving 99%

conclusion:

The iterative compilation have hacky approach in reading of error messages. I would definitley kept it as optional only. so there is no confusion for suer - compile - init - relist - compile ...is not suepr expected

However iterative compilation may remove last cornercases.

mkoncek commented 3 years ago

You are right that re-adding cplisting will probably work most of the time and it solves the example issue i pointed out here. Now i don't know what to do more? Implement optional lazy reading from error messages in order it is supposed to?

judovana commented 3 years ago

first - apend te cplisting after you collect deps from byte[] second - may play with incremental compilation - but be sure it can be turned off.

-- Mgr. Jiri Vanek @.***

---------- Původní e-mail ---------- Od: mkoncek @.> Komu: pmikova/java-runtime-decompiler @. github.com> Datum: 6. 10. 2021 13:09:37 Předmět: Re: [pmikova/java-runtime-decompiler] CPLC: new dependency resolution model leads to bootstrap problem (#168) "

You are right that re-adding cplisting will probably work most of the time and it solves the example issue i pointed out here. Now i don't know what to do more? Implement optional lazy reading from error messages in order it is supposed to?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub (https://github.com/pmikova/java-runtime-decompiler/issues/168#issuecomment-935996121) , or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAWFCS7IEMRSCBO4S2JVP3DUFQU6XANCNFSM5FM43ICQ) . Triage notifications on the go with GitHub Mobile for iOS (https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or Android (https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub) . "

mkoncek commented 3 years ago

Ok, simple fix has just been pushed into upstream. Now i wonder, did that lazy class loading bite us only in two cases? 1) nested classes 2) @Override because the decompilers try to be more user friendly

Override has a simple fix, just add it into the set of exposed classes. For nested classes i can imagine that whenever the compiler actually tries to read their content, i would apply the algorithm for searching for outer class and import it with all its nested classes. THo i am not sure if it is necessary. We would first have to make a test case that shows it.

judovana commented 3 years ago

seesm to be working against cplc head. Looking forward for release

mkoncek commented 3 years ago

I delved a bit into the iterative compilation idea, but i don't like it. It actually doesn't really make sense to resolve dependencies from compiler messages itself. If the current solution is ad-hoc, then this would be ad-hoc ^ 2. There are separate error messages for missing package and for unresolved typename. I haven't checked the order of reported messages but i don't think we want to go this way now that we have cplc wokring quite well.

I think that if we needed to extend the approach somehow then other solutions regarding the classpath listing should be employed instead of reading compiler messages.

judovana commented 3 years ago

yy. agree

-- Mgr. Jiri Vanek @.***

---------- Původní e-mail ---------- Od: mkoncek @.> Komu: pmikova/java-runtime-decompiler @. github.com> Datum: 7. 10. 2021 22:50:39 Předmět: Re: [pmikova/java-runtime-decompiler] CPLC: new dependency resolution model leads to bootstrap problem (#168) "

I delved a bit into the iterative compilation idea, but i don't like it. It actually doesn't really make sense to resolve dependencies from compiler messages itself. If the current solution is ad-hoc, then this would be ad- hoc ^ 2. There are separate error messages for missing package and for unresolved typename. I haven't checked the order of reported messages but i don't think we want to go this way now that we have cplc wokring quite well.

I think that if we needed to extend the approach somehow then other solutions regarding the classpath listing should be employed instead of reading compiler messages.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub (https://github.com/pmikova/java-runtime-decompiler/issues/168#issuecomment-938145905) , or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAWFCSZD4LKDR73DT4H54R3UFYBZNANCNFSM5FM43ICQ) . Triage notifications on the go with GitHub Mobile for iOS (https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or Android (https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub) . "

mkoncek commented 3 years ago

I think the discussion is finished.

judovana commented 3 years ago

Thanx.. yes. All is pushed and all seems working. TY!