-
Notifications
You must be signed in to change notification settings - Fork 27
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
TypeIterator for processing Runnable in Thread.start #229
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow that was fast, thanks for looking into the issue! 🤓
Code-wise these changes look good to me, and the initial issue has been fully solved. However, since i had my testbench still available from debugging the issue #227, i did some further tests and ran into weird behavior. I initialized a Project
(same Demo.class
as in #227) in three different ways:
Only the Project Class
val project = Project(new java.io.File("Demo.class"))
Project and JDK (Interfaces)
val project = Project(new java.io.File("Demo.class"), RTJar)
Project and JDK (full)
val libFiles = Java17Framework.AllClassFiles(Seq(RTJar))
val projectFiles = Java17Framework.AllClassFiles(Seq(new java.io.File("Demo.class")))
val project = Project(projectFiles, libFiles, libraryClassFilesAreInterfacesOnly = false)
I tested each instance with the default domain and with explicitly setting the L2 domain, and used different CG Keys to find out whether they'd mark println
as reachable. These are my results:
Default Domain | L2 Domain | |
---|---|---|
Only Project File | CHA, RTA, XTA, CFA_1_1 | CHA, RTA, XTA, CFA_1_1 |
Project and RTJar (Interfaces) | CHA | CHA |
Project and RTJar (Full) | None | None |
The initial issue seems solved: If i'm only loading the project itself, all CG-Keys i tested find the correct invocation.
The issue with not finding the correct call when fully loading the RTJar might be due to the way i load the ClassFile
instances - during debugging i saw that they did not rewrite Lambdas and used an INVOKEDYNAMIC
instead of an INVOKESTATIC
which the other two ways of building the project did.
However, i am very confused about the fact that when i'm loading the RTJar
with interfaces only, CHA will detect the invocation, but RTA, XTA and CFA_1_1 will not. I will try to understand what's happening here, but so far i don't know why loading library files would influence the results of this computation.
That is weird behavior indeed. Would be helpful if you could debug why that happens. As a first check, I think it would be interesting what happens in the full RTJar case if you do rewrite invokedynamic. |
After some more debugging i found what i think is the reason for RTA (and i guess XTA and CFA_1_1) not working when loading the The I'll go on looking into the issue when loading the RTJar fully. |
I thought we had things like PrintStream configured in the tac reference.conf, but it seems this is not really the case if the RTJar is not fully loaded. Maybe we need to look into whether and how we should configure more such types in that case. Why does it work without the RTJar present at all, though? |
This piece of code from |
So essentially the situation is this:
If yes, this seems like intended behavior for the most part but we should look into whether further modelling of JDK internals would be sensible in order to make types like PrintStream considered instantiated. I still wonder if it does work with the full RTJar code if invokedynamics are rewritten - does the modelling that we have then cause PrintStream to be considered instantiated? As a second issue besides potentially better modelling of JDK internals, we might want to look into making these call graph analyses work at least for some cases of invokedynamic. Do you want to look into these issues? |
I created #231 and #230 to document these issue, i will look into them and see if i can find a solution. I will also have a look into loading the RTJar with code when dynamics are rewritten and report back here. You did understand the situation correctly, so i guess everything else is working as intended - i'll approve this PR as soon as i got some insights on loading the full RTJar 👍 |
For these tests, i initialized my project to rewrite invokedynamics by doing this:
I confirmed that the dynamic invocation is in fact rewritten as expected. I found that both CHA and RTA work in this setting (i.e. resolve the lambda call and the call to However, for XTA it again does not find the call to I also don't know if CFA_1_1 (or CFA_1_0) work for the full RTJar, my laptop takes way to long computing those CGs... 🥹 |
CFA_1_0 at least should work (but maybe not on 'your laptop', it is memory intensive). If RTA works but XTA doesn't there are two main possible reasons: XTA doesn't properly treat PrintStream as instantiated because its InstantiatedTypesAnalysis works differently or the type PrintStream doesn't reach the type set for the calling method. Both could be checked by inspecting XTA's type sets after running the CG. If conditional breakpoints incur a large performance penalty, try putting the condition into the actual code, just place an empty println after it and put an unconditional breakpoint on that. That usually doesn't have a noticeable impact on performance. |
Okay, so here's a few things i found:
I didn't yet look into where in the JDK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The remaining issues i had don't really originate from the changes in this PR, i think they are there own separate issue that we documented in #230. This PR restores the functionality of the ThreadRelatedCallsAnalysis
as expected and looks good to me 👍
Fixes #227