Hi - Comments for patch 3 below.
> From 5da60d8daa72da33fd91b7425357ee4783b98fc6 Mon Sep 17 00:00:00 2001 > From: Christopher Koch <[email protected]> > Date: Tue, 12 Jul 2016 11:56:10 -0700 > Subject: Asynchronous client; full end-to-end single-threaded debugging > working. > Need some sort of description for the commit. Since you're changing the toolchain too, mark the commit with (XCC) and say something like "Rebuild glibc". Incidentally, you could also split that out into another commit. > Change-Id: Iadf0c6b1c2c26a86e9ed7296b6fe459b92d8f180 > Signed-off-by: Christopher Koch <[email protected]> > --- > kern/arch/x86/trap.c | 20 +- > .../glibc-2.19-akaros/sysdeps/akaros/sigaction.c | 3 +- > user/parlib/debug.c | 476 > +++++++++++++++++---- > user/parlib/include/parlib/debug.h | 97 ++++- > user/parlib/include/parlib/uthread.h | 1 + > user/parlib/include/parlib/x86/debug.h | 1 + > user/parlib/include/parlib/x86/trap.h | 1 + > user/parlib/thread0_sched.c | 8 +- > user/parlib/uthread.c | 30 +- > user/parlib/x86/debug.c | 95 +++- > 10 files changed, 620 insertions(+), 112 deletions(-) > > diff --git a/kern/arch/x86/trap.c b/kern/arch/x86/trap.c > index 53615af930d2..0f921cf1ecf3 100644 > --- a/kern/arch/x86/trap.c > +++ b/kern/arch/x86/trap.c > @@ -543,6 +543,8 @@ void handle_nmi(struct hw_trapframe *hw_tf) > static void trap_dispatch(struct hw_trapframe *hw_tf) > { > struct per_cpu_info *pcpui; > + struct preempt_data *vcpd; > + struct proc *p; > bool handled = FALSE; > unsigned long aux = 0; > uintptr_t fixup_ip; > @@ -551,10 +553,20 @@ static void trap_dispatch(struct hw_trapframe *hw_tf) > switch(hw_tf->tf_trapno) { > case T_DEBUG: > case T_BRKPT: > - enable_irq(); > - monitor(hw_tf); > - disable_irq(); > - handled = TRUE; > + pcpui = &per_cpu_info[core_id()]; > + p = pcpui->cur_proc; > + vcpd = > &p->procdata->vcore_preempt_data[pcpui->owning_vcoreid]; > + > + if (in_kernel(hw_tf) || !proc_is_vcctx_ready(p) || > + vcpd->notif_disabled) { > + /* Trap is in kernel, vcore, or early SCP > context. */ > + enable_irq(); > + monitor(hw_tf); > + disable_irq(); > + handled = TRUE; > + } else { > + handled = FALSE; > + } This still breaks the various usages of breakpoint(), such as in parlib's panic(). But we can fix it later. Specifically, we can use another trap for the old "panic debugging". That way, T_DEBUG and T_BRKPT can just return 'handled' for vc ctx code (which is what we want) and not drop into the monitor. > break; > case T_ILLOP: > { > diff --git > a/tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigaction.c > b/tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigaction.c > index 4096eeb48fb7..bf6760c3a5ae 100644 > --- a/tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigaction.c > +++ b/tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/sigaction.c > @@ -40,7 +40,8 @@ static void default_term_handler(int signr, siginfo_t > *info, void *ctx) > > static void default_core_handler(int signr, siginfo_t *info, void *ctx) > { > - akaros_printf("Segmentation Fault (sorry, no core dump yet)\n"); > + akaros_printf("Segmentation Fault on PID %d (sorry, no core dump > yet)\n", > + __procinfo.pid); > if (ctx) > print_user_context((struct user_context*)ctx); > else > diff --git a/user/parlib/debug.c b/user/parlib/debug.c > index 9f967f09f109..3395524e7e96 100644 > --- a/user/parlib/debug.c > +++ b/user/parlib/debug.c > @@ -6,6 +6,7 @@ > #include <parlib/event.h> > #include <parlib/parlib.h> > #include <parlib/spinlock.h> > +#include <parlib/vcore.h> > #include <ros/common.h> > #include <stdio.h> > #include <stdlib.h> > @@ -92,14 +93,19 @@ static int debug_send_packet(int fd, void *data, size_t > size); > static int debug_send_error(uint32_t errnum); > static int debug_send_response(struct d9_header *packet); > static void *d9_read_packet(struct d9_header *hdr); > -static int d9_treadmem(struct d9_header *hdr, void *data); > -static int d9_tstoremem(struct d9_header *hdr, void *data); > -static int d9_tfetchreg(struct d9_header *hdr, void *data); > +static int d9s_treadmem(struct d9_header *hdr, void *data); > +static int d9s_tstoremem(struct d9_header *hdr, void *data); > +static int d9s_tfetchreg(struct d9_header *hdr, void *data); > +static int d9s_tstorereg(struct d9_header *hdr, void *data); > +static int d9s_tresume(struct d9_header *hdr, void *data); > + > +static int d9c_thitbreakpoint(struct d9_header *hdr, void *data); > +static int d9c_taddthread(struct d9_header *hdr, void *data); > > /* ev_q for read syscall on debug pipe. */ > static struct event_queue *debug_read_ev_q; > static struct syscall debug_read_sysc; > -static int debug_fd; > +static int debug_fd = -1; > > /* async_read_buf is the buffer used to read the header of a packet. > * > @@ -116,7 +122,8 @@ static struct d9_ops *d9_ops; > * gdbserver. */ > typedef int (*message_handler)(struct d9_header *hdr, void *data); > #define D9_HANDLER(x) (x - D9_TREADMEM) > -static message_handler msg_handlers[D9_HANDLER(D9_NUM_MESSAGES)]; > +static message_handler srv_msg_handlers[D9_HANDLER(D9_NUM_MESSAGES)]; > +static message_handler clt_msg_handlers[D9_HANDLER(D9_NUM_MESSAGES)]; > > /* queue_handle_one_message extracts one message from the mbox and calls the > * appropriate handler for that message. */ > @@ -144,7 +151,7 @@ void queue_read_handler(struct event_queue *ev_q) > ; > } > > -void d9_server_init(struct schedule_ops *ops, struct d9_ops *dops) > +void d9s_init(struct d9_ops *dops) > { > int fd; > int p[2]; > @@ -158,9 +165,11 @@ void d9_server_init(struct schedule_ops *ops, struct > d9_ops *dops) > debug_read_ev_q->ev_handler = queue_read_handler; > > /* Set up d9 handlers. */ > - msg_handlers[D9_HANDLER(D9_TREADMEM)] = d9_treadmem; > - msg_handlers[D9_HANDLER(D9_TSTOREMEM)] = d9_tstoremem; > - msg_handlers[D9_HANDLER(D9_TFETCHREG)] = d9_tfetchreg; > + srv_msg_handlers[D9_HANDLER(D9_TREADMEM)] = d9s_treadmem; > + srv_msg_handlers[D9_HANDLER(D9_TSTOREMEM)] = d9s_tstoremem; > + srv_msg_handlers[D9_HANDLER(D9_TFETCHREG)] = d9s_tfetchreg; > + srv_msg_handlers[D9_HANDLER(D9_TSTOREREG)] = d9s_tstorereg; > + srv_msg_handlers[D9_HANDLER(D9_TRESUME)] = d9s_tresume; > > /* Set d9 ops. */ > d9_ops = dops; > @@ -185,8 +194,6 @@ void d9_server_init(struct schedule_ops *ops, struct > d9_ops *dops) > > debug_fd = p[0]; > debug_read(debug_fd, async_read_buf, sizeof(struct d9_header)); > - > - asm("int $3"); > } > > void debug_read(int fd, void *buf, size_t len) > @@ -223,15 +230,32 @@ void handle_debug_msg(struct syscall *sysc) > > memcpy(hdr, (void *) sysc->arg1, sizeof(struct d9_header)); > > - void *data = d9_read_packet(hdr); > + /* Read the remaining bytes of the message. */ > + size_t msg_size = hdr->size - sizeof(struct d9_header); > + void *data = NULL; > + > + if (msg_size > 0) { > + /* Read the remaining data of this packet. */ > + data = calloc(msg_size, 1); > + if (data == NULL) { > + perror("calloc"); > + return; > + } > + > + if (read(debug_fd, data, msg_size) != msg_size) { > + perror("reading remaining message"); > + free(data); > + return; > + } > + } > > /* It is now safe to set off another async read syscall, > reusing the > * buffer. */ > debug_read(debug_fd, async_read_buf, sizeof(struct d9_header)); > > /* Call the appropriate handler for this packet. */ > - if (msg_handlers[D9_HANDLER(hdr->msg_type)] != NULL) > - msg_handlers[D9_HANDLER(hdr->msg_type)](hdr, data); > + if (srv_msg_handlers[D9_HANDLER(hdr->msg_type)] != NULL) > + srv_msg_handlers[D9_HANDLER(hdr->msg_type)](hdr, data); > else > panic("2LS debug: no message handler found."); > > @@ -259,36 +283,119 @@ struct d9_header *alloc_packet(size_t pck_len, enum > d9_msg_t msg_type) > return rhdr; > } > > -/* d9_read_memory is the d9_ops func called to fulfill a TREADMEM request. */ > -int d9_read_memory(const struct d9_treadmem *req, struct d9_rreadmem *resp) > +/* d9s_read_memory is the d9_ops func called to fulfill a TREADMEM request. > */ > +int d9s_read_memory(const struct d9_treadmem *req, struct d9_rreadmem *resp) > { > + resp->length = req->length; > + /* TODO(chrisko): can check page tables to see whether this should > actually > + * succeed instead of letting it fault. */ > memcpy(resp->data, (void *) req->address, req->length); > return 0; > } > > -/* d9_store_memory is the d9_ops func called to fulfill a TSTOREMEM request. > */ > -int d9_store_memory(const struct d9_tstoremem *req, uint32_t length) > +/* d9s_store_memory is the d9_ops func called to fulfill a TSTOREMEM > request. */ > +int d9s_store_memory(const struct d9_tstoremem *req) > { > - memcpy((void *) req->address, req->data, length); > + /* TODO(chrisko): can check page tables to see whether this should > actually > + * succeed instead of letting it fault. */ > + memcpy((void *) req->address, req->data, req->length); > return 0; > } > > -/* d9_tstoremem allocates the response packet, calls the user-supplied ops > +/* d9s_resume is the d9_ops func called to fulfill a TRESUME request. */ > +void d9s_resume(void) > +{ > + uthread_apply_all(uthread_runnable); > +} > + > +int d9s_notify_hit_breakpoint(uint64_t tid, uint64_t address) > +{ > + struct d9_thitbreakpoint *thb; > + struct d9_header *rpack; > + > + if (debug_fd == -1) > + return 0; > + > + rpack = alloc_packet(sizeof(struct d9_thitbreakpoint), > D9_THITBREAKPOINT); > + if (rpack == NULL) > + return -1; > + > + thb = (struct d9_thitbreakpoint *) (rpack + 1); > + thb->pid = __procinfo.pid; You can just use getpid(). > + thb->tid = tid; > + thb->address = address; > + > + return debug_send_response(rpack); > +} > + > +int d9s_notify_add_thread(uint64_t tid) > +{ > + struct d9_taddthread *tat; > + struct d9_header *rpack; > + > + if (debug_fd == -1) > + return 0; > + > + rpack = alloc_packet(sizeof(struct d9_taddthread), D9_TADDTHREAD); > + if (rpack == NULL) > + return -1; > + > + tat = (struct d9_taddthread *) (rpack + 1); > + tat->pid = __procinfo.pid; You can just use getpid(). > + tat->tid = tid; > + > + return debug_send_response(rpack); > +} > + > +/* d9s_tresume resumes execution in all threads. > + * > + * This looks a bit different than all the other routines: The actual op is > done > + * after sending a successful response. The response basically serves to let > the > + * client know that the message was received, but not that the work was done. > + * > + * There's two scenarios for resume we care about at the moment: > + * 1) We resume and run until the program hits another breakpoint. > + * 2) We resume and run until the program exits. > + * > + * In the second case, the program could exit before the 2LS has a chance to > + * send the successful response. So we send the response first and assume > that > + * resume cannot fail. (If it does, it wouldn't fail in a way we can > currently > + * detect anyway.) > + */ > +int d9s_tresume(struct d9_header *hdr, void *data) > +{ > + int ret; > + struct d9_header *rpack; > + > + if (d9_ops == NULL || d9_ops->resume == NULL) > + return debug_send_error(EBADF /* TODO: better error code */); > + > + /* Successful response. */ > + rpack = alloc_packet(0, D9_RRESUME); > + if (rpack == NULL) > + return debug_send_error(ENOMEM); > + > + ret = debug_send_response(rpack); > + free(rpack); > + > + /* Call user-supplied routine. */ > + d9_ops->resume(); > + return ret; > +} > + > +/* d9s_tstoremem allocates the response packet, calls the user-supplied ops > * function for storing memory, and sends the response packet. */ > -int d9_tstoremem(struct d9_header *hdr, void *data) > +int d9s_tstoremem(struct d9_header *hdr, void *data) > { > - int length, ret; > + int ret; > struct d9_header *resp; > struct d9_tstoremem *tstoremem = (struct d9_tstoremem *) data; > > if (d9_ops == NULL || d9_ops->store_memory == NULL) > return debug_send_error(EBADF /* TODO: better error code */); > > - length = hdr->size - sizeof(struct d9_header) - 1 - > - sizeof(struct d9_tstoremem); > - > /* Call user-supplied routine for filling in response packet. */ > - ret = d9_ops->store_memory(tstoremem, length); > + ret = d9_ops->store_memory(tstoremem); > > if (ret < 0) { > /* Error with errno -ret. */ > @@ -305,9 +412,9 @@ int d9_tstoremem(struct d9_header *hdr, void *data) > return ret; > } > > -/* d9_treadmem allocates the response packet, calls the user-supplied ops > +/* d9s_treadmem allocates the response packet, calls the user-supplied ops > * function for reading from memory, and sends the response packet. */ > -int d9_treadmem(struct d9_header *hdr, void *data) > +int d9s_treadmem(struct d9_header *hdr, void *data) > { > int ret; > struct d9_header *rpack; > @@ -318,7 +425,8 @@ int d9_treadmem(struct d9_header *hdr, void *data) > return debug_send_error(EBADF /* TODO */); > > /* Allocate response packet. */ > - rpack = alloc_packet(treadmem->length, D9_RREADMEM); > + rpack = alloc_packet( > + sizeof(struct d9_rreadmem) + treadmem->length, D9_RREADMEM); > if (rpack == NULL) > return debug_send_error(ENOMEM); > > @@ -339,10 +447,51 @@ int d9_treadmem(struct d9_header *hdr, void *data) > return ret; > } > > -/* d9_tfetchreg allocates the response packet, finds the appropriate thread > +/* d9s_tstorereg allocates the response packet, finds the appropriate thread > + * structure, calls the user-supplied ops function for storing its registers, > + * and sends the response packet. */ > +int d9s_tstorereg(struct d9_header *hdr, void *data) > +{ > + int ret; > + struct uthread *t; > + struct d9_header *rpack; > + struct d9_rstorereg *resp; > + struct d9_tstorereg *req = (struct d9_tstorereg *) data; > + > + if (d9_ops == NULL || d9_ops->store_registers == NULL) > + return debug_send_error(EBADF /* TODO */); > + > + /* Find the appropriate thread. */ > + t = uthread_get_thread_by_id(req->threadid); > + if (t == NULL) > + return debug_send_error(EBADF /* TODO */); > + > + /* Allocate response packet. */ > + rpack = alloc_packet(sizeof(struct d9_rstorereg), D9_RSTOREREG); > + if (rpack == NULL) > + return debug_send_error(ENOMEM); > + > + resp = (struct d9_rstorereg *) (rpack + 1); > + > + /* Call user-supplied routine for filling in response packet. */ > + ret = d9_ops->store_registers(t, &(req->regs)); > + > + if (ret < 0) { > + /* Error with errno -ret. */ > + free(rpack); > + return debug_send_error(-ret); > + } > + > + /* Successful response. */ > + ret = debug_send_response(rpack); > + free(rpack); > + return ret; > +} > + > +/* d9s_tfetchreg allocates the response packet, finds the appropriate thread > * structure, calls the user-supplied ops function for reading its registers, > * and sends the response packet. */ > -int d9_tfetchreg(struct d9_header *hdr, void *data) > +int d9s_tfetchreg(struct d9_header *hdr, void *data) > { > int ret; > struct uthread *t; > @@ -407,30 +556,8 @@ int debug_send_response(struct d9_header *packet) > return debug_send_packet(debug_fd, (void *) packet, packet->size); > } > > -/* d9_read_packet reads the rest of the packet given the packet header. > - * > - * The header contains the size of the packet. */ > -void *d9_read_packet(struct d9_header *hdr) > -{ > - size_t msg_size = hdr->size - sizeof(struct d9_header); > - void *data = NULL; > - int ret; > - > - if (msg_size > 0) { > - /* Read the remaining data of this packet. */ > - data = calloc(msg_size, 1); > - if (data == NULL) > - return NULL; > - > - if (read(debug_fd, data, msg_size) != msg_size) { > - free(data); > - return NULL; > - } > - } > - > - return data; > -} > - > +/* TODO(chrisko): Is this necessary? On Linux, writes on pipes up to PIPE_BUF > + * (4096) are guaranteed to be atomic. */ I wouldn't rely on it, especially since we haven't limited the size of readmem (yet?). Also, we might be using something other than a pipe in the future. > int debug_send_packet(int fd, void *data, size_t size) > { > ssize_t wlen, len_written = 0; > @@ -467,7 +594,98 @@ int check_error_packet(struct d9_header *hdr, enum > d9_msg_t msg_type) > return 1; > } > > -int debug_read_memory(int fd, uintptr_t address, uint32_t length, uint8_t > *buf) > + > +/* Globals to deal with incoming messages on gdbserver side. > + * > + * TODO(chrisko): Instead of making these global, make a struct to pass to > + * read_thread and the d9c_ functions. > + */ > +static int new_message; > +static uth_mutex_t sync_lock; > +static uth_cond_var_t sync_cv; > +static struct d9_header *response_header; > + > +void *d9c_read_thread(void *arg) > +{ > + struct d9_header *hdr; > + char buf[4096] = {0}; Careful - that's a large stack allocation. Should be okay for pthreads, assuming this is run from outside vcore context. > + int fd = *((int *) arg); > + ssize_t rlen = 0; > + uint32_t bufindex = 0; > + > + while (1) { > + /* TODO(chrisko): current max message size is 4096 with this. > + * Investigate what the largest memory chunk gdb will ask for > is. */ > + rlen = read(fd, buf+bufindex, 4096); Checkpatch often misses this, but you should have spaces around the + operator. > + if (rlen <= 0) { > + perror("d9 read"); > + return NULL; > + } > + > + hdr = (struct d9_header *) buf; > + if (hdr->size < rlen) { > + bufindex += rlen; > + continue; > + } Looks like this loop assumes that you had no partial reads. If you got just one byte back, you'd be looking at an old hdr struct. > + > + if (hdr->msg_type % 2 == 1) { > + /* If this is a response message type, the main > gdbserver thread is > + * blocked on this response. */ I'd use a helper for this, instead of doing mod 2. It looks like a big invariant here is that we only ever receive one response at a time and only when we expect it. (More on this later). > + memcpy(response_header, hdr, hdr->size); > + uth_mutex_lock(sync_lock); > + new_message = 1; > + uth_mutex_unlock(sync_lock); > + uth_cond_var_signal(sync_cv); > + } else if (clt_msg_handlers[D9_HANDLER(hdr->msg_type)]) { > + /* This is a message that isn't a response to a request > (e.g. a > + * thread was added or we hit a breakpoint). */ > + clt_msg_handlers[D9_HANDLER(hdr->msg_type)](hdr, hdr+1); It looks like these handlers can run async with respect to the main gdb server thread. Hopefully that's OK. > + } else { > + panic("2LS received invalid message type (type = %d, > size = %d)\n", > + hdr->msg_type, hdr->size); > + } > + > + /* Move the next message if it was already read. > + * buf = buf[hdr->size:4096] */ > + if (rlen != hdr->size) { > + for (uint32_t i = 0; i + hdr->size < 4096; i++) > + buf[i] = buf[i + hdr->size]; > + } > + bufindex = 0; > + } > +} > + > +static struct d9c_ops *client_ops; Something to think about for future commits: should we have these client ops in parlib or in gdbserver itself? > + > +/* d9c_thitbreakpoint is called when the 2LS sends a THITBREAKPOINT msg. */ > +int d9c_thitbreakpoint(struct d9_header *hdr, void *data) > +{ > + struct d9_thitbreakpoint *thb = (struct d9_thitbreakpoint *) data; > + > + return client_ops->hit_breakpoint(thb->pid, thb->tid, thb->address); > +} > + > +/* d9c_taddthread is called when the 2LS sends a TADDTHREAD msg. */ > +int d9c_taddthread(struct d9_header *hdr, void *data) > +{ > + struct d9_taddthread *tat = (struct d9_taddthread *) data; > + > + return client_ops->add_thread(tat->pid, tat->tid); > +} > + > +void d9c_init(struct d9c_ops *ops) > +{ > + sync_lock = uth_mutex_alloc(); > + sync_cv = uth_cond_var_alloc(); > + new_message = 0; > + client_ops = ops; > + clt_msg_handlers[D9_HANDLER(D9_THITBREAKPOINT)] = &d9c_thitbreakpoint; > + clt_msg_handlers[D9_HANDLER(D9_TADDTHREAD)] = &d9c_taddthread; Same as the server side - do we need these function pointers to be dynamically set up? > +} > + > +/* d9c_read_memory communicates with the 2LS to read from an address in > memory. > +*/ > +int d9c_read_memory(int fd, uintptr_t address, uint32_t length, uint8_t *buf) > { > size_t return_packet_size; > struct d9_header *hdr, *rhdr; > @@ -484,33 +702,84 @@ int debug_read_memory(int fd, uintptr_t address, > uint32_t length, uint8_t *buf) > > int ret = debug_send_packet(fd, hdr, hdr->size); > > - /* Block on return packet. */ > - > - /* TODO(chrisko): need to do this asynchronously */ > - return_packet_size = sizeof(struct d9_header) + length; > + /* Provide space for the message to be stored in. */ > + return_packet_size = sizeof(struct d9_header) + > + MAX(sizeof(struct d9_rreadmem) + length, sizeof(struct > d9_rerror)); > char pkt_buf[return_packet_size]; > > - if (read(fd, pkt_buf, return_packet_size) <= 0) { > + /* Set globals */ > + response_header = (struct d9_header *) pkt_buf; > + > + /* Wait for response message. */ > + uth_mutex_lock(sync_lock); > + while (new_message != 1) > + uth_cond_var_wait(sync_cv, sync_lock); Does this need to be async? Given that you only ever have one response coming in for a given request, you could just block on reading the FD. Yes, you could get either the response or a server-initiated message (like breakpoint). But in the latter case, you can just handle that message and then re-block waiting for the response. This could be done in a helper that all of these d9c ops call. There are two benefits to the synchronous approach. The first is simplicity. The second is that you never have the main gdb server running concurrently with the callbacks. For instance, what happens if a 'new thread' callback fires while gdb is doing other work? I imagine that's not supposed to happen in the current scheme, but it'd be nice for it to never be able to cause a problem. Anyway, it's probably OK to leave it async. Though perhaps it could use a helper function. All of these d9c client ops do almost the same thing, and whether it is blocking on an FD or a {CV + bool} can be hidden. Here's a potential issue due to the async model. What happens if d9c_read_thread runs concurrently with this function? Say we call debug_send_packet(), then the target wakes up and responds. That wakes our d9c_read_thread(), and all of that happens before we set response_header = pkt_buf. Then the read_thread writes into the wrong response_header, and we get garbage back. You can fix that problem and keep it async by grabbing the sync_lock before calling debug_send_packet(). > + > + /* Got response. */ > + if (check_error_packet(response_header, D9_RREADMEM)) { > perror("d9 read memory"); > - return -1; > + ret = -1; > + } else { > + resp = (struct d9_rreadmem *) (response_header + 1); > + memcpy(buf, resp->data, resp->length); > + ret = 0; > } > > - rhdr = (struct d9_header *) pkt_buf; > - if (check_error_packet(rhdr, D9_RREADMEM)) { > - perror("d9 read memory"); > + new_message = 0; > + uth_mutex_unlock(sync_lock); > + return ret; > +} > + > +/* d9c_store_registers communicates with the 2LS to change register values. > */ > +int d9c_store_registers(int fd, uint64_t tid, struct d9_regs *regs) > +{ > + size_t return_packet_size; > + struct d9_header *hdr; > + struct d9_tstorereg *req; > + > + hdr = alloc_packet(sizeof(struct d9_tstorereg), D9_TSTOREREG); > + if (hdr == NULL) > return -1; > + > + req = (struct d9_tstorereg *) (hdr + 1); > + req->threadid = tid; > + memcpy(&(req->regs), regs, sizeof(struct d9_regs)); > + > + int ret = debug_send_packet(fd, hdr, hdr->size); > + > + /* Block on getting a response packet. */ > + return_packet_size = sizeof(struct d9_header) + > + MAX(sizeof(struct d9_rstorereg), sizeof(struct d9_rerror)); > + char pkt_buf[return_packet_size]; > + > + /* Set globals. */ > + response_header = (struct d9_header *) pkt_buf; > + > + /* Block on return packet. */ > + uth_mutex_lock(sync_lock); > + while (new_message != 1) > + uth_cond_var_wait(sync_cv, sync_lock); > + > + /* Got response. */ > + if (check_error_packet(response_header, D9_RSTOREREG)) { > + perror("d9 store registers"); > + ret = -1; > + } else { > + ret = 0; > } > > - resp = (struct d9_rreadmem *) (rhdr + 1); > - memcpy(buf, resp->data, length); > - return 0; > + new_message = 0; > + uth_mutex_unlock(sync_lock); > + return ret; > } > > -int debug_fetch_registers(int fd, uint64_t tid, struct d9_regs *regs) > +/* d9c_fetch_registers communicates with the 2LS to read register values. */ > +int d9c_fetch_registers(int fd, uint64_t tid, struct d9_regs *regs) > { > size_t return_packet_size; > struct d9_header *hdr, *rhdr; > struct d9_tfetchreg *req; > + struct d9_rfetchreg *resp; > > hdr = alloc_packet(sizeof(struct d9_tfetchreg), D9_TFETCHREG); > if (hdr == NULL) > @@ -521,23 +790,70 @@ int debug_fetch_registers(int fd, uint64_t tid, struct > d9_regs *regs) > > int ret = debug_send_packet(fd, hdr, hdr->size); > > - /* Block on return packet. */ > - > - /* TODO(chrisko): need to do this asynchronously */ > - return_packet_size = sizeof(struct d9_header) + sizeof(struct > d9_rfetchreg); > + /* Block on getting a response packet. */ > + return_packet_size = sizeof(struct d9_header) + > + MAX(sizeof(struct d9_rfetchreg), sizeof(struct d9_rerror)); > char pkt_buf[return_packet_size]; > > - if (read(fd, pkt_buf, return_packet_size) != return_packet_size) { > + /* Set globals. */ > + response_header = (struct d9_header *) pkt_buf; > + > + /* Block on return packet. */ > + uth_mutex_lock(sync_lock); > + while (new_message != 1) > + uth_cond_var_wait(sync_cv, sync_lock); > + > + /* Got response. */ > + if (check_error_packet(response_header, D9_RFETCHREG)) { > perror("d9 fetch registers"); > - return -1; > + ret = -1; > + } else { > + resp = (struct d9_rfetchreg *) (response_header + 1); > + memcpy(regs, &(resp->regs), sizeof(struct d9_regs)); > + ret = 0; > } > > - rhdr = (struct d9_header *) pkt_buf; > - if (check_error_packet(rhdr, D9_RFETCHREG)) { > - perror("d9 fetch registers"); > + new_message = 0; > + uth_mutex_unlock(sync_lock); > + return ret; > +} > + > +/* d9c_resume tells the 2LS to resume all threads. > + * > + * TODO(chrisko): resume w/ hardware-step enabled. */ > +int d9c_resume(int fd) > +{ > + size_t return_packet_size; > + struct d9_header *hdr; > + > + hdr = alloc_packet(sizeof(struct d9_tresume), D9_TRESUME); > + if (hdr == NULL) > return -1; > + > + int ret = debug_send_packet(fd, hdr, hdr->size); > + > + /* Block on getting a response packet. */ > + return_packet_size = sizeof(struct d9_header) + > + MAX(sizeof(struct d9_rresume), sizeof(struct d9_rerror)); > + char pkt_buf[return_packet_size]; > + > + /* Set globals. */ > + response_header = (struct d9_header *) pkt_buf; > + > + /* Block on return packet. */ > + uth_mutex_lock(sync_lock); > + while (new_message != 1) > + uth_cond_var_wait(sync_cv, sync_lock); > + > + /* Got response. */ > + if (check_error_packet(response_header, D9_RRESUME)) { > + perror("d9 resume"); > + ret = -1; > + } else { > + ret = 0; > } > > - memcpy(regs, rhdr + 1, sizeof(struct d9_regs)); > - return 0; > + new_message = 0; > + uth_mutex_unlock(sync_lock); > + return ret; > } > diff --git a/user/parlib/include/parlib/debug.h > b/user/parlib/include/parlib/debug.h > index 68b462dca58f..c4e6a0eb8629 100644 > --- a/user/parlib/include/parlib/debug.h > +++ b/user/parlib/include/parlib/debug.h > @@ -3,6 +3,10 @@ > #include <parlib/arch/debug.h> > #include <parlib/uthread.h> > > +/* Message types of D9. > + * T messages have to be even, R messages have to be odd. T messages are sent > + * from gdbserver to 2LS, R messages the other way. > + */ > enum d9_msg_t { > D9_TREADMEM = 10, > D9_RREADMEM, > @@ -10,8 +14,16 @@ enum d9_msg_t { > D9_RSTOREMEM, > D9_TFETCHREG, > D9_RFETCHREG, > + D9_TSTOREREG, > + D9_RSTOREREG, > D9_TERROR, /* Do not use. */ > D9_RERROR, > + D9_THITBREAKPOINT, > + D9_RHITBREAKPOINT, > + D9_TRESUME, > + D9_RRESUME, > + D9_TADDTHREAD, > + D9_RADDTHREAD, > D9_NUM_MESSAGES, /* Do not use. */ > }; > > @@ -20,6 +32,7 @@ struct d9_header { > uint32_t msg_type; > }; > > +/* Messages of D9. */ > struct d9_rerror { > uint32_t errnum; > }; > @@ -30,16 +43,17 @@ struct d9_treadmem { > }; > > struct d9_rreadmem { > - uint8_t data[1]; /* Must be the last member. */ > + uint32_t length; > + uint8_t data[]; /* Variable length; must be the last member. */ > }; > > struct d9_tstoremem { > uintptr_t address; > - uint8_t data[1]; /* Must be the last member. */ > + uint32_t length; > + uint8_t data[]; /* Variable length; must be the last member. */ > }; > > -struct d9_rstoremem { > -}; > +struct d9_rstoremem {}; > > struct d9_tfetchreg { > uint64_t threadid; > @@ -49,19 +63,74 @@ struct d9_rfetchreg { > struct d9_regs regs; /* Architecture-dependent. */ > }; > > -/* Server-side ops. */ > +struct d9_tstorereg { > + uint64_t threadid; > + struct d9_regs regs; > +}; > + > +struct d9_rstorereg {}; > + > +struct d9_tresume {}; > + > +struct d9_rresume {}; > + > +struct d9_thitbreakpoint { > + pid_t pid; > + uint64_t tid; > + uint64_t address; > +}; > + > +struct d9_taddthread { > + pid_t pid; > + uint64_t tid; > +}; > + > +/* 2LS ops. > + * > + * These represent the actual operations to be carried out for each message > + * type sent to the 2LS. This serves to keep the actual operations separate > from > + * the implementation of the protocol itself. > + */ > struct d9_ops { > int (*read_memory)(const struct d9_treadmem *req, struct d9_rreadmem > *resp); > - int (*store_memory)(const struct d9_tstoremem *req, uint32_t length); > + int (*store_memory)(const struct d9_tstoremem *req); > int (*fetch_registers)(struct uthread *t, struct d9_regs *resp); > + int (*store_registers)(struct uthread *t, struct d9_regs *resp); > + void (*resume)(void); > }; > > -/* Server-side functions. */ > -void d9_server_init(struct schedule_ops *ops, struct d9_ops *debug_ops); > -int d9_read_memory(const struct d9_treadmem *req, struct d9_rreadmem *resp); > -int d9_store_memory(const struct d9_tstoremem *req, uint32_t length); > +/* gdbserver ops. > + * > + * These represent the actual operations to be carried out for each message > type > + * sent to gdbserver. > + */ > +struct d9c_ops { > + /* hit_breakpoint is called when the process hits a breakpoint. */ > + int (*hit_breakpoint)(pid_t pid, uint64_t tid, uint64_t address); > + > + /* add_thread is called when the process adds a new thread. */ > + int (*add_thread)(pid_t pid, uint64_t tid); > +}; > + > +/* 2LS-side functions. */ > +void d9s_init(struct d9_ops *debug_ops); > + > +/* Implementations of d9_ops. */ > +int d9s_read_memory(const struct d9_treadmem *req, struct d9_rreadmem *resp); > +int d9s_store_memory(const struct d9_tstoremem *req); > +void d9s_resume(void); > + > +/* Helpers to send messages from 2LS to gdbserver. */ > +int d9s_notify_hit_breakpoint(uint64_t tid, uint64_t address); > +int d9s_notify_add_thread(uint64_t tid); > + > +/* gdbserver-side functions. */ > +void d9c_init(struct d9c_ops *ops); > +void *d9c_read_thread(void *arg); > > -/* Client-side ops. */ > -int debug_read_memory(int fd, uintptr_t address, uint32_t length, uint8_t > *buf); > -int debug_store_memory(int fd, uintptr_t address, void *data, uint32_t > length); > -int debug_fetch_registers(int fd, uint64_t tid, struct d9_regs *regs); > +/* Helpers to send messages from gdbserver to 2LS. */ > +int d9c_read_memory(int fd, uintptr_t address, uint32_t length, uint8_t > *buf); > +int d9c_store_memory(int fd, uintptr_t address, void *data, uint32_t length); > +int d9c_fetch_registers(int fd, uint64_t tid, struct d9_regs *regs); > +int d9c_store_registers(int fd, uint64_t tid, struct d9_regs *regs); > +int d9c_resume(int fd); > diff --git a/user/parlib/include/parlib/uthread.h > b/user/parlib/include/parlib/uthread.h > index f8592b2a1cba..72fd5eea6244 100644 > --- a/user/parlib/include/parlib/uthread.h > +++ b/user/parlib/include/parlib/uthread.h > @@ -108,6 +108,7 @@ void uthread_paused(struct uthread *uthread); > /* Look up and return uthreads. */ > struct uthread *uthread_get_thread_by_id(uint64_t id); > void uthread_put_thread(struct uthread *uth); > +void uthread_apply_all(void (*fn)(struct uthread *)); This uthread helper can be its own commit. > > /* Utility functions */ > bool __check_preempt_pending(uint32_t vcoreid); /* careful: check the > code */ > diff --git a/user/parlib/include/parlib/x86/debug.h > b/user/parlib/include/parlib/x86/debug.h > index 2fe452744abf..2c75a4f2093c 100644 > --- a/user/parlib/include/parlib/x86/debug.h > +++ b/user/parlib/include/parlib/x86/debug.h > @@ -30,3 +30,4 @@ struct d9_regs { > }; > > int d9_fetch_registers(struct uthread *t, struct d9_regs *resp); > +int d9_store_registers(struct uthread *t, struct d9_regs *resp); > diff --git a/user/parlib/include/parlib/x86/trap.h > b/user/parlib/include/parlib/x86/trap.h > index 541fd04103c7..3c9e907032db 100644 > --- a/user/parlib/include/parlib/x86/trap.h > +++ b/user/parlib/include/parlib/x86/trap.h > @@ -12,6 +12,7 @@ > __BEGIN_DECLS > > #define HW_TRAP_DIV_ZERO 0 > +#define HW_TRAP_BRKPT 3 > #define HW_TRAP_GP_FAULT 13 > #define HW_TRAP_PAGE_FAULT 14 > > diff --git a/user/parlib/thread0_sched.c b/user/parlib/thread0_sched.c > index 45a47a5d1f31..ef993392c2b9 100644 > --- a/user/parlib/thread0_sched.c > +++ b/user/parlib/thread0_sched.c > @@ -1,4 +1,4 @@ > -/* Copyright (c) 2015 Google, Inc. > +/* Copyright (c) 2015-2016 Google, Inc. > * Barret Rhoden <[email protected]> > * See LICENSE for details. > * > @@ -14,6 +14,7 @@ > #include <parlib/uthread.h> > #include <parlib/event.h> > #include <parlib/arch/trap.h> > +#include <parlib/debug.h> > #include <stdlib.h> > > static void thread0_sched_entry(void); > @@ -131,6 +132,11 @@ static void thread0_thread_refl_fault(struct uthread > *uth, > if (!handle_page_fault(uth, err, aux)) > refl_error(uth, trap_nr, err, aux); > break; > + case HW_TRAP_BRKPT: > + /* We only have one thread, no need to stop other threads. */ > + uthread_has_blocked(uth, 0); > + d9s_notify_hit_breakpoint(uth->id, 0); > + break; > default: > refl_error(uth, trap_nr, err, aux); > } > diff --git a/user/parlib/uthread.c b/user/parlib/uthread.c > index 4b53d0f2b141..ef92273968fd 100644 > --- a/user/parlib/uthread.c > +++ b/user/parlib/uthread.c > @@ -1,7 +1,7 @@ > /* Copyright (c) 2011-2014 The Regents of the University of California > + * Copyright (c) 2016 Google Inc. > * Barret Rhoden <[email protected]> > * See LICENSE for details. */ > - > #include <parlib/arch/atomic.h> > #include <parlib/arch/trap.h> > #include <parlib/assert.h> > @@ -180,12 +180,6 @@ void uthread_2ls_init(struct uthread *uthread, struct > schedule_ops *ops) > cmb(); > __vcore_context = FALSE; > enable_notifs(0); /* will trigger a self_notif if we missed a > notif */ > - > - d9ops.read_memory = &d9_read_memory; > - d9ops.store_memory = &d9_store_memory; > - d9ops.fetch_registers = &d9_fetch_registers; > - /* Make ourselves available for debugging */ > - d9_server_init(ops, &d9ops); > } > > /* Helper: tells the kernel our SCP is capable of going into vcore context on > @@ -245,6 +239,15 @@ void __attribute__((constructor)) uthread_lib_init(void) > /* Init the 2LS, which sets up current_uthread, before thread0 lib */ > uthread_2ls_init(thread0_uth, &thread0_2ls_ops); > thread0_lib_init(); > + > + d9ops.read_memory = &d9s_read_memory; > + d9ops.store_memory = &d9s_store_memory; > + d9ops.fetch_registers = &d9_fetch_registers; > + d9ops.store_registers = &d9_store_registers; > + d9ops.resume = &d9s_resume; > + /* Make ourselves available for debugging */ > + d9s_init(&d9ops); > + I think you still might want to do this at the end of uthread_lib_init, not in the middle. And probably have the d9ops be static. Also, be aware that the initialization/accounting of thread0 happens twice, if you have a 2LS other than thread0. Specifically, uthread_init_thread0 is called from uthread_2ls_init, which is called both from uthread_lib_init() as well as from any 2LS that is linked in, e.g. pthreads. What's happening is that thread0_sched tracks thread0, and then pthreads comes along with its own structure to track thread0. Both of those are probably on your all_threads list. That will cause massive confusion later. (this should be fixed in your first patch of the series, since it's an accounting problem). > scp_vcctx_ready(); > /* Change our blockon from glibc's internal one to the regular one, > which > * uses vcore context and works for SCPs (with or without 2LS) and MCPs. > @@ -329,6 +332,8 @@ void uthread_init(struct uthread *new_thread, struct > uth_thread_attr *attr) > > uthread_assign_id(new_thread); > > + d9s_notify_add_thread(new_thread->id); > + I guess you don't need to notify for thread0, since its existence is implied? Barret > if (attr && attr->want_tls) { > /* Get a TLS. If we already have one, reallocate/refresh it */ > if (new_thread->tls_desc) > @@ -1193,6 +1198,17 @@ static void __uthread_free_tls(struct uthread *uthread) > uthread->tls_desc = NULL; > } > > +void uthread_apply_all(void (*fn)(struct uthread *)) > +{ > + struct uthread *t = NULL; > + > + spin_pdr_lock(&thread_list_lock); > + LIST_FOREACH(t, &all_uthreads, entry) > + fn(t); > + > + spin_pdr_unlock(&thread_list_lock); > +} > + > /* TODO(chrisko): hash table instead of list. */ > struct uthread *uthread_get_thread_by_id(uint64_t id) > { > diff --git a/user/parlib/x86/debug.c b/user/parlib/x86/debug.c > index 0cc976b7f4f1..7dd7b589b04f 100644 > --- a/user/parlib/x86/debug.c > +++ b/user/parlib/x86/debug.c > @@ -1,16 +1,101 @@ > #include <parlib/arch/debug.h> > > +/* TODO(chrisko): add a way to signal that a register isn't supplied for sw > + * contexts; because gdbserver has a notion of not knowing a register's > value. > + */ > + > int d9_fetch_registers(struct uthread *t, struct d9_regs *regs) > { > +#define reg_from_hwtf(fld) \ > + regs->reg_##fld = t->u_ctx.tf.hw_tf.tf_##fld > + > +#define reg_from_swtf(fld) \ > + regs->reg_##fld = t->u_ctx.tf.sw_tf.tf_##fld > + > + switch (t->u_ctx.type) { > + case ROS_HW_CTX: > + reg_from_hwtf(rax); > + reg_from_hwtf(rbx); > + reg_from_hwtf(rcx); > + reg_from_hwtf(rdx); > + reg_from_hwtf(rsi); > + reg_from_hwtf(rdi); > + reg_from_hwtf(rbp); > + reg_from_hwtf(rsp); > + reg_from_hwtf(r8); > + reg_from_hwtf(r9); > + reg_from_hwtf(r10); > + reg_from_hwtf(r11); > + reg_from_hwtf(r12); > + reg_from_hwtf(r13); > + reg_from_hwtf(r14); > + reg_from_hwtf(r15); > + reg_from_hwtf(rip); > + reg_from_hwtf(cs); > + reg_from_hwtf(ss); > + regs->reg_eflags = (uint32_t) t->u_ctx.tf.hw_tf.tf_rflags; > + break; > + case ROS_SW_CTX: > + reg_from_hwtf(rbx); > + reg_from_hwtf(rbp); > + reg_from_hwtf(rsp); > + reg_from_hwtf(rip); > + reg_from_swtf(r12); > + reg_from_swtf(r13); > + reg_from_swtf(r14); > + reg_from_swtf(r15); > + break; > + case ROS_VM_CTX: > + panic("2LS debug: VM context unsupported\n"); > + break; > + } > + > + return 0; > +} > + > +int d9_store_registers(struct uthread *t, struct d9_regs *regs) > +{ > +#define reg_to_hwtf(fld) \ > + t->u_ctx.tf.hw_tf.tf_##fld = regs->reg_##fld > + > +#define reg_to_swtf(fld) \ > + t->u_ctx.tf.sw_tf.tf_##fld = regs->reg_##fld > + > switch (t->u_ctx.type) { > case ROS_HW_CTX: > - regs->reg_rax = t->u_ctx.tf.hw_tf.tf_rax; > - regs->reg_rbx = t->u_ctx.tf.hw_tf.tf_rbx; > - regs->reg_rip = t->u_ctx.tf.hw_tf.tf_rip; > + reg_to_hwtf(rax); > + reg_to_hwtf(rbx); > + reg_to_hwtf(rcx); > + reg_to_hwtf(rdx); > + reg_to_hwtf(rsi); > + reg_to_hwtf(rdi); > + reg_to_hwtf(rbp); > + reg_to_hwtf(rsp); > + reg_to_hwtf(r8); > + reg_to_hwtf(r9); > + reg_to_hwtf(r10); > + reg_to_hwtf(r11); > + reg_to_hwtf(r12); > + reg_to_hwtf(r13); > + reg_to_hwtf(r14); > + reg_to_hwtf(r15); > + reg_to_hwtf(rip); > + reg_to_hwtf(cs); > + reg_to_hwtf(ss); > + t->u_ctx.tf.hw_tf.tf_rflags = regs->reg_eflags; > break; > case ROS_SW_CTX: > - regs->reg_rbx = t->u_ctx.tf.sw_tf.tf_rbx; > - regs->reg_rip = t->u_ctx.tf.sw_tf.tf_rip; > + reg_to_hwtf(rbx); > + reg_to_hwtf(rip); > + reg_to_hwtf(rsp); > + reg_to_hwtf(rbp); > + reg_to_hwtf(r12); > + reg_to_hwtf(r13); > + reg_to_hwtf(r14); > + reg_to_hwtf(r15); > + break; > + case ROS_VM_CTX: > + panic("2LS debug: VM context unsupported\n"); > break; > } > > -- > 2.8.0.rc3.226.g39d4020 > -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
