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

Mon Aug 1 21:09:04 UTC 2022

Hi Marko,
I took a look at your PR:

https://github.com/openjdk/jextract/pull/58/files

What you do there works because, even though the downcall that 
`ofAddress` creates accepts an additional SegmentAllocator parameter, 
you provide such extra parameter in here:

```
      if(fiType.returnType().equals(MemorySegment.class)) {
                 append(", (SegmentAllocator)session");
             }
```

E.g. you take the session passed to `ofAddress` and turn that into a 
`SegmentAllocator` (which works, since MemorySession implements 
SegmentAllocator), and use that to allocate.

By doing that, you effectively inject a "default" allocator into the 
downcall method handle, which makes sure that the returned MemorySegment 
belongs to the provided session.

This is a very simple, yet effective fix (well done!), and I think one 
that could be good enough, at least for now.

Where this fix type of fix leaves a bit to be desired is that it doesn't 
provide the degree of flexibility that a client might want. Say you have 
a more efficient SegmentAllocator, and you want to use _that_ to 
allocate the returned struct; that is not possible with the approach in 
your PR.

That said, I'm not sure how big of a problem that is, given that in 
special situations clients can still wrap function pointers on their 
own, if they are not happy with what jextract provides. So I think your 
simple solution probably goes a long way (and we can always come back 
later and tweak this again).

Thanks!
Maurizio

On 01/08/2022 20:08, Marko wrote:
> Further email difficulties/default settings are odd, apologies!
>
> Hey Maurizio,
> Thanks for the explanation, it helped a lot with understanding with
> what's going on. I'm not quite sure how to cause a problem with the
> two use cases you described, though.
>
> Scenario 1 would be equivalent to using the allocate method in the FI I believe.
> Scenario 2 would be calling ofAddress on the memory segment/address we
> got from 1.
>
> This is what I tried, and it seemed to work, though. The linker does
> not seem to add a SegmentAllocator when creating the upcall, like you
> said. However, wrapping the returned memory segment/address, from
> scenario 1, inside the FI using ofAddress and calling it works...? I'm
> assuming somewhere my logic is flawed, and the method handle should
> not accept the additional SegmentAllocator when invoking. I just don't
> quite understand why it doesn't fail. Anyway, I think your solution is
> quite reasonable and that it should use the implicit allocator by
> default as well, as that seems to be the behavior in other places of
> jextract.
>
> Thanks
> Marko
>
> On Fri, Jul 29, 2022 at 5:41 PM Maurizio Cimadamore
> <maurizio.cimadamore at oracle.com> wrote:
>> 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://urldefense.com/v3/__https://github.com/openjdk/jextract/blob/0582eaf1b4cdba95f0ee8c2480767433bb647d0d/src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java*L122-L127__;Iw!!ACWV5N9M2RV99hQ!Ju4E0gBlSpNpLkrFM97aohppFrNxuYCupnykO3jWsUarbLQ7KY33yrIGCfiv5-bwrlqaHWkcFleOyftmGi4snw$ .
>>>>
>>>> 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://urldefense.com/v3/__https://github.com/openjdk/jextract/blob/0582eaf1b4cdba95f0ee8c2480767433bb647d0d/src/main/java/org/openjdk/jextract/impl/StructBuilder.java*L128=__;Iw!!ACWV5N9M2RV99hQ!Ju4E0gBlSpNpLkrFM97aohppFrNxuYCupnykO3jWsUarbLQ7KY33yrIGCfiv5-bwrlqaHWkcFleOyftu-dxHNA$
>>>> 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://urldefense.com/v3/__https://github.com/openjdk/jextract/blob/0582eaf1b4cdba95f0ee8c2480767433bb647d0d/src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java*L122-L127__;Iw!!ACWV5N9M2RV99hQ!Ju4E0gBlSpNpLkrFM97aohppFrNxuYCupnykO3jWsUarbLQ7KY33yrIGCfiv5-bwrlqaHWkcFleOyftmGi4snw$ .
>>>>>>
>>>>>> 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://urldefense.com/v3/__https://github.com/lost-illusi0n/jextract-func-ptr-seg-alloc-repro__;!!ACWV5N9M2RV99hQ!Ju4E0gBlSpNpLkrFM97aohppFrNxuYCupnykO3jWsUarbLQ7KY33yrIGCfiv5-bwrlqaHWkcFleOyfuzpLEnvg$
>>>>>> 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.