On Fri, 11 Apr 2025 22:45:55 GMT, Chen Liang <li...@openjdk.org> wrote:
> When jdeps was migrated from old classfile to ClassFile API, the parsing
> semantic changed - error checks are now made lazily, and nested crashes from
> malformed signature or other problems is now latent, after a `ClassModel`
> instance is available. (The old error check existed only for constructing a
> `ClassModel`)
>
> To address this issue, I have updated the way of iterating class files to be
> handler/consumer based like in the ClassFile API. This has the advantage that
> when one invocation of the handler fails of a `ClassFileError`, other
> invocations for other class files can proceed, and the exception handler has
> sufficient information to report a useful message indicating the source of
> error.
>
> For the particular example of examining a proguard processed
> `dummy-scala.jar`, here is the new output of `jdeps dummy-scala.jar`:
>
> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected
> character ; at position 59, expected an identifier:
> Lscala/collection/immutable/TreeMap$TreeMapBuilder<TA;TB;>.;:
> scala/collection/immutable/TreeMap$TreeMapBuilder.class (dummy-scala.jar)
> Warning: com.sun.tools.jdeps.Dependencies$ClassFileError: Unexpected
> character ; at position 49, expected an identifier:
> Lscala/collection/parallel/mutable/ParArray<TT;>.;:
> scala/collection/parallel/mutable/ParArray.class (dummy-scala.jar)
>
>
> Now, jdeps shows the bad class files. Inspection into the files reveal that
> proguard incorrectly deleted the simple class names with trailing `$`, for
> example, the original signature of the broken ParArray was
> `Lscala/collection/parallel/mutable/ParArray<TT;>.ParArrayIterator$;`, so the
> `ParArrayIterator$` part was incorrectly dropped by proguard.
>
> Testing: langtools/tools/jdeps.
src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java line 105:
> 103:
> 104: protected void skipEntry(ClassFileError ex, String fileName) {
> 105: skippedEntries.add(String.format("%s: %s", ex.toString(),
> fileName));
The second parameter is not always a straightforward filename, consider to
rename it, perhaps `entryPath`?
src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java line 237:
> 235: skipEntry(ex, e.toString());
> 236: } catch (IOException ex) {
> 237: throw new UncheckedIOException(ex);
Just to point out this was leading to ClassFileError in the old implementation,
new implementation will relay the IOException, which I think is proper.
src/jdk.jdeps/share/classes/com/sun/tools/jdeps/ClassFileReader.java line 339:
> 337: });
> 338: } catch (UncheckedIOException ex) {
> 339: throw ex.getCause();
IOException used to skip entry with message and continue, the new behavior
would change. I am not certain this is behavior compatible.
I doubt IOException would be recoverable on different entry, but it might in
rare cases?
src/jdk.jdeps/share/classes/com/sun/tools/jdeps/DependencyFinder.java line 176:
> 174: FutureTask<Set<Location>> task = new FutureTask<>(() -> {
> 175: Set<Location> targets = new HashSet<>();
> 176: archive.reader().processClassFiles(cf -> {
I prefer the naming convention with forEach for operation to iterate through
rather than have a `process` on a reader. Perhaps named like `forEachClassFile`?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049319406
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049411874
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049425413
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049378312
