-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
Use information of operand value to determine type for java locals #789
base: develop
Are you sure you want to change the base?
Conversation
58d745e
to
726b6df
Compare
This PR uses the already existing information of the operand's value type to determine the type for java locals. Previously, most of the stack variables were of "unknown" type . Fixes soot-oss#635
726b6df
to
8481c4b
Compare
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.
Thx for your contribution, @oxisto !
- We have to check whether / how this interferes with the
BodyInterceptor
that is used for typingTypeAssigner
I am currently trying to fix the test, the actual outcome after by patch is:
But shouldn't the type of $l2 then be of Super1 instead of Super1[]? I am not an expert on Simple but I suspect that |
Update: This looks to me to be a bug in SootUp/sootup.core/src/main/java/sootup/core/jimple/common/ref/JArrayRef.java Lines 88 to 92 in 23e5a3e
|
Unfortunately, it seems it does. Especially with casting, since the casted variable (which was previously unknown), now still has the type pre-cast. |
OK it seems I misunderstood how things work. I was not using these body interceptors at all (they seem to be empty when using JavaClassPathAnalysisInputLocation with only the path argument). After using It would be good to add a remark to the doc about this or even specificy the |
1:
yes $l2 should be the arrays element type ad not the array type itself. was the output from your change or from typeassigner? 2:
direct assignment seems logical and maybe reduces the workload of the typpeassigner - that has to be investigated ;) 3:
as default option is/was the plan :) |
It was from my change. It seems that the type resolver does not use the Lines 87 to 134 in bc6f0bc
I can continue working on this PR if you want - even though my original problem was actually "solved" by just activating the type assigner. The only issue the PR has remaining is the one described above with the casts - although I am not sure if I can solve this or someone from the soot team would need to take over. The question would be: Is there really a valid reason not to use the type assigner?
Sounds reasonable :) |
TODO
|
This PR uses the already existing information of the operand's value type to determine the type for java locals. Previously, most of the stack / local variables were of "unknown" type.
I am not sure if this catches all the cases where type information about the operand are available, so feel free to add to this PR.
Fixes #635