Hi - In commit:
> From 888a38168d015aa9433d034c23f43bb97c4e40a2 Mon Sep 17 00:00:00 2001 > From: Christopher Koch <[email protected]> > Date: Thu, 9 Jun 2016 11:12:19 -0700 > Subject: Defines protocol between gdbserver and a 2LS. > > diff --git a/user/parlib/debug.c b/user/parlib/debug.c > index 46a3e4ed72c1..9996429102d5 100644 > @@ -18,7 +26,7 @@ int akaros_printf(const char *format, ...) > > /* Poor man's Ftrace, won't work well with concurrency. */ > static const char *blacklist[] = { > - "whatever", > + "whatever", The previous formatting was fine. You can indent with a tab here. > static bool is_blacklisted(const char *s) > @@ -53,7 +61,7 @@ void __print_func_entry(const char *func, const char *file) > if (is_blacklisted(func)) > return; > spinlock_lock(&lock); > - printd("Vcore %2d", vcore_id()); /* helps with multicore output > */ > + printd("Vcore %2d", vcore_id()); /* helps with multicore output */ The previous formatting was fine. > +static message_handler srv_msg_handlers[D9_HANDLER(D9_NUM_MESSAGES)] = { > + d9s_treadmem, /* TREADMEM */ > + NULL, /* RREADMEM */ > + d9s_tstoremem, /* TSTOREMEM */ > + NULL, /* RSTOREMEM */ > + d9s_tfetchreg, /* TFETCHREG */ All of these should be tab indented for the initial indent. Spaces or tabs are fine for the trailing comment alignment. > +void d9s_init(struct d9_ops *dops) > +{ > + int fd; > + int p[2]; > + char buf[60]; > + int ret; > + > + /* Set up parlib queue for asynchronous read notifications. */ > + debug_read_ev_q = get_eventq(EV_MBOX_UCQ); > + debug_read_ev_q->ev_flags = > + EVENT_IPI | EVENT_INDIR | EVENT_SPAM_INDIR | EVENT_WAKEUP; > + debug_read_ev_q->ev_handler = queue_read_handler; > + > + /* Set d9 ops. */ > + d9_ops = dops; I'm still not convinced that you need 2LS-specific ops, but I'm OK with it. However, the way d9s_init() interacts with the 2LSs will have issues. The problem is that thread0 initializes (down below in the patch), but presumably every 2LS will need to initialize too, and (like with the uthread list issue) *both* thread0 and the other 2LS will call d9s_init(). Perhaps if you split up d9s_init into two parts: 1) a 2LS-independent, one-time initialization that sets up the pipes/#proc files, etc, and 2) a way to change the 2LS-specific d9ops. Given that most 2LSs will use the default ops, I'd just set d9ops statically to the defaults that everyone will use, then expose the d9 ops struct (or otherwise have an API to set members) so that specific 2LSs can override those ops. As it is now, I think you'll have two FDs open and various things clobbered when you try this with pthreads. Also, if a process becomes debuggable at a certain point, and that point is before all of the constructors run (which means before the 2LSs have initialized), then some settings can change out from under you. > +/* d9s_read_memory is the d9_ops func called to fulfill a TREADMEM request. > */ > +int d9s_read_memory(const struct d9_treadmem_msg *req, > + struct d9_rreadmem_msg *resp) > +{ > + memcpy(resp->data, (void *)req->address, req->length); > + return 0; > +} > + > +/* d9s_store_memory is the d9_ops func called to fulfill a TSTOREMEM > request. */ > +int d9s_store_memory(const struct d9_tstoremem_msg *req) > +{ > + memcpy((void *)req->address, req->data, req->length); > + return 0; > +} Something to think about in the future: will gdb (or other interrogations) ask us for an address that isn't mapped? If so, these ops will fault, and the process will probably die. We can build something to handle this when it comes up. > +/* d9s_tstoremem allocates the response packet, calls the user-supplied ops > + * function for storing memory, and sends the response packet. */ > +static int d9s_tstoremem(struct d9_header *hdr) > +{ > + int ret; > + struct d9_rstoremem resp = > + D9_INIT_HDR(sizeof(struct d9_rstoremem), D9_RSTOREMEM); > + struct d9_tstoremem *req = (struct d9_tstoremem *)hdr; > + > + if (d9_ops == NULL || d9_ops->store_memory == NULL) > + return debug_send_error(EBADF /* TODO: better error code */); If you have a static set of d9s ops (that the 2LS can override), you can avoid these checks. (Here and anywhere d9_ops is used). Also, if the d9_ops == NULL check is also meant to check if we're ready for debugging ops, then we should be explicit about that. (I think it isn't, since we shouldn't be here if we didn't init, right?). > + /* Call user-supplied routine for filling in response packet. */ > + ret = d9_ops->store_memory(&(req->msg)); > + > + if (ret < 0) > + return debug_send_error(-ret); > + > + return debug_send_packet(debug_fd, &(resp.hdr)); > +} It's not incorrect, but you don't need the () around req->msg and resp.hdr. This is done in a few other places in this file. > +static int debug_read_packet(int fd, struct d9_header *hdr) > +{ > + /* Read the header. */ > + if (read_all(fd, hdr, sizeof(struct d9_header))) > + return -1; > + > + /* Read the remaining bytes of the packet. */ > + return read_all(fd, hdr + 1, hdr->size - sizeof(struct d9_header)); > +} This seems a little dangerous. It takes a *hdr, but will write beyond the header. That could be okay. But to use debug_read_packet, the caller needs to have allocated at least hdr->size. hdr->size is actually set by whoever sent the packet. The two callers of this seem to have some logic determining the length. We should pass in the max size_t that this function can write, to avoid any situation where we accidentally write too much. > diff --git a/user/parlib/thread0_sched.c b/user/parlib/thread0_sched.c > index 45a47a5d1f31..14adc425ddb9 100644 > --- a/user/parlib/thread0_sched.c > +++ b/user/parlib/thread0_sched.c > @@ -29,16 +30,22 @@ static void thread0_mtx_unlock(uth_mutex_t m); > > /* externed into uthread.c */ > struct schedule_ops thread0_2ls_ops = { > - .sched_entry = thread0_sched_entry, > - .thread_blockon_sysc = thread0_thread_blockon_sysc, > - .thread_refl_fault = thread0_thread_refl_fault, > - .thread_runnable = thread0_thread_runnable, > - .thread_paused = thread0_thread_runnable, > - .thread_has_blocked = thread0_thread_has_blocked, > - .mutex_alloc = thread0_mtx_alloc, > - .mutex_free = thread0_mtx_free, > - .mutex_lock = thread0_mtx_lock, > - .mutex_unlock = thread0_mtx_unlock, > + .sched_entry = thread0_sched_entry, > + .thread_blockon_sysc = thread0_thread_blockon_sysc, > + .thread_refl_fault = thread0_thread_refl_fault, > + .thread_runnable = thread0_thread_runnable, > + .thread_paused = thread0_thread_runnable, > + .thread_has_blocked = thread0_thread_has_blocked, > + .mutex_alloc = thread0_mtx_alloc, > + .mutex_free = thread0_mtx_free, > + .mutex_lock = thread0_mtx_lock, > + .mutex_unlock = thread0_mtx_unlock, The formatting on these was correct before. > +static struct d9_ops thread0_d9ops = { > + .read_memory = &d9s_read_memory, > + .store_memory = &d9s_store_memory, > + .fetch_registers = &d9_fetch_registers, Tabs for the leading indent here. > @@ -48,7 +55,7 @@ struct uthread *thread0_uth; > * don't actually attach this mgmt info to it. But since we just have one > * thread, it doesn't matter. */ > struct thread0_info { > - bool is_blocked; > + bool is_blocked; The formatting of this was fine before. I tab-align non-function-pointer struct members. It's much nicer to look at. For instance, look at struct fd_table in k/i/vfs.h. That'd be a mess if there was just a space after the type. > @@ -66,6 +73,9 @@ void thread0_lib_init(void) > sysc_evq = get_eventq(EV_MBOX_BITMAP); > sysc_evq->ev_flags = EVENT_INDIR | EVENT_WAKEUP; > register_ev_handler(EV_SYSCALL, thread0_handle_syscall, 0); > + > + /* Make ourselves available for debugging */ > + d9s_init(&thread0_d9ops); This is what I was talking about above. Other 2LSs will need to do this too, and then they'd reinit/clobber some of the state. > @@ -90,7 +100,7 @@ static void thread0_sched_entry(void) > > static void thread0_thread_blockon_sysc(struct uthread *uthread, void *arg) > { > - struct syscall *sysc = (struct syscall*)arg; > + struct syscall *sysc = (struct syscall *)arg; The previous formatting was fine. > diff --git a/user/parlib/x86/debug.c b/user/parlib/x86/debug.c > new file mode 100644 > index 000000000000..f9b2edb6aee4 > --- /dev/null > +++ b/user/parlib/x86/debug.c > @@ -0,0 +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); Shouldn't these be reg_from_swtf? You'd be reading the wrong field... The fact that the compiler didn't catch this shows me that the macro use here is borderline abuse. Maybe just have a static inline helper that gets the sw_tf, and then do something like regs->reg_rip = sw_ctx_of(t)->tf_rip; You already ran into an issue with the naming when you needed to hardcode the assignment for eflags == rflags above. > +int d9_store_registers(struct uthread *t, struct d9_regs *regs) > +{ > + case ROS_SW_CTX: > + 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; Same applies for d9_store_registers. Thanks, Barret -- 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.
