From 5f172878dd2b7bebfb50cb1b785e6bfafbd281f7 Mon Sep 17 00:00:00 2001 From: Geoffrey Challen Date: Thu, 9 Feb 2017 09:45:04 -0500 Subject: [PATCH 01/11] Updated CHANGELOG. --- CHANGES | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/CHANGES b/CHANGES index 804f1cb..5dde7e1 100644 --- a/CHANGES +++ b/CHANGES @@ -12,6 +12,96 @@ various other people, all of whom are (hopefully) listed below. ------------------------------------------------------------ +OS/161 2.0.3 released 20160124 +------------------------------ + +20170124 dholland in base + - Remove obsolete, redundant, or not useful test programs: + guzzle (same as hog) + kitchen (equivalent to multiexec -n 4 sink) + sink (same as conman) + sty (equivalent to multiexec -n 6 hog) + quinthuge (offers little over triplehuge, can be done with multiexec) + quintmat, quintsort (ditto) + +20170118 dholland in base + - Make -g -Og the flags when "debug" is enabled in a kernel config + (which it is in the non-OPT ones) and add an additional kernel + config verb "debugonly" to get just -g in case that becomes + necessary. This should significantly improve the output code + quality from gcc without compromising debugging. (However, gcc + being gcc, it also sometimes leads to additional spurious + warnings that don't occur with either -g or -O2.) + +20170118 dholland in base + - Add a MI mainbus_debugger() function that goes through the right + MD paths to trigger the debugger hook in the ltrace device. Also + add a menu function "debug" to trigger it. + +20170118 dholland in base + - Add some bits to forktest to try to catch the case where the fork + child returns from the next system call instead of from fork. + (Which is a moderately common bug, caused by races copying the + trapframe information in the kernel.) + +20170117 dholland in base + - Add a menu command "deadlock" to intentionally deadlock. + +20170117 dholland in base, from Sam Fishman + - Fix parallelvm -w so that if one of the forks fails the whole + thing doesn't wedge. + +20170117 dholland in base, reported by Sam Fishman + - Don't do semfs I/O from NULL, or from/to insufficiently sized + buffers. Like the 20150615 change, except covering the rest of + the tests that use the semaphores that were doing it wrong: + multiexec, parallelvm, and schedpong. + +20170117 dholland in base, from Sam Fishman + - Add assembler directives to exception-mips1.S that tell gdb how + to read trap frames correctly. Garbage-collect old stuff left + over from making it work with a (much) older version of gdb a + long time back. This also usually makes it possible to trace back + through a syscall into a userlevel process; include a gdb script + with tools for making this useful. + +20170117 dholland in base + - Merge the deadlock detector into base. It was a success last year. + - Mention in the comments that the hangman hooks in locks need to + be called atomically. + +20170117 dholland in base, reported by Jeffrey Cai, patch from Sam Fishman + - Fix off-by-one in tac that makes it skip the first line of files. + +20170117 dholland in base, from Sam Fishman + - Make badcall's "pipe with unaligned pointer" test clean up after + itself if the operation succeeds. Otherwise it leaks fds and that + can intefere with other tests. + +20170116 dholland in base, reported by Sam Fishman + - Don't allow opening an entirely empty pathname to succeed, and + don't allow success for this case in badcall either. + +20170116 dholland in base, from Sasha Fedorova + - Fix write buffer size in filetest. + +20160325 dholland in base + - Fix macro parenthesis bug in ROUNDUP(). + +20160304 dholland in base, from Sam Fishman + - Make runtest.py handle spacing in the command strings it's given. + +20160216 dholland in base + - Fix spacing problems in ls -l output for large files. + +20160203 dholland in base + - Expand comments attached to cpustacks[]/cputhreads[], prompted by + James Mickens. + +20160125 dholland in base, from Nikhil Benesch. + - Fix stupid argument handling bug in test.py. + + OS/161 2.0.2 released 20160112 ------------------------------ @@ -53,6 +143,14 @@ OS/161 2.0.2 released 20160112 anyway. If they aren't actually constant because of bugs, reading a stale or even garbage value is not going to hurt more. +20160107 dholland in deadlock-detector + - Add a deadlock detector. For now this will be supplied to + instructors as a supplementary patch, because it intrudes into + the synchronization primitives and affects what students do + there. We are planning to try it on our students this coming + semester; if that works out well, I'll probably merge it into + base. + 20160107 dholland in base - In testbin/multiexec, if fork fails partway through, continue with the forks we got. Otherwise the subprocesses we started hang From 5f8b9f7ff4a684692fd8e98c567ced34b5ba978f Mon Sep 17 00:00:00 2001 From: Geoffrey Challen Date: Thu, 9 Feb 2017 09:47:06 -0500 Subject: [PATCH 02/11] Low-level stack frame changes. --- kern/arch/mips/include/trapframe.h | 2 - kern/arch/mips/locore/exception-mips1.S | 111 +++++++++++++----------- kern/arch/mips/locore/trap.c | 4 +- kern/arch/mips/thread/cpu.c | 8 ++ kern/arch/sys161/dev/lamebus_machdep.c | 10 +++ 5 files changed, 81 insertions(+), 54 deletions(-) diff --git a/kern/arch/mips/include/trapframe.h b/kern/arch/mips/include/trapframe.h index 19d08bb..931ed89 100644 --- a/kern/arch/mips/include/trapframe.h +++ b/kern/arch/mips/include/trapframe.h @@ -69,8 +69,6 @@ struct trapframe { uint32_t tf_s7; uint32_t tf_t8; uint32_t tf_t9; - uint32_t tf_k0; /* dummy (see exception-mips1.S comments) */ - uint32_t tf_k1; /* dummy */ uint32_t tf_gp; uint32_t tf_sp; uint32_t tf_s8; diff --git a/kern/arch/mips/locore/exception-mips1.S b/kern/arch/mips/locore/exception-mips1.S index ad99c2a..f0fec13 100644 --- a/kern/arch/mips/locore/exception-mips1.S +++ b/kern/arch/mips/locore/exception-mips1.S @@ -101,6 +101,8 @@ mips_general_end: .text .type common_exception,@function .ent common_exception + .cfi_startproc + .cfi_signal_frame common_exception: mfc0 k0, c0_status /* Get status register */ andi k0, k0, CST_KUp /* Check the we-were-in-user-mode bit */ @@ -130,11 +132,12 @@ common_exception: */ /* - * Allocate stack space for 37 words to hold the trap frame, + * Allocate stack space for 35 words to hold the trap frame, * plus four more words for a minimal argument block, plus * one more for proper (64-bit) stack alignment. */ - addi sp, sp, -168 + addi sp, sp, -160 + .cfi_def_cfa sp, 0 /* * Save general registers. @@ -143,70 +146,89 @@ common_exception: * * The order here must match mips/include/trapframe.h. * - * gdb disassembles this code to try to figure out what registers - * are where, and it isn't very bright. So in order to make gdb be - * able to trace the stack back through here, we play some silly - * games. + * gdb uses the .cfi_offset assembler directives inserted below to + * to figure out where each register is stored. Since we've marked + * this function as a "signal handler" with the .cfi_signal_frame + * directive, gdb won't complain about the fact that the stack + * is noncontiguous (if we're coming from userland). * - * In particular: - * (1) We store the return address register into the epc slot, - * which makes gdb think it's the return address slot. Then - * we store the real epc value over that. - * (2) We store the current sp into the sp slot, which makes gdb - * think it's the stack pointer slot. Then we store the real - * value. - * (3) gdb also assumes that saved registers in a function are - * saved in order. This is why we put epc where it is, and - * handle the real value of ra afterwards. - * (4) Because gdb will think we're saving k0 and k1, we need to - * leave slots for them in the trap frame, even though the - * stuff we save there is useless. + * We also play a trick with the return address: we mark the ra + * register as stored to the stack normally and then mark the + * return address for *this* function as being in the k1 register + * using the .cfi_return_column directive. gdb is then able to + * recognize that the ra we've stored here is the return address + * for the function that was executing when this exception was + * taken. * - * This logic has not been tested against a recent gdb and has - * probably bitrotted. Someone(TM) should figure out what gdb - * currently expects -- or maybe even patch gdb to understand a - * better form of this that doesn't waste so many cycles. + * All of the cfi (call frame information) material is compiled + * into the .eh_frame section of the compiled kernel. */ - sw ra, 160(sp) /* dummy for gdb */ - sw s8, 156(sp) /* save s8 */ - sw sp, 152(sp) /* dummy for gdb */ - sw gp, 148(sp) /* save gp */ - sw k1, 144(sp) /* dummy for gdb */ - sw k0, 140(sp) /* dummy for gdb */ - - sw k1, 152(sp) /* real saved sp */ + sw s8, 148(sp) /* save s8 */ + .cfi_offset s8, 148 + sw k1, 144(sp) /* real saved sp */ + .cfi_offset sp, 144 + sw gp, 140(sp) /* save gp */ nop /* delay slot for store */ + .cfi_offset gp, 140 + .cfi_return_column k1 mfc0 k1, c0_epc /* Copr.0 reg 13 == PC for exception */ - sw k1, 160(sp) /* real saved PC */ + sw k1, 152(sp) /* real saved PC */ + .cfi_offset k1, 152 sw t9, 136(sp) + .cfi_offset t9, 136 sw t8, 132(sp) + .cfi_offset t8, 132 sw s7, 128(sp) + .cfi_offset s7, 128 sw s6, 124(sp) + .cfi_offset s6, 124 sw s5, 120(sp) + .cfi_offset s5, 120 sw s4, 116(sp) + .cfi_offset s4, 116 sw s3, 112(sp) + .cfi_offset s3, 112 sw s2, 108(sp) + .cfi_offset s2, 108 sw s1, 104(sp) + .cfi_offset s1, 104 sw s0, 100(sp) + .cfi_offset s0, 100 sw t7, 96(sp) + .cfi_offset t7, 96 sw t6, 92(sp) + .cfi_offset t6, 92 sw t5, 88(sp) + .cfi_offset t5, 88 sw t4, 84(sp) + .cfi_offset t4, 84 sw t3, 80(sp) + .cfi_offset t3, 80 sw t2, 76(sp) + .cfi_offset t2, 76 sw t1, 72(sp) + .cfi_offset t1, 72 sw t0, 68(sp) + .cfi_offset t0, 68 sw a3, 64(sp) + .cfi_offset a3, 64 sw a2, 60(sp) + .cfi_offset a2, 60 sw a1, 56(sp) + .cfi_offset a1, 56 sw a0, 52(sp) + .cfi_offset a0, 52 sw v1, 48(sp) + .cfi_offset v1, 48 sw v0, 44(sp) + .cfi_offset v0, 44 sw AT, 40(sp) + .cfi_offset AT, 40 sw ra, 36(sp) + .cfi_offset ra, 36 /* * Save special registers. @@ -227,11 +249,6 @@ common_exception: mfc0 t4, c0_cause sw t4, 24(sp) /* Copr.0 reg 13 == exception cause */ - /* - * Pretend to save $0 for gdb's benefit. - */ - sw $0, 12(sp) - /* * Load the curthread register if coming from user mode. */ @@ -260,9 +277,6 @@ common_exception: jal mips_trap /* call it */ nop /* delay slot */ - /* Something must be here or gdb doesn't find the stack frame. */ - nop - /* * Now restore stuff and return from the exception. * Interrupts should be off. @@ -309,20 +323,17 @@ exception_return: lw s7, 128(sp) lw t8, 132(sp) lw t9, 136(sp) + lw gp, 140(sp) /* restore gp */ + /* 144(sp) stack pointer - below */ + lw s8, 148(sp) /* restore s8 */ + lw k1, 152(sp) /* fetch exception return PC into k1 */ - /* 140(sp) "saved" k0 was dummy garbage anyway */ - /* 144(sp) "saved" k1 was dummy garbage anyway */ - - lw gp, 148(sp) /* restore gp */ - /* 152(sp) stack pointer - below */ - lw s8, 156(sp) /* restore s8 */ - lw k0, 160(sp) /* fetch exception return PC into k0 */ - - lw sp, 152(sp) /* fetch saved sp (must be last) */ + lw sp, 144(sp) /* fetch saved sp (must be last) */ /* done */ - jr k0 /* jump back */ + jr k1 /* jump back */ rfe /* in delay slot */ + .cfi_endproc .end common_exception /* diff --git a/kern/arch/mips/locore/trap.c b/kern/arch/mips/locore/trap.c index d3873eb..69ae12c 100644 --- a/kern/arch/mips/locore/trap.c +++ b/kern/arch/mips/locore/trap.c @@ -130,8 +130,8 @@ mips_trap(struct trapframe *tf) bool iskern; int spl; - /* The trap frame is supposed to be 37 registers long. */ - KASSERT(sizeof(struct trapframe)==(37*4)); + /* The trap frame is supposed to be 35 registers long. */ + KASSERT(sizeof(struct trapframe)==(35*4)); /* * Extract the exception code info from the register fields. diff --git a/kern/arch/mips/thread/cpu.c b/kern/arch/mips/thread/cpu.c index 5acce5b..d43a4ec 100644 --- a/kern/arch/mips/thread/cpu.c +++ b/kern/arch/mips/thread/cpu.c @@ -54,6 +54,14 @@ * * These arrays are also used to start up new CPUs, for roughly the * same reasons. + * + * The values in the current cpu's slots in these arrays are updated + * with the current thread's information in trap.c before heading to + * userlevel, as well as being initialized in cpu_machdep_init below. + * This means that (unless something really horrible happens) on entry + * to the kernel, and when a new CPU starts up in cpu_start_secondary, + * they will have the information needed to figure out who we are and + * proceed. */ vaddr_t cpustacks[MAXCPUS]; diff --git a/kern/arch/sys161/dev/lamebus_machdep.c b/kern/arch/sys161/dev/lamebus_machdep.c index c6c62ce..0cc1fb2 100644 --- a/kern/arch/sys161/dev/lamebus_machdep.c +++ b/kern/arch/sys161/dev/lamebus_machdep.c @@ -42,6 +42,7 @@ #include #include #include +#include #include "autoconf.h" /* @@ -275,6 +276,15 @@ mainbus_send_ipi(struct cpu *target) lamebus_assert_ipi(lamebus, target); } +/* + * Trigger the debugger. + */ +void +mainbus_debugger(void) +{ + ltrace_stop(0); +} + /* * Interrupt dispatcher. */ From 8435ba643640f7c16f0db8ac9e29e97c16e2d539 Mon Sep 17 00:00:00 2001 From: Geoffrey Challen Date: Thu, 9 Feb 2017 09:48:06 -0500 Subject: [PATCH 03/11] New debug options. --- kern/conf/DUMBVM | 4 +++- kern/conf/DUMBVM-OPT | 1 + kern/conf/GENERIC | 2 ++ kern/conf/GENERIC-OPT | 1 + kern/conf/conf.kern | 3 +++ kern/conf/config | 7 ++++++- 6 files changed, 16 insertions(+), 2 deletions(-) diff --git a/kern/conf/DUMBVM b/kern/conf/DUMBVM index 7862c64..2a19bee 100644 --- a/kern/conf/DUMBVM +++ b/kern/conf/DUMBVM @@ -3,7 +3,9 @@ include conf/conf.kern # get definitions of available options -debug # Compile with debug info. +debug # Compile with debug info and -Og. +#debugonly # Compile with debug info only (no -Og). +#options hangman # Deadlock detection. (off by default) # # Device drivers for hardware. diff --git a/kern/conf/DUMBVM-OPT b/kern/conf/DUMBVM-OPT index e878c35..115016f 100644 --- a/kern/conf/DUMBVM-OPT +++ b/kern/conf/DUMBVM-OPT @@ -7,6 +7,7 @@ include conf/conf.kern # get definitions of available options #debug # Optimizing compile (no debug). +#debugonly options noasserts # Disable assertions. # diff --git a/kern/conf/GENERIC b/kern/conf/GENERIC index 13ff40e..a726c3c 100644 --- a/kern/conf/GENERIC +++ b/kern/conf/GENERIC @@ -5,6 +5,8 @@ include conf/conf.kern # get definitions of available options debug # Compile with debug info. +#debugonly # Compile with debug info only (no -Og). +#options hangman # Deadlock detection. (off by default) # # Device drivers for hardware. diff --git a/kern/conf/GENERIC-OPT b/kern/conf/GENERIC-OPT index f78637d..7bdc740 100644 --- a/kern/conf/GENERIC-OPT +++ b/kern/conf/GENERIC-OPT @@ -7,6 +7,7 @@ include conf/conf.kern # get definitions of available options #debug # Optimizing compile (no debug). +#debugonly options noasserts # Disable assertions. # diff --git a/kern/conf/conf.kern b/kern/conf/conf.kern index 48ed457..ad58977 100644 --- a/kern/conf/conf.kern +++ b/kern/conf/conf.kern @@ -326,6 +326,9 @@ file thread/synch.c file thread/thread.c file thread/threadlist.c +defoption hangman +optfile hangman thread/hangman.c + # # Process system # diff --git a/kern/conf/config b/kern/conf/config index 01bca35..f49c4ab 100755 --- a/kern/conf/config +++ b/kern/conf/config @@ -12,7 +12,8 @@ # Recognized directives: # # file use source file -# debug turn on debug info +# debug turn on debug info and -Og +# debugonly turn on debug info without -Og # defoption define an option # optfile if option is enabled, use file # optofffile if option is disabled, use file @@ -97,6 +98,7 @@ echo "$CONFNAME" $CONFTMP | awk ' nfields["include"] = 2; nfields["file"] = 2; nfields["debug"] = 1; + nfields["debugonly"] = 1; nfields["defoption"] = 2; nfields["optfile"] = 3; nfields["optofffile"] = 3; @@ -776,6 +778,9 @@ echo -n ' files.mk' # Default: optimize. BEGIN { debugflags="-O2"; } $1=="debug" { + debugflags="-g -Og"; + } + $1=="debugonly" { debugflags="-g"; } From 8af1edae0e220d79aada134845d869d1792c77f6 Mon Sep 17 00:00:00 2001 From: Geoffrey Challen Date: Thu, 9 Feb 2017 09:49:03 -0500 Subject: [PATCH 04/11] Hang detection additions. --- kern/include/cpu.h | 5 +++ kern/include/hangman.h | 84 +++++++++++++++++++++++++++++++++++++++++ kern/include/lib.h | 2 +- kern/include/mainbus.h | 3 ++ kern/include/spinlock.h | 7 ++++ kern/include/synch.h | 1 + kern/include/thread.h | 1 + kern/include/version.h | 2 +- 8 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 kern/include/hangman.h diff --git a/kern/include/cpu.h b/kern/include/cpu.h index cd386ec..687df77 100644 --- a/kern/include/cpu.h +++ b/kern/include/cpu.h @@ -87,6 +87,11 @@ struct cpu { struct tlbshootdown c_shootdown[TLBSHOOTDOWN_MAX]; unsigned c_numshootdown; struct spinlock c_ipi_lock; + + /* + * Accessed by other cpus. Protected inside hangman.c. + */ + HANGMAN_ACTOR(c_hangman); }; /* diff --git a/kern/include/hangman.h b/kern/include/hangman.h new file mode 100644 index 0000000..91f3de2 --- /dev/null +++ b/kern/include/hangman.h @@ -0,0 +1,84 @@ +/* + * Copyright (c) 2015 + * The President and Fellows of Harvard College. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * 3. Neither the name of the University nor the names of its contributors + * may be used to endorse or promote products derived from this software + * without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE UNIVERSITY AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE UNIVERSITY OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#ifndef HANGMAN_H +#define HANGMAN_H + +/* + * Simple deadlock detector. Enable with "options hangman" in the + * kernel config. + */ + +#include "opt-hangman.h" + +#if OPT_HANGMAN + +struct hangman_actor { + const char *a_name; + const struct hangman_lockable *a_waiting; +}; + +struct hangman_lockable { + const char *l_name; + const struct hangman_actor *l_holding; +}; + +void hangman_wait(struct hangman_actor *a, struct hangman_lockable *l); +void hangman_acquire(struct hangman_actor *a, struct hangman_lockable *l); +void hangman_release(struct hangman_actor *a, struct hangman_lockable *l); + +#define HANGMAN_ACTOR(sym) struct hangman_actor sym +#define HANGMAN_LOCKABLE(sym) struct hangman_lockable sym + +#define HANGMAN_ACTORINIT(a, n) ((a)->a_name = (n), (a)->a_waiting = NULL) +#define HANGMAN_LOCKABLEINIT(l, n) ((l)->l_name = (n), (l)->l_holding = NULL) + +#define HANGMAN_LOCKABLE_INITIALIZER { "spinlock", NULL } + +#define HANGMAN_WAIT(a, l) hangman_wait(a, l) +#define HANGMAN_ACQUIRE(a, l) hangman_acquire(a, l) +#define HANGMAN_RELEASE(a, l) hangman_release(a, l) + +#else + +#define HANGMAN_ACTOR(sym) +#define HANGMAN_LOCKABLE(sym) + +#define HANGMAN_ACTORINIT(a, name) +#define HANGMAN_LOCKABLEINIT(a, name) + +#define HANGMAN_LOCKABLE_INITIALIZER + +#define HANGMAN_WAIT(a, l) +#define HANGMAN_ACQUIRE(a, l) +#define HANGMAN_RELEASE(a, l) + +#endif + +#endif /* HANGMAN_H */ diff --git a/kern/include/lib.h b/kern/include/lib.h index 7478ce6..66e5da3 100644 --- a/kern/include/lib.h +++ b/kern/include/lib.h @@ -192,7 +192,7 @@ void kprintf_bootstrap(void); */ #define DIVROUNDUP(a,b) (((a)+(b)-1)/(b)) -#define ROUNDUP(a,b) (DIVROUNDUP(a,b)*b) +#define ROUNDUP(a,b) (DIVROUNDUP(a,b)*(b)) #endif /* _LIB_H_ */ diff --git a/kern/include/mainbus.h b/kern/include/mainbus.h index 3ebbdea..4d7fff0 100644 --- a/kern/include/mainbus.h +++ b/kern/include/mainbus.h @@ -55,6 +55,9 @@ size_t mainbus_ramsize(void); /* Switch on an inter-processor interrupt. (Low-level.) */ void mainbus_send_ipi(struct cpu *target); +/* Request breaking into the debugger, where available. */ +void mainbus_debugger(void); + /* * The various ways to shut down the system. (These are very low-level * and should generally not be called directly - md_poweroff, for diff --git a/kern/include/spinlock.h b/kern/include/spinlock.h index d3b0ca0..e28ec90 100644 --- a/kern/include/spinlock.h +++ b/kern/include/spinlock.h @@ -36,6 +36,7 @@ */ #include +#include /* Inlining support - for making sure an out-of-line copy gets built */ #ifndef SPINLOCK_INLINE @@ -57,12 +58,18 @@ struct spinlock { volatile spinlock_data_t splk_lock; /* Memory word where we spin. */ struct cpu *splk_holder; /* CPU holding this lock. */ + HANGMAN_LOCKABLE(splk_hangman); /* Deadlock detector hook. */ }; /* * Initializer for cases where a spinlock needs to be static or global. */ +#ifdef OPT_HANGMAN +#define SPINLOCK_INITIALIZER { SPINLOCK_DATA_INITIALIZER, NULL, \ + HANGMAN_LOCKABLE_INITIALIZER } +#else #define SPINLOCK_INITIALIZER { SPINLOCK_DATA_INITIALIZER, NULL } +#endif /* * Spinlock functions. diff --git a/kern/include/synch.h b/kern/include/synch.h index fee3b07..0431cd3 100644 --- a/kern/include/synch.h +++ b/kern/include/synch.h @@ -74,6 +74,7 @@ void V(struct semaphore *); */ struct lock { char *lk_name; + HANGMAN_LOCKABLE(lk_hangman); /* Deadlock detector hook. */ // add what you need here // (don't forget to mark things volatile as needed) }; diff --git a/kern/include/thread.h b/kern/include/thread.h index 48ca76d..8017994 100644 --- a/kern/include/thread.h +++ b/kern/include/thread.h @@ -83,6 +83,7 @@ struct thread { struct switchframe *t_context; /* Saved register context (on stack) */ struct cpu *t_cpu; /* CPU thread runs on */ struct proc *t_proc; /* Process thread belongs to */ + HANGMAN_ACTOR(t_hangman); /* Deadlock detector hook */ /* * Interrupt state fields. diff --git a/kern/include/version.h b/kern/include/version.h index 43f66be..4d9fa1c 100644 --- a/kern/include/version.h +++ b/kern/include/version.h @@ -34,7 +34,7 @@ * Leave this alone, so we can tell what version of the OS/161 base * code we gave you. */ -#define BASE_VERSION "2.0.2" +#define BASE_VERSION "2.0.3" /* * Change this as you see fit in the course of hacking the system. From e809face5f91b736b1a11372e99d5a1a35449d12 Mon Sep 17 00:00:00 2001 From: Geoffrey Challen Date: Thu, 9 Feb 2017 09:49:42 -0500 Subject: [PATCH 05/11] New debugging commands. --- kern/main/menu.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/kern/main/menu.c b/kern/main/menu.c index 8905f09..1f5723a 100644 --- a/kern/main/menu.c +++ b/kern/main/menu.c @@ -35,6 +35,8 @@ #include #include #include +#include +#include #include #include #include @@ -237,6 +239,21 @@ cmd_sync(int nargs, char **args) return 0; } +/* + * Command for dropping to the debugger. + */ +static +int +cmd_debug(int nargs, char **args) +{ + (void)nargs; + (void)args; + + mainbus_debugger(); + + return 0; +} + /* * Command for doing an intentional panic. */ @@ -251,6 +268,81 @@ cmd_panic(int nargs, char **args) return 0; } +/* + * Subthread for intentially deadlocking. + */ +struct deadlock { + struct lock *lock1; + struct lock *lock2; +}; + +static +void +cmd_deadlockthread(void *ptr, unsigned long num) +{ + struct deadlock *dl = ptr; + + (void)num; + + /* If it doesn't wedge right away, keep trying... */ + while (1) { + lock_acquire(dl->lock2); + lock_acquire(dl->lock1); + kprintf("+"); + lock_release(dl->lock1); + lock_release(dl->lock2); + } +} + +/* + * Command for doing an intentional deadlock. + */ +static +int +cmd_deadlock(int nargs, char **args) +{ + struct deadlock dl; + int result; + + (void)nargs; + (void)args; + + dl.lock1 = lock_create("deadlock1"); + if (dl.lock1 == NULL) { + kprintf("lock_create failed\n"); + return ENOMEM; + } + dl.lock2 = lock_create("deadlock2"); + if (dl.lock2 == NULL) { + lock_destroy(dl.lock1); + kprintf("lock_create failed\n"); + return ENOMEM; + } + + result = thread_fork(args[0] /* thread name */, + NULL /* kernel thread */, + cmd_deadlockthread /* thread function */, + &dl /* thread arg */, 0 /* thread arg */); + if (result) { + kprintf("thread_fork failed: %s\n", strerror(result)); + lock_release(dl.lock1); + lock_destroy(dl.lock2); + lock_destroy(dl.lock1); + return result; + } + + /* If it doesn't wedge right away, keep trying... */ + while (1) { + lock_acquire(dl.lock1); + lock_acquire(dl.lock2); + kprintf("."); + lock_release(dl.lock2); + lock_release(dl.lock1); + } + /* NOTREACHED */ + return 0; +} + /* * Command for shutting down. */ @@ -441,7 +533,9 @@ static const char *opsmenu[] = { "[cd] Change directory ", "[pwd] Print current directory ", "[sync] Sync filesystems ", + "[debug] Drop to debugger ", "[panic] Intentional panic ", + "[deadlock] Intentional deadlock ", "[q] Quit and shut down ", NULL }; @@ -547,7 +641,9 @@ static struct { { "cd", cmd_chdir }, { "pwd", cmd_pwd }, { "sync", cmd_sync }, + { "debug", cmd_debug }, { "panic", cmd_panic }, + { "deadlock", cmd_deadlock }, { "q", cmd_quit }, { "exit", cmd_quit }, { "halt", cmd_quit }, From 52d122b854072cdfee11d102a44f070229280dbd Mon Sep 17 00:00:00 2001 From: Geoffrey Challen Date: Thu, 9 Feb 2017 09:50:07 -0500 Subject: [PATCH 06/11] Minor test changes. --- kern/test/arraytest.c | 3 +++ kern/test/fstest.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/kern/test/arraytest.c b/kern/test/arraytest.c index a63405f..b55d3a7 100644 --- a/kern/test/arraytest.c +++ b/kern/test/arraytest.c @@ -156,6 +156,9 @@ arraytest2(int nargs, char **args) (void)nargs; (void)args; + /* Silence warning with gcc 4.8 -Og (but not -O2) */ + x = 0; + kprintf("Beginning large array test...\n"); a = array_create(); KASSERT(a != NULL); diff --git a/kern/test/fstest.c b/kern/test/fstest.c index 9120920..1617027 100644 --- a/kern/test/fstest.c +++ b/kern/test/fstest.c @@ -681,7 +681,7 @@ checkfilesystem(int nargs, char **args) char *device; if (nargs != 2) { - kprintf("Usage: fs[12345] filesystem:\n"); + kprintf("Usage: fs[123456] filesystem:\n"); return EINVAL; } From 4c9b79877e82e4df23b9edcb99cd03f67bfeaa7b Mon Sep 17 00:00:00 2001 From: Geoffrey Challen Date: Thu, 9 Feb 2017 09:51:11 -0500 Subject: [PATCH 07/11] Build system changes. --- mk/os161.mkdirs.mk | 2 +- mk/os161.script.mk | 92 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 mk/os161.script.mk diff --git a/mk/os161.mkdirs.mk b/mk/os161.mkdirs.mk index 8e4fe40..370eb87 100644 --- a/mk/os161.mkdirs.mk +++ b/mk/os161.mkdirs.mk @@ -1,5 +1,5 @@ # -# MKDIRS logic +# OS/161 build environment: MKDIRS logic # # This generates rules for all intermediate directories as well as # the directories listed. (That is, if you say MKDIRS=/usr/bin/foo diff --git a/mk/os161.script.mk b/mk/os161.script.mk new file mode 100644 index 0000000..6d579c8 --- /dev/null +++ b/mk/os161.script.mk @@ -0,0 +1,92 @@ +# +# OS/161 build environment: install scripts +# +# Usage: +# TOP=../.. +# .include "$(TOP)/mk/os161.config.mk" +# [defs go here] +# .include "$(TOP)/mk/os161.man.mk" +# [any extra rules go here] +# +# Variables controlling this file: +# +# EXECSCRIPTS Executable files to install. +# NONEXECSCRIPTS Non-executable files to install. +# +# SCRIPTDIR Directory under $(OSTREE) to install into, +# e.g. /hostbin. Should have a leading slash. +# + +ALLSCRIPTS=$(NONEXECSCRIPTS) + +# We may want these directories created. (Used by os161.baserules.mk.) +MKDIRS+=$(MYBUILDDIR) +MKDIRS+=$(INSTALLTOP)$(SCRIPTDIR) +MKDIRS+=$(OSTREE)$(SCRIPTDIR) + +# Default rule: "build" the executable scripts. This sets the hash-bang +# header with the interpreter path. +# (In make the first rule found is the default.) +all: all-local +all-local: $(MYBUILDDIR) .WAIT +.for S in $(EXECSCRIPTS) +ALLSCRIPTS+=$(MYBUILDDIR)/$(S) +all-local: $(MYBUILDDIR)/$(S) +$(MYBUILDDIR)/$(S): $(S) +# This must test $@, or assign another variable; it can't test $(S). +# (This is a bmake bug.) +.if !empty(@:M*.py) + echo '#!'"$(PYTHON_INTERPRETER)" > $@.new +.else + sed -n -e '1p' < $(S) > $@.new +.endif + sed -e '1d' < $(S) >> $@.new + chmod 755 $@.new + mv -f $@.new $@ +.endfor + +# depend doesn't need to do anything +depend-local: ; + +# +# Install: we can install into either $(INSTALLTOP) or $(OSTREE). +# When building the whole system, we always install into the staging +# area. However, if you're working on a particular program it is +# usually convenient to be able to install it directly to $(OSTREE) +# instead of doing a complete top-level install. +# +# Note that we make a hard link instead of a copy by default to reduce +# overhead. +# +install-staging-local: $(INSTALLTOP)$(SCRIPTDIR) .WAIT + +.for _F_ in $(ALLSCRIPTS) +install-staging-local: $(INSTALLTOP)$(SCRIPTDIR)/$(_F_:T) +$(INSTALLTOP)$(SCRIPTDIR)/$(_F_:T): $(_F_) + rm -f $(.TARGET) + ln $(_F_) $(.TARGET) || cp $(_F_) $(.TARGET) +.endfor + +install-local: $(OSTREE)$(SCRIPTDIR) .WAIT installscripts +installscripts: +.for _F_ in $(ALLSCRIPTS) + rm -f $(OSTREE)$(SCRIPTDIR)/$(_F_:T) + ln $(_F_) $(OSTREE)$(SCRIPTDIR)/$(_F_:T) || \ + cp $(_F_) $(OSTREE)$(SCRIPTDIR)/$(_F_:T) +.endfor + +# clean: remove build products +clean-local: +.for S in $(EXECSCRIPTS) + rm -f $(MYBUILDDIR)/$(S) +.endfor + +# Mark targets that don't represent files PHONY, to prevent various +# lossage if files by those names appear. +.PHONY: all all-local install-staging-local install-local installscripts +.PHONY: clean-local + +# Finally, get the shared definitions for the most basic rules. +.include "$(TOP)/mk/os161.baserules.mk" + +# End. From 9986e078100e27ead1cc9e37451be7326b2d4744 Mon Sep 17 00:00:00 2001 From: Geoffrey Challen Date: Thu, 9 Feb 2017 09:51:39 -0500 Subject: [PATCH 08/11] Userland test changes. --- userland/bin/ls/ls.c | 2 +- userland/bin/tac/tac.c | 6 ++---- userland/lib/libtest/Makefile | 2 +- userland/testbin/Makefile | 6 +++--- userland/testbin/badcall/bad_open.c | 2 +- userland/testbin/badcall/bad_pipe.c | 5 +++++ userland/testbin/filetest/filetest.c | 2 +- userland/testbin/forktest/forktest.c | 27 ++++++++++++++++++++++++ userland/testbin/hog/hog.c | 2 +- userland/testbin/multiexec/multiexec.c | 1 + userland/testbin/parallelvm/parallelvm.c | 19 ++++++++++++++--- userland/testbin/schedpong/usem.c | 12 +++++++---- 12 files changed, 67 insertions(+), 19 deletions(-) diff --git a/userland/bin/ls/ls.c b/userland/bin/ls/ls.c index 6dd8c88..2041bc8 100644 --- a/userland/bin/ls/ls.c +++ b/userland/bin/ls/ls.c @@ -177,7 +177,7 @@ print(const char *path) typech = '?'; } - printf("%crwx------ %2d root %-8llu", + printf("%crwx------ %2d root %-7llu ", typech, statbuf.st_nlink, statbuf.st_size); diff --git a/userland/bin/tac/tac.c b/userland/bin/tac/tac.c index 3e0a260..edaa48d 100644 --- a/userland/bin/tac/tac.c +++ b/userland/bin/tac/tac.c @@ -197,11 +197,9 @@ dumpdata(void) indexsize = dolseek(indexfd, indexname, 0, SEEK_CUR); pos = indexsize; - while (1) { + assert(pos % sizeof(x) == 0); + while (pos > 0) { pos -= sizeof(x); - if (pos == 0) { - break; - } assert(pos >= 0); dolseek(indexfd, indexname, pos, SEEK_SET); diff --git a/userland/lib/libtest/Makefile b/userland/lib/libtest/Makefile index cf0422a..2752d19 100644 --- a/userland/lib/libtest/Makefile +++ b/userland/lib/libtest/Makefile @@ -5,7 +5,7 @@ TOP=../../.. .include "$(TOP)/mk/os161.config.mk" -SRCS=triple.c quint.c +SRCS=triple.c LIB=test .include "$(TOP)/mk/os161.lib.mk" diff --git a/userland/testbin/Makefile b/userland/testbin/Makefile index e5cf502..647f229 100644 --- a/userland/testbin/Makefile +++ b/userland/testbin/Makefile @@ -7,10 +7,10 @@ TOP=../.. SUBDIRS=add argtest badcall bigexec bigfile bigfork bigseek bloat conman \ crash ctest dirconc dirseek dirtest f_test factorial farm faulter \ - filetest forkbomb forktest frack guzzle hash hog huge kitchen \ + filetest forkbomb forktest frack hash hog huge \ malloctest matmult multiexec palin parallelvm poisondisk psort \ - quinthuge quintmat quintsort randcall redirect rmdirtest rmtest \ - sbrktest schedpong sink sort sparsefile sty tail tictac triplehuge \ + randcall redirect rmdirtest rmtest \ + sbrktest schedpong sort sparsefile tail tictac triplehuge \ triplemat triplesort usemtest zero # But not: diff --git a/userland/testbin/badcall/bad_open.c b/userland/testbin/badcall/bad_open.c index 5ce66cd..3d317ed 100644 --- a/userland/testbin/badcall/bad_open.c +++ b/userland/testbin/badcall/bad_open.c @@ -62,7 +62,7 @@ open_empty(void) report_begin("open empty string"); rv = open("", O_RDONLY); - report_check2(rv, errno, 0, EINVAL); + report_check(rv, errno, EINVAL); if (rv>=0) { close(rv); } diff --git a/userland/testbin/badcall/bad_pipe.c b/userland/testbin/badcall/bad_pipe.c index 350c3d8..172579c 100644 --- a/userland/testbin/badcall/bad_pipe.c +++ b/userland/testbin/badcall/bad_pipe.c @@ -68,6 +68,11 @@ pipe_unaligned(void) rv = pipe((int *)ptr); report_survival(rv, errno); + if (rv == 0) { + memmove(fds, ptr, 2*sizeof(int)); + close(fds[0]); + close(fds[1]); + } } void diff --git a/userland/testbin/filetest/filetest.c b/userland/testbin/filetest/filetest.c index ae93c38..dc062ec 100644 --- a/userland/testbin/filetest/filetest.c +++ b/userland/testbin/filetest/filetest.c @@ -46,7 +46,7 @@ int main(int argc, char *argv[]) { - static char writebuf[40] = "Twiddle dee dee, Twiddle dum dum.......\n"; + static char writebuf[41] = "Twiddle dee dee, Twiddle dum dum.......\n"; static char readbuf[41]; const char *file; diff --git a/userland/testbin/forktest/forktest.c b/userland/testbin/forktest/forktest.c index 4f7fed3..46e5da2 100644 --- a/userland/testbin/forktest/forktest.c +++ b/userland/testbin/forktest/forktest.c @@ -133,25 +133,52 @@ void test(int nowait) { int pid0, pid1, pid2, pid3; + int depth = 0; /* * Caution: This generates processes geometrically. * * It is unrolled to encourage gcc to registerize the pids, * to prevent wait/exit problems if fork corrupts memory. + * + * Note: if the depth prints trigger and show that the depth + * is too small, the most likely explanation is that the fork + * child is returning from the write() inside putchar() + * instead of from fork() and thus skipping the depth++. This + * is a fairly common problem caused by races in the kernel + * fork code. */ pid0 = dofork(); + depth++; putchar('A'); + if (depth != 1) { + warnx("depth %d, should be 1", depth); + } check(); + pid1 = dofork(); + depth++; putchar('B'); + if (depth != 2) { + warnx("depth %d, should be 2", depth); + } check(); + pid2 = dofork(); + depth++; putchar('C'); + if (depth != 3) { + warnx("depth %d, should be 3", depth); + } check(); + pid3 = dofork(); + depth++; putchar('D'); + if (depth != 4) { + warnx("depth %d, should be 4", depth); + } check(); /* diff --git a/userland/testbin/hog/hog.c b/userland/testbin/hog/hog.c index 7b69f15..9c6e2b5 100644 --- a/userland/testbin/hog/hog.c +++ b/userland/testbin/hog/hog.c @@ -31,7 +31,7 @@ * hog.c * Spawned by several other user programs to test time-slicing. * - * This does not differ from guzzle in any important way. + * Loops consuming CPU cycles. */ int diff --git a/userland/testbin/multiexec/multiexec.c b/userland/testbin/multiexec/multiexec.c index a7d41c9..b6f157d 100644 --- a/userland/testbin/multiexec/multiexec.c +++ b/userland/testbin/multiexec/multiexec.c @@ -134,6 +134,7 @@ semP(struct usem *sem, size_t num) if (read(sem->fd, c, num) < 0) { err(1, "%s: read", sem->name); } + (void)c; } static diff --git a/userland/testbin/parallelvm/parallelvm.c b/userland/testbin/parallelvm/parallelvm.c index a213197..953764a 100644 --- a/userland/testbin/parallelvm/parallelvm.c +++ b/userland/testbin/parallelvm/parallelvm.c @@ -283,16 +283,24 @@ static void semP(struct usem *sem, size_t num) { - if (read(sem->fd, NULL, num) < 0) { + char c[num]; + + if (read(sem->fd, c, num) < 0) { err(1, "%s: read", sem->name); } + (void)c; } static void semV(struct usem *sem, size_t num) { - if (write(sem->fd, NULL, num) < 0) { + char c[num]; + + /* semfs does not use these values, but be conservative */ + memset(c, 0, num); + + if (write(sem->fd, c, num) < 0) { err(1, "%s: write", sem->name); } } @@ -336,7 +344,12 @@ makeprocs(bool dowait) for (i=0; i #include +#include #include #include @@ -84,9 +85,9 @@ void Pn(struct usem *sem, unsigned count) { ssize_t r; - char c; + char c[count]; - r = read(sem->fd, &c, count); + r = read(sem->fd, c, count); if (r < 0) { err(1, "%s: read", sem->name); } @@ -105,9 +106,12 @@ void Vn(struct usem *sem, unsigned count) { ssize_t r; - char c; + char c[count]; - r = write(sem->fd, &c, count); + /* semfs does not use these values, but be conservative */ + memset(c, 0, count); + + r = write(sem->fd, c, count); if (r < 0) { err(1, "%s: write", sem->name); } From a0ecc1e7e5c739b173e535f5aa36fa604425f9ac Mon Sep 17 00:00:00 2001 From: Geoffrey Challen Date: Thu, 9 Feb 2017 09:52:18 -0500 Subject: [PATCH 09/11] Changes to use deadlock detection. --- kern/thread/hangman.c | 182 +++++++++++++++++++++++++++++++++++++++++ kern/thread/spinlock.c | 8 ++ kern/thread/synch.c | 11 +++ kern/thread/thread.c | 3 + 4 files changed, 204 insertions(+) create mode 100644 kern/thread/hangman.c diff --git a/kern/thread/hangman.c b/kern/thread/hangman.c new file mode 100644 index 0000000..127167a --- /dev/null +++ b/kern/thread/hangman.c @@ -0,0 +1,182 @@ +/* + * Copyright (c) 2015 + * The President and Fellows of Harvard College. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * 3. Neither the name of the University nor the names of its contributors + * may be used to endorse or promote products derived from this software + * without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE UNIVERSITY AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE UNIVERSITY OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +/* + * Simple deadlock detector. + */ + +#include +#include +#include +#include +#include + +static struct spinlock hangman_lock = SPINLOCK_INITIALIZER; + +/* + * Look for a path through the waits-for graph that goes from START to + * TARGET. + * + * Because lockables can only be held by one actor, and actors can + * only be waiting for one thing at a time, this turns out to be + * quite simple. + */ +static +void +hangman_check(const struct hangman_lockable *start, + const struct hangman_actor *target) +{ + const struct hangman_actor *cur; + + cur = start->l_holding; + while (cur != NULL) { + if (cur == target) { + goto found; + } + if (cur->a_waiting == NULL) { + break; + } + cur = cur->a_waiting->l_holding; + } + return; + + found: + /* + * None of this can change while we print it (that's the point + * of it being a deadlock) so drop hangman_lock while + * printing; otherwise we can come back via kprintf_spinlock + * and that makes a mess. But force splhigh() explicitly so + * the console prints in polled mode and to discourage other + * things from running in the middle of the printout. + */ + splhigh(); + spinlock_release(&hangman_lock); + + kprintf("hangman: Detected lock cycle!\n"); + kprintf("hangman: in %s (%p);\n", target->a_name, target); + kprintf("hangman: waiting for %s (%p), but:\n", start->l_name, start); + kprintf(" lockable %s (%p)\n", start->l_name, start); + cur = start->l_holding; + while (cur != target) { + kprintf(" held by actor %s (%p)\n", cur->a_name, cur); + kprintf(" waiting for lockable %s (%p)\n", + cur->a_waiting->l_name, cur->a_waiting); + cur = cur->a_waiting->l_holding; + } + kprintf(" held by actor %s (%p)\n", cur->a_name, cur); + panic("Deadlock.\n"); +} + +/* + * Note that a is about to wait for l. + * + * Note that there's no point calling this if a isn't going to wait, + * because in that case l->l_holding will be null and the check + * won't go anywhere. + * + * One could also maintain in memory a graph of all requests ever + * seen, in order to detect lock order inversions that haven't + * actually deadlocked; but there are a lot of ways in which this is + * tricky and problematic. For now we'll settle for just detecting and + * reporting deadlocks that do happen. + */ +void +hangman_wait(struct hangman_actor *a, + struct hangman_lockable *l) +{ + if (l == &hangman_lock.splk_hangman) { + /* don't recurse */ + return; + } + + spinlock_acquire(&hangman_lock); + + if (a->a_waiting != NULL) { + spinlock_release(&hangman_lock); + panic("hangman_wait: already waiting for something?\n"); + } + + hangman_check(l, a); + a->a_waiting = l; + + spinlock_release(&hangman_lock); +} + +void +hangman_acquire(struct hangman_actor *a, + struct hangman_lockable *l) +{ + if (l == &hangman_lock.splk_hangman) { + /* don't recurse */ + return; + } + + spinlock_acquire(&hangman_lock); + + if (a->a_waiting != l) { + spinlock_release(&hangman_lock); + panic("hangman_acquire: not waiting for lock %s (%p)\n", + l->l_name, l); + } + if (l->l_holding != NULL) { + spinlock_release(&hangman_lock); + panic("hangman_acquire: lock %s (%p) still held by %s (%p)\n", + l->l_name, l, a->a_name, a); + } + + l->l_holding = a; + a->a_waiting = NULL; + + spinlock_release(&hangman_lock); +} + +void +hangman_release(struct hangman_actor *a, + struct hangman_lockable *l) +{ + if (l == &hangman_lock.splk_hangman) { + /* don't recurse */ + return; + } + + spinlock_acquire(&hangman_lock); + + if (a->a_waiting != NULL) { + spinlock_release(&hangman_lock); + panic("hangman_release: waiting for something?\n"); + } + if (l->l_holding != a) { + spinlock_release(&hangman_lock); + panic("hangman_release: not the holder\n"); + } + + l->l_holding = NULL; + + spinlock_release(&hangman_lock); +} diff --git a/kern/thread/spinlock.c b/kern/thread/spinlock.c index a7b50b1..0cdf9aa 100644 --- a/kern/thread/spinlock.c +++ b/kern/thread/spinlock.c @@ -52,6 +52,7 @@ spinlock_init(struct spinlock *splk) { spinlock_data_set(&splk->splk_lock, 0); splk->splk_holder = NULL; + HANGMAN_LOCKABLEINIT(&splk->splk_hangman, "spinlock"); } /* @@ -85,6 +86,8 @@ spinlock_acquire(struct spinlock *splk) panic("Deadlock on spinlock %p\n", splk); } mycpu->c_spinlocks++; + + HANGMAN_WAIT(&curcpu->c_hangman, &splk->splk_hangman); } else { mycpu = NULL; @@ -112,6 +115,10 @@ spinlock_acquire(struct spinlock *splk) membar_store_any(); splk->splk_holder = mycpu; + + if (CURCPU_EXISTS()) { + HANGMAN_ACQUIRE(&curcpu->c_hangman, &splk->splk_hangman); + } } /* @@ -125,6 +132,7 @@ spinlock_release(struct spinlock *splk) KASSERT(splk->splk_holder == curcpu->c_self); KASSERT(curcpu->c_spinlocks > 0); curcpu->c_spinlocks--; + HANGMAN_RELEASE(&curcpu->c_hangman, &splk->splk_hangman); } splk->splk_holder = NULL; diff --git a/kern/thread/synch.c b/kern/thread/synch.c index d083f4b..ad58934 100644 --- a/kern/thread/synch.c +++ b/kern/thread/synch.c @@ -154,6 +154,8 @@ lock_create(const char *name) return NULL; } + HANGMAN_LOCKABLEINIT(&lock->lk_hangman, lock->lk_name); + // add stuff here as needed return lock; @@ -173,14 +175,23 @@ lock_destroy(struct lock *lock) void lock_acquire(struct lock *lock) { + /* Call this (atomically) before waiting for a lock */ + //HANGMAN_WAIT(&curthread->t_hangman, &lock->lk_hangman); + // Write this (void)lock; // suppress warning until code gets written + + /* Call this (atomically) once the lock is acquired */ + //HANGMAN_ACQUIRE(&curthread->t_hangman, &lock->lk_hangman); } void lock_release(struct lock *lock) { + /* Call this (atomically) when the lock is released */ + //HANGMAN_RELEASE(&curthread->t_hangman, &lock->lk_hangman); + // Write this (void)lock; // suppress warning until code gets written diff --git a/kern/thread/thread.c b/kern/thread/thread.c index a41dc58..e6fe983 100644 --- a/kern/thread/thread.c +++ b/kern/thread/thread.c @@ -140,6 +140,7 @@ thread_create(const char *name) thread->t_context = NULL; thread->t_cpu = NULL; thread->t_proc = NULL; + HANGMAN_ACTORINIT(&thread->t_hangman, thread->t_name); /* Interrupt state fields */ thread->t_in_interrupt = false; @@ -239,6 +240,8 @@ cpu_create(unsigned hardware_number) curcpu->c_curthread = curthread; } + HANGMAN_ACTORINIT(&c->c_hangman, "cpu"); + result = proc_addthread(kproc, c->c_curthread); if (result) { panic("cpu_create: proc_addthread:: %s\n", strerror(result)); From 0f0c5fcfc7acb1cf1e77d7d0fb5cc6e57a7a0242 Mon Sep 17 00:00:00 2001 From: Geoffrey Challen Date: Thu, 9 Feb 2017 09:52:39 -0500 Subject: [PATCH 10/11] New GDB script. --- kern/gdbscripts/mips-userland | 40 +++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 kern/gdbscripts/mips-userland diff --git a/kern/gdbscripts/mips-userland b/kern/gdbscripts/mips-userland new file mode 100644 index 0000000..0485261 --- /dev/null +++ b/kern/gdbscripts/mips-userland @@ -0,0 +1,40 @@ +# commands to interact with userland + +define load-userprogram + init-if-undefined $userprog_loaded = 0 + if !$userprog_loaded || !$_streq($arg0, $cur_userprog) + add-symbol-file $arg0 0x4000b0 + # remove after adding the new file in case add-symbol-file fails + if $userprog_loaded + remove-symbol-file -a 0x4000b0 + end + set $userprog_loaded = 1 + set $cur_userprog = $arg0 + end +end +document load-userprogram +Loads a single user program into gdb's symbol table, removing +the previous user program loaded if one exists. +Usage: load-userprogram +end +alias -a lu = load-userprogram + +define auto-userprogram + set $pname = curthread->t_proc->p_name + if $pname != 0 && $pname[0] != 0 && !$_streq($pname, "[kernel]") + if $pname[0] == '/' + set $pname = $pname + 1 + end + # This dumbness works around the fact that gdb doesn't + # expand convenience variables when you execute commands. + # And setting the third parameter to True silences the command. + python gdb.execute("load-userprogram " + str(gdb.parse_and_eval("$pname")).split()[1], False, True) + end +end +document auto-userprogram +Calls load-userprogram on whatever is curproc->p_name. It is meant +to work with kernels that set p_name to an absolute path to the +program, and it is designed to be called from hook-stop. +Usage: auto-userprogram +end + From 15abe49f09f426a8926134c57020ca1f630da31f Mon Sep 17 00:00:00 2001 From: Geoffrey Challen Date: Thu, 9 Feb 2017 09:53:00 -0500 Subject: [PATCH 11/11] Minor kernel changes. --- kern/vfs/vfslist.c | 20 ++++++++++++++++++++ kern/vfs/vfslookup.c | 7 +++++++ kern/vm/kmalloc.c | 4 ++++ 3 files changed, 31 insertions(+) diff --git a/kern/vfs/vfslist.c b/kern/vfs/vfslist.c index 9cd2f4a..64f6a9d 100644 --- a/kern/vfs/vfslist.c +++ b/kern/vfs/vfslist.c @@ -129,6 +129,23 @@ vfs_biglock_acquire(void) if (!lock_do_i_hold(vfs_biglock)) { lock_acquire(vfs_biglock); } + else if (vfs_biglock_depth == 0) { + /* + * Supposedly we hold it, but the depth is 0. This may + * mean: (1) the count is messed up, or (2) + * lock_do_i_hold is lying. Since OS/161 ships out of + * the box with unimplemented locks (students + * implement them) that always return true, assume + * situation (2). In this case acquire the lock + * anyway. + * + * Once you have working locks, this won't be the + * case, and if you get here it should be situation + * (1), in which case the count is messed up and one + * can panic. + */ + lock_acquire(vfs_biglock); + } vfs_biglock_depth++; } @@ -381,6 +398,9 @@ vfs_doadd(const char *dname, int mountable, struct device *dev, struct fs *fs) unsigned index; int result; + /* Silence warning with gcc 4.8 -Og (but not -O2) */ + index = 0; + vfs_biglock_acquire(); name = kstrdup(dname); diff --git a/kern/vfs/vfslookup.c b/kern/vfs/vfslookup.c index 773cbcd..afeacea 100644 --- a/kern/vfs/vfslookup.c +++ b/kern/vfs/vfslookup.c @@ -135,6 +135,13 @@ getdevice(char *path, char **subpath, struct vnode **startvn) KASSERT(vfs_biglock_do_i_hold()); + /* + * Entirely empty filenames aren't legal. + */ + if (path[0] == 0) { + return EINVAL; + } + /* * Locate the first colon or slash. */ diff --git a/kern/vm/kmalloc.c b/kern/vm/kmalloc.c index 453b3cc..b8a1204 100644 --- a/kern/vm/kmalloc.c +++ b/kern/vm/kmalloc.c @@ -1065,6 +1065,10 @@ subpage_kfree(void *ptr) checksubpages(); + /* Silence warnings with gcc 4.8 -Og (but not -O2) */ + prpage = 0; + blktype = 0; + for (pr = allbase; pr; pr = pr->next_all) { prpage = PR_PAGEADDR(pr); blktype = PR_BLOCKTYPE(pr);