[foreign-preview] RFR: 8281228: Preview branch's CLinker.downcallHandle crashes inside asm

[Maurizio Cimadamore]

On Fri, 4 Feb 2022 04:06:15 GMT, Athijegannathan Sundararajan <sundar at openjdk.org> wrote:

>> This patch addresses a low level crash occuring in the ASM library, triggered by the new binding specialization logic. After some debugging I realized that there was no real "bug", but something subtle that we missed during review. The binding specializer uses an internal operand stack to keep track of the types maniupulated when processing ABI bindings. Most of the "pop" operation on this stack were done inside an `assert` statement, which meant that the pop operation would not be executed when running without `-esa`. Unfortunately this issue was not caught because the makefile always runs tests with assertions enabled.
>> 
>> After fixing this, I have also verified that the existing test (if ran without assertion enabled) would indeed have been enough to trigger the failure; in other words, the failure has not been detected because of *the way* in which tests were ran, and not because we were lacking in test coverage (for instance, TestDowncall showed 812 failures without the fix). For these reasons I did not include any test in the fix.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 153:
> 
>> 151:         new BindingSpecializer(mv, callerMethodType, callingSequence, abi, leafHandle.type()).specialize();
>> 152: 
>> 153:         mv.visitMaxs(-1, -1);
> 
> visitMaxs arguments are ignored with COMPUTE_FRAMES flag, right? any specific reason for -1, -1 instead of 0, 0?

I think you are right. I got confused initially as I thought I've seen another place in this class where -1 was used, and I was trying to experiment making ASM spit the bytecode (and not crash) so that I could look at the actual bytecode :-)

This can be reverted

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

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