AJC compilation with --release is not working

[kriegaex]

Inherited by ECJ, the AspectJ batch compiler accepts the --release argument in order to compile not only to a byte code target, say Java 8, but actually to the corresponding JRE's API, ensuring that the code is actually going to run on the target platform instead of running into API compatibility issues.

Unfortunately, compiling to e.g. --release 8 is not working as expected. I think that since Java 9 adoption in 1.9.0 nobody ever tested that, at least I cannot find any test cases. Compilation passes, but later runtime issues ensue. I noticed by chance when simply trying after reading JDT Core issue 574181, using the sample project there, manually compiling with

• Javac (working),
• ECJ 3.25.0 (working),
• AJC 1.9.7.M3 (compiling, but runtime error when running the JUnit test on JDK 8).

When compiling something like this on JDK 11+ (I tried on JDK 16) with --release 8

ByteBuffer buffer = ByteBuffer.allocate(10);
Buffer flipped = buffer.flip();

and then running the code on JDK 8, it should work. Instead, with AJC we see something like

java.lang.NoSuchMethodError: java.nio.ByteBuffer.flip()Ljava/nio/ByteBuffer;
        at jdktest.Buffers.dummy(Buffers.java:14)
        at jdktest.BuffersTest.testBuffer(BuffersTest.java:11)

because in JDK 8 there simply was not ByteBuffer.flip(), only Buffer.flip(). I.e., in this case compiling with -source 8 -target 8 is insufficient. BTW, the combination of -8 --release 8 is illegal, if anyone wanted to suggest trying that.

I have not investigated yet, why it is not working. Maybe a parameter is not passed on as it should, maybe we do not forward to the actual batch compiler but talking to ECJ's JSR-199 API, running into the same bug 574181. This is pure speculation. I am just creating this issue, because I think it should be fixed before the 1.9.7 release, because it is a major issue IMO.

@aclement, do you agree that we should fix it before the release, or does it mean that Eclipse needs to re-approve?

[aclement]

If it didn't work in 1.9.6 that drastically lessens the urgency for me. However, there would be no need to re-approve anything if you want to dig into it. I suspect you are right about options passing through given it works on ECJ.

Just to indicate where my level is - issues that tend to be urgent for me are the red ones on https://bugs.eclipse.org/bugs/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&f0=OP&f1=OP&f3=CP&f4=CP&j1=OR&list_id=20738136&product=AspectJ&query_format=advanced - blockers that are preventing folks from getting their work done.

Unfortunately on GitHub we don't have that kind of severity indicator setup yet.

[kriegaex]

If it didn't work in 1.9.6 that drastically lessens the urgency for me.

It never worked before, I guess. I just tried 1.9.4 and 1.9.6 in order to be sure.

Just to indicate where my level is (...) blockers that are preventing folks from getting their work done.

I see, thanks.

Unfortunately on GitHub we don't have that kind of severity indicator setup yet.

How far are we with

• getting me more privileges both here (GitHub, i.e. AspectJ and JDT Core repos) and on Bugzilla, so I can help with configuration, bug management, merging PRs?
• migrating AJDT to GitHub + access rights for me?

I wish to be able to support you more in these regards, hopefully in exchange for your support in other areas with what little time you might win with me unburdening you a bit.

[kriegaex]

What I have found out so far is

• --release N is handled as an unparsed option and added to the option string passed on to the JDT Core args parser, where it is recognised and handled correctly.
• But henceforth, AspectJ ignores it.
• According to JEP-247 and the nice explanation here, basically the --release N option is semantically equivalent with -source N -target N -bootclasspath <bootclasspath-from-N>. But AJC simply sets the bootclasspath from the current JDK, which is wrong.
• The bootclasspath issue is not trivial, though, because the compiler needs to look into $JDK_ROOT/lib/ct.sym, which is a ZIP file containing API information about previous JDKs, where differences to the current one exist. In JDT Core, this is abstracted into classes like CtSym, but also ClasspathJrtWithReleaseOption and JRTUtil.JrtFileSystemWithOlderRelease.
• If you want to know more about $JDK_ROOT/lib/ct.sym, please read this article. I found out about the details by myself, but the article (which I found later) is a convenient shortcut. The gist of it is that the *.sig files are simply class files which only contain class skeletons with empty methods, as can be seen when decompiling or disassembling those files using javap.

Probably somehow we need to make sure that delegation to those JDT Core classes occurs, which currently is not the case, because the AJC batch compiler does not extend the ECJ batch compiler, but only uses a subclass of it for argument parsing, then doing its own thing. So possibly we need to replicate something from there into our batch compiler, which does not sound so nice, but is the current state of affairs. I am afraid that this might be above my current pay grade, and I know you are busy, @aclement. But nevertheless it is kind of a glaring hole in our compiler since 1.9.0. I do see the priority way higher than you. For me this would be red in Bugzilla, because it is a major feature in any Javac-compatible Java compiler since Java 9.

[kriegaex]

I had a few minutes to conduct another little experiment: compiling with the regular ECJ batch compiler, packaged inside aspectjtools as the relocated class org.aspectj.org.eclipse.jdt.internal.compiler.batch.Main. In this case --release N works, so at least we know that the AspectJ modifications did not destroy any existing functionality, but that

• either we are hitting JDT bug 574181, as mentioned in the issue description,
• or somewthing is wrong about the way we handle the boot classpath,
• or maybe both.

[aclement]

As far as I can see, the option gets through just fine. Breakpoint on the --release handling in JDT Core Main.java we see it handled and the compliance value set. Subsequently in CompilerOptions.set(Map) we see OPTION_Compliancebeing processed and it being set into the map correctly.

My suspicions then fall on MessageSend.resolveType() where the actual receiver type is computed for the flip call. I worry one of the aspects is perhaps interfering but what I'd be doing next is probably running JDT core alongside AspectJ JDT Core, stepping through each in MessageSend type resolution and seeing where the mistake is made.

[aclement]

I wasn't totally happy with that train of thought when I woke up this morning. Dug around a little more. The problem is in here I would suspect: AjBuildManager.getLibraryAccess where you can see we create FileSystem objects without supplying the final parameter which would be the release. I haven't built the solution to test it, but it looks like it will be that. There is a build config options object that hopefully has the OPTION_Compliance value set. How did I get to that point? I grepped the git history for the JDT Core project for --release to see how it was first implemented, that pointed me to some classes to dig around in. Kinded ended up with where you were looking ClasspathJrtWithReleaseOption.

[kriegaex]

I was quite busy and did not do too much digging. At the same time, I was watching and commenting on JDT bug 574181 in order to maybe cherry-pick from the bugfix (in progress) there, so as to avoid that we indirectly run into the same problem, if for some reason we might end up on one of the execution paths not setting the compiler options correctly.

Why cherry-pick? Because I want to avoid a big JDT 4.19 to 4.20 upgrade merge before the AspectJ 1.9.7 release. My last merge point was after the initial Java 16 merge onto master there, so I think we have what we need. Not sure how much has changed there since the last merge, but 4.20 now features Java 16 support which came shortly after 4.19 and had to be installed in Eclipse via Marketplace. So I guess, if there is anything new, it is bugfixes.

[kriegaex]

Do you think that maybe https://github.com/eclipse/org.aspectj/blob/d17189c430a7ffd1ec966759a93b3ed348766650/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/ajc/BuildArgParser.java#L204

is too simple and

https://github.com/eclipse/org.aspectj/blob/d17189c430a7ffd1ec966759a93b3ed348766650/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/ajc/BuildArgParser.java#L349

needs to handle the boot classpath specially, using ClasspathJrtWithReleaseOption or so? Or is this the wrong place and should be handled by JDT Core internally? I am really just speculating. But having the notion in my mind that --release N actually means to use the boot classpath from another JDK (which in this case means, reading information from ct.sym), I know that at some point this needs to be done.

[kriegaex]

OK, debugging in parallel through AJC vs. ECJ batch compilers, shows a difference in method parameter Classpath[] paths:

AJC:

ECJ:

Both times, the methods is called by a method getLibraryAccess, which in turn is called my a method performCompilation, but those method pairs are in different classes:

• ECJ: org.aspectj.org.eclipse.jdt.internal.compiler.batch.Main
• AJC: org.aspectj.ajdt.internal.core.builder.AjBuildManager

Either the naming is a coincidence or AJC AjBuildManager is actually mimicking ECJ Main. Not knowing the project history, I am assuming the latter.

[kriegaex]

I think, this is what we need to emulate in AJC, possibly in AjBuildConfig.computeCheckedClasspath():

https://github.com/eclipse/org.aspectj/blob/d17189c430a7ffd1ec966759a93b3ed348766650/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/internal/core/builder/AjBuildConfig.java#L924

[kriegaex]

I noticed that member BuildArgParser.checkedClasspaths actually contains a ClasspathJep247 instance, i.e. the upstream build config is created correctly, handling --release N as expected. I exploited that fact for a quick & dirty proof-of-concept (PoC) hack, just in order to see if the compilation result is correct. I simply added a getter for the protected member and used this ugly hack in order to check for ClasspathJep247 elements:

--- a/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/ajc/BuildArgParser.java	(revision HEAD)
+++ b/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/ajc/BuildArgParser.java	(revision Staged)
@@ -415,6 +415,10 @@
 		return ret;
 	}
 
+	public FileSystem.Classpath[] getCheckedClasspaths() {
+		return checkedClasspaths;
+	}
+
 	private void addExtDirs(String extdirs, List classpathCollector) {
 		StringTokenizer tokenizer = new StringTokenizer(extdirs, File.pathSeparator);
 		while (tokenizer.hasMoreTokens()) {
--- a/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/internal/core/builder/AjBuildConfig.java	(revision HEAD)
+++ b/org.aspectj.ajdt.core/src/main/java/org/aspectj/ajdt/internal/core/builder/AjBuildConfig.java	(revision Staged)
@@ -19,6 +19,7 @@
 import java.io.File;
 import java.io.FileFilter;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
@@ -27,9 +28,11 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.StringTokenizer;
+import java.util.stream.Collectors;
 
 import org.aspectj.ajdt.ajc.BuildArgParser;
 import org.aspectj.ajdt.internal.compiler.CompilationResultDestinationManager;
+import org.aspectj.org.eclipse.jdt.internal.compiler.batch.ClasspathJep247;
 import org.aspectj.org.eclipse.jdt.internal.compiler.batch.ClasspathLocation;
 import org.aspectj.org.eclipse.jdt.internal.compiler.batch.FileSystem;
 import org.aspectj.org.eclipse.jdt.internal.compiler.batch.FileSystem.Classpath;
@@ -930,7 +933,10 @@
 
 		// ArrayList<Classpath> allPaths = handleBootclasspath(bootclasspaths, customEncoding);
 		ArrayList<FileSystem.Classpath> allPaths = new ArrayList<>();
-	 	allPaths.addAll(processStringPath(bootclasspath, encoding));
+		if (Arrays.stream(buildArgParser.getCheckedClasspaths()).anyMatch(cp -> cp instanceof ClasspathJep247))
+			allPaths.addAll(Arrays.stream(buildArgParser.getCheckedClasspaths()).filter(cp -> cp instanceof ClasspathJep247).collect(Collectors.toList()));
+		else
+			allPaths.addAll(processStringPath(bootclasspath, encoding));
 		allPaths.addAll(processFilePath(inJars, encoding));
 	 	allPaths.addAll(processFilePath(inPath, encoding));
 	 	allPaths.addAll(processFilePath(aspectpath, encoding));

Rationale: I simply use the existence of a ClasspathJep247 classpath member as a proxy marker for this condition in ECJ:

	if (this.releaseVersion != null && this.complianceLevel < jdkLevel) {

This is of course not beautiful, hence me posting it in a comment rather than a PR. But what can I say? The compilation result is correct, the subsequent test on JDK 8 passes, proving that the correct API was used for compilation.

Now we have a clue what could be done and a PoC that in theory it can be done without changing anything in JDT Core. @aclement, maybe you can take it from here and advise what to do next and how you would like it to be implemented.

[aclement]

some good digging there, you know this bit of the code better than me now. Really I did the absolute bare minimum to support the Java repackaging that occurred when modules arrived in 9, so yes, anything that goes beyond the test cases we have (like testing release level) is bound to be missing.

Where you are fiddling there with the path, the options are available

		String optionCompliance = options.getMap().get(CompilerOptions.OPTION_Compliance);
		long complianceLevel = options.complianceLevel;

just not quite sure the jdk level is accessible there currently, will have to have a look around.

[kriegaex]

Where you are fiddling there with the path, the options are available

Yes, I had noticed that.

just not quite sure the jdk level is accessible there currently, will have to have a look around.

I had not found it when looking around, which is why I used the trick to directly use the path already detected by ECJ, which not only saved me the effort to re-create and re-evaluate the same conditions ECJ already must have evaluated, but also directly gave me the classpath type needed to deal with the ct.sym layer instead of having to also re-create that, mimicking ECJ. Cheap trick, but it works.

I would like to hand over to you to devise a better solution, if mine is too hacky. I am also unsure, if mine covers all possible cases or just works in my simple test case. But the idea to use more existing information and infrastructure from ECJ instead of re-building it in AJC is a good one, of course not necessarily the way I did it for the PoC.

---

Update: Because 1.9.7 is out already, I guess we should assign this to the 1.9.8 milestone.

[kriegaex]

@aclement, the PR was merged, so unless you wish to revise the simple solution I implemented, we could close this issue.

[aclement]

needs assigning to a release but I don't believe we've decided on that being 1.9.8 or not...

[kriegaex]

Given the fact that is has been merged after the 1.9.7 release, just like #77, I thought it was a no-brainer to assign it to 1.9.8. Both PRs are already contained in 1.9.8.M1, too. So unless you want to create an extra milestone for 1.9.8.M1, I think 1.9.8 it is.

If however you are referring to our conversation about adjusting the AspectJ release numbers to the corresponding JDK releases, e.g. the next release being something like 1.17, we can always change that later, if we decide to go that way instead of 1.9.8.