ECJ fails to track operand stack size across branch edges

[srikanth-sankaran]

What it does

• Fixes VerifyError with code with record patterns and Optional #2160
• Revert fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=544298 and refix
• Revert fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=544556 and refix
• Revert fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=544204 and refix
• Fix arbitrary and incorrect stack manipulation code in string concatenation invoke dynamic call
• Fix incorrect stack manipulation in string templates invoke dynamic call
• Fix argument and receiver size calculation error for captured outer locals
• Replace various ad-hoc stack adjustment calls with structured stack size tracking
• Fixes [Switch Expression] NPE in StackMapFrameCodeStream.recordExpressionType #2072

How to test

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[srikanth-sankaran]

@laeubi - is there a way I can trigger an I build without merging to get a preview of (a) any build issues due to new asserts added to the compiler (b) tests status (c) comparator errors ? ?

I have JDT specific data from the PR here : https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-2171/12/consoleText and that indicates massive changes - nearly 100+ class files change. Basically there is a bug fix to indy string concatenation - it was computing the stack size incorrectly and it is now rectified. Being a common operation string concatenation code generation change will result in large number of differences.

I haven't yet sanity checked JDT comparator differences - I will before the merge,

[laeubi]

This one shows how to collect the comparator data but it was not merged and now has conflicts, maybe you still want to enable these options in either a local build or for your PR:

• Collect artifact comparison delta #2100

[srikanth-sankaran]

This one shows how to collect the comparator data but it was not merged and now has conflicts, maybe you still want to enable these options in either a local build or for your PR:

• Collect artifact comparison delta #2100

Hmm. I think for now, I will validate JDT class files changed + get code review before merge. Thanks.

[mpalat]

please see the interim review comments

[mpalat]

Rest of the code looks good

[srikanth-sankaran]

Question still remains what we should do when we see an instruction with a label which has never so far been branched into (i.e., no forward branches) and is preceded by a goto or return or throw - ATM we don't do anything and wait for a backward branch. It may not be unreasonable to update BranchLabel.targetStackDepth with CodeStream.stackDepth

[srikanth-sankaran]

The last commit introduces further asserts during code generation to ensure operand stack is empty after a full statement's translation. These asserts also help catch a problem in indy string concat code generation. There was a wrong decrement of stackDepth by 189.

[laeubi]

@srikanth-sankaran this is now merged:

• Collect artifact comparison delta #2100

so if you rebase your PR you should get artifact comparison zips for every project that has a baseline difference.

[akurtakov]

I took the freedom to rebase it so we can see it in action.

[srikanth-sankaran]

@srikanth-sankaran this is now merged:

• Collect artifact comparison delta #2100

so if you rebase your PR you should get artifact comparison zips for every project that has a baseline difference.

When you say every project - you mean every project in jdt.core ? In my conversation with @iloveeclipse yesterday I understood it to be the case that when a JDT PR is raised, the proposed code is used to build jdt core projects but rest of SDK will get built using the PR's changes only post merge to master

[srikanth-sankaran]

I took the freedom to rebase it so we can see it in action.

No problem. Is https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-2171/14/artifact/ the place one would look for files that differ ? In particular does the single download link labelled "(all files in zip)" suffice ?

[laeubi]

You can download all or for example individual items like
https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-2171/14/artifact/org.eclipse.jdt.compiler.tool.tests/target/artifactcomparison/no-classifier/

or even individual class files:

https://ci.eclipse.org/jdt/job/eclipse.jdt.core-Github/job/PR-2171/14/artifact/org.eclipse.jdt.compiler.tool.tests/target/artifactcomparison/no-classifier/org/eclipse/jdt/compiler/tool/tests/

[trancexpress]

Opened issue for the comparator errors: eclipse-platform/eclipse.platform.releng.aggregator#1923

@srikanth-sankaran , @sravanlakkimsetti , how do we continue here? Who checks if the changes are expected?

Ideally we would have had 2 builds between the changes from #2181 and #2171, unfortunately we have only 1 build for both changes. So we don't know which PR results in which comparator errors.

[laeubi]

Ideally we would have had 2 builds between the changes from #2181 and #2171, unfortunately we have only 1 build for both changes. So we don't know which PR results in which comparator errors.

This is only partially true. If you look at the log then search for eclipse.jdt.core you will see that these comparator errors would have been detectable and analyzed as part of the PR itself.

Its a bit unfortunate that this is not done, I would have expsected that a PR is never merged with comparator errors in JDT as otherwise build failures are guaranteed!

[srikanth-sankaran]

Ideally we would have had 2 builds between the changes from #2181 and #2171, unfortunately we have only 1 build for both changes. So we don't know which PR results in which comparator errors.

This is only partially true. If you look at the log then search for eclipse.jdt.core you will see that these comparator errors would have been detectable and analyzed as part of the PR itself.

Its a bit unfortunate that this is not done, I would have expsected that a PR is never merged with comparator errors in JDT as otherwise build failures are guaranteed!

Quoting from an earlier comment in this thread:

"I have inspected several dozens of different class files from jdt projects and they all seem reasonable. Unfortunately this PR fixes an issue in a super-common operation: string concatenation with + that it is going to trigger gazillion comparator errors in the SDK build.

The only difference pattern is MAXSTACK estimate going down wrt to baseline; The new value can be lower by as many makeConcatWithConstants invokedynamic instructions needed in a statement in the method."

[srikanth-sankaran]

@laeubi I am trying to comprehend what you mean by "PR is never merged with comparator errors in JDT" - in this case there are comparator errors and having inspected several dozens of differing class files I concluded these are along the expected lines. So no bug is suspected. What should I have done differently before merging ??

This is NOT asked confrontationally.
I am happy to adopt a different methodology if prescribed. At the moment I don't know what alternatives exist.

Thanks for educating/enlightening me.

[srikanth-sankaran]

Opened issue for the comparator errors: eclipse-platform/eclipse.platform.releng.aggregator#1923

@srikanth-sankaran , @sravanlakkimsetti , how do we continue here? Who checks if the changes are expected?

Ideally we would have had 2 builds between the changes from #2181 and #2171, unfortunately we have only 1 build for both changes. So we don't know which PR results in which comparator errors.

Thanks @trancexpress : I'll follow up by inspecting the class files. I'll rope in help from @mpalat and @jarthana as needed.

Help from any one else is welcome too.

The only difference pattern is MAXSTACK estimate going down wrt to baseline; The new value can be lower by as many makeConcatWithConstants invokedynamic instructions needed in a statement in the method."

Anything else needs to be analyzed for suspect change.

[trancexpress]

Its a bit unfortunate that this is not done, I would have expsected that a PR is never merged with comparator errors in JDT as otherwise build failures are guaranteed!

Sorry, JDT core is its own repository. You expect a PR in it to be blocked by the rest of the Eclipse platform having some checker that complains about the latest code in JDT core? That doesn't sound reasonable to me.

Maybe using the latest JDT for the platform build should be blocked by comparator errors, that sounds OK to me.

Or you mean comparator errors in just JDT core itself? That I guess could be checked, but considering there are 200+ bundles with comparator errors right now, I don't think that does much.

[srikanth-sankaran]

And @mpalat and @jarthana can confirm. I don't expect Java22 work to result in any comparator errors.

To clarify, my recent PR corrects problems in code generation in miscomputing stack size. So it WILL produce class files that WILL DIFFER. So the comparator errors are not avoidable.

[laeubi]

@srikanth-sankaran if you have inspected a difference and think its fine there are two options:

1. The affected bundle was not bumped, then increment the version by +100
2. It already was, in such case you usually add the bug number to the forceQualifierUpdate.txt

That way you should have a "clean" PR (of course other non jdt bundles later would maybe need version increments).

@trancexpress yes I mean comparator errors in jdt.core, even if they are only a few, they should be analyzed and fixed as part of the PR otherwise it might result in "unexpected" changes that the put pressure on fixing them when they are already in.

Of course there is no guarantee for that we not discover anything later, but what could be analyzed and fixed locally should be done at JDT, at best one run the aggregator build and check a wider range of bundle like described here:

https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/?tab=readme-ov-file#build-with-custom-compiler

if you pass in -Dcompare-version-with-baselines.skip=false and -Dtycho.debug.artifactcomparator you can discover any changes (and even prepare PRs if you want) before running the full ibuild.

[srikanth-sankaran]

@srikanth-sankaran if you have inspected a difference and think its fine there are two options:

1. The affected bundle was not bumped, then increment the version by +100

2. It already was, in such case you usually add the bug number to the forceQualifierUpdate.txt

That way you should have a "clean" PR (of course other non jdt bundles later would maybe need version increments).

Let me understand. By a "clean" PR you mean a PR with no comparator errors against jdt core itself. Is that right ?

But a clean PR can produce comparator error in 200 other bundles legitimately, because a super basic super ubiquitous operation like string concatenation gets a bug fix ??

If so, the steps you suggest above don't sound like a big win to me. Or have I misunderstood ??

I am with you that before merging in a PR, it is useful and necessary to see if there are comparator errors in JDT itself and validate them.

In the last round for PR 2011 I didn't do it - it resulted in comparator errors in 167 bundles IIRC, from 1 bug and 2 not-incorrect but still avoidable change. It would have been prudent to have looked at jdt logs.

So far I am with you. Where you lost me is what a compiler engineer is supposed to do when he has validated the comparator diffs in jdt core but still anticipates thousands of comparator errors across the SDK build. As it happened this time. I did inspect several dozens of class files before merging.

@trancexpress yes I mean comparator errors in jdt.core, even if they are only a few, they should be analyzed and fixed as part of the PR otherwise it might result in "unexpected" changes that the put pressure on fixing them when they are already in.

Of course there is no guarantee for that we not discover anything later, but what could be analyzed and fixed locally should be done at JDT, at best one run the aggregator build and check a wider range of bundle like described here:

ISTM the raison d'etre for an integration build is to validate a build by bringing together components. And to discover problems in the process.

Your suggestion seems to want to hoist the purpose of an integration build to outside the loop.

Ultimately it could be a problem of nomenclature. I challenge the labeling of builds with comparator errors as "unstable". That is poor choice perhaps.

See that all tests across all SDK projects may be green and the build can still be labeled "unstable."

https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/?tab=readme-ov-file#build-with-custom-compiler

if you pass in -Dcompare-version-with-baselines.skip=false and -Dtycho.debug.artifactcomparator you can discover any changes (and even prepare PRs if you want) before running the full ibuild.

[sravanlakkimsetti]

I challenge the labeling of builds with comparator errors as "unstable". That is poor choice perhaps.

@srikanth-sankaran I am just listing the reasons for taking the approach of labelling "unstable".

Earlier we used to publish the build and add its p2 repo to I-build composite. This has caused a problem when there are comparator errors specifically when we missed increment of swt versions(earlier this used to be a manual process).
We used to have SWT jar having latest code and binaries jar having old code. This caused eclipse not to work. But comparator errors always caught this problem. Another side effect here is if anyone tried to upgrade using I build repository, their eclipse becomes unusable and recovery path is complete reinstallation.

To avoid this situation we came up with this approach Whenever there are comparator errors we label the build as unstable and not add to I-build repository. This way we have a clear warning to identify what is causing the trouble and at the same time protect upgrades from not using problematic build.

See that all tests across all SDK projects may be green and the build can still be labeled "unstable."

When this labelling was introduced we have introduced a process to remove this label and add to I-build repository as well. Mark Stable job is specifically written to remove the label. This can be used if build is verified as stable build(tests are green and sanity tested).

We went with this approach to be better safe than sorry

PS: we also have Mark Unstable to mark a build unstable in case a build found to be problematic during the testing.

[srikanth-sankaran]

Thanks for the explanation @sravanlakkimsetti

[srikanth-sankaran]

Opened issue for the comparator errors: eclipse-platform/eclipse.platform.releng.aggregator#1923

@srikanth-sankaran , @sravanlakkimsetti , how do we continue here? Who checks if the changes are expected?

Ideally we would have had 2 builds between the changes from #2181 and #2171, unfortunately we have only 1 build for both changes. So we don't know which PR results in which comparator errors.

POA: I will come up with a script to mechanically check if there are any differences other than stack size going down. If there is no other differences and the tests are all green, we can move forward to updating the bundles.

[laeubi]

Let me understand. By a "clean" PR you mean a PR with no comparator errors against jdt core itself. Is that right ?

Correct.

But a clean PR can produce comparator error in 200 other bundles legitimately, because a super basic super ubiquitous operation like string concatenation gets a bug fix ??

Yes that's something one currently can't avoid, but at least for the scope of the PR (this repository) everything is already fixed (so we save one additional commit afterwards) and probably an undesired change can be identified before the merge.

If so, the steps you suggest above don't sound like a big win to me. Or have I misunderstood ??

Every little gain is worth it, you can't avoid the work anyways so why postpone it if it is guaranteed to fail?

Where you lost me is what a compiler engineer is supposed to do when he has validated the comparator diffs in jdt core but still anticipates thousands of comparator errors across the SDK build. As it happened this time. I did inspect several dozens of class files before merging.

I'm not that deep inside the compiler topic itself but in the end it is the question who if not the PR writer is able to validate the side-effects? So it is more if one want to make additional check before the submission, we can even enhance the tools if someone can give hints on how to do it, e.g can some things be automated (one example would be to map changed byte code back to the source code, give some comprehensive analysis about stack sizes and so on. But for that we need some support of the compiler experts.

In this particular case the question would be:

If I have two classes A and B and the only differ in the stack size, is this a relevant change that should lead to a comparator error? If not, a maybe mor sustainable fix would be to enhance the comparator to ignore stacksize changes.

[iloveeclipse]

If I have two classes A and B and the only differ in the stack size, is this a relevant change that should lead to a comparator error?

Yes, it is. PR fixes stack size calculation and in worst case the bytecode emitted before could lead to VerifyError at runtime.

[srikanth-sankaran]

Let me understand. By a "clean" PR you mean a PR with no comparator errors against jdt core itself. Is that right ?

Correct.

But a clean PR can produce comparator error in 200 other bundles legitimately, because a super basic super ubiquitous operation like string concatenation gets a bug fix ??

Yes that's something one currently can't avoid, but at least for the scope of the PR (this repository) everything is already fixed (so we save one additional commit afterwards) and probably an undesired change can be identified before the merge.

If so, the steps you suggest above don't sound like a big win to me. Or have I misunderstood ??

Every little gain is worth it, you can't avoid the work anyways so why postpone it if it is guaranteed to fail?

Can't argue with that 😄

If I have two classes A and B and the only differ in the stack size, is this a relevant change that should lead to a comparator error? If not, a maybe mor sustainable fix would be to enhance the comparator to ignore stacksize changes.

Yes, if we underestimate the stack size, we can get a verify error at run time. If we overestimate the stack, we can get stack overflow error at runtime on deeply recursive call chains sooner than otherwise. So yes it is material and should not be ignored.

I have some suggestions for how we may improve this overall process, I will list them after we cross the bridge. For the time being, just to set the record straight, I reiterate (a) that the jdt.core logs were inspected (several dozen class files out of hundred+ that differed) and found to be reasonable ahead of the merge and (b) three days ago, I asked in this thread is there a way I can trigger an I build without merging to get a preview of (a) any build issues due to new asserts added to the compiler (b) tests status (c) comparator errors ? ? and the suggestion `Of course there is no guarantee for that we not discover anything later, but what could be analyzed and fixed locally should be done at JDT, at best one run the aggregator build and check a wider range of bundle like described here:

https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/?tab=readme-ov-file#build-with-custom-compiler

if you pass in -Dcompare-version-with-baselines.skip=false and -Dtycho.debug.artifactcomparator you can discover any changes (and even prepare PRs if you want) before running the full ibuild.` came only hours ago.

Not finding fault @laeubi - your suggestions, inputs and help have been valuable! Thanks.

[srikanth-sankaran]

Good morning gentlemen,

The build looks good from tests p.o.v. I inspected the tests that fail on all 5 platforms and we have only this one new:

• [Java 22] HierarchicalASTVisitor must be updated to reflect a change in the ASTNode hierarchy #2193

(We also have #2187 - don't know yet why this didn't fail in I-build)

[srikanth-sankaran]

1. On a linux VM, downloaded and unzipped artifactcomparisons.zip into current directory

Except for these errors - which I think are due to excessively long path names - the unzip went fine

error:  cannot create rt.equinox.p2/bundles/org.eclipse.equinox.p2.metadata.repository/target/artifactcomparison/no-classifier/ant_tasks/metadataRepository-ant.jar/org/eclipse/equinox/internal/p2/metadata/repository/ant/CreateCompositeMetadataRepositoryTask.class-baseline
        Input/output error
error:  cannot create rt.equinox.p2/bundles/org.eclipse.equinox.p2.metadata.repository/target/artifactcomparison/no-classifier/ant_tasks/metadataRepository-ant.jar/org/eclipse/equinox/internal/p2/metadata/repository/ant/CreateCompositeMetadataRepositoryTask.class-build
        Input/output error
error:  cannot create rt.equinox.p2/bundles/org.eclipse.equinox.p2.metadata.repository/target/artifactcomparison/no-classifier/ant_tasks/metadataRepository-ant.jar/org/eclipse/equinox/internal/p2/metadata/repository/ant/CreateCompositeMetadataRepositoryTask.class-baseline.class
        Input/output error
error:  cannot create rt.equinox.p2/bundles/org.eclipse.equinox.p2.metadata.repository/target/artifactcomparison/no-classifier/ant_tasks/metadataRepository-ant.jar/org/eclipse/equinox/internal/p2/metadata/repository/ant/CreateCompositeMetadataRepositoryTask.class-build.class
        Input/output error
error:  cannot create rt.equinox.p2/bundles/org.eclipse.equinox.p2.repository.tools/target/artifactcomparison/no-classifier/lib/repository-tools-ant.jar/org/eclipse/equinox/p2/internal/repository/tools/tasks/CreateCompositeArtifactRepositoryTask.class-baseline.class
        Input/output error

2. Issued these commands:

`$ find 2>/tmp/err . -name '*.class-baseline' > baseline-classes

$ cat /tmp/err #### To verify no errors - yes no errors.

$ head baseline-classes
./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/readAnnotationType/AnnotationReader.class-baseline
./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/pause/PauseAnnotationProcessor.class-baseline
./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/nestedhelloworld/NestedHelloWorldAnnotationProcessor.class-baseline
./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/mirrortest/MirrorUtilTestAnnotationProcessor.class-baseline
./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/mirrortest/MirrorTestAnnotationProcessor.class-baseline
./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/mirrortest/DefaultConstantProcessor.class-baseline
./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/helloworld/HelloWorldAnnotationProcessor.class-baseline
./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/generic/AbstractGenericProcessor.class-baseline
./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/aptrounding/Round1GenAnnotationProcessor.class-baseline
./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/apitest/APIAnnotationProcessorFactory$APIAnnotationProcessor.class-baseline

$ wc -l baseline-classes
5236 baseline-classes

So, OK we got 5236 classes to verify.

3. Using this script, I gathered all differences into a single text file:

#! /bin/bash
rm -f classfile-diff.out
for baseline_class in `cat baseline-classes`
do
    build_class=`dirname $baseline_class`/`basename -s .class-baseline $baseline_class`.class-build
    printf "diff $baseline_class $build_class\n" >> classfile-diff.out
    diff $baseline_class $build_class >> classfile-diff.out
done

The accumulated text diffs look like this:

$head -30 classfile-diff.out

diff ./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/readAnnotationType/AnnotationReader.class-baseline ./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/readAnnotationType/AnnotationReader.class-build
127c127
<     MAXSTACK = 5
---
>     MAXSTACK = 2
diff ./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/pause/PauseAnnotationProcessor.class-baseline ./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/pause/PauseAnnotationProcessor.class-build
136c136
<     MAXSTACK = 5
---
>     MAXSTACK = 4
diff ./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/nestedhelloworld/NestedHelloWorldAnnotationProcessor.class-baseline ./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/nestedhelloworld/NestedHelloWorldAnnotationProcessor.class-build
34c34
<     MAXSTACK = 2
---
>     MAXSTACK = 1
diff ./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/mirrortest/MirrorUtilTestAnnotationProcessor.class-baseline ./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/mirrortest/MirrorUtilTestAnnotationProcessor.class-build
615c615
<     MAXSTACK = 8
---
>     MAXSTACK = 5
diff ./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/mirrortest/MirrorTestAnnotationProcessor.class-baseline ./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/mirrortest/MirrorTestAnnotationProcessor.class-build
136c136
<     MAXSTACK = 4
---
>     MAXSTACK = 3
diff ./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/mirrortest/DefaultConstantProcessor.class-baseline ./eclipse.jdt.core/org.eclipse.jdt.apt.tests/target/artifactcomparison/no-classifier/apt.jar/org/eclipse/jdt/apt/tests/annotations/mirrortest/DefaultConstantProcessor.class-build
237c237
<     MAXSTACK = 6
---
>     MAXSTACK = 2

Now the job is to verify that only stack depth changes are encountered in this file. Anything else needs scruitiny

[srikanth-sankaran]

I have mechanically verified that the textual diffs contains only stack size reductions. The one lone exception is called out below.

Now corelating the stacksize reductions to indy instructions count is not undertaken for the full set of 5236 classes. I had already verified it for several dozens of class files in JDT-Core before merge and given the tests are all green would assert we are good to go.

The exception is

(1) this diff below that needs to be studied.

diff ./eclipse.jdt.core/org.eclipse.jdt.core.tests.performance/target/artifactcomparison/no-classifier/org/eclipse/jdt/core/tests/performance/FullSourceWorkspaceBuildTests.class-baseline ./eclipse.jdt.core/org.eclipse.jdt.core.tests.performance/target/artifactcomparison/no-classifier/org/eclipse/jdt/core/tests/performance/FullSourceWorkspaceBuildTests.class-build
428c428
---
580c580
---
639c639
---
895c895
---
1191c1191
<       64: L6
---
>       38: L6
1201c1201
<       64: L10
---
>       38: L10
1248c1248
<       64: L19
---
>       38: L19
1258c1258
<       64: L23
---
>       38: L23
1326c1326
---

(2) the unzip errors: these class files would have to be analyzed somehow:

error:  cannot create rt.equinox.p2/bundles/org.eclipse.equinox.p2.metadata.repository/target/artifactcomparison/no-classifier/ant_tasks/metadataRepository-ant.jar/org/eclipse/equinox/internal/p2/metadata/repository/ant/CreateCompositeMetadataRepositoryTask.class-baseline
        Input/output error
error:  cannot create rt.equinox.p2/bundles/org.eclipse.equinox.p2.metadata.repository/target/artifactcomparison/no-classifier/ant_tasks/metadataRepository-ant.jar/org/eclipse/equinox/internal/p2/metadata/repository/ant/CreateCompositeMetadataRepositoryTask.class-build
        Input/output error
error:  cannot create rt.equinox.p2/bundles/org.eclipse.equinox.p2.metadata.repository/target/artifactcomparison/no-classifier/ant_tasks/metadataRepository-ant.jar/org/eclipse/equinox/internal/p2/metadata/repository/ant/CreateCompositeMetadataRepositoryTask.class-baseline.class
        Input/output error
error:  cannot create rt.equinox.p2/bundles/org.eclipse.equinox.p2.metadata.repository/target/artifactcomparison/no-classifier/ant_tasks/metadataRepository-ant.jar/org/eclipse/equinox/internal/p2/metadata/repository/ant/CreateCompositeMetadataRepositoryTask.class-build.class
        Input/output error
error:  cannot create rt.equinox.p2/bundles/org.eclipse.equinox.p2.repository.tools/target/artifactcomparison/no-classifier/lib/repository-tools-ant.jar/org/eclipse/equinox/p2/internal/repository/tools/tasks/CreateCompositeArtifactRepositoryTask.class-baseline.class
        Input/output error

I'll continue.

[jarthana]

Its a bit unfortunate that this is not done, I would have expsected that a PR is never merged with comparator errors in JDT as otherwise build failures are guaranteed!

Sorry, JDT core is its own repository. You expect a PR in it to be blocked by the rest of the Eclipse platform having some checker that complains about the latest code in JDT core? That doesn't sound reasonable to me.

Maybe using the latest JDT for the platform build should be blocked by comparator errors, that sounds OK to me.

I agree.
This is the reason why, imo, upgrading the compiler as a separate task periodically was better, as we will be expecting such troubles and even be better prepared.

[srikanth-sankaran]

I'm not that deep inside the compiler topic itself but in the end it is the question who if not the PR writer is able to validate the side-effects? So it is more if one want to make additional check before the submission, we can even enhance the tools if someone can give hints on how to do it,

I agree the PR author is in the best position to validate.

What would help me personally is:
0. The steps to "run the aggregator build and check a wider range of bundle" should be a web-service. A check box is all that should be required from the PR page - well a guy can hope 😁

1. On a JDT/Core PR if a new "check" is added that would bubble up the presence of comparator errors. Checking logs is all OK, but humans being humans are going to fail/forget/neglect to do that with discipline. Tools calling attention would be welcome. If this check is implemented, let comparator errors be flagged with orange rather than with red X. Believe me sometimes, subtle changes in other phases could result in code generation change that the PR author would be completely surprised about. Happens.
2. On the I-Build download/tests results page, I would like to view just failures. I don't want to search for a needle in a haystack
3. On the I-Build tests results page, I would like to see new failures color coded differently compared to failures in previous builds.
4. I would like flaky tests that repeatedly fail on a sporadic basis to be flagged as flaky - they should not "Fail" they should "Flaky Fail"
5. I would prefer the "Unstable" nomenclature to go away and become "Under-validation" and red letters to become orange letters :)
6. Regarding Jay's comment above about upgrading the compiler as a periodic task: I may have had an hand in advocating the present practice - by having called out the "best practice" from javac/OpenJDK - every line of change you make is used to build the entire OpenJDK forest and it is the bootstrapped compiler that is used to run the test suites. But they don't have comparators :)

[iloveeclipse]

This is the reason why, imo, upgrading the compiler as a separate task periodically was better, as we will be expecting such troubles and even be better prepared.

The problem with that approach is, that it makes it harder to find out which change in compiler caused regression and to provide and validate an appropriate fix in same release. We had bugs appearing in RC builds. Patching bugs in code generation at this time is risky, not patching is shipping buggy compiler. So I believe current immediate validation is the right thing. The extra work we have results in a better quality of the release.

[laeubi]

The steps to "run the aggregator build and check a wider range of bundle" should be a web-service. A check box is all that should be required from the PR page - well a guy can hope 😁

We already have something similar for SWT performance tests, I'll try to provide an extra validation step to JDT, given we have these comparator errors now seems a good way to test ist but I would maybe need some "extra power" from JDT commiters to allow workflow runs.

[laeubi]

See PR here:

• Add a validation step that check if a possible baseline problem is found #2194