-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[GR-26186] Various fixes. #4111
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
Conversation
graalvmbot
commented
Dec 10, 2021
- Use correct version of toJavaName in Debug info generation
- Fix reachability handler for static methods
- Lower AArch64ReadNodes to proper AArch64Kinds.
- Avoid endless loop when parsing reason has cycles
| @Override | ||
| public String valueType() { | ||
| return hostedMethod.getSignature().getReturnType(null).toJavaName(); | ||
| return toJavaName((HostedType) hostedMethod.getSignature().getReturnType(null)); |
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.
@adinn @zakkak This fixes some errors related to Lambda types. LambdaSubstitutionType injects a new stable name for such types. For the CE debug information, you unwrap the substitutions (I don't really know why, but certainly don't want to change it either). These two usages were the only ones I could find that were missing the unwrapping.
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.
@christianwimmer thanks for catching and fixing this. Your changes look good to me and I was also not able to find other usages missing the unwrapping.
I don't really get how this change relates to LambdaSubstitutionType's stableName though. AFAICS LambdaSubstitutionType#toJavaName() invokes toJavaName() on the original ResolvedJavaType, so it's now not using the stableName.
Lines 310 to 313 in 5882aaa
| @Override | |
| public String toJavaName() { | |
| return original.toJavaName(); | |
| } |
As for the why we do the unwrapping:
Lines 177 to 187 in 5882aaa
| /* | |
| * HostedType wraps an AnalysisType and both HostedType and AnalysisType punt calls to | |
| * getSourceFilename to the wrapped class so for consistency we need to do type names and path | |
| * lookup relative to the doubly unwrapped HostedType. | |
| * | |
| * However, note that the result of the unwrap on the AnalysisType may be a SubstitutionType | |
| * which wraps both an original type and the annotated type that substitutes it. Unwrapping | |
| * normally returns the AnnotatedType which we need to use to resolve the file name. However, we | |
| * need to use the original to name the owning type to ensure that names found in method param | |
| * and return types resolve correctly. | |
| */ |
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.
With my current work to use more factory methods in #4085 it is now possible that a lambda type is used as the return type of a method (that cannot occur in "normal" bytecode because the lambda type is only created on-the-fly by the LambdaMetaFactory and not mentioned as a return type in any generated method).
With a lambda type as a method return type, some code paths were using the class name generated by the LambdaMetaFactory, and some code paths were using the new name as rewritten by LambdaSubstitutionType. Your debug info code goes back to the original name from HotSpot, so it is now (consistently) using the original name that comes from LambdaMetaFactory.