Another question: Whenever we create a thread, we're now calling d9s_notify_add_thread(), which does a syscall, writing to a pipe that no one might be listening on. If I create 10k threads or something, that might eventually block. Plus, we now have a syscall in the thread creation path.
Instead of always emitting, can we emit only when we are actively being debugged? And when GDB initially connects to us, we can give it a list of all existing threads. We do something similar in perf. When perf record starts, we emit a bunch of samples saying which processes exist. When we're tracing, we emit the samples on the fly. When we aren't tracing, we just check a variable that tells us not to emit (roughly). Barret On 2016-08-10 at 19:21 "'Christopher Koch' via Akaros" <[email protected]> wrote: > Thanks for all the comments! It'll be a day or so before I fix > everything up -- I'd actually started changing gdbserver from async > back to sync again like you're also suggesting. Async *was* a hard > requirement when I started, but I've since changed the protocol such > that it's not actually a requirement anymore. > > On Wed, Aug 10, 2016, 12:17 Barret Rhoden <[email protected]> > wrote: > > > Hi - > > > > Patch 4 comments. > > > > > From 4127acb18d0ce2e19bb4f3b6bf491b162c3c13f3 Mon Sep 17 00:00:00 > > > 2001 From: Christopher Koch <[email protected]> > > > Date: Thu, 4 Aug 2016 14:18:28 -0700 > > > Subject: parlib/debug: Added ability to store memory and > > > single-step. > > > > > > Removed some debugging print messages. > > > > > > Change-Id: I4263112b2e340ea02e56c273529de291a22ba9dc > > > Signed-off-by: Christopher Koch <[email protected]> > > > --- > > > tests/Makefile | 2 +- > > > user/parlib/debug.c | 68 > > ++++++++++++++++++++++++++++++---- > > > user/parlib/include/parlib/debug.h | 13 ++++--- > > > user/parlib/include/parlib/x86/debug.h | 2 + > > > user/parlib/include/parlib/x86/trap.h | 1 + > > > user/parlib/thread0_sched.c | 1 + > > > user/parlib/x86/debug.c | 24 ++++++++++++ > > > 7 files changed, 97 insertions(+), 14 deletions(-) > > > > > > diff --git a/tests/Makefile b/tests/Makefile > > > index 3127f376e4f2..aabaaa8ebff0 100644 > > > --- a/tests/Makefile > > > +++ b/tests/Makefile > > > @@ -5,7 +5,7 @@ all: > > > # TODO: when we clean this up, if we ditch OBJDIR, change the > > > root > > makefile > > > TESTS_DIR = tests > > > > > > -CFLAGS_TESTS += $(CFLAGS_USER) -g > > > +CFLAGS_TESTS += $(CFLAGS_USER) -g -ggdb -O0 > > > > Is the -O0 still necessary? I'd rather not build our tests like > > that. > > > > If you want to set CFLAGS_TESTS for your own use (and not for the > > repo), you can do so in your Makelocal, which isn't under version > > control. > > > > > TESTS_CXXFLAGS += $(CXXFLAGS_USER) -g -std=gnu++11 > > > > > > TESTS_LDLIBS := -lpthread -lvmm -lbenchutil -lm -liplib -lndblib > > > diff --git a/user/parlib/debug.c b/user/parlib/debug.c > > > index 3395524e7e96..5117acb593f6 100644 > > > --- a/user/parlib/debug.c > > > +++ b/user/parlib/debug.c > > > @@ -92,7 +92,6 @@ static void debug_read_handler(struct > > > event_queue > > *ev_q); > > > 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); > > > > You can squash this and other fixups into older commits in this > > patchset. > > > > > 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); > > > @@ -303,8 +302,12 @@ int d9s_store_memory(const struct > > > d9_tstoremem *req) } > > > > > > /* d9s_resume is the d9_ops func called to fulfill a TRESUME > > > request. */ -void d9s_resume(void) > > > +void d9s_resume(bool singlestep) > > > { > > > + if (singlestep) > > > + uthread_apply_all(uthread_enable_single_step); > > > + else > > > + uthread_apply_all(uthread_disable_single_step); > > > > Does single-step need to be applied to every thread, or just the > > thread that triggered the breakpoint? From our discussions, I > > thought we'd single-step the thread that hit the breakpoint, and > > then the others would just stop and start subject to whatever > > stop-the-world policy we came up with. > > > > As you saw below, if we need to single-step arbitrary uthreads, > > we're going to run into problems if they are SW or VM ctxs. > > > > Barret > > > > > > > > > > > uthread_apply_all(uthread_runnable); > > > } > > > > > > @@ -366,6 +369,7 @@ int d9s_tresume(struct d9_header *hdr, void > > > *data) { > > > int ret; > > > struct d9_header *rpack; > > > + struct d9_tresume *dtr = (struct d9_tresume *) (hdr + 1); > > > > > > if (d9_ops == NULL || d9_ops->resume == NULL) > > > return debug_send_error(EBADF /* TODO: better error > > > code > > */); > > > @@ -379,7 +383,7 @@ int d9s_tresume(struct d9_header *hdr, void > > > *data) free(rpack); > > > > > > /* Call user-supplied routine. */ > > > - d9_ops->resume(); > > > + d9_ops->resume(dtr->singlestep); > > > return ret; > > > } > > > > > > @@ -618,7 +622,7 @@ void *d9c_read_thread(void *arg) > > > * Investigate what the largest memory chunk gdb > > > will ask > > for is. */ > > > rlen = read(fd, buf+bufindex, 4096); > > > if (rlen <= 0) { > > > - perror("d9 read"); > > > + /* TODO(chrisko): error message? */ > > > return NULL; > > > } > > > > > > @@ -683,6 +687,52 @@ void d9c_init(struct d9c_ops *ops) > > > clt_msg_handlers[D9_HANDLER(D9_TADDTHREAD)] = > > > &d9c_taddthread; } > > > > > > +/* d9c_store_memory communicates with the 2LS to store from an > > > address > > in > > > + * memory. */ > > > +int d9c_store_memory(int fd, uintptr_t address, const void > > > *const data, > > > + uint32_t length) > > > +{ > > > + size_t return_packet_size; > > > + struct d9_header *hdr; > > > + struct d9_tstoremem *req; > > > + > > > + hdr = alloc_packet(sizeof(struct d9_tstoremem), > > > D9_TSTOREMEM); > > > + if (hdr == NULL) > > > + return -1; > > > + > > > + req = (struct d9_tstoremem *) (hdr + 1); > > > + req->address = address; > > > + req->length = length; > > > + memcpy(&(req->data), data, length); > > > + > > > + int ret = debug_send_packet(fd, hdr, hdr->size); > > > + > > > + /* Provide space for the message to be stored in. */ > > > + return_packet_size = sizeof(struct d9_header) + > > > + MAX(sizeof(struct d9_rstoremem), sizeof(struct > > > d9_rerror)); > > > + char pkt_buf[return_packet_size]; > > > + > > > + /* 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); > > > + > > > + /* Got response. */ > > > + if (check_error_packet(response_header, D9_RSTOREMEM)) { > > > + perror("d9 store memory"); > > > + ret = -1; > > > + } else { > > > + ret = 0; > > > + } > > > + > > > + new_message = 0; > > > + uth_mutex_unlock(sync_lock); > > > + return ret; > > > +} > > > + > > > /* 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) > > > @@ -818,18 +868,20 @@ int d9c_fetch_registers(int fd, uint64_t > > > tid, > > struct d9_regs *regs) > > > return ret; > > > } > > > > > > -/* d9c_resume tells the 2LS to resume all threads. > > > - * > > > - * TODO(chrisko): resume w/ hardware-step enabled. */ > > > -int d9c_resume(int fd) > > > +/* d9c_resume tells the 2LS to resume all threads. */ > > > +int d9c_resume(int fd, bool singlestep) > > > { > > > size_t return_packet_size; > > > struct d9_header *hdr; > > > + struct d9_tresume *req; > > > > > > hdr = alloc_packet(sizeof(struct d9_tresume), D9_TRESUME); > > > if (hdr == NULL) > > > return -1; > > > > > > + req = (struct d9_tresume *) (hdr + 1); > > > + req->singlestep = singlestep; > > > + > > > int ret = debug_send_packet(fd, hdr, hdr->size); > > > > > > /* Block on getting a response packet. */ > > > diff --git a/user/parlib/include/parlib/debug.h > > b/user/parlib/include/parlib/debug.h > > > index c4e6a0eb8629..a068e1d8dbfe 100644 > > > --- a/user/parlib/include/parlib/debug.h > > > +++ b/user/parlib/include/parlib/debug.h > > > @@ -70,7 +70,9 @@ struct d9_tstorereg { > > > > > > struct d9_rstorereg {}; > > > > > > -struct d9_tresume {}; > > > +struct d9_tresume { > > > + bool singlestep:1; > > > +}; > > > > > > struct d9_rresume {}; > > > > > > @@ -96,7 +98,7 @@ struct d9_ops { > > > 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); > > > + void (*resume)(bool singlestep); > > > }; > > > > > > /* gdbserver ops. > > > @@ -118,7 +120,7 @@ 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); > > > +void d9s_resume(bool singlestep); > > > > > > /* Helpers to send messages from 2LS to gdbserver. */ > > > int d9s_notify_hit_breakpoint(uint64_t tid, uint64_t address); > > > @@ -130,7 +132,8 @@ void *d9c_read_thread(void *arg); > > > > > > /* 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_store_memory(int fd, uintptr_t address, const void > > > *const 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); > > > +int d9c_resume(int fd, bool singlestep); > > > diff --git a/user/parlib/include/parlib/x86/debug.h > > b/user/parlib/include/parlib/x86/debug.h > > > index 2c75a4f2093c..46400779344c 100644 > > > --- a/user/parlib/include/parlib/x86/debug.h > > > +++ b/user/parlib/include/parlib/x86/debug.h > > > @@ -29,5 +29,7 @@ struct d9_regs { > > > uint64_t reg_gs; > > > }; > > > > > > +void uthread_disable_single_step(struct uthread *t); > > > +void uthread_enable_single_step(struct uthread *t); > > > 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 3c9e907032db..957ec4e214be 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_DEBUG 1 > > > #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 ef993392c2b9..3d4d912472bf > > > 100644 --- a/user/parlib/thread0_sched.c > > > +++ b/user/parlib/thread0_sched.c > > > @@ -133,6 +133,7 @@ static void thread0_thread_refl_fault(struct > > > uthread > > *uth, > > > refl_error(uth, trap_nr, err, aux); > > > break; > > > case HW_TRAP_BRKPT: > > > + case HW_TRAP_DEBUG: > > > /* We only have one thread, no need to stop other > > > threads. > > */ > > > uthread_has_blocked(uth, 0); > > > d9s_notify_hit_breakpoint(uth->id, 0); > > > diff --git a/user/parlib/x86/debug.c b/user/parlib/x86/debug.c > > > index 7dd7b589b04f..8c005fbfff34 100644 > > > --- a/user/parlib/x86/debug.c > > > +++ b/user/parlib/x86/debug.c > > > @@ -1,4 +1,28 @@ > > > #include <parlib/arch/debug.h> > > > +#include <parlib/assert.h> > > > +#include <ros/arch/mmu.h> > > > + > > > +void uthread_enable_single_step(struct uthread *t) > > > +{ > > > + switch (t->u_ctx.type) { > > > + case ROS_HW_CTX: > > > + t->u_ctx.tf.hw_tf.tf_rflags |= FL_TF; > > > + break; > > > + default: > > > + panic("bad context type\n"); > > > + } > > > +} > > > + > > > +void uthread_disable_single_step(struct uthread *t) > > > +{ > > > + switch (t->u_ctx.type) { > > > + case ROS_HW_CTX: > > > + t->u_ctx.tf.hw_tf.tf_rflags &= ~FL_TF; > > > + break; > > > + default: > > > + panic("bad context type\n"); > > > + } > > > +} > > > > > > /* 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. > > > -- > > > 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. > > -- 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.
