[feature] Add serial option in `gdbtarget`

[andy9a9]

I would like to use this extension for kgdbc. First step is to set the baudrate which is basically not possible due to late initialization (discussion ongoing in #432 ). So maybe would be nice to have options like:

"target":{
    "parameters": {
        "type": "tcp" # or serial
         "args": [
         ]
    },
    "serial": {
        "device": "/dev/ttyX",
        "baudrate": "115200"
    }
}

This will allow to directly select the type and custom commands like set serial baud 115200 won't be needed.
Next idea, while debugging the kernel on embedded systems should be fine to have an option to stop refreshing the threads. With this baudrate the response is so slow there is basically no way to use it. On the other hand, if I comment out the thread request like:

try {
    /*if (!this.isRunning) {
    const result = await mi.sendThreadInfoRequest(this.gdb, {});
    this.threads = result.threads
           .map((thread) => this.convertThread(thread))
           .sort((a, b) => a.id - b.id);
    }*/

    response.body = {
        threads: this.threads,
    };

    this.sendResponse(response);

system is more-less responsible and could be used for this purposes. Maybe the no-stop mode is for this purposes, but the kgdbc is not supporting it.

wdty?

[cwalther]

Not directly applicable to your question, but thinking out loud:

Some flexibility regarding the thread info requests is probably a good thing. In contrast to you who wants to do fewer of them, I am currently working on a situation where I want to do more of them: My problem is that right after attaching we don’t have thread names yet (because the adapter only knows about the threads from the thread-created notifications, which don’t include names) and the threads appear numbered 1 2 3 … in VS Code instead of with their proper names, the names only appear after hitting a breakpoint. (Thread names are important for us because we have many threads.)

One issue I am running into with that is that it is unclear when -thread-info commands are allowed. https://sourceware.org/gdb/current/onlinedocs/gdb.html/Asynchronous-and-non_002dstop-modes.html seems to claim that they are always allowed, on the other hand I have seen tests fail with GDB complaining that they are not allowed on a target with all threads running. Maybe the claim only applies to non-stop mode (which is what I am interested in) and the failures were in all-stop mode, I don’t remember exactly, still need to do further investigation.

[jreineckearm]

@andy9a9 , it looks like you did already have a look at what it takes to support this. How substantial would the changes be?
Do you know if it is only your specific setup that may run into performance issues? Or would any serial connection to kgdbc suffer from that?

What I am really trying to find out is whether such extension would become part of the gdbtarget type. Or if it would become a new debug adapter type derived from it.

Given that #432 may be quicker to implement and may have its charm in other use cases, my recommendation would be to start with implementing #432 . And by that have a basis for more investigations what it means to support kgdbc through a serial connection.

[andy9a9]

You know, the reason for requesting the preConnectCommands was to have an option to set serial device and baudrate before the target is connected. If you implement this variant like a target, basically I don't need to have these options. On the other hand I would say it makes sense to have it anyway.

Regarding to "performance" issues, I would say, it's a normal behaviour, while the serial is too slow (even if the speed was set to 115200. Kernel has more than 200 threads and resolving them all for takes some time. I made a dirty hack (mentioned in description) and it was usable again. So I'm thinking like, have them parameter (no-info-threads: false). If someone wants to have the info, he can send the command manually anyway.

[cwalther]

Now that #439 is settled, maybe we can continue the discussion about the -thread-info matter. (By the way, I find it a bit confusing that we are discussing two unrelated topics in this issue, serial options / preConnectCommands on the one hand and thread info requests on the other hand. @andy9a9, do you want to split that up?)

So, the current condition on whether a -thead-info command is sent to GDB when handling a threads request from the client is (and this satisfies my current requirements) (A):

!this.isRunning ||
(this.missingThreadNames && threadInfoAllowedWhenRunning)

As mentioned in the description of #439, I might in the future have a use case for having it be (B):

!this.isRunning || threadInfoAllowedWhenRunning

when thread details can change on the target at any time. I have not tried that yet, and I am not sure whether I would additionally need a way for the stub to tell the adapter to send an invalidated event to the client when that happens, to request a threads request and subsequent UI update. A would no longer be sufficient in that case, while having only B already today would probably be fine with me.

As far as I understand @andy9a9, he would like a way for the condition to be (C):

false

which does not satisfy my requirements.

A fourth possibility would be (D):

this.missingThreadNames &&
(!this.isRunning || threadInfoAllowedWhenRunning)

I have no particular preference between A and D.

So, shall we introduce an option in the launch/attach arguments that switches between three of these conditions? Or just two? Are there any other needs by other users? What should the option and its values be named? updateThreadInfo = "missing" | "always" | "never"?

[andy9a9]

Yes, looks like that we're discussing 2 things together. But I would say, the first "request" should be done by implementing section, which is send before the connection is established. So maybe we don't need the new target for serial.
The second thing is the threads stuff, which is more important for me. Yes, in my case the result of the condition should be false, that threads are not read.

[jreineckearm]

So, when set to never you would rely on the -thread-xxx async records to build any thread information? Sounds OK to me.

updateThreadInfo sounds OK to me, too. Alternative could be pollThreadInfo to more clearly call out the the nature of the thread info update.

always might raise more expectations than what it currently is, especially when we look at this option to be present for both all-stop and non-stop. normal or standard might be a bit fuzzy. But would leave always available for example for something that polls while running?