-
Notifications
You must be signed in to change notification settings - Fork 124
Add multi-process support #461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Apparently there have been some improvements to the debug adapter protocol specification to get some assumptions out of the way but they are not implemented in VSCode. Momentarily this is the best support I think is possible for multi-processing without needing DAP v1.51.x. When resuming and stepping some assumptions had to be made about which thread group needed to be continued/stepped, the reason being is that it is indistinguishable if the command came from the top-ribbon or the process/threads window.
|
There is a simple C++ program that I used to test. In fact I asked ChatGPT to directly translate #133 (comment) to C++ as the D code seemed less stable to me when I started testing. Also I added inferior processes that immediately exit, since this is a particular use case I'm interested in. #include <iostream>
#include <thread>
#include <chrono>
#include <unistd.h> // fork, sleep
#include <map>
#include <string>
#include <vector>
struct T {
uint8_t d;
char e;
T* child;
};
struct S {
int a;
long b;
std::string c;
T* f;
};
void fork_and_exit_other();
void secondThread();
int main(int argc, char* argv[]) {
// Print environment variables
extern char **environ;
for (char **env = environ; *env != nullptr; env++) {
std::string entry = *env;
auto pos = entry.find('=');
if (pos != std::string::npos) {
std::string key = entry.substr(0, pos);
std::string value = entry.substr(pos + 1);
std::cout << key << " = " << value << "\n";
}
}
std::cout.flush();
// Print 0, 1, 2
for (int i = 0; i < 3; i++) {
std::cout << i << "\n";
std::cout.flush();
}
// Handle args
std::vector<std::string> args;
for (int i = 0; i < argc; i++)
args.emplace_back(argv[i]);
if (args.size() > 1 && args[1] == "fork") {
pid_t origin = getpid();
pid_t i = fork();
if (i != 0) {
secondThread();
}
fork_and_exit_other();
} else if (args.size() == 1 || args[1] != "single") {
std::thread th(secondThread);
th.detach();
}
// Write without newline
std::cout << "No newline!";
std::cout.flush();
int count = 5;
for (; count < 20; count++) {
S s;
s.f = new T();
s.f->child = new T();
s.f->child->d = 4;
s.a = count;
}
// Print arguments
std::cout << "Got Arguments: ";
for (auto &a : args)
std::cout << a << " ";
std::cout << "\n";
// Main infinite loop
for (thread_local int i = 0; ; i++) {
std::cout << "Multithreaded Main thread " << i << "\n";
std::cout.flush();
std::this_thread::sleep_for(std::chrono::seconds(1));
}
return 0;
}
void fork_and_exit_other() {
pid_t pid = fork();
if (pid == 0) {
exit(1);
}
}
void secondThread() {
for (thread_local int i = 0; ; i++) {
std::cout << "Multithreaded " << i << "\n";
std::cout.flush();
std::this_thread::sleep_for(std::chrono::seconds(1));
}
} |
|
There are still some problems in the combination of threads and processes because the threadId is common among threads and processes. I'm looking for a solution. |
…bprocesses Still some polishing is needed since it is not very stable
WebFreak001
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work! Looks pretty good to me, but I can't guarantee any timeframe when this will be merged since I've been only very sporadically looking at my projects recently.
I have looked at this very briefly, I was not able to review the code for more deeply contained bugs or issues yet - but perhaps it's fine to just move them to live testing with users due to constrained reviewing resources.
Got a few requests already though, also can you resolve the linter/formatting issues that github just added in annotations?
Perhaps @GitMensch is interested in this too?
|
Thanks for the feedback. Right now the PR is still a WIP, since the scope increased because I went down the rabbit hole of starting multiple debug sessions. I have a very strong feeling that once every works stably, there is a huge opportunity for refactoring which I think is necessary prior to merging. |
|
Nevertheless, I have relatively little experience in Node.js and TypeScript so I will welcome feedback on style en conventions. Also I also had to add some features in the VSCode debug adapter to support this PR. Once the changes settle I will open a PR over there as well. So it will take a while before this PR can be merged. |
|
I think I have gotten to the point that it works relatively stable. Let's see if I can test drive this in the coming days 😄 |
| continue(): Thenable<boolean>; | ||
| interrupt(threadId?: number): Thenable<boolean>; | ||
| continue(reverse?: boolean, threadId?: number): Thenable<boolean>; | ||
| next(): Thenable<boolean>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface does not correspond with the actual implementation
src/mibase.ts
Outdated
| fs.unlink(this.serverPath, (err) => { | ||
| // eslint-disable-next-line no-console | ||
| console.error("Failed to unlink debug server"); | ||
| // eslint-disable-next-line no-console |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing Indendentation
src/mibase.ts
Outdated
| this.shared.miDebugger.once("debug-ready", (() => this.sendEvent(new InitializedEvent()))); | ||
| this.shared.miDebugger.on("thread-group-started", this.threadGroupStartedEvent.bind(this)); | ||
| this.shared.miDebugger.on("thread-group-exited", this.threadGroupExitedEvent.bind(this)); | ||
| this.sendEvent(new InitializedEvent()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line seems to break ssh connections. Removing it fixed it but I need to access its impact

I have rebased #133 to the new master since it hasn't been updated. Then I fixed some things to make the extension more respectable to pausing and continuing individual processes. I think only gdb is supported.
Please let me know if the problems that existed in the past are still present. I did some manual fuzzy testing on a C++ file.