RFR: 8337221: CompileFramework: test library to conveniently compile java and jasm sources for fuzzing

[Christian Hagedorn]

On Mon, 15 Jul 2024 15:56:10 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 inevitably breaks in the VM code (i.e. most likely in loop-opts or SuperW...

The framework is a very nice idea! It will definitely improve our testing coverage. It seems simple to use and also easier to review instead of having the fully generated test files to go through.

I did a first pass and left some comments.

Should we also add a `README.md`? You could add good the introduction and motivation in the PR summary to it. Additionally, you can mention the core features, reference your examples, give hints how the JTreg header comment should look like, and also add a section for future work if there is anything you think should/could be explored (not a conclusive list, just a few ideas what you could mention in the `README` :-) )

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 41:

> 39: import java.util.List;
> 40: 
> 41: public class CompileFramework {

General comment here: I suggest to add Javadocs to the public API methods in this class that the user can call.

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 45:

> 43:     private static final boolean VERBOSE = Boolean.getBoolean("CompileFrameworkVerbose");
> 44: 
> 45:     private List<SourceCode> sourceCodes = new ArrayList<SourceCode>();

Can be omitted, same at other places in code:

Suggestion:

    private List<SourceCode> sourceCodes = new ArrayList<>();

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 53:

> 51:         String cp = System.getProperty("java.class.path") +
> 52:                     File.pathSeparator +
> 53:                     classesDir.toAbsolutePath().toString();

`toString()` is implicit and can be removed, same at other places in code:
Suggestion:

                    classesDir.toAbsolutePath();

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 54:

> 52:                     File.pathSeparator +
> 53:                     classesDir.toAbsolutePath().toString();
> 54:         return cp.replace("\\", "\\\\"); // For windows paths

Is this really required and not automatically handled correctly by `Path` on Windows?

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 110:

> 108:     }
> 109: 
> 110:     private void compileJasmSources(List<SourceCode> jasmSources) {

The `compileJasmSources()/compileJasmFiles()` and `compileJavaSources()/compileJavaFiles()` method seem to be very specific to whether it's a Java or Jasm file. I'm wondering if we could do the following:

- Make class `SourceCode` an interface and create two implementing classes `JavaSourceCode` and `JasmSourceCode`.
- Move the `compileJasm/JavaSources()/compileJasm/JavaFiles()` to the corresponding classes (the interface method could be `compile()`).
- This allows us to get ride of the `enum` as well in `SourceCode`. The interface could look like this:

interface SourceCode {
    void compile();
    String code();
    String fileExtension();
    String className();
    String filePathName(); // Could probably just provide the current implementation as default
}

I have not thought it through completely and I might be missing some things - but might be worth a shot :-)

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 111:

> 109: 
> 110:     private void compileJasmSources(List<SourceCode> jasmSources) {
> 111:         if (jasmSources.size() == 0) {

Suggestion:

        if (jasmSources.isEmpty() {

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 123:

> 121: 
> 122:     private void compileJasmFiles(List<Path> paths) {
> 123:         // Compile JASM files with asmtools.jar, shipped with jtreg.

Can be moved to be a method comment instead.

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 160:

> 158: 
> 159:     private void compileJavaSources(List<SourceCode> javaSources) {
> 160:         if (javaSources.size() == 0) {

Suggestion:

        if (javaSources.isEmpty()) {

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 206:

> 204:         } catch (Exception e) {
> 205:             throw new CompileFrameworkException("Could not create directory: " + dir.toString(), e);
> 206:         }

Could be extracted to a method `ensureDirectoryExists()`. Same below (i.e. `writeToFile()`).

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 212:

> 210:             writer.write(code);
> 211:         } catch (Exception e) {
> 212:             throw new CompileFrameworkException("Could not write file: " + path.toString(), e);

Suggestion:

            throw new CompileFrameworkException("Could not write file: " + path, e);

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 266:

> 264:     }
> 265: 
> 266:     public Class getClass(String name) {

You should not use `Class` without type parameter. Suggestion:
Suggestion:

    public Class<?> getClass(String name) {

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 275:

> 273: 
> 274:     public Object invoke(String className, String methodName, Object[] args) {
> 275:         Class c = getClass(className);

Suggestion:

        Class<?> c = getClass(className);

test/hotspot/jtreg/compiler/lib/compile_framework/CompileFramework.java line 292:

> 290:         if (method == null) {
> 291:             throw new CompileFrameworkException("Method \"" + methodName + "\" not found in class \n" + className + "\".");
> 292:         }

Could be extracted to a method which returns a non-null method object (i.e. `findMethod()`) or something like that.

test/hotspot/jtreg/compiler/lib/compile_framework/SourceCode.java line 29:

> 27:  * This class represents the source code of a specific class.
> 28:  */
> 29: public class SourceCode {

I'm wondering if this class even needs to be exposed? AFAICS, you always do:

compileFramework.add(SourceCode.newJava/JasmSourceCode());
``` 
This raises the question if you should just directly provide a `addJava/JasmSourceCode()` API method in `CompileFramework`?

test/hotspot/jtreg/compiler/lib/compile_framework/SourceCode.java line 34:

> 32:     public final String className;
> 33:     public final String code;
> 34:     public final Kind kind;

I think you can make this `private` since you do not expose this enum.
Suggestion:

    private final Kind kind;

test/hotspot/jtreg/compiler/lib/compile_framework/SourceCode.java line 36:

> 34:     public final Kind kind;
> 35: 
> 36:     public SourceCode(String className, String code, Kind kind) {

Since you have static initializers, you can also make this constructor private.
Suggestion:

    private SourceCode(String className, String code, Kind kind) {

test/hotspot/jtreg/compiler/lib/compile_framework/SourceCode.java line 57:

> 55:         StringBuilder builder = new StringBuilder();
> 56:         String extension = this.kind.name().toLowerCase();
> 57:         builder.append(this.className.replace('.','/')).append(".").append(extension);

`this` is not required:

Suggestion:

        return kind.name().toLowerCase();
    }

    public String filePathName() {
        StringBuilder builder = new StringBuilder();
        String extension = kind.name().toLowerCase();
        builder.append(className.replace('.','/')).append(".").append(extension);

-------------

PR Review: https://git.openjdk.org/jdk/pull/20184#pullrequestreview-2305813129
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760600362
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760589000
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760589787
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760602492
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760731871
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760639645
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760640431
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760592083
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760733614
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760733767
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760736648
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760736892
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760740227
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760636551
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760620303
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760621415
PR Review Comment: https://git.openjdk.org/jdk/pull/20184#discussion_r1760622702