Skip to content

[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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Dec 16, 2021
Merged

[GR-26186] Various fixes. #4111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d6de092
Use correct version of toJavaName in Debug info generation
Dec 6, 2021
00964dd
Fix reachability handler for static methods
Dec 8, 2021
064a375
Lower AArch64ReadNodes to proper AArch64Kinds.
teshull Dec 9, 2021
c44838e
Avoid endless loop when parsing reason has cycles
Dec 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ...ler.core.aarch64/src/org/graalvm/compiler/core/aarch64/AArch64ArithmeticLIRGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,11 @@ public Value emitExtendMemory(boolean isSigned, AArch64Kind accessKind, int resu
* Issue an extending load of the proper bit size and set the result to the proper kind.
*/
GraalError.guarantee(accessKind.isInteger(), "can only extend integer kinds");
Variable result = getLIRGen().newVariable(LIRKind.value(resultBits == 32 ? AArch64Kind.DWORD : AArch64Kind.QWORD));
AArch64Kind resultKind = resultBits <= 32 ? AArch64Kind.DWORD : AArch64Kind.QWORD;
Variable result = getLIRGen().newVariable(LIRKind.value(resultKind));

int dstBitSize = resultBits <= 32 ? 32 : 64;
AArch64Move.ExtendKind extend = isSigned ? AArch64Move.ExtendKind.SIGN_EXTEND : AArch64Move.ExtendKind.ZERO_EXTEND;
getLIRGen().append(new AArch64Move.LoadOp(accessKind, dstBitSize, extend, result, address, state));
getLIRGen().append(new AArch64Move.LoadOp(accessKind, resultKind.getSizeInBytes() * Byte.SIZE, extend, result, address, state));
return result;
}

Expand Down
5 changes: 4 additions & 1 deletion ...tevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlow.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,10 @@ public StackTraceElement[] getParsingContext() {
List<StackTraceElement> parsingContext = new ArrayList<>();
InvokeTypeFlow invokeFlow = parsingReason;

while (invokeFlow != null) {
/* Defend against cycles in the parsing context. GR-35744 should fix this properly. */
int maxSize = 100;

while (invokeFlow != null && parsingContext.size() < maxSize) {
parsingContext.add(invokeFlow.getSource().getMethod().asStackTraceElement(invokeFlow.getSource().getBCI()));
invokeFlow = ((PointsToAnalysisMethod) invokeFlow.getSource().getMethod()).getTypeFlow().parsingReason;
}
Expand Down
2 changes: 1 addition & 1 deletion ...vm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/meta/AnalysisUniverse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ private void collectMethodImplementations() {
public static Set<AnalysisMethod> getMethodImplementations(BigBang bb, AnalysisMethod method, boolean includeInlinedMethods) {
Set<AnalysisMethod> implementations = new LinkedHashSet<>();
if (method.wrapped.canBeStaticallyBound() || method.isConstructor()) {
if (method.isImplementationInvoked()) {
if (includeInlinedMethods ? method.isReachable() : method.isImplementationInvoked()) {
implementations.add(method);
}
} else {
Expand Down
4 changes: 2 additions & 2 deletions ...c/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageDebugInfoProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ public String name() {

@Override
public String valueType() {
return hostedMethod.getSignature().getReturnType(null).toJavaName();
return toJavaName((HostedType) hostedMethod.getSignature().getReturnType(null));
Copy link

@christianwimmer christianwimmer Dec 10, 2021

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.

Copy link
Collaborator

@zakkak zakkak Dec 13, 2021 •
edited
Loading

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.

graal/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/lambda/LambdaSubstitutionType.java

Lines 310 to 313 in 5882aaa

@Override
public String toJavaName() {
return original.toJavaName();
}

As for the why we do the unwrapping:

graal/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageDebugInfoProvider.java

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.
*/

Copy link

@christianwimmer christianwimmer Dec 14, 2021 •
edited
Loading

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.

}

@Override
Expand All @@ -669,7 +669,7 @@ public List<String> paramTypes() {
int parameterCount = signature.getParameterCount(false);
List<String> paramTypes = new ArrayList<>(parameterCount);
for (int i = 0; i < parameterCount; i++) {
paramTypes.add(signature.getParameterType(i, null).toJavaName());
paramTypes.add(toJavaName((HostedType) signature.getParameterType(i, null)));
}
return paramTypes;
}
Expand Down