On Fri, 24 May 2024 17:57:28 GMT, Oussama Louati <d...@openjdk.org> wrote:
>> An indify tool in j.l.i tests (also in vmTestBase) convert some source-code >> private static methods with MT_ MH_, and INDY_ prefixes into MethodHandle, >> MethodType, and CallSite constants. >> It currently uses ad-hoc code to process class files and intends to migrate >> to ASM; but since we have the Classfile API, we can migrate to Classfile API >> instead. > > Oussama Louati has updated the pull request incrementally with 19 additional > commits since the last revision: > > - fix: fixed whitespaces > - fix: fixed whitespaces > - chore: used Class::descriptorString > - remove: added removed test comments > - remove: added removed test comments > - remove: removed changes in tests > - update: optimize imports and remove unnecessary utils > - update: optimize imports > - update: 5th patch of code review > - update: 5th patch of code review > - ... and 9 more: https://git.openjdk.org/jdk/compare/781c951d...2b8c94a7 I would recommend to use more high-level models of the Class-File API instead of filling the original code from a new source. There are also some unnecessary reformats of the javadoc, which is very hard to review and forgotten debug prints. For details see my inline comments. Generally it is a move forward. Changes requested by asotona (Reviewer). test/jdk/java/lang/invoke/CallSiteTest.java line 96: > 94: } > 95: > 96: public static class MyCCS extends ConstantCallSite { Any reason for this change? test/jdk/java/lang/invoke/CallStaticInitOrder.java line 54: > 52: > 53: static int Init1Tick; > 54: public static class Init1 { Is there a reason to change all the classes and methods to public? test/jdk/java/lang/invoke/indify/Indify.java line 61: > 59: * meaning. > 60: * </p> > 61: * Why is the javadoc reformatted? It is not possible to review the changes if has changed. test/jdk/java/lang/invoke/indify/Indify.java line 271: > 269: switch (a) { > 270: case "--java": > 271: return; Where this conditional code disappeared? test/jdk/java/lang/invoke/indify/Indify.java line 352: > 350: if (verbose) System.err.println("reading " + f); > 351: ClassModel model = parseClassFile(f); > 352: if (model == null) throw new IOException("Failed to parse class > file: " + f.getName()); How it suppose to happen the model is null? test/jdk/java/lang/invoke/indify/Indify.java line 354: > 352: if (model == null) throw new IOException("Failed to parse class > file: " + f.getName()); > 353: Logic logic = new Logic(model); > 354: if(logic.classModel == null) throw new IOException("Failed to > create logic for class file: " + f.getName()); And null here again? test/jdk/java/lang/invoke/indify/Indify.java line 383: > 381: if (verbose) System.err.println("writing "+destFile); > 382: Files.write(destFile.toPath(), new_bytes); > 383: System.err.println("Wrote New ClassFile to: "+destFile); This code assumes `dest` is always specified? For me it looks like it diverges from the original behavior. test/jdk/java/lang/invoke/indify/Indify.java line 438: > 436: } catch (Exception ex) { > 437: // pass error from reportPatternMethods, etc. > 438: throw (RuntimeException) ex; Casting all remaining exceptions to `RuntimeException` ? I would suggest to keep the original code or include `RuntimeException` in the fall through section above and wrap all remaining exceptions into a `RuntimeException` to replicate the original behavior. test/jdk/java/lang/invoke/indify/Indify.java line 470: > 468: Logic logic = new Logic(model); > 469: Boolean isChanged = logic.transform(); > 470: if(!isChanged) throw new IOException("No transformation has > been done"); Throwing an exception when there is not change also significantly changes the original behavior. test/jdk/java/lang/invoke/indify/Indify.java line 503: > 501: > 502: Iterator<Instruction> instructionIterator > =getInstructions(m).iterator(); > 503: final Stack<Boolean> shouldProceedAfterIndyAdded = new > Stack<>(); What this stack of booleans suppose to do? test/jdk/java/lang/invoke/indify/Indify.java line 540: > 538: assert (i.sizeInBytes() == 3 || i2.sizeInBytes() > == 3); > 539: > System.err.println("----------------------------------------------------------------Transforming > Method INDY Instructions & Creating New > ClassModels------------------------------------------------------------------}}}"); > 540: if (!quiet) System.err.println(":::Transfmoring > the Method: "+ m.methodName() +" instruction: " + i + " invokedynamic: " + > con.index() ); This debug print should be probably removed before integration. test/jdk/java/lang/invoke/indify/Indify.java line 550: > 548: if (e instanceof InvokeInstruction && > Objects.equals(a1, a2)) { > 549: System.err.println(">> Removing > instruction invokestatic for Method: " + ((InvokeInstruction) e).name()); > 550: b.andThen(b); What this suppose to do? test/jdk/java/lang/invoke/indify/Indify.java line 553: > 551: } else if (shouldProceed.peek() && e > instanceof InvokeInstruction && ((InvokeInstruction) > e).method().equals(((InvokeInstruction) i2).method())) { > 552: System.err.println(">> Removing > instruction invokevirtual for Method: " + ((InvokeInstruction) e).name()); > 553: b.andThen(b); And this? test/jdk/java/lang/invoke/indify/Indify.java line 572: > 570: } else { > 571: assert(i.sizeInBytes() == 3); > 572: > System.err.println("----------------------------------------------------------------Transforming > Method LDC Instructions & Creating New > ClassModels------------------------------------------------------------------}}}"); This debug print should be probably removed before integration. test/jdk/java/lang/invoke/indify/Indify.java line 576: > 574: codeTransform = (b, e) ->{ > 575: String a1 = null, a2 = null; > 576: if(e instanceof InvokeInstruction){ Please use pattern matching and consolidate repeated `(e instanceof InvokeInstruction)`, followed by casting. test/jdk/java/lang/invoke/indify/Indify.java line 578: > 576: classTransform = > ClassTransform.transformingMethodBodies(filter, codeTransform); > 577: classModel = of().parse( > 578: of().transform(classModel, classTransform)); What this transform (in the middle of instructions iteration) does? test/jdk/java/lang/invoke/indify/Indify.java line 582: > 580: if(e instanceof InvokeInstruction && > Objects.equals(a1, a2)){ > 581: System.err.println(":::Transfmoring the > Method: "+ m.methodName() +" instruction: invokestatic " + > ((InvokeInstruction) e).type() + " to ldc: " + con.index() ); > 582: b.constantInstruction(Opcode.LDC_W, > ((LoadableConstantEntry) con).constantValue()); `b.ldc((LoadableConstantEntry) con)` would do the same job, but so many blind casting is not a good pattern. test/jdk/java/lang/invoke/indify/Indify.java line 599: > 597: ClassModel removePatternMethodsAndVerify(ClassModel classModel){ > 598: > 599: ClassModel newClassModel = > of(StackMapsOption.GENERATE_STACK_MAPS).parse( `StackMapsOption.GENERATE_STACK_MAPS` is the default and it is not necessary to set it explicitly. test/jdk/java/lang/invoke/indify/Indify.java line 610: > 608: }) > 609: ); > 610: ClassHierarchyResolver classHierarchyResolver = classDesc -> > ClassHierarchyResolver.ClassHierarchyInfo.ofInterface(); It is not necessary to fake the `ClassHierarchyResolver` for the verification if the above code generates stack maps and is able to resolve all necessary classes. test/jdk/java/lang/invoke/indify/Indify.java line 645: > 643: case GETSTATIC, GETFIELD, PUTSTATIC, PUTFIELD -> > pops.replace("Q", type); > 644: default -> { > 645: if (!type.startsWith("(")) This is very low-level of bytecode instructions parsing. Class-File API already contains high-level models of the instructions and it would be worth to use that models or instead of the tricky up and down conversions and auxiliary structures describing the instructions. test/jdk/java/lang/invoke/indify/Indify.java line 660: > 658: * > 659: * @return true if any marks were changed, false otherwise. > 660: */ This method does incremental analysis of the constant pool. With Class-File API CP model it can be done in one pass, or even in no-pass and all requests to "marks" can be answered directly. test/jdk/java/lang/invoke/indify/Indify.java line 678: > 676: if (!Modifier.isPrivate(m.flags().flagsMask())) > continue; > 677: if (!Modifier.isStatic(m.flags().flagsMask())) > continue; > 678: if(nameAndTypeMark(m.methodName().index(), > m.methodType().index()) == mark) { Passing CP indexes instead of the entries (or maybe String values) is no more necessary. Original code didn't have a full model of the constant pool. test/jdk/java/lang/invoke/indify/Indify.java line 725: > 723: } > 724: if (entry instanceof IntegerEntry integerEntry) { > 725: > clb.constantPool().intEntry(integerEntry.intValue()); This whole code does effectively nothing and can be removed. All the entries present in the `oldClassModel` are iterated and and re-entered into the same constant pool. test/jdk/java/lang/invoke/indify/Indify.java line 809: > 807: } > 808: > 809: switch (poolEntry.tag()) { I would recommend to convert this to a switch expression: mark = switch (poolEntry) { case Utf8Entry utf -> nameMark(utf.stringValue()); case NameAndTypeEntry nat -> nameAndTypeMark(nat.name(), nat.type()); Questionable is why to pass CP indexes to the auxiliary methods (for further entries lookup by index), when the pool entries contain all the necessary information. test/jdk/java/lang/invoke/indify/Indify.java line 956: > 954: case "double" -> 'D'; > 955: default -> throw new > InternalError(argClass.toString()); > 956: }; This code looks like composition of `ClassDesc`, I would look at `Class::descriptorString` test/jdk/java/lang/invoke/indify/Indify.java line 1249: > 1247: } else { > 1248: return null; > 1249: } How does it happen that the argument might be a String or a StringEntry. Also instead of: if (x instanceof PoolEntry && ((PoolEntry) x).tag() == TAG_STRING) { utf8Entry = ((StringEntry) x).utf8(); You can write: if (x instanceof StringEntry se) { utf8Entry = se.utf8(); test/jdk/java/lang/invoke/indify/Indify.java line 1273: > 1271: if(((con = args.get(argi)) instanceof MethodTypeEntry) || > (con instanceof ClassEntry)){ > 1272: assert con instanceof MethodTypeEntry; > 1273: type = ((MethodTypeEntry) con).descriptor(); This is a lot of low-level stuff, where a bunch of entries are filled into a List<Object> and then identified back as entries. It is very hard to read and debug, I would suggest to organize it a type-safe way. test/jdk/java/lang/invoke/indify/Indify.java line 1334: > 1332: if (x instanceof Float) { x = > poolBuilder.floatEntry((Float) x); } > 1333: if (x instanceof Long) { x = > poolBuilder.longEntry((Long) x); } > 1334: if (x instanceof Double) { x = > poolBuilder.doubleEntry((Double) x); } This is a harder way, when you convert all the arguments into entries. `ConstantPoolBuilder::bsmEntry(DirectMethodHandleDesc methodReference, List<ConstantDesc> arguments)` would do it for you. test/jdk/java/lang/invoke/indify/Indify.java line 1368: > 1366: if (count == 0){ > 1367: poolBuilder.utf8Entry("BootstrapMethods"); > 1368: return specs; It is not necessary to manually insert "BootstrapMethods" entry, Class-File API generates all the stuff if you add a bootstrap method, no matter if it is the first one or just another addition. test/jdk/java/lang/invoke/indify/Indify.java line 1403: > 1401: System.err.println(e.getMessage()); > 1402: } > 1403: throw new IOException("Verification failed"); Is there a reason to verify source class files? ------------- Changes requested by asotona (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18841#pullrequestreview-2070398734 PR Review: https://git.openjdk.org/jdk/pull/18841#pullrequestreview-2076472008 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609572765 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609572065 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609564032 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613228454 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609557109 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609557531 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613231918 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613245509 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613247462 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613250834 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609549832 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609555023 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609555309 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609548549 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609547516 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613256739 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609541900 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609536757 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609535430 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609527200 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613260699 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609520089 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609507447 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609498352 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1613263637 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609486000 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609480304 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609473262 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609468693 PR Review Comment: https://git.openjdk.org/jdk/pull/18841#discussion_r1609462205
