BUG: function pointers inside a struct do not generate SegmentAllocators when needed

Fri Jul 29 21:40:55 UTC 2022

Hi Marko,
I think adding the missing SegmentAllocator parameter can be possible 
fix, but the reality here is that there is an asymmetry between two use 
cases:

1. when you create an upcall from a lambda expression, the segment 
allocator parameter is not required (in fact the upcall machinery will 
_not_ provide one to the upcall, see below)
2. when you want to wrap a functional interface around an upcall stub 
address, the struct returned by the stub will be copied into a _fresh_ 
segment allocated using the provided segment allocator

Because of this, I think that just prepending a SegmentAllocator 
parameter in front of the functional descriptor is not gonna cut it. 
That is, if you have a C function pointer like:

```
struct Foo (*f)(int)
```

The Linker will expect an upcall MethodHandle with signature 
`(int)MemorySegment`, and no SegmentAllocator parameter. If you add an 
extra SegmentAllocator parameter, then the method handle we derive from 
the lambda expression in (1) is going to have the wrong type (unless we 
insert a dummy SegmentAllocator argument).

When chatting with Jorn offline about this, we were discussing the 
possibility of emitting, for the above signature, a functional interface 
like the one below:

```
interface FI$f {

    MemorySegment apply(int i);
    default MemorySegment(SegmentAllocator allocator, int i) { throw new 
UnsupportedOperationException(); }

    ...
}
```

This has some advantages:

* we can still provide a lambda-friendly factory for (1) - note that the 
functional interface signature would still be the "correct" one for the 
upcall (e.g. the one w/o the SegmentAllocator), so nothing changes here
* we can also provide the `ofAddress` method for (2), which builds a new 
FI$f given a memory address; to do that, we create an instance as follows:

```
FI$f ofAddress(MemoryAddress address) {
    ...

    return new FI$f() {
       MemorySegment apply(int i) { return 
apply(SegmentAllocator.implicitAllocator(), i); }
       MemorySegment apply(SegmentAllocator, int i) { <call downcall 
handle> }
    };
}
```

in other words, the returned functional interface instance supports two 
invocation modes, w/ and w/o a SegmentAllocator. If no SegmentAllocator 
is provided, we just use the implicit allocator (another option would be 
to throw, but perhaps that would be too harsh?)

Thoughts?

Cheers
Maurizio

On 29/07/2022 00:42, Marko wrote:
> Perhaps the additional parameter was not as weird as I originally thought.
>> It seems a bit odd though IMHO, as then
>> HeaderFileBuilder#addVar and ToplevelBuilder#addVar are also forced to
>> have this parameter even though it goes unused there.
> I just tried it out and the getters are also missing when generated as
> a part of the header file.
> I'll polish my fix and send a PR for a better opinion.
>
> Thanks
> Marko
>
>
> On Thu, Jul 28, 2022 at 7:25 PM Marko <im at lostillusion.net> wrote:
>> My apologies, gmail seems to have only replied directly to Jorn
>> previously. Here is the original message.
>>
>> Hey Jorn,
>> Thanks for filing the report. I agree that the problem is where the
>> `ofAddress` function is generated. It does not check to see if a
>> segment allocator is needed. It seems quite trivial to add though as
>> it is already implemented for regular functions,
>> https://github.com/openjdk/jextract/blob/0582eaf1b4cdba95f0ee8c2480767433bb647d0d/src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java#L122-L127.
>>
>> However, when this is fixed, the getter must also be updated to
>> reflect the addition of the SegmentAllocator parameter.
>> I believe this constitutes an additional parameter here
>> https://github.com/openjdk/jextract/blob/0582eaf1b4cdba95f0ee8c2480767433bb647d0d/src/main/java/org/openjdk/jextract/impl/StructBuilder.java#L128=
>> similar to fiName, that represents whether a segment allocator is
>> needed or not. It seems a bit odd though IMHO, as then
>> HeaderFileBuilder#addVar and ToplevelBuilder#addVar are also forced to
>> have this parameter even though it goes unused there.
>>
>> I implemented the fix and the proper getter generation as well, and it
>> seems to all work on my end. I wonder if this boolean parameter is the
>> way to go before submitting a PR though.
>>
>> Thanks,
>> Marko
>>
>> On Thu, Jul 28, 2022 at 5:20 PM Jorn Vernee <jorn.vernee at oracle.com> wrote:
>>> Hello Marko,
>>>
>>> Thanks for the report! I can reproduce it here as well. I think the
>>> problem is the lambda in the `ofAddress` factory that is generated in
>>> the interface for the callback type. Note that the getter will just end
>>> up calling that factory. (you can see that also in your stack trace: `at
>>> Foo$bar.lambda$ofAddress$0(Foo.java:32)`).
>>>
>>> I've filed: https://bugs.openjdk.org/browse/CODETOOLS-7903239
>>>
>>> Cheers,
>>> Jorn
>>>
>>> On 28/07/2022 02:00, Marko wrote:
>>>> Hello again!
>>>> When trying to call a function pointer inside a struct which returns a
>>>> MemorySegment, I receive the following error:
>>>> ```
>>>> Exception in thread "main" java.lang.AssertionError: should not reach here
>>>> at Foo$bar.lambda$ofAddress$0(Foo.java:34)
>>>> at Main.main(Main.java:8)
>>>> Caused by: java.lang.invoke.WrongMethodTypeException: expected
>>>> (NativeSymbol,SegmentAllocator)MemorySegment but found
>>>> (NativeSymbol)MemorySegment
>>>> at java.base/java.lang.invoke.Invokers.newWrongMethodTypeException(Invokers.java:523)
>>>> at java.base/java.lang.invoke.Invokers.checkExactType(Invokers.java:532)
>>>> at Foo$bar.lambda$ofAddress$0(Foo.java:32)
>>>> ... 1 more
>>>> ```
>>>> I believe this is due to
>>>> `FunctionalInterfaceBuilder#emitFunctionalFactoryForPointer` not
>>>> checking to see if the returnType is a MemorySegment, as it does in
>>>> `HeaderFileBuilder` for regular functions, see
>>>> https://github.com/openjdk/jextract/blob/0582eaf1b4cdba95f0ee8c2480767433bb647d0d/src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java#L122-L127.
>>>>
>>>> I began implementing a fix by copying this functionality over, however
>>>> there is a bit of an issue I came across. When jextract emits the
>>>> getter for the functional interface in a struct (StructBuilder), only
>>>> the name of the interface is known. AFAIK, it does not know whether it
>>>> needs a SegmentAllocator and cannot currently generate the correct
>>>> code.
>>>>
>>>> I thought about adding another field to the method
>>>> `emitFunctionalFactoryForPointer`, similar to fiName, like a boolean
>>>> for whether a segment allocator is needed or not, however, I can't help but
>>>> feel it is a bit hacky. I wonder what your opinions are on this.
>>>>
>>>> A repro can be found here:
>>>> https://github.com/lost-illusi0n/jextract-func-ptr-seg-alloc-repro
>>>> Just run the run script if you are on Linux. You will have to change the
>>>> library output in the script if you are on another os.