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

[GR-44098] Add lambda methods test in debuginfotest #4311

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

zakkak
Copy link
Collaborator

@zakkak zakkak commented Feb 10, 2022

Catches cases like graalvm#355 caused by JDK-8281266

@zakkak
Copy link
Collaborator Author

zakkak commented Mar 23, 2022

The underlying issue has been fixed in jdk17u-dev and is expected to land in OpenJDK 17.0.4 (July 19 2022). As a result we could consider this PR for inclusion in GraalVM 22.2.0 (July 26, 2022).

@oubidar-Abderrahim
Copy link
Member

Hi, Thank you for contributing to GraalVM, when this PR is ready for review, please let me know so that I can take care of it and assign it to the proper engineer.

@zakkak zakkak marked this pull request as ready for review July 30, 2022 12:06
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 20, 2022
@olpaw
Copy link
Member

olpaw commented Sep 20, 2022

Hi @zakkak,
while already at it, it would be great to also test setting and entering breakpoints in lambdas via

  • a line breakpoint in the body of the lambda function
  • method breakpoint on the name of the lambda function

@@ -229,4 +229,6 @@ private static void inlineReceiveConstants(byte b, int i, long l, String s, floa
System.out.println(String.format("q = %g\n", q));
System.out.println(String.format("t = %s\n", t));
}

final static java.util.function.Supplier<String> lambda = () -> "lambdaText";
Copy link
Member

Choose a reason for hiding this comment

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

I do not see any use of that lambda elsewhere. Thus, it is not guaranteed that the reachability analysis will keep it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, this actually needs to be called by the test program.

@olpaw olpaw requested a review from adinn September 20, 2022 10:51
Copy link
Collaborator

@adinn adinn left a comment

Choose a reason for hiding this comment

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

The current check is good. However, as Paul said, we really need this to check breakpoints work at the point of lambda invocation and in the lambda itself. I think it would be best to add that from the start rather than via a follow-up.

@zakkak zakkak self-assigned this Sep 29, 2022
@zakkak
Copy link
Collaborator Author

zakkak commented Sep 29, 2022

Hi all, this is on my to-do list (just not high enough :) )

@zakkak zakkak force-pushed the debuginfotest-lambda-test branch 3 times, most recently from d6d2b01 to 6302e97 Compare October 3, 2022 11:36
@zakkak zakkak requested review from olpaw and adinn October 3, 2022 12:34
@olpaw olpaw self-requested a review November 3, 2022 12:37
@olpaw
Copy link
Member

olpaw commented Nov 3, 2022

@zakkak please fix the merge conflict before we make the internal PR.

@zakkak
Copy link
Collaborator Author

zakkak commented Nov 4, 2022

@zakkak please fix the merge conflict before we make the internal PR.

Done, should be good to go.

@zakkak
Copy link
Collaborator Author

zakkak commented Nov 9, 2022

@olpaw could you please create the internal PR for this?

@zakkak
Copy link
Collaborator Author

zakkak commented Dec 2, 2022

@olpaw @oubidar-Abderrahim I rebased once again to resolve some new conflicts. Please review and merge if OK.

@zakkak
Copy link
Collaborator Author

zakkak commented Jan 30, 2023

Ping @olpaw @oubidar-Abderrahim ^^

@oubidar-Abderrahim
Copy link
Member

Hi @zakkak , it seems there are some more conflicts to solve, please do so when possible.
@olpaw Once the conflicts are solved, can we integrate this PR into main, or are there any more changes required?

@zakkak
Copy link
Collaborator Author

zakkak commented Feb 8, 2023

Hi @zakkak , it seems there are some more conflicts to solve, please do so when possible.

@oubidar-Abderrahim done. Please note that this is the 3rd rebase after the PR's approval. Since this has already been approved, and the conflicts were mostly on line number differences, could you please push it for internal review once the github actions run?

@zakkak
Copy link
Collaborator Author

zakkak commented Feb 14, 2023

@oubidar-Abderrahim the CI is happy and there are no conflicts, could you please push this to the internal queue?

@zakkak zakkak force-pushed the debuginfotest-lambda-test branch 2 times, most recently from 7c003a3 to 550da8b Compare February 28, 2023 13:18
@fniephaus fniephaus changed the title Add lambda methods test in debuginfotest [GR-44098] Add lambda methods test in debuginfotest Mar 22, 2023
@zakkak zakkak force-pushed the debuginfotest-lambda-test branch 2 times, most recently from 26053bb to a1939b1 Compare June 26, 2023 12:18
@fniephaus
Copy link
Member

Our CI has unfortunately mirrored this to #6895.

@@ -453,6 +457,10 @@ def test():
# print details of Hello type
exec_string = execute("ptype 'hello.Hello'")
rexp = [r"type = class hello\.Hello : public java\.lang\.Object {",
# r"%spublic:"%spaces_pattern,
# r"%sstatic %sjava\.util\.function\.Supplier \*lambda;"%(spaces_pattern, "_z_\." if isolates else ""),
Copy link
Member

Choose a reason for hiding this comment

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

Why is this commented out?

Copy link
Collaborator Author

@zakkak zakkak Jun 27, 2023

Choose a reason for hiding this comment

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

Accidentally, it should just be removed, good catch! Do you want me to fix it here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please. :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@zakkak zakkak force-pushed the debuginfotest-lambda-test branch from a1939b1 to 4c2436c Compare June 27, 2023 14:23
This way we can add methods on top of main without affecting
significantly the linenumbers in testhello.py
@zakkak zakkak force-pushed the debuginfotest-lambda-test branch from 4c2436c to 15bd5b5 Compare June 27, 2023 14:25
@graalvmbot graalvmbot merged commit 15bd5b5 into oracle:master Jun 29, 2023
12 checks passed
@zakkak zakkak deleted the debuginfotest-lambda-test branch June 29, 2023 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement. oca-signed redhat-interest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants