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

[Maurizio Cimadamore]

On Sun, 16 Feb 2020 20:56:35 GMT, Henry Jen <henryjen at openjdk.org> wrote:

> The fundamental issue of the crash is that clang cursor/type may no longer valid after reparse another macro, and with pointer type lazily resolve the pointee type, if the clang cursor/type no longer valid, access invalid memory cause the crash.
> 
> The other issue is the a macro pointer to a record type like struct/union can cause NPE, and that cause the macro to be ignored on generation without a warning.
> 
> The fix remove the laziness from Type API perspective, but leave that as an implementation detail, so implementation of Pointer type need to figure out how to do that safely internally.
> 
> Record type in macro are reduces to void, as currently the Declaration implementation is depending on clang as well, we cannot guarantee that works after reparse.

Overall the approach seems promising, but I suggest to think about it some more to make sure it can handle the full complexity of what will come up in real headers - beside macro evaluation, as the problem you found on macros is a particular instance of a more general issue.

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?

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/TypeMaker.java line 53:

> 52:      */
> 53:     void resolveTypeReferences() {
> 54:         pointers.forEach(TypeImpl.PointerImpl::type);

This approach seems flexible. It relies on keeping track of which pointers are created and then have the client call, at a later point 'resolveTypeReferences' to make sure that we fully convert all pointee clang types into clang-agnostic types.

I have few observations here:
* this mechanism is, general and should allow for all pointers to remove dependencies from clang-originated types, which should allow us to be able to close the clang index after we parse
* I'm a bit worried that the resolve logic is a bit too simplistic. In principle, it could be possible for resolve() to add new pointer types into `pointers`, so, resolve should use some kind of fixed point logic - e.g. keep going until we're sure _all_ types have been resolved, no matter how deep.

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/MacroParserImpl.java line 100:

> 99:                 .findAny().get();
> 100:         typeMaker.resolveTypeReferences();
> 101:         return rv;

Should this call go here? Probably - if we drop the toplevel unit after subsequent reparse. But, I think we should also put a call on this at the end of parsing? Let's say I have  something like:

struct A {
   struct B *b;
}

struct B {
  struct A *a;
}

No macros here - how does this work? I believe you will add pointers in the todo list (e.g. when we parse `struct A` we will add `B*`). Who resolves that? And, if we did a resolve, wouldn't that end up adding another pointer (`A*`) to the list? Who would resolve that? And if we re-run resolve on the new pointer, how do we prevent the logic from re-adding the already resolved pointer type `B*` ?

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



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