[foreign-jextract] RFR: 8239128: Macro defines constant pointers can crash VM via jextract API

[Maurizio Cimadamore]

On Tue, 18 Feb 2020 01:13:06 GMT, Henry Jen <henryjen at openjdk.org> wrote:

>> src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/TypeImpl.java line 150:
>> 
>>> 149: 
>>> 150:         public PointerImpl(Type pointee) {
>>> 151:             super(Kind.POINTER, Optional.empty());
>> 
>> Can't we implement this constructor by creating a factory which always return the same pointee (e.g. the constructor parameter) and drop the pointee field?
> 
> No, note that the design is to throw away factory after used(resolved), because  there is no guarantee that factory can still work in this case. Which is why we call type() in TypeMaker.resolveTypeReferences()

Yeah - I think what I'm saying here is that you are using a side-effect to subtly alter the way pointer types are represented. Before the type() call there's a factory, after the type() call, the factory is gone and you have a pointee. So you have two mutable things, and some side-effects.

Another way to approach this problem would be to add an indirection. Let's say that you call `makeType` and you find a pointer type; what we could do is to setup a PointerTypeImpl, whose 'pointee' supplied does something like this:

new TypeImpl.PointerImpl(() -> getPointee(pointerId++));

That is, as we turn clang types into high-level types, we also build a side-map from pointers to pointees. Initially this map will contain no entries, but after you resolve all the pending pointers, you will start adding entries for them in the table. Which means that, when the client calls PointerTypeImpl::type(), the lookup succeeds and the client gets the type it wants (w/o looking up into clang specific info).

I think an approach like this can be made to work for all the situations in which we have to 'delay' pointee info computation.

Actually, if all clang types get registered into the TypeMaker cache (as they are now), we don't even to do anything special - the pointee supplier can simply do a lookup on the cache using the identity of the clang type. Note that, while, after we get rid of the index, or after reparsing we can no longer use a clang type with the clang API, the Type object we have is still valid and can be used to to e.g. Map lookups.

So, I think there is a simpler solution here, where we:

* retain the old `PointerImpl` implementation
* when we create a pointer type, we setup a supplier lambda - but instead of having the lambda call `makeType` as before, we make the lambda to a direct lookup on the TypeMaker cache using the clang type object as a key
* we also register the pointer to the 'todo' list, as you are doing now
* when `resolveTypeReferences` is called, we go through the todo list, and call makeType on all the pointees in there - thus populating the cache

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/21