Hi Ralf,
On Wed, Jul 4, 2018 at 3:47 PM, Schmelter, Ralf <ralf.schmel...@sap.com> wrote:
> 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;
> }
>
Okay, in that case I prefer the second variant. At least only one
deallocate call then.
>
>
>> + 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.
>
>
Thanks.
>
>> 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).
Okay. Just did a quick calculation, we need now 33 bytes per frame in
the outputstream, and now we need 16 more. But I find it difficult to
see how one would be a problem and the other would not. So okay, lets
keep the code simple.
>
>
>> 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
Okay. From my point, this is reviewed.
Thanks & Best Regards, Thomas
>
