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

[Marko]

Disregard.

On Mon, Aug 1, 2022 at 2:56 PM Marko <im at lostillusion.net> wrote:
>
> 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.