RFR: 8341608: jdeps in JDK 23 crashes when parsing signatures while jdeps in JDK 22 works fine [v2]

[Chen Liang]

On Thu, 17 Apr 2025 16:44:02 GMT, Henry Jen <henryjen at openjdk.org> wrote:

>> Chen Liang has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Review remarks
>
> 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`?

Indeed. Renamed to `entryPath`. Also adjusted this to accept `Throwable`, so we can skip an entry due to either IOException or ClassFileError.

> 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.

I have updated this to skip as well - if the outer structure is malformed, we should not have reached this far and would have encountered IOException earlier.

> 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?

Yep, I think skipping for IOE is correct. If IOE is serious enough that no element is accessible, it should usually be thrown earlier.

> 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`?

Done in the update.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049625074
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049626124
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049626614
PR Review Comment: https://git.openjdk.org/jdk/pull/24604#discussion_r2049625262