[foreign-memaccess+abi] RFR: 8267989: Exceptions thrown during upcalls should be handled

[Jorn Vernee]

On Mon, 31 May 2021 17:47:47 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Hi,
>> 
>> This patch regularizes exception handling for exceptions thrown during upcalls. Exceptions thrown during upcalls are now always handled by printing out the stack trace and then calling `System::exit` (see the JBS issue for some motivation).
>> 
>> I've added some documentation for the exception handling to `CLinker::upcallStub`, as well as a new public `int` constant in `CLinker` which is the error code that is passed to `System::exit`. The returned error code can also be configured by a system property, which for now is mostly useful for testing purposes to make sure we don't get a consistent false positive.
>> 
>> Thanks,
>> Jorn
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 123:
> 
>> 121:      * @see CLinker#upcallStub(MethodHandle, FunctionDescriptor, ResourceScope)
>> 122:      */
>> 123:     int ERR_UNCAUGHT_EXCEPTION = privilegedGetProperty("jdk.incubator.foreign.uncaught_exception_code", 1);
> 
> Do we need a system property? E.g. can it be a constant value?

We could just use `1` here as well, but during testing I've found that the default uncaught exception handler also return `1`, so I was getting a false positive.

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 380:
> 
>> 378:         if (specializedHandle.type().returnType() == void.class) {
>> 379:             if (!upcall) {
>> 380:                 closer = empty(methodType(void.class, Throwable.class)); // (Throwable) -> void
> 
> Shouldn't ProgrammableUpcallHandler do the same thing?

It does? Both call SharedUtils.handelUncaughtException

> test/jdk/java/foreign/TestUpcallException.java line 80:
> 
>> 78: 
>> 79:         int result = process.waitFor();
>> 80:         assertEquals(result, exitCode);
> 
> Should we check for stacktrace output - e.g. the name of ThrowingUpcall" ?

That's probably a good idea. Though, I'll note that it still allows for false positives if the tests throws an exception from ThrowingUpcall another way. Comparing the exit codes should catch that though.

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

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