Change kcov semantics, kernel code coverage tracing is now enabled on a per

thread basis instead of process. The decision to enable on process made
development easier initially but could lead to non-deterministic results for
processes with more than one thread. This behavior matches the implementation
found on both Linux and FreeBSD.

With help and ok mpi@ visa@

diff --git a/sys/dev/kcov.c b/sys/dev/kcov.c
index 1406969..5f82b2a 100644 (file)
--- a/sys/dev/kcov.c
+++ b/sys/dev/kcov.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: kcov.c,v 1.2 2018/08/21 18:06:12 anton Exp $  */
+/*     $OpenBSD: kcov.c,v 1.3 2018/08/25 15:38:07 anton Exp $  */
 
 /*
  * Copyright (c) 2018 Anton Lindqvist <anton@openbsd.org>
@@ -39,9 +39,9 @@ struct kd {
                KCOV_MODE_DISABLED,
                KCOV_MODE_INIT,
                KCOV_MODE_TRACE_PC,
+               KCOV_MODE_DYING,
        }                kd_mode;
        int              kd_unit;       /* device minor */
-       pid_t            kd_pid;        /* process being traced */
        uintptr_t       *kd_buf;        /* traced coverage */
        size_t           kd_nmemb;
        size_t           kd_size;
@@ -52,9 +52,9 @@ struct kd {
 void kcovattach(int);
 
 int kd_alloc(struct kd *, unsigned long);
+void kd_free(struct kd *);
 struct kd *kd_lookup(int);
 
-static inline struct kd *kd_lookup_pid(pid_t);
 static inline int inintr(void);
 
 TAILQ_HEAD(, kd) kd_list = TAILQ_HEAD_INITIALIZER(kd_list);
@@ -69,8 +69,8 @@ int kcov_debug = 1;
  * each block instructions that maps to a single line in the original source
  * code.
  *
- * If kcov is enabled for the current process, the executed address will be
- * stored in the corresponding coverage buffer.
+ * If kcov is enabled for the current thread, the kernel program counter will
+ * be stored in its corresponding coverage buffer.
  * The first element in the coverage buffer holds the index of next available
  * element.
  */
@@ -89,8 +89,8 @@ __sanitizer_cov_trace_pc(void)
        if (inintr())
                return;
 
-       kd = kd_lookup_pid(curproc->p_p->ps_pid);
-       if (kd == NULL)
+       kd = curproc->p_kd;
+       if (kd == NULL || kd->kd_mode != KCOV_MODE_TRACE_PC)
                return;
 
        idx = kd->kd_buf[0];
@@ -132,9 +132,11 @@ kcovclose(dev_t dev, int flag, int mode, struct proc *p)
 
        DPRINTF("%s: unit=%d\n", __func__, minor(dev));
 
-       TAILQ_REMOVE(&kd_list, kd, kd_entry);
-       free(kd->kd_buf, M_SUBPROC, kd->kd_size);
-       free(kd, M_SUBPROC, sizeof(struct kd));
+       if (kd->kd_mode == KCOV_MODE_TRACE_PC)
+               kd->kd_mode = KCOV_MODE_DYING;
+       else
+               kd_free(kd);
+
        return (0);
 }
 
@@ -159,30 +161,30 @@ kcovioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct proc *p)
                        kd->kd_mode = KCOV_MODE_INIT;
                break;
        case KIOENABLE:
-               if (kd->kd_mode != KCOV_MODE_INIT) {
+               /* Only one kcov descriptor can be enabled per thread. */
+               if (p->p_kd != NULL || kd->kd_mode != KCOV_MODE_INIT) {
                        error = EBUSY;
                        break;
                }
                kd->kd_mode = KCOV_MODE_TRACE_PC;
-               kd->kd_pid = p->p_p->ps_pid;
+               p->p_kd = kd;
                break;
        case KIODISABLE:
-               /* Only the enabled process may disable itself. */
-               if (kd->kd_pid != p->p_p->ps_pid ||
-                   kd->kd_mode != KCOV_MODE_TRACE_PC) {
+               /* Only the enabled thread may disable itself. */
+               if (p->p_kd != kd || kd->kd_mode != KCOV_MODE_TRACE_PC) {
                        error = EBUSY;
                        break;
                }
                kd->kd_mode = KCOV_MODE_INIT;
-               kd->kd_pid = 0;
+               p->p_kd = NULL;
                break;
        default:
                error = EINVAL;
                DPRINTF("%s: %lu: unknown command\n", __func__, cmd);
        }
 
-       DPRINTF("%s: unit=%d, mode=%d, pid=%d, error=%d\n",
-                   __func__, kd->kd_unit, kd->kd_mode, kd->kd_pid, error);
+       DPRINTF("%s: unit=%d, mode=%d, error=%d\n",
+           __func__, kd->kd_unit, kd->kd_mode, error);
 
        return (error);
 }
@@ -212,12 +214,17 @@ kcov_exit(struct proc *p)
 {
        struct kd *kd;
 
-       kd = kd_lookup_pid(p->p_p->ps_pid);
+       kd = p->p_kd;
        if (kd == NULL)
                return;
 
-       kd->kd_mode = KCOV_MODE_INIT;
-       kd->kd_pid = 0;
+       DPRINTF("%s: unit=%d\n", __func__, kd->kd_unit);
+
+       if (kd->kd_mode == KCOV_MODE_DYING)
+               kd_free(kd);
+       else
+               kd->kd_mode = KCOV_MODE_INIT;
+       p->p_kd = NULL;
 }
 
 struct kd *
@@ -250,16 +257,14 @@ kd_alloc(struct kd *kd, unsigned long nmemb)
        return (0);
 }
 
-static inline struct kd *
-kd_lookup_pid(pid_t pid)
+void
+kd_free(struct kd *kd)
 {
-       struct kd *kd;
+       DPRINTF("%s: unit=%d mode=%d\n", __func__, kd->kd_unit, kd->kd_mode);
 
-       TAILQ_FOREACH(kd, &kd_list, kd_entry) {
-               if (kd->kd_pid == pid && kd->kd_mode == KCOV_MODE_TRACE_PC)
-                       return (kd);
-       }
-       return (NULL);
+       TAILQ_REMOVE(&kd_list, kd, kd_entry);
+       free(kd->kd_buf, M_SUBPROC, kd->kd_size);
+       free(kd, M_SUBPROC, sizeof(struct kd));
 }
 
 static inline int

diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c
index 0fff6ed..0ae129e 100644 (file)
--- a/sys/kern/kern_exit.c
+++ b/sys/kern/kern_exit.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: kern_exit.c,v 1.168 2018/08/21 18:06:12 anton Exp $   */
+/*     $OpenBSD: kern_exit.c,v 1.169 2018/08/25 15:38:07 anton Exp $   */
 /*     $NetBSD: kern_exit.c,v 1.39 1996/04/22 01:38:25 christos Exp $  */
 
 /*
@@ -181,6 +181,10 @@ exit1(struct proc *p, int rv, int flags)
        }
        p->p_siglist = 0;
 
+#if NKCOV > 0
+       kcov_exit(p);
+#endif
+
        if ((p->p_flag & P_THREAD) == 0) {
                /* close open files and release open-file table */
                fdfree(p);
@@ -194,10 +198,6 @@ exit1(struct proc *p, int rv, int flags)
                acct_process(p);
 #endif
 
-#if NKCOV > 0
-               kcov_exit(p);
-#endif
-
 #ifdef KTRACE
                /* release trace file */
                if (pr->ps_tracevp)

diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index 94ac3ea..c3bbe84 100644 (file)
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: kern_fork.c,v 1.205 2018/07/20 07:28:36 beck Exp $    */
+/*     $OpenBSD: kern_fork.c,v 1.206 2018/08/25 15:38:07 anton Exp $   */
 /*     $NetBSD: kern_fork.c,v 1.29 1996/02/09 18:59:34 christos Exp $  */
 
 /*
@@ -65,6 +65,8 @@
 #include <uvm/uvm.h>
 #include <machine/tcb.h>
 
+#include "kcov.h"
+
 int    nprocesses = 1;         /* process 0 */
 int    nthreads = 1;           /* proc 0 */
 int    randompid;              /* when set to 1, pid's go random */
@@ -178,6 +180,10 @@ thread_new(struct proc *parent, vaddr_t uaddr)
        p->p_sleeplocks = NULL;
 #endif
 
+#if NKCOV > 0
+       p->p_kd = NULL;
+#endif
+
        return p;
 }
 

diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 63aca57..924413f 100644 (file)
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: proc.h,v 1.256 2018/08/13 15:26:17 visa Exp $ */
+/*     $OpenBSD: proc.h,v 1.257 2018/08/25 15:38:07 anton Exp $        */
 /*     $NetBSD: proc.h,v 1.44 1996/04/22 01:23:21 christos Exp $       */
 
 /*-
@@ -288,6 +288,7 @@ struct process {
      "\024NOBROADCASTKILL" "\025PLEDGE" "\026WXNEEDED" "\027EXECPLEDGE" )
 
 
+struct kd;
 struct lock_list_entry;
 
 struct proc {
@@ -374,6 +375,8 @@ struct proc {
        u_short p_xstat;        /* Exit status for wait; also stop signal. */
 
        struct  lock_list_entry *p_sleeplocks;
+
+       struct  kd *p_kd;       /* kcov descriptor */
 };
 
 /* Status values. */