Skip to content

Commit 20fe19f

Browse files
zapsterzapster
committed
[GR-70793] Fix more style issues in native image code
PullRequest: graal/22463
2 parents a0dc586 + f387142 commit 20fe19f

File tree

11 files changed

+68
-55
lines changed

11 files changed

+68
-55
lines changed

‎compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/CheckGraalInvariants.java‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,9 @@ public boolean checkAssertions() {
253253
return true;
254254
}
255255

256+
public boolean shouldVerifyWordFactory(@SuppressWarnings("unused") ResolvedJavaMethod method) {
257+
return true;
258+
}
256259
}
257260

258261
@Test
@@ -416,7 +419,7 @@ public static void runTest(InvariantsTool tool) {
416419
graphBuilderSuite.apply(graph, context);
417420
// update phi stamps
418421
graph.getNodes().filter(PhiNode.class).forEach(PhiNode::inferStamp);
419-
checkGraph(verifiers, context, graph);
422+
checkGraph(verifiers, context, graph, tool);
420423
errors.add(String.format("Expected error while checking %s", m));
421424
} catch (VerificationError e) {
422425
// expected!
@@ -477,7 +480,7 @@ public static void runTest(InvariantsTool tool) {
477480
// update phi stamps
478481
graph.getNodes().filter(PhiNode.class).forEach(PhiNode::inferStamp);
479482
collectOptionFieldUsages(optionFieldUsages, optionDescriptorsType, method, graph);
480-
checkGraph(verifiers, context, graph);
483+
checkGraph(verifiers, context, graph, tool);
481484
} catch (VerificationError e) {
482485
errors.add(e.getMessage());
483486
} catch (BailoutException e) {
@@ -701,13 +704,15 @@ private static void checkMethod(ResolvedJavaMethod method) {
701704
/**
702705
* Checks the invariants for a single graph.
703706
*/
704-
private static void checkGraph(List<VerifyPhase<CoreProviders>> verifiers, HighTierContext context, StructuredGraph graph) {
707+
private static void checkGraph(List<VerifyPhase<CoreProviders>> verifiers, HighTierContext context, StructuredGraph graph, InvariantsTool tool) {
705708
for (VerifyPhase<CoreProviders> verifier : verifiers) {
706-
if (!(verifier instanceof VerifyUsageWithEquals) || shouldVerifyEquals(graph.method())) {
707-
verifier.apply(graph, context);
708-
} else {
709-
verifier.apply(graph, context);
709+
if (verifier instanceof VerifyUsageWithEquals && !shouldVerifyEquals(graph.method())) {
710+
continue;
711+
}
712+
if (verifier instanceof VerifyWordFactoryUsage && !tool.shouldVerifyWordFactory(graph.method())) {
713+
continue;
710714
}
715+
verifier.apply(graph, context);
711716
}
712717
if (graph.method().isBridge()) {
713718
BridgeMethodUtils.getBridgedMethod(graph.method());

‎compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/VerifyPhase.java‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,13 @@
2727
import java.util.Optional;
2828

2929
import jdk.graal.compiler.nodes.CallTargetNode;
30+
import jdk.graal.compiler.nodes.FixedNode;
31+
import jdk.graal.compiler.nodes.FrameState;
3032
import jdk.graal.compiler.nodes.GraphState;
3133
import jdk.graal.compiler.nodes.Invoke;
3234
import jdk.graal.compiler.nodes.StructuredGraph;
35+
import jdk.graal.compiler.nodes.ValueNode;
36+
import jdk.graal.compiler.nodes.util.GraphUtil;
3337
import jdk.graal.compiler.phases.BasePhase;
3438
import jdk.vm.ci.meta.MetaAccessProvider;
3539
import jdk.vm.ci.meta.ResolvedJavaType;
@@ -55,12 +59,31 @@ public VerificationError(String message, Throwable cause) {
5559
}
5660

5761
public VerificationError(Invoke invoke, String format, Object... args) {
58-
this(String.format("In %s: %s", invoke.asNode().graph().method().asStackTraceElement(invoke.bci()), format), args);
62+
this(String.format("In %s: %s", getStackTraceElement(invoke.asNode(), invoke.bci()), format), args);
5963
}
6064

6165
public VerificationError(CallTargetNode target, String format, Object... args) {
6266
this(target.invoke(), format, args);
6367
}
68+
69+
public VerificationError(ValueNode node, String format, Object... args) {
70+
this(String.format("In %s: %s", getMethodString(node), format), args);
71+
}
72+
73+
private static String getMethodString(ValueNode node) {
74+
if (node instanceof FixedNode fixedNode) {
75+
FrameState lastFrameState = GraphUtil.findLastFrameState(fixedNode);
76+
if (lastFrameState != null) {
77+
return getStackTraceElement(node, lastFrameState.bci).toString();
78+
}
79+
}
80+
return node.graph().method().format("%H.%n(%p)");
81+
}
82+
83+
private static StackTraceElement getStackTraceElement(ValueNode node, int bci) {
84+
return node.graph().method().asStackTraceElement(bci);
85+
}
86+
6487
}
6588

6689
@Override

‎compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/VerifyUsageWithEquals.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -194,8 +194,10 @@ protected void verify(StructuredGraph graph, CoreProviders context) {
194194
if (restrictedType.isAssignableFrom(method.getDeclaringClass())) {
195195
// Allow violation in methods of the restricted type itself and its subclasses.
196196
} else if (isIllegalUsage(method, cn.getX(), cn.getY(), context)) {
197-
throw new VerificationError("Verification of " + restrictedClass.getName() + " usage failed: Comparing " + cn.getX() + " and " + cn.getY() + " in " + method +
198-
" must use .equals() for object equality, not '==' or '!='");
197+
throw new VerificationError(cn, "Verification of %s usage failed: Comparing %s and %s must use .equals() for object equality, not '==' or '!='",
198+
restrictedClass.getName(),
199+
cn.getX(),
200+
cn.getY());
199201
}
200202
}
201203
}

‎compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/VerifyVariableCasts.java‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import jdk.graal.compiler.core.common.type.TypeReference;
3030
import jdk.graal.compiler.lir.LIRValueUtil;
3131
import jdk.graal.compiler.lir.Variable;
32+
import jdk.graal.compiler.nodes.NodeView;
3233
import jdk.graal.compiler.nodes.PiNode;
3334
import jdk.graal.compiler.nodes.StructuredGraph;
3435
import jdk.graal.compiler.nodes.java.InstanceOfNode;
@@ -62,21 +63,21 @@ protected void verify(StructuredGraph graph, CoreProviders context) {
6263
Stamp stamp = cast.piStamp();
6364
if (stamp instanceof AbstractObjectStamp) {
6465
ResolvedJavaType castType = stamp.javaType(metaAccess);
65-
if (variableType.isAssignableFrom(castType)) {
66-
throw new VerificationError("Cast to %s in %s is prohibited as it might skip checks for LIR CastValues. Use LIRValueUtil.asVariable instead.",
67-
variableType.toJavaName(),
68-
method.format("%H.%n(%p)"));
66+
ResolvedJavaType sourceType = cast.object().stamp(NodeView.DEFAULT).javaType(metaAccess);
67+
if (variableType.isAssignableFrom(castType) && !variableType.isAssignableFrom(sourceType)) {
68+
throw new VerificationError(cast, "Cast to %s is prohibited as it might skip checks for LIR CastValues. Use LIRValueUtil.asVariable instead.",
69+
variableType.toJavaName());
6970
}
7071
}
7172
}
7273

7374
for (InstanceOfNode instanceOf : graph.getNodes().filter(InstanceOfNode.class)) {
7475
TypeReference typeRef = instanceOf.type();
76+
ResolvedJavaType sourceType = instanceOf.getValue().stamp(NodeView.DEFAULT).javaType(metaAccess);
7577
if (typeRef != null) {
76-
if (variableType.isAssignableFrom(typeRef.getType())) {
77-
throw new VerificationError("Instanceof check on %s in %s is prohibited as it might skip checks for LIR CastValues. Use LIRValueUtil.isVariable instead.",
78-
variableType.toJavaName(),
79-
method.format("%H.%n(%p)"));
78+
if (variableType.isAssignableFrom(typeRef.getType()) && !variableType.isAssignableFrom(sourceType)) {
79+
throw new VerificationError(instanceOf, "Instanceof check on %s is prohibited as it might skip checks for LIR CastValues. Use LIRValueUtil.isVariable instead.",
80+
variableType.toJavaName());
8081
}
8182
}
8283
}

‎compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/VerifyWordFactoryUsage.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,8 @@ protected void verify(StructuredGraph graph, CoreProviders context) {
4949
ResolvedJavaType wordFactory = context.getMetaAccess().lookupJavaType(WordFactory.class);
5050
for (MethodCallTargetNode t : graph.getNodes(MethodCallTargetNode.TYPE)) {
5151
if (t.targetMethod().getDeclaringClass().equals(wordFactory)) {
52-
throw new VerificationError("accessing %s in %s is prohibited - use %s.%s instead",
52+
throw new VerificationError(t, "accessing %s is prohibited - use %s.%s instead",
5353
wordFactory.toJavaName(),
54-
graph.method().format("%H.%n(%p)"),
5554
Word.class.getName(),
5655
t.targetMethod().format("%n(%p)"));
5756

‎substratevm/src/com.oracle.graal.pointsto.standalone/src/com/oracle/graal/pointsto/standalone/MethodConfigReader.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ public static int forMethodList(DebugContext debug, List<String> methods, BigBan
106106
validMethodsNum.incrementAndGet();
107107
} catch (Throwable t) {
108108
// Checkstyle: Allow raw info or warning printing - begin
109-
debug.log(DebugContext.VERBOSE_LEVEL, "Warning: Can't add method " + method + " as analysis root method. Reason: " + t.getMessage());
109+
debug.log(DebugContext.VERBOSE_LEVEL, "Warning: Can't add method %s as analysis root method. Reason: %s", method, t.getMessage());
110110
// Checkstyle: Allow raw info or warning printing - end
111111
}
112112
}

‎substratevm/src/com.oracle.graal.pointsto.standalone/src/com/oracle/graal/pointsto/standalone/PointsToAnalyzer.java‎

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,6 @@
8282
import jdk.graal.compiler.phases.util.Providers;
8383
import jdk.graal.compiler.printer.GraalDebugHandlersFactory;
8484
import jdk.graal.compiler.word.WordTypes;
85-
import jdk.vm.ci.amd64.AMD64Kind;
8685
import jdk.vm.ci.meta.JavaKind;
8786
import jdk.vm.ci.meta.MetaAccessProvider;
8887
import jdk.vm.ci.meta.ResolvedJavaMethod;
@@ -122,11 +121,10 @@ private PointsToAnalyzer(String mainEntryClass, OptionValues options, ClassLoade
122121
MetaAccessProvider originalMetaAccess = originalProviders.getMetaAccess();
123122
debugContext = new DebugContext.Builder(options, new GraalDebugHandlersFactory(snippetReflection)).build();
124123
StandaloneHost standaloneHost = new StandaloneHost(options);
125-
int wordSize = getWordSize();
126124
AnalysisPolicy analysisPolicy = PointstoOptions.AllocationSiteSensitiveHeap.getValue(options) ? new BytecodeSensitiveAnalysisPolicy(options)
127125
: new DefaultAnalysisPolicy(options);
128126

129-
JavaKind wordKind = JavaKind.fromWordSize(wordSize);
127+
JavaKind wordKind = originalProviders.getWordTypes().getWordKind();
130128
AnalysisUniverse aUniverse = new AnalysisUniverse(standaloneHost, wordKind,
131129
analysisPolicy, SubstitutionProcessor.IDENTITY, originalMetaAccess, new PointsToAnalysisFactory(), new StandaloneAnnotationExtractor());
132130
AnalysisMetaAccess aMetaAccess = new StandaloneAnalysisMetaAccess(aUniverse, originalMetaAccess);
@@ -230,23 +228,6 @@ private String getAnalysisName(String entryClass) {
230228
}
231229
}
232230

233-
private static int getWordSize() {
234-
int wordSize;
235-
String archModel = System.getProperty("sun.arch.data.model");
236-
switch (archModel) {
237-
case "64":
238-
wordSize = AMD64Kind.QWORD.getSizeInBytes();
239-
break;
240-
case "32":
241-
wordSize = AMD64Kind.DWORD.getSizeInBytes();
242-
break;
243-
default:
244-
throw new RuntimeException("Property sun.arch.data.model should only be 64 or 32, but is " + archModel);
245-
246-
}
247-
return wordSize;
248-
}
249-
250231
/**
251232
* @see #createAnalyzer(String[], ClassLoaderAccess)
252233
*/

‎substratevm/src/com.oracle.svm.agent/src/com/oracle/svm/agent/NativeImageAgent.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@
8383
import com.oracle.svm.configure.filters.HierarchyFilterNode;
8484
import com.oracle.svm.configure.trace.AccessAdvisor;
8585
import com.oracle.svm.configure.trace.TraceProcessor;
86+
import com.oracle.svm.core.JavaVersionUtil;
8687
import com.oracle.svm.core.jni.headers.JNIEnvironment;
8788
import com.oracle.svm.core.jni.headers.JNIJavaVM;
8889
import com.oracle.svm.core.jni.headers.JNIObjectHandle;
@@ -439,7 +440,7 @@ private static int usage(String message) {
439440

440441
private static boolean checkJVMVersion(JvmtiEnv jvmti) {
441442
String agentVersion = System.getProperty("java.vm.version");
442-
int agentMajorVersion = Runtime.version().feature();
443+
int agentMajorVersion = JavaVersionUtil.JAVA_SPEC;
443444

444445
String vmVersion = Support.getSystemProperty(jvmti, "java.vm.version");
445446
if (vmVersion == null) {

‎substratevm/src/com.oracle.svm.core.graal.llvm/src/com/oracle/svm/core/graal/llvm/LLVMGenerator.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@
113113
import jdk.graal.compiler.graph.Node;
114114
import jdk.graal.compiler.lir.LIRFrameState;
115115
import jdk.graal.compiler.lir.LIRInstruction;
116+
import jdk.graal.compiler.lir.LIRValueUtil;
116117
import jdk.graal.compiler.lir.LabelRef;
117118
import jdk.graal.compiler.lir.Variable;
118119
import jdk.graal.compiler.lir.VirtualStackSlot;
@@ -582,8 +583,8 @@ public AllocatableValue asAllocatable(Value value) {
582583

583584
@Override
584585
public Variable emitMove(Value input) {
585-
if (input instanceof LLVMVariable) {
586-
return (LLVMVariable) input;
586+
if (LIRValueUtil.isVariable(input) && LIRValueUtil.asVariable(input) instanceof LLVMVariable) {
587+
return LIRValueUtil.asVariable(input);
587588
} else if (input instanceof LLVMValueWrapper) {
588589
return new LLVMVariable(getVal(input));
589590
}
@@ -619,7 +620,7 @@ public void emitMove(AllocatableValue dst, Value src) {
619620
} else if (LLVMIRBuilder.isWordType(destType) && LLVMIRBuilder.isObjectType(sourceType)) {
620621
source = builder.buildPtrToInt(source);
621622
}
622-
((LLVMVariable) dst).set(source);
623+
((LLVMVariable) LIRValueUtil.asVariable(dst)).set(source);
623624
}
624625

625626
@Override

‎substratevm/src/com.oracle.svm.core.graal.llvm/src/com/oracle/svm/core/graal/llvm/NodeLLVMBuilder.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@
8787
import jdk.graal.compiler.graph.iterators.NodeIterable;
8888
import jdk.graal.compiler.lir.ConstantValue;
8989
import jdk.graal.compiler.lir.LIRFrameState;
90+
import jdk.graal.compiler.lir.LIRValueUtil;
9091
import jdk.graal.compiler.lir.Variable;
9192
import jdk.graal.compiler.nodes.AbstractBeginNode;
9293
import jdk.graal.compiler.nodes.AbstractEndNode;
@@ -224,7 +225,8 @@ public void doBlock(HIRBlock block, StructuredGraph graph, BlockMap<List<Node>>
224225
if (processedBlocks.contains(predecessor)) {
225226
ValueNode phiValue = phiNode.valueAt((AbstractEndNode) predecessor.getEndNode());
226227
LLVMValueRef value;
227-
if (operand(phiValue) instanceof LLVMPendingSpecialRegisterRead) {
228+
Value operand = operand(phiValue);
229+
if (LIRValueUtil.isVariable(operand) && LIRValueUtil.asVariable(operand) instanceof LLVMPendingSpecialRegisterRead) {
228230
/*
229231
* The pending read may need to perform instructions to load the
230232
* value, so we put them at the end of the predecessor block
@@ -508,7 +510,7 @@ public void emitInvoke(Invoke i) {
508510

509511
if (nextMemoryAccessNeedsDecompress) {
510512
computedAddress = builder.buildAddrSpaceCast(computedAddress, builder.objectType(true));
511-
LLVMValueRef heapBase = ((LLVMVariable) gen.emitReadRegister(ReservedRegisters.singleton().getHeapBaseRegister(), null)).get();
513+
LLVMValueRef heapBase = ((LLVMVariable) LIRValueUtil.asVariable(gen.emitReadRegister(ReservedRegisters.singleton().getHeapBaseRegister(), null))).get();
512514
computedAddress = builder.buildUncompress(computedAddress, heapBase, true, compressionShift);
513515
}
514516

@@ -777,14 +779,12 @@ public Value setResult(ValueNode node, Value operand) {
777779
assert kind == ValueKind.Illegal.getPlatformKind();
778780
llvmOperand = new LLVMVariable(builder.getUndef());
779781
}
780-
} else if (operand instanceof LLVMAddressValue) {
781-
LLVMAddressValue addressValue = (LLVMAddressValue) operand;
782+
} else if (operand instanceof LLVMAddressValue addressValue) {
782783
Value wrappedBase = addressValue.getBase();
783784
Value index = addressValue.getIndex();
784785

785-
if (wrappedBase instanceof LLVMPendingSpecialRegisterRead) {
786-
LLVMPendingSpecialRegisterRead pendingRead = (LLVMPendingSpecialRegisterRead) wrappedBase;
787-
if (index != null && index != Value.ILLEGAL) {
786+
if (LIRValueUtil.isVariable(wrappedBase) && LIRValueUtil.asVariable(wrappedBase) instanceof LLVMPendingSpecialRegisterRead pendingRead) {
787+
if (index != null && !index.equals(Value.ILLEGAL)) {
788788
pendingRead = new LLVMPendingSpecialRegisterRead(pendingRead, LLVMUtils.getVal(addressValue.getIndex()));
789789
}
790790
llvmOperand = pendingRead;
@@ -800,7 +800,7 @@ public Value setResult(ValueNode node, Value operand) {
800800
}
801801

802802
LLVMValueRef intermediate;
803-
if (index == null || index == Value.ILLEGAL) {
803+
if (index == null || index.equals(Value.ILLEGAL)) {
804804
intermediate = base;
805805
} else {
806806
intermediate = builder.buildGEP(base, LLVMUtils.getVal(index));

0 commit comments

Comments
 (0)