RFR (S) 8205608: Fix 'frames()' in ThreadReferenceImpl.c to prevent quadratic runtime behavior

[Schmelter, Ralf]

Hi Thomas,

thank you for reviewing the change.


> +    /* Should not usually happen. */
> +    if (length != count) {
> +        error = JVMTI_ERROR_INTERNAL;
> +    }
>
> Cosmetics: I would also probably explicitly return:
>
> /* Should not usually happen. */
> if (length != count) {
>   jvmtiDeallocate(frames);
>   outStream_setError(out, JDWP_ERROR(INTERNAL));
>   return JNI_TRUE;
> }
>
> .. makes the code clearer and should someone change the loop cancel
> condition it will still work.

This would still rely on the error check in the loop, since the GetStackTrace JVMTI call sets the error variable too.

This means it should be either explicit for both cases:
    error = JVMTI_FUNC_PTR(gdata->jvmti, GetStackTrace)
                          (gdata->jvmti, thread, startIndex, length, frames, &count);

    If (error != JVMTI_ERROR_NONE) {
        jvmtiDeallocate(frames);
        outStream_setError(out, map2jdwpError(error));
        return JNI_TRUE;
    }

    /* Should not happen. */
    if (length != count) {
        jvmtiDeallocate(frames);
        outStream_setError(out, JDWP_ERROR(INTERNAL));
        return JNI_TRUE;
    }

or none (note that the original code could overwrite the error from the GetStackTrace call, which is fixed here):
    error = JVMTI_FUNC_PTR(gdata->jvmti, GetStackTrace)
                          (gdata->jvmti, thread, startIndex, length, frames, &count);

    /* Should not happen. */
    if (error == JVMTI_ERROR_NONE  && length != count) {
        error = JVMTI_ERROR_INTERNAL;
    }



> +    for (fnum = 0; (fnum < count) && (error == JVMTI_ERROR_NONE); ++fnum) {
>
> you could loose the inner brackets.
>
> --
>
> Cosmetics: you changed meaning of fnum. Before it was really the frame
> number. Now, fnum is a zero based index into your array. So I would
> probably have renamed the variable too, maybe index? or somesuch.

Ok, index it is. 



> Do we not have to handle opaque frames like the code before did? Or
> does GetStackTrace already filter out opaque frames? Would that not
> mean that GetStackTrace returns fewer frames than expected, and then
> count could be smaller than length?

> -- oh wait I see GetFrameLocation never really returned
> JVMTI_ERROR_OPAQUE_FRAME? So it is probably fine.

Exactly. The old code would have skipped native methods in the stack trace, if JVMTI_ERROR_OPAQUE_FRAME would have been returned. But since this was in fact not returned, the stacks should look the same.




> How large can the depth get? In stack overflow scenarios?
>
> To limit memory usage and to make it more predictable, I would not
> retrieve all frames in one go but in a loop, in bulks a n frames. E.g.
> 4086 frames would mean your buffer never exceeds 64K on 64bit
> platforms. You would sacrifice a tiny bit of performance (again
> needless walking up to starting position) but would not choke out when
> stacks are ridiculously large.

In theory or in practice? Practically a stack overflow will have at most a few 100 thousand frames, usually much less (10 to 20 thousand). But one can image a scenario where the JIT could statically inline a lot of calls, leading to many Java frames per (small) physical frame.

But you should consider, that the whole stack is written 'to memory' already, since the packet output stream is backed completely by memory. So the memory requirement is already O(nrOfFrames).


> I cannot comment on the jtreg test. Looks fine to me, but I wonder
> whether there is a better way to script jdb, is this how we are
> supposed to do this?

I don't know. But the ShellScaffold.sh library is used by over 40 other JDI test, so I used it too.

Best regards,
Ralf