|
From: Philippe W. <phi...@sk...> - 2023-04-15 20:27:53
|
Some additional comments below ... Thanks Philippe On Thu, 2023-04-13 at 22:09 +0000, Mark Wielaard wrote: > https://sourceware.org/git/gitweb.cgi?p=valgrind.git;h=0432ce486d61f84f6fcbeab0d812bb1f61c57561 > > commit 0432ce486d61f84f6fcbeab0d812bb1f61c57561 > Author: Alexandra Petlanova Hajkova <aha...@re...> > Date: Thu Mar 3 04:46:03 2022 -0500 > > vgdb: implement the extended-remote protocol > > > > Executing vgdb --multi makes vgdb talk the gdb extended-remote > protocol. This means that the gdb run command is supported and > vgdb will start up the program under valgrind. Which means you > don't need to run gdb and valgrind from different terminals. > Also vgdb keeps being connected to gdb after valgrind exits. So > you can easily rerun the program with the same breakpoints in > place. > > > > vgdb now implements a minimal gdbserver that just recognizes > a few extended-remote protocol packets. Once it starts up valgrind > it sets up noack and qsupported then it will forward packets > between gdb and valgrind gdbserver. After valgrind shutsdown it > resumes handling gdb packets itself. > > > > https://bugs.kde.org/show_bug.cgi?id=434057 > > > > Co-authored-by: Mark Wielaard <ma...@kl...> valgrind --help does not describe the 'multi' related option, but --help-debug describes it. That was a little bit surprising to me. As this is not a real user option, it however does not fully fit in the section: uncommon user options for all Valgrind tools: > > diff --git a/coregrind/vgdb.c b/coregrind/vgdb.c > index 9a9b90e585..1cc37a3566 100644 > --- a/coregrind/vgdb.c > +++ b/coregrind/vgdb.c > > /* vgdb has two usages: Maybe this should describe here 3 usages ? > @@ -169,7 +175,17 @@ void map_vgdbshared(char* shared_mem) > { > struct stat fdstat; > void **s; > - shared_mem_fd = open(shared_mem, O_RDWR); > + int tries = 50; > + int err; > + > + /* valgrind might still be starting up, give it 5 seconds. */ Maybe the --wait arg (if provided) could override this hard-coded value ? > +/* Returns an allocated hex-decoded string from the buf starting at offset > + off. Will update off to where the buf has been decoded. Stops decoding > + at end of buf (zero) or when seeing the delim char. */ The comment seems not aligned with the below function (e.g. there is no off arg). > +static > +char *decode_hexstring (const char *buf, size_t prefixlen, size_t len) > +{ > + int buflen; > + char *buf_print; > + > + if (len) > + buflen = len; > + else > + buflen = strlen(buf) - prefixlen; > + > + buf_print = vmalloc (buflen/2 + 1); > + > + for (int i = 0; i < buflen; i = i + 2) { > + buf_print[i/2] = ((fromhex(buf[i+prefixlen]) << 4) > + + fromhex(buf[i+prefixlen+1])); > + } > + buf_print[buflen/2] = '\0'; > + DEBUG(1, "decode_hexstring: %s\n", buf_print); > + return buf_print; > +} > + > > > > + > +/* Creates a packet from a string message, called needs to free. */ caller needs to free ? > +static char * > +create_packet(const char *msg) > In some functions from here onwards, there are a bunch of indentation with 4 spaces while the rest of vgdb.c is 3. > + > +static int read_one_char (char *c) > +{ > + int i; > + do > + i = read (from_gdb, c, 1); > + while (i == -1 && errno == EINTR); > + > + return i; > +} > + > > + > +// Throws away the packet name and decodes the hex string, which is placed in What is the packet name ? > +// decoded_string (the caller owns this and is responsible for freeing it). > +static int split_hexdecode(const char *buf, const char *string, > + const char *delim, char **decoded_string) > +{ > + const char *next_str = next_delim_string(buf, *delim); > + if (next_str) { > + *decoded_string = decode_hexstring (next_str, 0, 0); > + DEBUG(1, "split_hexdecode decoded %s\n", *decoded_string); > + return 1; > + } else { > + TSFPRINTF(stderr, "%s decoding error: finding the hex string in %s failed!\n", string, buf); > + return 0; > + } > +} > + > +static int count_delims(char delim, char *buf) This should return a size_t > +{ > + size_t count = 0; > + char *ptr = buf; > + > + while (*ptr) > + count += *ptr++ == delim; > + return count; > +} > + > > > +/* Do multi stuff. */ > +static > +void do_multi_mode(void) > +{ > + char *buf = vmalloc(PBUFSIZ+1); > + char *q_buf = vmalloc(PBUFSIZ+1); //save the qSupported packet sent by gdb > + //to send it to the valgrind gdbserver later > + q_buf[0] = '\0'; > + int noackmode = 0, pkt_size = 0, bad_unknown_packets = 0; > + char *string = NULL; > + char *working_dir = NULL; > + DEBUG(1, "doing multi stuff...\n"); > + while (1){ > + /* We get zero if the pipe was closed (EOF), or -1 on error reading from > + the pipe to gdb. */ > + pkt_size = receive_packet(buf, noackmode); > + if (pkt_size <= 0) { > + DEBUG(1, "receive_packet: %d\n", pkt_size); > + break; > + } > + > + DEBUG(1, "packet recieved: '%s'\n", buf); typo: received > + > +#define QSUPPORTED "qSupported:" > +#define STARTNOACKMODE "QStartNoAckMode" > +#define QRCMD "qRcmd" // This is the monitor command in gdb > +#define VRUN "vRun" > +#define XFER "qXfer" > +#define QATTACHED "qAttached" > +#define QENVIRONMENTHEXENCODED "QEnvironmentHexEncoded" > +#define QENVIRONMENTRESET "QEnvironmentReset" > +#define QENVIRONMENTUNSET "QEnvironmentUnset" > +#define QSETWORKINGDIR "QSetWorkingDir" > +#define QTSTATUS "qTStatus" > + > + if (strncmp(QSUPPORTED, buf, strlen(QSUPPORTED)) == 0) { > + DEBUG(1, "CASE %s\n", QSUPPORTED); > + // And here is our reply. > + // XXX error handling? We don't check the arguments. > + char *reply; > + strcpy(q_buf, buf); > + // Keep this in sync with coregrind/m_gdbserver/server.c Worth adding a note in server.c to also point at this place ? > + asprintf (&reply, > + "PacketSize=%x;" > + "QStartNoAckMode+;" > + "QPassSignals+;" > + "QCatchSyscalls+;" > + /* Just report support always. */ > + "qXfer:auxv:read+;" > + /* We'll force --vgdb-shadow-registers=yes */ > + "qXfer:features:read+;" > + "qXfer:exec-file:read+;" > + "qXfer:siginfo:read+;" > + /* Some extra's vgdb support before valgrind starts up. */ > + "QEnvironmentHexEncoded+;" > + "QEnvironmentReset+;" > + "QEnvironmentUnset+;" > + "QSetWorkingDir+", (UInt)PBUFSIZ - 1); > + send_packet(reply, noackmode); > + free (reply); > + } > + else if (strncmp(STARTNOACKMODE, buf, strlen(STARTNOACKMODE)) == 0) { > + // We have to ack this one > + send_packet("OK", 0); > + noackmode = 1; > + } > + else if (buf[0] == '!') { > + send_packet("OK", noackmode); > + } > + else if (buf[0] == '?') { > + send_packet("W00", noackmode); > + } > + else if (strncmp("H", buf, strlen("H")) == 0) { > + // Set thread packet, but we are not running yet. > + send_packet("E01", noackmode); > + } > + else if (strncmp("vMustReplyEmpty", buf, strlen("vMustReplyEmpty")) == 0) { > + send_packet ("", noackmode); > + } > + else if (strncmp(QRCMD, buf, strlen(QRCMD)) == 0) { > + send_packet ("No running target, monitor commands not available yet.", noackmode); > + > + char *decoded_string = decode_hexstring (buf, strlen (QRCMD) + 1, 0); > + DEBUG(1, "qRcmd decoded: %s\n", decoded_string); > + free (decoded_string); > + } > + else if (strncmp(VRUN, buf, strlen(VRUN)) == 0) { > + // vRun;filename[;argument]* > + // vRun, filename and arguments are split on ';', > + // no ';' at the end. > + // If there are no arguments count is one (just the filename). > + // Otherwise it is the number of arguments plus one (the filename). > + // The filename must be there and starts after the first ';'. > + // TODO: Handle vRun;[;argument]* > + // https://www.sourceware.org/gdb/onlinedocs/gdb/Packets.html#Packets > + // If filename is an empty string, the stub may use a default program > + // (e.g. the last program run). > + size_t count = count_delims(';', buf); > + size_t *len = vmalloc(count * sizeof(count)); > + const char *delim = ";"; > + const char *next_str = next_delim_string(buf, *delim); > + char **decoded_string = vmalloc(count * sizeof (char *)); > + > + // Count the lenghts of each substring, init to -1 to compensate for typo: lenghts > + // each substring starting with a delim char. > + for (int i = 0; i < count; i++) > + len[i] = -1; > + count_len(';', buf, len); > + if (next_str) { > + DEBUG(1, "vRun: next_str %s\n", next_str); > + for (int i = 0; i < count; i++) { > + /* Handle the case when the arguments > + * was specified to gdb's run command > + * but no remote exec-file was set, > + * so the first vRun argument is missing. > + * For example vRun;;6c. */ > + if (*next_str == *delim) { > + next_str++; > + /* empty string that can be freed. */ > + decoded_string[i] = strdup(""); > + } > + else { > + decoded_string[i] = decode_hexstring (next_str, 0, len[i]); > + if (i < count - 1) > + next_str = next_delim_string(next_str, *delim); > + } > + DEBUG(1, "vRun decoded: %s, next_str %s, len[%d] %d\n", decoded_string[i], next_str, i, len[i]); Line too long. > + } > + > + /* If we didn't get any arguments or the filename is an empty > + string, valgrind won't know which program to run. */ > + DEBUG (1, "count: %d, len[0]: %d\n", count, len[0]); > + if (! count || len[0] == 0) { > + free(len); > + for (int i = 0; i < count; i++) > + free (decoded_string[i]); > + free (decoded_string); > + send_packet ("E01", noackmode); > + continue; > + } > + > + /* We have collected the decoded strings so we can use them to > + launch valgrind with the correct arguments... We then use the > + valgrind pid to start relaying packets. */ > + pid_t valgrind_pid = -1; > + int res = fork_and_exec_valgrind (count, > + decoded_string, > + working_dir, > + &valgrind_pid); > + > + if (res == 0) { > + // Lets report we Stopped with SIGTRAP (05). > + send_packet ("S05", noackmode); > + prepare_fifos_and_shared_mem(valgrind_pid); > + DEBUG(1, "from_gdb_to_pid %s, to_gdb_from_pid %s\n", from_gdb_to_pid, to_gdb_from_pid); line too long > + // gdb_rely is an endless loop till valgrind quits. typo: gdb_rely > + shutting_down = False; > + > + gdb_relay (valgrind_pid, 1, q_buf); > + cleanup_fifos_and_shared_mem(); > + DEBUG(1, "valgrind relay done\n"); > + int status; > + pid_t p = waitpid (valgrind_pid, &status, 0); > + DEBUG(2, "waitpid: %d\n", (int) p); > + if (p == -1) > + DEBUG(1, "waitpid error %s\n", strerror (errno)); > + else { > + if (WIFEXITED(status)) > + DEBUG(1, "valgrind exited with %d\n", > + WEXITSTATUS(status)); > + else if (WIFSIGNALED(status)) > + DEBUG(1, "valgrind kill by signal %d\n", > + WTERMSIG(status)); > + else > + DEBUG(1, "valgind unexpectedly stopped or continued"); typo: valgind > + } > + } else { > + send_packet ("E01", noackmode); > + DEBUG(1, "OOPS! couldn't launch valgrind %s\n", > + strerror (res)); > + } > + > + free(len); > + for (int i = 0; i < count; i++) > + free (decoded_string[i]); > + free (decoded_string); > + } else { > + free(len); > + send_packet ("E01", noackmode); missing indentation > + DEBUG(1, "vRun decoding error: no next_string!\n"); > + continue; > + } > + } else if (strncmp(QATTACHED, buf, strlen(QATTACHED)) == 0) { > + send_packet ("1", noackmode); > + DEBUG(1, "qAttached sent: '1'\n"); > + const char *next_str = next_delim_string(buf, ':'); > + if (next_str) { > + char *decoded_string = decode_hexstring (next_str, 0, 0); > + DEBUG(1, "qAttached decoded: %s, next_str %s\n", decoded_string, next_str); > + free (decoded_string); > + } else { > + DEBUG(1, "qAttached decoding error: strdup of %s failed!\n", buf); > + continue; > + } > + } /* Reset the state of environment variables in the remote target before starting > + the inferior. In this context, reset means unsetting all environment variables > + that were previously set by the user (i.e., were not initially present in the environment). */ line too long Below we have indentation with 5 chars at many places. > + else if (strncmp(QENVIRONMENTRESET, buf, > + strlen(QENVIRONMENTRESET)) == 0) { > + send_packet ("OK", noackmode); > + // TODO clear all environment strings. We're not using > + // environment strings now. But we should. > + } else if (strncmp(QENVIRONMENTHEXENCODED, buf, > + strlen(QENVIRONMENTHEXENCODED)) == 0) { > + send_packet ("OK", noackmode); > + if (!split_hexdecode(buf, QENVIRONMENTHEXENCODED, ":", &string)) > + break; > + // TODO Collect all environment strings and add them to environ > + // before launcing valgrind. typo: launcing > + free (string); > + string = NULL; > + } else if (strncmp(QENVIRONMENTUNSET, buf, > + strlen(QENVIRONMENTUNSET)) == 0) { > + send_packet ("OK", noackmode); > + if (!split_hexdecode(buf, QENVIRONMENTUNSET, ":", &string)) > + break; > + // TODO Remove this environment string from the collection. > + free (string); > + string = NULL; > + } else if (strncmp(QSETWORKINGDIR, buf, > + strlen(QSETWORKINGDIR)) == 0) { > + // Silly, but we can only reply OK, even if the working directory is > + // bad. Errors will be reported when we try to execute the actual > + // process. > + send_packet ("OK", noackmode); > + // Free any previously set working_dir > + free (working_dir); > + working_dir = NULL; > + if (!split_hexdecode(buf, QSETWORKINGDIR, ":", &working_dir)) { > + continue; // We cannot report the error to gdb... > + } > + DEBUG(1, "set working dir to: %s\n", working_dir); > + } else if (strncmp(XFER, buf, strlen(XFER)) == 0) { > + char *buf_dup = strdup(buf); > + DEBUG(1, "strdup: buf_dup %s\n", buf_dup); > + if (buf_dup) { > + const char *delim = ":"; > + size_t count = count_delims(delim[0], buf); > + if (count < 4) { > + strsep(&buf_dup, delim); > + strsep(&buf_dup, delim); > + strsep(&buf_dup, delim); > + char *decoded_string = decode_hexstring (buf_dup, 0, 0); > + DEBUG(1, "qXfer decoded: %s, buf_dup %s\n", decoded_string, buf_dup); > + free (decoded_string); > + } > + free (buf_dup); > + } else { > + DEBUG(1, "qXfer decoding error: strdup of %s failed!\n", buf); > + free (buf_dup); > + continue; > + } > + // Whether we could decode it or not, we cannot handle it now. We > + // need valgrind gdbserver to properly reply. So error out here. > + send_packet ("E00", noackmode); > + } else if (strncmp(QTSTATUS, buf, strlen(QTSTATUS)) == 0) { > + // We don't support trace experiments > + DEBUG(1, "Got QTSTATUS\n"); > + send_packet ("", noackmode); > + } else if (strcmp("qfThreadInfo", buf) == 0) { > + DEBUG(1, "Got qfThreadInfo\n"); > + /* There are no threads yet, reply 'l' end of list. */ > + send_packet ("l", noackmode); > + } else if (buf[0] != '\0') { > + // We didn't understand. > + DEBUG(1, "Unknown packet received: '%s'\n", buf); > + bad_unknown_packets++; > + if (bad_unknown_packets > 10) { > + DEBUG(1, "Too many bad/unknown packets received\n"); > + break; > + } > + send_packet ("", noackmode); > + } > + } > + DEBUG(1, "done doing multi stuff...\n"); > + free(working_dir); > + free(buf); > + free(q_buf); > + > + shutting_down = True; > + close (to_gdb); > + close (from_gdb); > +} > + > +/* Relay data between gdb and Valgrind gdbserver, till EOF or an error is > + encountered. q_buf is the qSupported packet received from gdb. */ > +static > +void gdb_relay(int pid, int send_noack_mode, char *q_buf) > { > int from_pid = -1; /* fd to read from pid */ > int to_pid = -1; /* fd to write to pid */ > @@ -863,6 +1569,16 @@ void gdb_relay(int pid) ... > @@ -1379,6 +2135,7 @@ void parse_options(int argc, char** argv, > TSFPRINTF(stderr, "%s is not a correct path. %s, exiting.\n", path, strerror (errno)); Line too long. ... > // argc - i is the number of left over arguments > // allocate enough space, but all args in it. Not clear what is meant by 'but all args in it.' > @@ -1275,6 +1298,19 @@ Therefore, it has two usage modes: > </para> > </listitem> > > + <listitem id="manual-core-adv.vgdb-multi" xreflabel="vgdb multi"> > + <para>In the <option>--multi</option> mode, Vgdb uses the extended Elsewhere in the doc, we use vgdb, not Vgdb. > + remote protocol to communicate with Gdb. This allows you to view And we use GDB, not Gdb. > + output from both valgrind and GDB in the GDB session. This is > + accomplished via the "target extended-remote | vgdb --multi". In > + this mode you no longer need to start valgrind yourself. vgdb will > + start up valgrind when gdb tells it to run a new program. For this > + usage, the vgdb OPTIONS(s) can also include <option>--valgrind</option> > + and <option>--vargs</option> to describe how valgrind should be > + started. > + </para> > + </listitem> |