Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance class file comparator diff report #1688

Open
iloveeclipse opened this issue Dec 22, 2023 · 6 comments
Open

Enhance class file comparator diff report #1688

iloveeclipse opened this issue Dec 22, 2023 · 6 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@iloveeclipse
Copy link
Member

As reported here:

Are the class files themselves readily accessible ?
The textual diff is not completely informative. For example the local variable table is missing.

For the class file comparator checks (like in #1686) it would be nice to:

  1. generate not only textual file diffs but also attach both versions of actual class files that are compared
  2. Provide line number table in ASM-generated bytecode text diff (right now it is omitted)

For the later, I have no idea which code generates textual bytecode diffs, however, that one should not omit call to org.objectweb.asm.util.Textifier.visitMaxs(int, int) which is currently the case looking on the diff.

@iloveeclipse iloveeclipse added enhancement New feature or request help wanted Extra attention is needed labels Dec 22, 2023
@srikanth-sankaran
Copy link

As reported here:

Are the class files themselves readily accessible ?
The textual diff is not completely informative. For example the local variable table is missing.

For the class file comparator checks (like in #1686) it would be nice to:

  1. generate not only textual file diffs but also attach both versions of actual class files that are compared

That would also allow one to use their favorite tool (javap in my case)

  1. Provide line number table in ASM-generated bytecode text diff (right now it is omitted)

For the later, I have no idea which code generates textual bytecode diffs, however, that one should not omit call to org.objectweb.asm.util.Textifier.visitMaxs(int, int) which is currently the case looking on the diff.

@laeubi
Copy link
Contributor

laeubi commented Dec 22, 2023

For the later, I have no idea which code generates textual bytecode diffs

It is done here:
https://github.com/eclipse-tycho/tycho/blob/main/tycho-artifactcomparator/src/main/java/org/eclipse/tycho/zipcomparator/internal/ClassfileComparator.java

any improvements are welcome, I have no particular knowledge about the details of class compare beside that is is done at this place.

@iloveeclipse
Copy link
Member Author

For the later, I have no idea which code generates textual bytecode diffs

It is done here: https://github.com/eclipse-tycho/tycho/blob/main/tycho-artifactcomparator/src/main/java/org/eclipse/tycho/zipcomparator/internal/ClassfileComparator.java

any improvements are welcome, I have no particular knowledge about the details of class compare beside that is is done at this place.

@laeubi : in this line

https://github.com/eclipse-tycho/tycho/blob/3cfab28b0c382d0b7f24bc208bc82787d91f7030/tycho-artifactcomparator/src/main/java/org/eclipse/tycho/zipcomparator/internal/ClassfileComparator.java#L60

change

        reader.accept(clazz, Opcodes.ASM9 | ClassReader.SKIP_DEBUG | ClassReader.SKIP_FRAMES);

to

        reader.accept(clazz, Opcodes.ASM9 | ClassReader.SKIP_FRAMES);

so one can see also local variables info generated by compiler.

@laeubi
Copy link
Contributor

laeubi commented Dec 22, 2023

@iloveeclipse
Copy link
Member Author

See

Thanks!

Please check if that would not create unexpected class file diffs before deploying this tycho version.
While it should not, I can't guarantee it and I will be not available for fixing unstable SDK builds from 24.12 till 04.01 as I will have no access to any PC in that time.

@laeubi
Copy link
Contributor

laeubi commented Dec 22, 2023

Please check if that would not create unexpected class file diffs before deploying this tycho version.

I have added some comments in the PR probably its better to continue discussion there. From my side there is no problem to hold this back until next year when everyone is back in the office, I'll mark the PR as draft to accommodate this, alternatively I can make the change of disassembly a different PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants