[PATCH 0/6] another batch of fixes

[PATCH 0/6] another batch of fixes

[Eric Wong]

Every bug I fix makes me feel dumber :<

Eric Wong (6):
  avoid leaking array returned by backtrace_symbols()
  mwrap_core: document the reason for padding the mutex
  wrap jhash calls to hash_loc
  fix under-allocated kbuf size
  mwrap_get: golf out redundant assignment across branches
  location_name: handle malloc failure on backtrace_symbols(3)

 Mwrap.xs     | 18 +++++++++++-------
 mwrap_core.h | 13 +++++++++----
 2 files changed, 20 insertions(+), 11 deletions(-)

[PATCH 1/6] avoid leaking array returned by backtrace_symbols()

[Eric Wong]

Oops :x  Fortunately, most allocations are tracked to a Perl
location and the non-Perl code path is infrequently hit.
---
 Mwrap.xs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Mwrap.xs b/Mwrap.xs
index d5336fa..8aeb425 100644
--- a/Mwrap.xs
+++ b/Mwrap.xs
@@ -20,8 +20,8 @@ static SV *location_string(struct src_loc *l)
 		char **s = backtrace_symbols((void *)l->k, 1);
 
 		ret = newSVpvn(s[0], strlen(s[0]));
-	}
-	else {
+		free(s);
+	} else {
 		ret = newSVpvn(l->k, l->capa - 1);
 	}
 

[PATCH 2/6] mwrap_core: document the reason for padding the mutex

[Eric Wong]

While 64 has an obvious meaning to me, I reject Moore's law
and have no idea what cache line sizes modern CPUs use.
---
 mwrap_core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mwrap_core.h b/mwrap_core.h
index 09b579d..94a6af3 100644
--- a/mwrap_core.h
+++ b/mwrap_core.h
@@ -68,7 +68,7 @@ static size_t *root_locating; /* determines if PL_curcop is our thread */
 static struct cds_lfht *totals;
 union padded_mutex {
 	pthread_mutex_t mtx;
-	char pad[64];
+	char pad[64]; /* cache alignment for common CPUs */
 };
 
 /* a round-robin pool of mutexes */

[PATCH 3/6] wrap jhash calls to hash_loc

[Eric Wong]

This will make it easier to keep a consistent hash seed
and/or switch to different hashes in the future.
---
 Mwrap.xs     | 4 ++--
 mwrap_core.h | 9 +++++++--
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/Mwrap.xs b/Mwrap.xs
index 8aeb425..a5ffeed 100644
--- a/Mwrap.xs
+++ b/Mwrap.xs
@@ -164,12 +164,12 @@ CODE:
 		k = (void *)kbuf;
 		memcpy(k->k, &p, sizeof(p));
 		k->capa = 0;
-		k->hval = jhash(k->k, sizeof(p), 0xdeadbeef);
+		hash_loc(k, sizeof(p));
 	} else {
 		k = (void *)kbuf;
 		memcpy(k->k, str, len + 1);
 		k->capa = len + 1;
-		k->hval = jhash(k->k, k->capa, 0xdeadbeef);
+		hash_loc(k, k->capa);
 	}
 
 	if (!k)
diff --git a/mwrap_core.h b/mwrap_core.h
index 94a6af3..d9396e6 100644
--- a/mwrap_core.h
+++ b/mwrap_core.h
@@ -272,6 +272,11 @@ static const COP *mwp_curcop(void)
 	return NULL;
 }
 
+static void hash_loc(struct src_loc *k, size_t len)
+{
+	k->hval = jhash(k->k, len, 0xdeadbeef);
+}
+
 static struct src_loc *assign_line(size_t size, const char *file, unsigned line)
 {
 	/* avoid vsnprintf or anything which could call malloc here: */
@@ -298,7 +303,7 @@ static struct src_loc *assign_line(size_t size, const char *file, unsigned line)
 	assert(dst && "bad math");
 	*dst = 0;	/* terminate string */
 	k->capa = (uint32_t)(dst - k->k + 1);
-	k->hval = jhash(k->k, k->capa, 0xdeadbeef);
+	hash_loc(k, k->capa);
 	return totals_add_rcu(k);
 }
 
@@ -325,7 +330,7 @@ update_stats_rcu_lock(size_t *generation, size_t size, uintptr_t caller)
 		k->total = size;
 		memcpy(k->k, &caller, xlen);
 		k->capa = 0;
-		k->hval = jhash(k->k, xlen, 0xdeadbeef);
+		hash_loc(k, xlen);
 		ret = totals_add_rcu(k);
 	}
 out:

[PATCH 4/6] fix under-allocated kbuf size

[Eric Wong]

kbuf is always `struct src_loc', not `struct alloc_hdr'.
Fortunately, nobody really needs PATH_MAX size for pathnames, so
this wasn't a problem in practice.
---
 mwrap_core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mwrap_core.h b/mwrap_core.h
index d9396e6..3abc3dd 100644
--- a/mwrap_core.h
+++ b/mwrap_core.h
@@ -179,7 +179,7 @@ struct alloc_hdr {
 
 /* $PATHNAME:$LINENO */
 static MWRAP_TSD char kbuf[
-	PATH_MAX + sizeof(":") + UINT2STR_MAX + sizeof(struct alloc_hdr)
+	sizeof(struct src_loc) + PATH_MAX + sizeof(":") + UINT2STR_MAX
 ];
 
 static struct alloc_hdr *ptr2hdr(void *p)

[PATCH 5/6] mwrap_get: golf out redundant assignment across branches

[Eric Wong]

Terminal space is precious with my font sizes.
---
 Mwrap.xs | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/Mwrap.xs b/Mwrap.xs
index a5ffeed..c8a4568 100644
--- a/Mwrap.xs
+++ b/Mwrap.xs
@@ -147,7 +147,7 @@ mwrap_get(loc)
 PREINIT:
 	STRLEN len;
 	const char *str;
-	struct src_loc *k = 0;
+	struct src_loc *k = (void *)kbuf;
 	uintptr_t p;
 	struct cds_lfht_iter iter;
 	struct cds_lfht_node *cur;
@@ -161,12 +161,10 @@ CODE:
 	if (len > PATH_MAX)
 		XSRETURN_UNDEF;
 	if (extract_addr(str, len, (void **)&p)) {
-		k = (void *)kbuf;
 		memcpy(k->k, &p, sizeof(p));
 		k->capa = 0;
 		hash_loc(k, sizeof(p));
 	} else {
-		k = (void *)kbuf;
 		memcpy(k->k, str, len + 1);
 		k->capa = len + 1;
 		hash_loc(k, k->capa);

[PATCH 6/6] location_name: handle malloc failure on backtrace_symbols(3)

[Eric Wong]

backtrace_symbols(3) may fail due to malloc failures, so we
must account for it and return `undef' in that case.
---
 Mwrap.xs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Mwrap.xs b/Mwrap.xs
index c8a4568..b515894 100644
--- a/Mwrap.xs
+++ b/Mwrap.xs
@@ -19,6 +19,12 @@ static SV *location_string(struct src_loc *l)
 	if (loc_is_addr(l)) {
 		char **s = backtrace_symbols((void *)l->k, 1);
 
+		if (!s) {
+			fprintf(stderr, "backtrace_symbols => NULL: %s\n",
+				strerror(errno));
+			return &PL_sv_undef;
+		}
+
 		ret = newSVpvn(s[0], strlen(s[0]));
 		free(s);
 	} else {