qbicc icon indicating copy to clipboard operation
qbicc copied to clipboard

Reduce amount of build-time only code that is considered reachable

Open dgrove-oss opened this issue 2 years ago • 5 comments

While debugging #1177, it came up that significant pieces of the class library machinery for supporting MethodHandles is getting into the final executable. This code is not designed to operate at runtime in qbicc (it will fail/crash in odd ways) and is probably dragging in quite a bit of code and data that we don't need.

We need to analyze why this code is being considered reachable and prevent it from happening. It might be as simple as putting some if (!Build.isHost()) { throw new UnsupportedOperationException()}` stanzas in the main entrypoints to the code.

dgrove-oss avatar Mar 11 '22 14:03 dgrove-oss

Do you have any examples on hand? Hopefully it'd be an easy fix...

dmlloyd avatar Mar 11 '22 14:03 dmlloyd

I'm going to try to poke at it this afternoon...no examples yet. Just that I know that when I was debugging the VarHandles stuff earlier this week I was multiple stack frames deep into code that had no way of working at runtime...so didn't want to forget about it over the weekend if today doesn't worked out as hoped.

dgrove-oss avatar Mar 11 '22 15:03 dgrove-oss

The entrypoint that was causing the crash earlier this week was in java.lang.invoke.VarHandle. Specifically,

    MethodHandle getMethodHandle(int mode) {
        TypesAndInvokers tis = getTypesAndInvokers();
        MethodHandle mh = tis.methodHandle_table[mode];
        if (mh == null) {
            mh = tis.methodHandle_table[mode] = getMethodHandleUncached(mode);
        }
        return mh;
    }

getMethodHandleUncached is only going to work at build time for us.

Although we could patch getMethodHandle to inject a if (!Build.isHost()) { ... } cutoff, it might be easier to maintain things if we had a BuildTimeOnly annotation that could be applied to methods. Then we could patch in just the annotation on getMethodHandleUncached without having to build the bigger patch to actually change getMethodHandle. The semantics of the BuildTimeOnly annotation would be something like replace the body of this method with an unconditional throw of an exception during the ANALYZE phase.

Does the annotation seem like a reasonable approach?

dgrove-oss avatar Mar 11 '22 16:03 dgrove-oss

Yeah I think that makes sense. We might also have to populate this cache more eagerly as well. The fix in #1201 - which I completely forgot to submit this week - should also help by removing most (hopefully all) usages of this method by the JDK. But I think that marking whole methods as build-time-only would be a useful tool as well - in this particular case, it is a public API so anyone could call into it at run time.

dmlloyd avatar Mar 11 '22 19:03 dmlloyd

Implemented annotation support in #1202

dgrove-oss avatar Mar 11 '22 22:03 dgrove-oss