[foreign-memaccess+abi] RFR: 8266626: Check that the target address of a native call is not NULL

[Jorn Vernee]

On Fri, 7 May 2021 13:54:02 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This patch adds a check when unboxing the target address of a native call to check that the address is not `NULL`. Additionally, it adds checks earlier when an address is specified with a linking request, to check that that address is not `NULL`.
>> 
>> I didn't see any regressions on the CallOverhead* benchmarks.
>> 
>> Thanks,
>> Jorn
>
> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/AbstractCLinker.java line 46:
> 
>> 44:     private static void checkSymbol(Addressable symbol) {
>> 45:         MemoryAddress symbolAddr = symbol.address();
>> 46:         if (symbolAddr.equals(MemoryAddress.NULL))
> 
> Do we already NPE for `null` ?

Yes, callsites of checkSymbol already do requireNonNull on the symbol (see below that)

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/AbstractCLinker.java line 47:
> 
>> 45:         MemoryAddress symbolAddr = symbol.address();
>> 46:         if (symbolAddr.equals(MemoryAddress.NULL))
>> 47:             throw new IllegalArgumentException("Symbol is NULL: " + symbolAddr);
> 
> IAE or NPE?

I think IAE, in order to avoid confusion with the case where `symbol` is `null` (This is also why I added the MA instance to the error message). NPE seems more appropriate for the `null` case I think.

> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/ProgrammableInvoker.java line 177:
> 
>> 175:     private static long unboxTargetAddress(Addressable addr) {
>> 176:         MemoryAddress ma = addr.address();
>> 177:         if (ma.equals(MemoryAddress.NULL)) {
> 
> Why not reusing checkSymbol?

Yes, that's a good point. Will do

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

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