RFR: 8337221: CompileFramework: test library to conveniently compile java and jasm sources for fuzzing [v13]
Christian Hagedorn
chagedorn at openjdk.org
Tue Sep 17 09:17:18 UTC 2024
On Mon, 16 Sep 2024 15:12:33 GMT, Emanuel Peter <epeter at openjdk.org> wrote:
>> **Motivation**
>>
>> I want to write small dedicated fuzzers:
>> - Generate `java` and `jasm` source code: just some `String`.
>> - Quickly compile it (with this framework).
>> - Execute the compiled code.
>>
>> The primary users of the CompileFramework are Compiler-Engineers. Imagine you are working on some optimization. You already have a list of **hand-written tests**, but you are worried that this does not give you good coverage. You also do not trust that an existing Fuzzer will catch your bugs (at least not fast enough). Hence, you want to **script-generate** a large list of tests. But where do you put this script? It would be nice if it was also checked in on git, so that others can modify and maintain the test easily. But with such a script, you can only generate a **static test**. In some cases that is good enough, but sometimes the list of all possible tests your script would generate is very very large. Too large. So you need to randomly sample some of the tests. At this point, it would be nice to generate different tests with every run: a "mini-fuzzer" or a **fuzzer dedicated to a compiler feature**.
>>
>> **The CompileFramework**
>>
>> Java sources are compiled with `javac`, jasm sources with `asmtools` that are delivered with `jtreg`.
>> An important factor: Integration with the IR-Framwrork (`TestFramework`): we want to be able to generate IR-rules for our tests.
>>
>> I implemented a first, simple version of the framework. I added some tests and examples.
>>
>> **Example**
>>
>>
>> CompileFramework comp = new CompileFramework();
>> comp.add(SourceCode.newJavaSourceCode("XYZ", "<source-code-here>"));
>> comp.compile();
>> comp.invoke("XYZ", "test", new Object[] {5}); // XYZ.test(5);
>>
>>
>> https://github.com/openjdk/jdk/blob/e869cce8092ee995cf2f3ad1ab2bca69c5e256ab/test/hotspot/jtreg/testlibrary_tests/compile_framework/examples/SimpleJavaExample.java#L42-L74
>>
>> **Below some use cases: tests that would have been better with the CompileFramework**
>>
>> **Use case: test/hotspot/jtreg/compiler/loopopts/superword/TestAlignVectorFuzzer.java**
>>
>> I needed to test loops with various `init / stride / limit / scale / unrolling-factor / ...`.
>>
>> For this I used `MethodHandle constant = MethodHandles.constant(int.class, value);`,
>>
>> to be able to chose different values before the C2 compilation, and then the C2 compilation would see them as constants and optimize assuming those constants. This works, but is difficult to extract reproducers once something i...
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>
> move some code around
Thanks for the updates - already looks much better! Some more comments.
test/hotspot/jtreg/compiler/lib/compile_framework/ClassLoaderBuilder.java line 37:
> 35: * Build a ClassLoader that loads from classpath and {@code classesDir}.
> 36: * Helper class that generates a ClassLoader which allows loading classes
> 37: * from the classpath (see {@code Utils.getClassPaths()}) and {@code classesDir}.
You can use a `@link` which you can follow inside an IDE:
Suggestion:
* from the classpath (see {@link Utils#getClassPaths()}) and {@code classesDir}.
test/hotspot/jtreg/compiler/lib/compile_framework/ClassLoaderBuilder.java line 38:
> 36: * Helper class that generates a ClassLoader which allows loading classes
> 37: * from the classpath (see {@code Utils.getClassPaths()}) and {@code classesDir}.
> 38: *
Suggestion:
* <p>
test/hotspot/jtreg/compiler/lib/compile_framework/ClassLoaderBuilder.java line 52:
> 50: try {
> 51: // Classpath for all included classes (e.g. IR Framework).
> 52: // Get all class paths, convert to urls.
Suggestion:
// Get all class paths, convert to URLs.
test/hotspot/jtreg/compiler/lib/compile_framework/ClassLoaderBuilder.java line 53:
> 51: // Classpath for all included classes (e.g. IR Framework).
> 52: // Get all class paths, convert to urls.
> 53: List<URL> urls = new ArrayList<URL>();
Suggestion:
List<URL> urls = new ArrayList<>();
test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 33:
> 31:
> 32: /**
> 33: * This is the entry-point for the Compile Framework. Its purpose it to allow
General comment about Javadocs. I think the convention is the indent to the first `*`:
Suggestion:
/**
* This is the entry-point for the Compile Framework. Its purpose it to allow
test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 35:
> 33: * This is the entry-point for the Compile Framework. Its purpose it to allow
> 34: * compilation and execution of Java and Jasm sources generated at runtime.
> 35: *
Blank lines are ignored in Javadocs:
You can add `<p>` which will add a new line in between:
(Rendered Javadocs by IDEA)
test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 36:
> 34: * compilation and execution of Java and Jasm sources generated at runtime.
> 35: *
> 36: * Please reference the README.md for more explanation.
Details?
Suggestion:
* Please reference the README.md for more details.
test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 40:
> 38: public class CompileFramework {
> 39: private List<SourceCode> javaSources = new ArrayList<>();
> 40: private List<SourceCode> jasmSources = new ArrayList<>();
Can also be made final
Suggestion:
private final List<SourceCode> javaSources = new ArrayList<>();
private final List<SourceCode> jasmSources = new ArrayList<>();
test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 57:
> 55: public void addJasmSourceCode(String className, String code) {
> 56: jasmSources.add(new SourceCode(className, "jasm", code));
> 57: }
Since this is the public API that the user should use, I suggest to also add a parameter description for all public API methods in this class with `@param` for completeness. Something like:
/**
* Add a Jasm source to the compilation.
*
* @param className The class name of the Jasm class.
* @param code The source code of the Jasm class as string.
*/
which renders to:
test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 59:
> 57: }
> 58:
> 59: private String sourceCodesAsString(List<SourceCode> sourceCodes) {
Just a side node, since this is not C where you need to define the methods first and then use it, I suggest to do it the other way round: Define methods below as they are used. This makes it easier to read I think
test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 72:
> 70: * Java and Jasm sources and store the generated class-files in the classes
> 71: * directory.
> 72: */
Not sure if it is clear what the sources directory and the classes directory are when just reading this comment. Do you want to mention the actual name of these directories? You could do something like:
Suggestion:
/**
* Compile all sources: store the sources to the {@link sourceDir} directory, compile
* Java and Jasm sources and store the generated class-files in the {@link classesDir}
* directory.
*/
test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 87:
> 85:
> 86: Compile.compileJasmSources(jasmSources, sourceDir, classesDir);
> 87: Compile.compileJavaSources(javaSources, sourceDir, classesDir);
Suggestion: Instead of having two static methods, how about making the `sourceDir` and `classesDir` fields of `Compile`? Then you can reuse them inside the class without passing them around.
test/hotspot/jtreg/compiler/lib/compile_framework/README.md line 2:
> 1: # Compile Framework
> 2: This compile framework allows the compilation and execution of Java and Jasm sources, which are generated at runtime.
I think you can use the given name "Compile Framework" here.
Suggestion:
The Compile Framework allows the compilation and execution of Java and Jasm sources, which are generated at runtime.
test/hotspot/jtreg/compiler/lib/compile_framework/README.md line 7:
> 5: We want to be able to generate Java and Jasm source code in the form of Strings at runtime, then compile them, load the classes and invoke some methods. This allows us to write more elaborate tests. For example small dedicated fuzzers that are targetted at some specific compiler optimization.
> 6:
> 7: This is more powerful than hand-written tests, as we can generalize tests and cover more examples. It can also be better than a script-generated test: those are static and often the script is not checked in with the test. Also, the script is only run once, giving a static tests. Compilation at runtime allows us to randomly generate tests each time.
Suggestion:
This is more powerful than hand-written tests, as we can generalize tests and cover more examples. It can also be better than a script-generated test: those are static and often the script is not integrated with the generated test. Another limitation of a generator script is that it is only run once, creating fixed static tests. Compilation at runtime allows us to randomly generate tests each time.
test/hotspot/jtreg/compiler/lib/compile_framework/README.md line 11:
> 9: Of course we could compile at runtime without this framework, but it abstracts away the complexity of compilation, and allows the test-writer to focus on the generation of the source code.
> 10:
> 11: ## How to Use the Framework
Suggestion:
## How to Use the Compile Framework
test/hotspot/jtreg/compiler/lib/compile_framework/README.md line 18:
> 16:
> 17: // Create a new CompileFramework instance.
> 18: CompileFramework comp = new CompileFramework();
Just a side note: Should we be explicit here and name the variable `compileFramework` since it is an illustrating how-to example?
test/hotspot/jtreg/compiler/lib/compile_framework/README.md line 27:
> 25:
> 26: // Object ret = XYZ.test(5);
> 27: Object ret = comp.invoke("XYZ", "test", new Object[] {5});
Same here, should we name it `returnValue` instead?
test/hotspot/jtreg/compiler/lib/compile_framework/README.md line 31:
> 29: ### Creating a new Compile Framework Instance
> 30:
> 31: First, one must create a `new CompileFramework()`, which creates two directories: a sources and a classes directory. The sources directory is where all the sources are placed by the Compile Framework, and the classes directory is where all the compiled classes are placed by the Compile Framework.
You could add here that this is a fixed-named directory created inside the JTreg scratch directory. You can also add a link to the actual name where this is created inside `CompileFramework`.
test/hotspot/jtreg/compiler/lib/compile_framework/README.md line 35:
> 33: ### Adding Sources to the Compilation
> 34:
> 35: Java and Jasm sources can be added to the compilation using `comp.addJavaSourceCode` and `comp.addJasmSourceCode`. The source classes can depend on each other, and they can also use the IR-Framework ([TestFrameworkJavaExample](../../../testlibrary_tests/compile_framework/examples/TestFrameworkJavaExample.java)).
Suggestion:
Java and Jasm sources can be added to the compilation using `comp.addJavaSourceCode()` and `comp.addJasmSourceCode()`. The source classes can depend on each other, and they can also use the IR Framework ([TestFrameworkJavaExample](../../../testlibrary_tests/compile_framework/examples/TestFrameworkJavaExample.java)).
test/hotspot/jtreg/compiler/lib/compile_framework/README.md line 39:
> 37: ### Compiling
> 38:
> 39: All sources are compiled with `comp.compile()`. First, the sources are stored to the srouces directory, then compiled, and then the class-files stored in the classes directory. The respective directory names are printed, so that the user can easily access the generated files for debugging.
Suggestion:
All sources are compiled with `comp.compile()`. First, the sources are stored to the sources directory, then compiled, and then the class-files stored in the classes directory. The respective directory names are printed, so that the user can easily access the generated files for debugging.
test/hotspot/jtreg/compiler/lib/compile_framework/README.md line 41:
> 39: All sources are compiled with `comp.compile()`. First, the sources are stored to the srouces directory, then compiled, and then the class-files stored in the classes directory. The respective directory names are printed, so that the user can easily access the generated files for debugging.
> 40:
> 41: ### Interacting with the compiled code
Suggestion:
### Interacting with the Compiled Code
test/hotspot/jtreg/compiler/lib/compile_framework/README.md line 43:
> 41: ### Interacting with the compiled code
> 42:
> 43: The compiled code is then loaded with a ClassLoader. The classes can be accessed directly with `comp.getClass(name)`. Specific methods can also be directly invoked with `comp.invoke`.
Suggestion:
The compiled code is then loaded with a `ClassLoader`. The classes can be accessed directly with `comp.getClass(name)`. Specific methods can also directly be invoked with `comp.invoke()`.
test/hotspot/jtreg/compiler/lib/compile_framework/README.md line 45:
> 43: The compiled code is then loaded with a ClassLoader. The classes can be accessed directly with `comp.getClass(name)`. Specific methods can also be directly invoked with `comp.invoke`.
> 44:
> 45: Should one require the modified classpath that includes the compiled classes, this is available with `comp.getEscapedClassPathOfCompiledClasses()`. This can be necessary if the test launches any other VM's that also access the compiled classes. This is for example necessary when using the IR-Framework.
The IR Framework is usually written without `-`:
Suggestion:
Should one require the modified classpath that includes the compiled classes, this is available with `comp.getEscapedClassPathOfCompiledClasses()`. This can be necessary if the test launches any other VMs that also access the compiled classes. This is for example necessary when using the IR Framework.
test/hotspot/jtreg/compiler/lib/compile_framework/Utils.java line 54:
> 52:
> 53: /**
> 54: * Create a temporary directory, with a unique name, so that there can be no collisions
Suggestion:
* Create a temporary directory with a unique name to avoid collisions
test/hotspot/jtreg/compiler/lib/compile_framework/Utils.java line 167:
> 165: System.out.println("Compilation failed.");
> 166: System.out.println("Exit code: " + exitCode);
> 167: System.out.println("Output: '" + output + "'");
You could print to System.err:
Suggestion:
System.err.println("Compilation failed.");
System.err.println("Exit code: " + exitCode);
System.err.println("Output: '" + output + "'");
test/hotspot/jtreg/testlibrary_tests/compile_framework/examples/CombinedJavaJasmExample.java line 37:
> 35:
> 36: /**
> 37: * This test shows a compilation of multiple java and jasm source code files.
Suggestion:
* This test shows a compilation of multiple Java and Jasm source code files.
test/hotspot/jtreg/testlibrary_tests/compile_framework/examples/TestFrameworkJavaExample.java line 49:
> 47: * classes (see {@code getEscapedClassPathOfCompiledClasses}).
> 48: */
> 49: public class TestFrameworkJavaExample {
Suggestion: This might be better named `IRFrameworkJavaExample`. It is a little bit unfortunate that I named the IR Framework main class `TestFramework` and not `IRFramework`...but I guess it's too late for that now.
-------------
PR Review: https://git.openjdk.org/jdk/pull/20184#pullrequestreview-2308665738
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762645614
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762643040
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762648591
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762648148
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762601530
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762562055
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762562856
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762563191
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762573429
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762577022
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762594396
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762608194
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762660512
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762668935
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762671770
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762674541
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762675097
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762677358
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762678789
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762706985
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762710573
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762713487
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762681300
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762720898
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762732870
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762737328
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1762689463
More information about the hotspot-compiler-dev
mailing list