[foreign-jextract] RFR: Improve jextract error reporting

[Henry Jen]

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

Looks good, my only concern is the disable clang crash recovery default was disabled but now default to enable.

The past experience is that crash recovery cause JVM crash from time to time, so unless we are certain that situation changes and should be default to enable, perhaps we should just default to disable as before.

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



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