On Mon, 16 Oct 2023 21:05:58 GMT, Johannes Bechberger <jbechber...@openjdk.org>
wrote:
>> Fix `onthrow` issue by passing the event info to the `initialize` method.
>>
>> This prevents `jdb` from receiving a broken exception event and throwing an
>> internal NullPointerException, upon attaching to the JDWP-agent.
>
> Johannes Bechberger has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Update test/jdk/com/sun/jdi/JdwpOnThrowTest.java
>
> Co-authored-by: Chris Plummer <chris.plum...@oracle.com>
I've included a fix. I'm still running it through more thorough testing.
However, I'm also wondering about this whole business of sending the
ExceptionEvent in the first place. It's very "Kludgy" as already called out in
a comment. The following also caught my eye:
`eventHelper_recordEvent(&info, 0, suspendPolicy, initEventBag);`
`0` is the event handlerID (sometimes known as the requestID in JDWP or JDI).
At that point I realized that we may be sending an event that the debugger
didn't even ask for. I don't think there is anything in the JDI spec covering
delivering an event in this manner. I guess event->request() is working because
JDB actually setup an EventRequest for ExceptionEvent, but what would happen if
it didn't? I'm thinking of also sending a second event that I know JDB is not
expecting just to see what happens.
I'm also wondering if we shouldn't just export `cbException()` and call it
directly rather than mimic a lot of what it is doing, although probably what
would be easier is a new API that handles the event described in the EventInfo
passed to it. Maybe `cbEI()`. This would resolve the handlerID issue I point
out (the event would not be sent if there is no request for it, and if there is
a request it will be sent with the correct handlerID, not 0).
src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c line 730:
> 728: if (opt_info != NULL) {
> 729: info = *opt_info;
> 730: }
Try the following code here. I don't think I changed anything outside this
area, but since I also have your diffs present I can't reacall for sure. I also
cleaned this section up a bit. It looks like it only has to deal with
EI_EXCEPTION and opt_info is always set.
/*
* TO DO: Kludgy way of getting the triggering event to the
* just-attached debugger. It would be nice to make this a little
* cleaner. There is also a race condition where other events
* can get in the queue (from other not-yet-suspended threads)
* before this one does. (Also need to handle allocation error below?)
*/
struct bag *initEventBag;
LOG_MISC(("triggering_ei == EI_EXCEPTION"));
JDI_ASSERT(triggering_ei == EI_EXCEPTION);
JDI_ASSERT(opt_info != NULL);
initEventBag = eventHelper_createEventBag();
threadControl_onEventHandlerEntry(currentSessionID, opt_info, NULL);
eventHelper_recordEvent(opt_info, 0, suspendPolicy, initEventBag);
(void)eventHelper_reportEvents(currentSessionID, initEventBag);
bagDestroyBag(initEventBag);
-------------
PR Review: https://git.openjdk.org/jdk/pull/16145#pullrequestreview-1683135388
PR Review Comment: https://git.openjdk.org/jdk/pull/16145#discussion_r1362615989
