Comparator issues in https://download.eclipse.org/eclipse/downloads/drops4/I20241126-0600/

[akurtakov]

https://download.eclipse.org/eclipse/downloads/drops4/I20241126-0600/ has many comparator issues.

[akurtakov]

@srikanth-sankaran Would you please investigate? As today there are only 2 PRs by you merged it should be coming from them.

[srikanth-sankaran]

@srikanth-sankaran Would you please investigate? As today there are only 2 PRs by you merged it should be coming from them.

Thanks, I will, first thing tomorrow morning.

[iloveeclipse]

Just checked few classes - they seem to have "usual" old style Java switches (no fancy patterns) but seem to use one local variable less inside the switch as before.

See org.eclipse.swt.widgets.DateTime.computeMaxTextSize(int, int, boolean) for example.

Links: https://download.eclipse.org/eclipse/downloads/drops4/I20241126-0600/buildlogs/comparatorlogs/artifactcomparisons.zip

[srikanth-sankaran]

A minor usability - not correctness - issue that results in one UI test failing is tracked here: eclipse-jdt/eclipse.jdt.core#3356

This has nothing to do with the comparator errors.

[srikanth-sankaran]

Does it make sense to omit all the lines with no failures in
https://download.eclipse.org/eclipse/downloads/drops4/I20241126-0600/testResults.php ?

Personally I see no value in seeing (0) replicated for each platform and for each suite,

Likewise, it would be so much better if clicking on failure count would take me to failed test instead of my having to search for "Failure" with many false hits.

[srikanth-sankaran]

Here is a tiny program that shows what has changed:

public class X {

	static void foo() {

		switch ("Hello") {
		case "disallow": //$NON-NLS-1$
			break;
		}

		try {
			System.out.println("Hello");
		} catch (Exception e) {
			throw e;
		}
	}
}

Local Variable Table from 4.34 RC2:

    Start  Length  Slot  Name   Signature
       45       2     1     e   Ljava/lang/Exception;

Local Variable Table from master:

    Start  Length  Slot  Name   Signature
       45       2     0     e   Ljava/lang/Exception;

Basically to switch on strings, the compiler uses a "secret" local variable it injects into the program (Strictly speaking in this program such a secret local is not required at all but that is not changed)

In 4.34, this secret variable is injected into the "upper" scope so, in this case into foo's scope and stays live till foo is live on the stack which means that the Exception e of the catch block cannot overlay with the secret and must co-exist with the secret variable. So the secret variable is alloted slot 0 (it being a secret you don't see it LVT) and Exception e gets assigned the slot 1.

In master, the secret variable is injected into the switch block's scope and gets reclaimed as soon that the switch block's body {} is exited. So slot 0 is immediately available for use and gets assigned to Exception e.

I'll look through the other class files to see if the symptoms are the same - if so we can simply declare we are fine. Not just fine, better than before - in the sense that a local variable slot is garbage collected as soon as it becomes garbage.

[srikanth-sankaran]

OK, I have scanned through the 35 files that are reported. No red flags. We can proceed to declare the build stable.

One interesting case is from org.eclipse.equinox.internal.p2.publisher.eclipse.ProductFile.startElement(String, String, String, Attributes) where we actually save as many as 3 local variable slots.

This is not just a space optimization. This may result in compact code sequences also. For example, aload_[0|1|2|3] takes one byte to encode in a class file, while aload_4+ takes two bytes.

[srikanth-sankaran]

@akurtakov @iloveeclipse - is there any automation that would generate the PRs needed for qualifier updates ?

[srikanth-sankaran]

@akurtakov @iloveeclipse - is there any automation that would generate the PRs needed for qualifier updates ?

@mpalat @MohananRahul - any help would be appreciated. Thanks!

[akurtakov]

I am not aware of any automation as this time it requires bumping the version for most(all maybe) as it's start of cycle.

[iloveeclipse]

See https://github.com/eclipse-platform/eclipse.platform.releng.buildtools/blob/master/bundles/org.eclipse.releng.tools/src/org/eclipse/releng/tools/TouchBundles.java

[iloveeclipse]

@trancexpress : could you please run the script & create PR's for touched repos? The good thing now, we don't need to check manifest files because github action will do that for us.

[srikanth-sankaran]

Thanks gentlemen.

[akurtakov]

@iloveeclipse So your plan is to add entries to forceQualifierUpdate.txt files (even if they are not needed) and let the automation update versions? That might be a nice workaround..

[iloveeclipse]

Yes, but since I'm offline today I hope Simeon can take over.

[trancexpress]

List of PRs:

• Updated bundles with comparator errors eclipse-jdt/eclipse.jdt.debug#569 (merged)
• Updated bundles with comparator errors eclipse-jdt/eclipse.jdt.ui#1821 (merged)
• Updated bundles with comparator errors eclipse-pde/eclipse.pde#1495 (merged)
• Updated bundles with comparator errors eclipse.platform#1642 (merged)
• Updated bundles with comparator errors eclipse.platform.ui#2547 (merged)
• Updated bundles with comparator errors eclipse-equinox/equinox#707 (merged)
• Updated bundles with comparator errors eclipse-equinox/p2#575 (merged)
• Updated bundles with comparator errors eclipse.platform.swt#1623 (merged)

Would be good if someone who is used to the process (I am not) can double check.

I've commented on the individual PRs, where I have doubt in what the TouchBundles script is doing.

For SWT I'm not sure, the script only creates files (in the binaries part):

        binaries/org.eclipse.swt.gtk.linux.aarch64/forceQualifierUpdate.txt
        binaries/org.eclipse.swt.gtk.linux.ppc64le/forceQualifierUpdate.txt
        binaries/org.eclipse.swt.gtk.linux.riscv64/forceQualifierUpdate.txt
        binaries/org.eclipse.swt.gtk.linux.x86_64/forceQualifierUpdate.txt
        binaries/org.eclipse.swt.win32.win32.aarch64/forceQualifierUpdate.txt
        binaries/org.eclipse.swt.win32.win32.x86_64/forceQualifierUpdate.txt

[akurtakov]

For swt I believe (90%) that forceQualifierUpdate for o.e.swt should be the right thing to do.

[trancexpress]

For swt I believe (90%) that forceQualifierUpdate for o.e.swt should be the right thing to do.

You mean just this? No manifest version bumps?

https://github.com/eclipse-platform/eclipse.platform.swt/pull/1623/files

[trancexpress]

Thank you @akurtakov for the help! And thank you @srikanth-sankaran for the diff analysis.

I'll check the next I-build to see if we need more changes.

[akurtakov]

Once https://ci.eclipse.org/releng/ completes (so rebuilt natives are pushed) I'll trigger new I-build.

[iloveeclipse]

@akurtakov , @MohananRahul : we should deploy new ecj build and use it in platform so jenkins builds are using same compiler as SDK build

[akurtakov]

https://download.eclipse.org/eclipse/downloads/drops4/I20241127-0510/ is clear from comparator issues.

[trancexpress]

Alright, great. Anything else here?

[srikanth-sankaran]

Thanks @trancexpress for your help here. Appreciated.

[trancexpress]

Closing this, let me know if there is more to do and I'll re-open.