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

Tue Aug 2 19:02:13 UTC 2022

Hey Maurizio,
Thanks for taking a look at the PR and the guidance so far. It is
great news that it properly injects the "default" allocator as I
imagined it to. It would be nice to allow the use of a custom
allocator though, however my knowledge of the project was not quite
there yet to know how that would look like. I committed a new test and
I think the PR is ready to be looked at.
Thanks,
Marko.

On Mon, Aug 1, 2022 at 5:09 PM Maurizio Cimadamore
<maurizio.cimadamore at oracle.com> wrote:
>
> 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.