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

[Marko]

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://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.