[DTrace-devel] [PATCH 1/2] Omit an aggregation record if [u][sym|mod] translation fails

[eugene.loh at oracle.com]

From: Eugene Loh <eugene.loh at oracle.com>

An aggregation key can be a sym(), mod(), usym(), or umod()
translation of an address.  It is passed from producer to user
space as an address, and the consumer must translate the address.
It is possible for the translation to fail.

Omit a record if the translation fails.  This addresses failures
seen in
test/unittest/profile-n/tst.ufunc.sh
test/unittest/profile-n/tst.usym.sh
The problem was that the kernel's aggregation buffers are snapshot
multiple times.  If a translation ever fails, the raw address is
used instead.  Later on, when the aggregation is printed, if the
translation is successful, the raw key will report a count of 0.

Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
---
 libdtrace/dt_aggregate.c | 51 ++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 15 deletions(-)

diff --git a/libdtrace/dt_aggregate.c b/libdtrace/dt_aggregate.c
index 86f9d4d5b..40c1ae44f 100644
--- a/libdtrace/dt_aggregate.c
+++ b/libdtrace/dt_aggregate.c
@@ -317,61 +317,76 @@ dt_aggregate_quantizedcmp(int64_t *lhs, int64_t *rhs)
 	return 0;
 }
 
-static void
+static int
 dt_aggregate_usym(dtrace_hdl_t *dtp, uint64_t *data)
 {
 	uint64_t tgid = data[0];
 	uint64_t *pc = &data[1];
 	pid_t pid;
 	GElf_Sym sym;
+	int rc = 0;
 
 	if (dtp->dt_vector != NULL)
-		return;
+		return -1;
 
 	pid = dt_proc_grab_lock(dtp, tgid, DTRACE_PROC_WAITING |
 	    DTRACE_PROC_SHORTLIVED);
 	if (pid < 0)
-		return;
+		return -1;
 
 	if (dt_Plookup_by_addr(dtp, pid, *pc, NULL, &sym) == 0)
 		*pc = sym.st_value;
+	else
+		rc = -1;
 
 	dt_proc_release_unlock(dtp, pid);
+
+	return rc;
 }
 
-static void
+static int
 dt_aggregate_umod(dtrace_hdl_t *dtp, uint64_t *data)
 {
 	uint64_t tgid = data[0];
 	uint64_t *pc = &data[1];
 	pid_t pid;
 	const prmap_t *map;
+	int rc = 0;
 
 	if (dtp->dt_vector != NULL)
-		return;
+		return -1;
 
 	pid = dt_proc_grab_lock(dtp, tgid, DTRACE_PROC_WAITING |
 	    DTRACE_PROC_SHORTLIVED);
 	if (pid < 0)
-		return;
+		return -1;
 
 	if ((map = dt_Paddr_to_map(dtp, pid, *pc)) != NULL)
 		*pc = map->pr_vaddr;
+	else
+		rc = -1;
 
 	dt_proc_release_unlock(dtp, pid);
+
+	return rc;
 }
 
-static void
+static int
 dt_aggregate_sym(dtrace_hdl_t *dtp, uint64_t *data)
 {
 	GElf_Sym sym;
 	uint64_t *pc = data;
+	int rc = 0;
 
 	if (dtrace_lookup_by_addr(dtp, *pc, &sym, NULL) == 0)
 		*pc = sym.st_value;
+	else
+		rc = -1;
+
+	return rc;
 }
 
-static void
+static int
 dt_aggregate_mod(dtrace_hdl_t *dtp, uint64_t *addr)
 {
 	dt_module_t *dmp;
@@ -385,7 +400,7 @@ dt_aggregate_mod(dtrace_hdl_t *dtp, uint64_t *addr)
 		 * appear more than once in aggregation output).  It seems
 		 * unlikely that anyone will ever notice or care...
 		 */
-		return;
+		return -1;
 	}
 
 	for (dmp = dt_list_next(&dtp->dt_modlist); dmp != NULL;
@@ -400,7 +415,7 @@ dt_aggregate_mod(dtrace_hdl_t *dtp, uint64_t *addr)
 			    dtrace_addr_range_cmp) != NULL) {
 
 			*addr = dmp->dm_text_addrs[0].dar_va;
-			return;
+			return 0;
 		}
 
 		if (dmp->dm_data_addrs != NULL &&
@@ -413,9 +428,11 @@ dt_aggregate_mod(dtrace_hdl_t *dtp, uint64_t *addr)
 			else
 				*addr = dmp->dm_data_addrs[0].dar_va;
 
-			return;
+			return 0;
 		}
 	}
+
+	return -1;
 }
 
 static dtrace_aggid_t
@@ -606,16 +623,20 @@ dt_aggregate_snap_one(dtrace_hdl_t *dtp, int aggid, int cpu, const char *key,
 
 		switch(agg->dtagd_krecs[i].dtrd_action) {
 		case DTRACEACT_USYM:
-			dt_aggregate_usym(dtp, p);
+			if (dt_aggregate_usym(dtp, p) == -1)
+				return 0;
 			break;
 		case DTRACEACT_UMOD:
-			dt_aggregate_umod(dtp, p);
+			if (dt_aggregate_umod(dtp, p) == -1)
+				return 0;
 			break;
 		case DTRACEACT_SYM:
-			dt_aggregate_sym(dtp, p);
+			if (dt_aggregate_sym(dtp, p) == -1)
+				return 0;
 			break;
 		case DTRACEACT_MOD:
-			dt_aggregate_mod(dtp, p);
+			if (dt_aggregate_mod(dtp, p) == -1)
+				return 0;
 			break;
 		default:
 			break;
-- 
2.43.5