[foreign-jextract] RFR: Improve jextract error reporting

[Athijegannathan Sundararajan]

On Thu, 5 Mar 2020 15:17:56 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

> Hi,
> 
> I spent the last couple of days tracking down a crash I was seeing sometimes when running jextract. In the process I ended up doing several improvements to jextract's error checking and reporting, that I think are useful to keep in the long run.
> 
> The changes include:
> - Using `clang_parseTranslationUnit2` to parse, since that returns an error code, instead of a null translation unit. This also means we can still process diagnostic for the returned translation unit (now returned in a buffer).
> - Checking the return codes for `clang_parseTranslationUnit2`, `clang_saveTranslationUnit`, and `clang_reparseTranslationUnit` (using a proper enum) and throwing an exception in case they are erroneous.
> - Catching BadMacroException instead of Throwable in the macro parser. We only care about the former, and catching Throwable could lead to too many exceptions being eaten. Technically, if there is an unknown exception, we could just skip the macro, but this isn't always the right strategy. For example, when re-parsing fails the used translation unit becomes invalid, so we really need to do something else than just skipping the macro in that case (to be addressed in another patch https://bugs.openjdk.java.net/browse/JDK-8240614). Imo it's better to just crash than to skip when we encounter an unknwon exception, so that we're more likely to find issues.
> - Enabling libclang crash recovery by default. By default the crash recovery was being disabled. This leads to the process exiting with OS exception code. If we enable this however, it will instead try to return an error code from the libclang call, which we can then check for and handle the failure however we want.
> 
> Thanks,
> Jorn

Marked as reviewed by sundar (Committer).

src/jdk.incubator.jextract/share/classes/jdk/internal/clang/SaveError.java line 59:

> 58:     public static SaveError valueOf(int code) {
> 59:         return lookup.computeIfAbsent(code, k -> { throw new NoSuchElementException("No SaveError with code: " + k); });
> 60:     }

Do we need this computeIfAbsent with lambda? All we do is to throw exception always if key is not found.

src/jdk.incubator.jextract/share/classes/jdk/internal/clang/TranslationUnit.java line 67:

> 66:             if (res != SaveError.None) {
> 67:                 throw new TranslationUnitSaveException(path);
> 68:             }

Would it be useful to store the SaveError kind in TranslationUnitSaveException?

src/jdk.incubator.jextract/share/classes/jdk/internal/clang/LibClang.java line 38:

> 37:         Index index = new Index(Index_h.clang_createIndex(local ? 1 : 0, 0));
> 38:         Index_h.clang_toggleCrashRecovery(NO_CRASH_RECOVERY ? 0 : 1);
> 39:         if (DEBUG && NO_CRASH_RECOVERY) {

I vaguely recall a specific reason for this default. Maurizio may recall about this

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

PR: https://git.openjdk.java.net/panama-foreign/pull/40