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

Fix/invoke dynamics #666

Merged
merged 19 commits into from
Oct 20, 2023
Merged

Fix/invoke dynamics #666

merged 19 commits into from
Oct 20, 2023

Conversation

JonasKlauke
Copy link
Collaborator

fixes the stack problem in the convertion of invokedynamics

@JonasKlauke JonasKlauke linked an issue Aug 11, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (5140a36) 64.90% compared to head (1ed3ab1) 65.31%.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop     #666      +/-   ##
=============================================
+ Coverage      64.90%   65.31%   +0.40%     
- Complexity      3368     3406      +38     
=============================================
  Files            313      313              
  Lines          14960    14974      +14     
  Branches        2525     2525              
=============================================
+ Hits            9710     9780      +70     
+ Misses          4351     4304      -47     
+ Partials         899      890       -9     
Files Coverage Δ
...otup/core/jimple/common/constant/MethodHandle.java 100.00% <100.00%> (+60.00%) ⬆️
...va/sootup/core/jimple/common/stmt/JAssignStmt.java 55.55% <ø> (ø)
...sootup/java/bytecode/frontend/AsmMethodSource.java 72.21% <100.00%> (+1.06%) ⬆️
...ain/java/sootup/java/core/language/JavaJimple.java 94.44% <100.00%> (+11.11%) ⬆️
...ain/java/sootup/jimple/parser/JimpleConverter.java 71.78% <90.90%> (+0.41%) ⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@swissiety swissiety marked this pull request as draft August 14, 2023 09:03
@JonasKlauke JonasKlauke requested a review from swissiety August 14, 2023 16:17
@JonasKlauke JonasKlauke marked this pull request as ready for review August 14, 2023 16:30
@SoniaZaldana
Copy link

Hi folks, I've been running some tests on my end with this patch and I wanted to note I still see some issues with indys.
For example

public Stream<LocalDate> test(LocalDate endExclusive, Period step) {
        long months = step.toTotalMonths();
        long days = step.getDays();
        int sign = months > 0 || days > 0 ? 1 : -1;
        long steps = 200000 / (months + days);
        return LongStream.rangeClosed(0, steps).mapToObj(
                n -> endExclusive.plusDays(days * n));
    }

issues the following error:

Error: java.lang.RuntimeException: Failed to convert <jlink.Q: java.util.stream.Stream test(java.time.LocalDate,java.time.Period)>

Printing the stack trace

java.lang.RuntimeException: Illegal Assignment statement. Make sure that right hand side has a valid operand.
	at sootup.core@1.1.2-SNAPSHOT/sootup.core.jimple.common.stmt.JAssignStmt.<init>(JAssignStmt.java:55)
	at sootup.core@1.1.2-SNAPSHOT/sootup.core.jimple.Jimple.newAssignStmt(Jimple.java:530)
	at sootup.java.bytecode@1.1.2-SNAPSHOT/sootup.java.bytecode.frontend.StackFrame.mergeIn(StackFrame.java:160)
	at sootup.java.bytecode@1.1.2-SNAPSHOT/sootup.java.bytecode.frontend.AsmMethodSource.convertInvokeDynamicInsn(AsmMethodSource.java:1402)
	at sootup.java.bytecode@1.1.2-SNAPSHOT/sootup.java.bytecode.frontend.AsmMethodSource.convert(AsmMethodSource.java:1774)
	at sootup.java.bytecode@1.1.2-SNAPSHOT/sootup.java.bytecode.frontend.AsmMethodSource.resolveBody(AsmMethodSource.java:215)
	at sootup.core@1.1.2-SNAPSHOT/sootup.core.model.SootMethod.lazyBodyInitializer(SootMethod.java:98)
	at com.google.common@32.1.2-jre/com.google.common.base.Suppliers$NonSerializableMemoizingSupplier.get(Suppliers.java:181)
	at sootup.core@1.1.2-SNAPSHOT/sootup.core.model.SootMethod.getBody(SootMethod.java:177)

I haven't had a chance to do much digging to see if this is still somehow related to the operand stack but I thought I would bring it up. Thanks!

@swissiety
Copy link
Collaborator

swissiety commented Aug 14, 2023

With Sonias example input there was null on the rhs of the assigment.
Then I merged the other porting regression bug fix where we missed handling one Operand.
And now we have:
a) "Invalid in operands length! " in StackFrame with the example

Bildschirmfoto von 2023-08-14 23-00-03
seems the static invoke itself is missing from before..?

b) SwitchExprWithYieldTest fails due to a stack underrun

so it seems there are missing operands that are not added.. or we are taking too much.

@JonasKlauke
Copy link
Collaborator Author

JonasKlauke commented Sep 15, 2023

I have investigated Test: SwitchExprWithYieldTest of java.bytecode.
The stack underrun is probaly caused by handling a virtual invoke by the dynamic invoke, but the invoke should be static. That consumes one element from the stack which is missing for the parameter of the dynamic invoke call.
This seems to only happen if a dynamic invoke is converted for a second time, which is caused by the switch since there are two branches over the same dynamic invoke instruction.

this line is probably the issue:
final boolean isStaticInvokeExpr = expr instanceof JStaticInvokeExpr;
since the expr is always a dynamic invoke. the coverage report also shows that the if branch (boolean true) is never entered

But couldnt find the time to further investigate this, today.

@swissiety
Copy link
Collaborator

swissiety commented Oct 18, 2023

@SoniaZaldana does the current state of this PR fix the indy problems in your project?

@swissiety swissiety self-requested a review October 20, 2023 15:21
Copy link
Collaborator

@swissiety swissiety left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 seems reasonable and fixes the submitted failing example input

@swissiety swissiety self-requested a review October 20, 2023 15:22
@swissiety swissiety merged commit 62a1d02 into develop Oct 20, 2023
8 checks passed
@swissiety swissiety deleted the fix/InvokeDynamics branch October 20, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invokedynamic failures
3 participants