You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Cliff Woolley <jw...@virginia.edu> on 2003/08/13 05:20:13 UTC
[PATCH] bucket debugging.
Any objections to the following patch? All you do is pass
-DAPR_BUCKET_DEBUG to ./configure and then several things happen:
1) the brigades get checked for consistency at various strategic points
2) and apr_bucket_free() calls are checked for double-free conditions
(eg, you destroyed the same bucket twice, or destroyed one and then
kept on using it, which causes all kinds of nasty and impossible to
debug things to happen).
If you don't define APR_BUCKET_DEBUG, it's all a no-op and is thrown away
by the preprocessor.
--Cliff
Index: buckets/apr_brigade.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
retrieving revision 1.57
diff -u -d -r1.57 apr_brigade.c
--- buckets/apr_brigade.c 1 Jan 2003 00:02:17 -0000 1.57
+++ buckets/apr_brigade.c 13 Aug 2003 03:14:43 -0000
@@ -127,6 +127,10 @@
APR_RING_UNSPLICE(e, f, link);
APR_RING_SPLICE_HEAD(&a->list, e, f, apr_bucket, link);
}
+
+ APR_BRIGADE_CHECK_CONSISTENCY(a);
+ APR_BRIGADE_CHECK_CONSISTENCY(b);
+
return a;
}
@@ -147,6 +151,8 @@
*after_point = APR_BRIGADE_FIRST(b);
return APR_SUCCESS;
}
+
+ APR_BRIGADE_CHECK_CONSISTENCY(b);
APR_BRIGADE_FOREACH(e, b) {
if ((e->length == (apr_size_t)(-1)) && (point > (apr_size_t)(-1))) {
Index: buckets/apr_buckets_alloc.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_buckets_alloc.c,v
retrieving revision 1.11
diff -u -d -r1.11 apr_buckets_alloc.c
--- buckets/apr_buckets_alloc.c 2 Jul 2003 06:35:59 -0000 1.11
+++ buckets/apr_buckets_alloc.c 13 Aug 2003 03:14:43 -0000
@@ -151,12 +151,33 @@
return ((char *)node) + SIZEOF_NODE_HEADER_T;
}
+#ifdef APR_BUCKET_DEBUG
+#include <assert.h>
+static int not_already_free(node_header_t *node)
+{
+ apr_bucket_alloc_t *list = node->alloc;
+ node_header_t *curr = list->freelist;
+
+ while (curr) {
+ if (node == curr) {
+ return 0;
+ }
+ curr = curr->next;
+ }
+ return 1;
+}
+#define CHECK_NOT_ALREADY_FREE(node) assert(not_already_free(node))
+#else
+#define CHECK_NOT_ALREADY_FREE(node)
+#endif
+
APU_DECLARE_NONSTD(void) apr_bucket_free(void *mem)
{
node_header_t *node = (node_header_t *)((char *)mem - SIZEOF_NODE_HEADER_T);
apr_bucket_alloc_t *list = node->alloc;
if (node->size == SMALL_NODE_SIZE) {
+ CHECK_NOT_ALREADY_FREE(node);
node->next = list->freelist;
list->freelist = node;
}
Index: include/apr_buckets.h
===================================================================
RCS file: /home/cvs/apr-util/include/apr_buckets.h,v
retrieving revision 1.152
diff -u -d -r1.152 apr_buckets.h
--- include/apr_buckets.h 2 Jul 2003 06:35:59 -0000 1.152
+++ include/apr_buckets.h 13 Aug 2003 03:14:43 -0000
@@ -315,6 +315,38 @@
*/
typedef apr_status_t (*apr_brigade_flush)(apr_bucket_brigade *bb, void *ctx);
+/*
+ * define APR_BUCKET_DEBUG if you want your brigades to be checked for
+ * validity at every possible instant. this will slow your code down
+ * substantially but is a very useful debugging tool.
+ */
+#ifdef APR_BUCKET_DEBUG
+#define APR_RING_DEBUG
+
+#define APR_BRIGADE_CHECK_CONSISTENCY(b) \
+ APR_RING_CHECK_CONSISTENCY(&(b)->list, apr_bucket, link)
+
+#define APR_BUCKET_CHECK_CONSISTENCY(e) \
+ APR_RING_CHECK_ELEM_CONSISTENCY((e), apr_bucket, link)
+
+#else
+/**
+ * checks the ring pointers in a bucket brigade for consistency. an
+ * assertion failure will occur if any inconsistencies are found.
+ * note: this is a no-op unless APR_BUCKET_DEBUG is defined.
+ * @param b The brigade
+ */
+#define APR_BRIGADE_CHECK_CONSISTENCY(b)
+/**
+ * checks the brigade a bucket is in for ring consistency. an
+ * assertion failure will occur if any inconsistencies are found.
+ * note: this is a no-op unless APR_BUCKET_DEBUG is defined.
+ * @param e The bucket
+ */
+#define APR_BUCKET_CHECK_CONSISTENCY(e)
+#endif
+
+
/**
* Wrappers around the RING macros to reduce the verbosity of the code
* that handles bucket brigades.
@@ -405,6 +437,7 @@
#define APR_BRIGADE_INSERT_HEAD(b, e) do { \
apr_bucket *ap__b = (e); \
APR_RING_INSERT_HEAD(&(b)->list, ap__b, apr_bucket, link); \
+ APR_BRIGADE_CHECK_CONSISTENCY((b)); \
} while (0)
/**
@@ -415,6 +448,7 @@
#define APR_BRIGADE_INSERT_TAIL(b, e) do { \
apr_bucket *ap__b = (e); \
APR_RING_INSERT_TAIL(&(b)->list, ap__b, apr_bucket, link); \
+ APR_BRIGADE_CHECK_CONSISTENCY((b)); \
} while (0)
/**
@@ -422,16 +456,20 @@
* @param a The first brigade
* @param b The second brigade
*/
-#define APR_BRIGADE_CONCAT(a, b) \
- APR_RING_CONCAT(&(a)->list, &(b)->list, apr_bucket, link)
+#define APR_BRIGADE_CONCAT(a, b) do { \
+ APR_RING_CONCAT(&(a)->list, &(b)->list, apr_bucket, link); \
+ APR_BRIGADE_CHECK_CONSISTENCY((a)); \
+ } while (0)
/**
* Prepend brigade b onto the beginning of brigade a, leaving brigade b empty
* @param a The first brigade
* @param b The second brigade
*/
-#define APR_BRIGADE_PREPEND(a, b) \
- APR_RING_PREPEND(&(a)->list, &(b)->list, apr_bucket, link)
+#define APR_BRIGADE_PREPEND(a, b) do { \
+ APR_RING_PREPEND(&(a)->list, &(b)->list, apr_bucket, link); \
+ APR_BRIGADE_CHECK_CONSISTENCY((a)); \
+ } while (0)
/**
* Insert a list of buckets before a specified bucket
@@ -441,6 +479,7 @@
#define APR_BUCKET_INSERT_BEFORE(a, b) do { \
apr_bucket *ap__a = (a), *ap__b = (b); \
APR_RING_INSERT_BEFORE(ap__a, ap__b, link); \
+ APR_BUCKET_CHECK_CONSISTENCY(ap__a); \
} while (0)
/**
@@ -451,6 +490,7 @@
#define APR_BUCKET_INSERT_AFTER(a, b) do { \
apr_bucket *ap__a = (a), *ap__b = (b); \
APR_RING_INSERT_AFTER(ap__a, ap__b, link); \
+ APR_BUCKET_CHECK_CONSISTENCY(ap__a); \
} while (0)
/**
Re: [PATCH] bucket debugging.
Posted by Cliff Woolley <jw...@virginia.edu>.
On Wed, 13 Aug 2003, Jeff Trawick wrote:
> Cliff Woolley wrote:
> > Any objections to the following patch? All you do is pass
> > -DAPR_BUCKET_DEBUG to ./configure and then several things happen:
>
> is --enable-bucket-debug preferable? a couple of positive aspects of that:
>
> a) natural place for help text (apr-util/configure --help)
> b) after introducing configure logic, it becomes natural for the
> compile-time setting to go in a header file instead of making the
> compiler command lines even longer
So it turns out this is kind of a pain because CPPFLAGS comes directly
from APR's rules.mk, which is copied in from APR as-is. apr-util has
APRUTIL_INCLUDES, APRUTIL_LDFLAGS, etc, but no APRUTIL_CPPFLAGS. And I
don't see a particularly clean way to add an APRUTIL_CPPFLAGS. I could
just stick the -DAPR_BUCKET_DEBUG in APRUTIL_INCLUDES, which works, but is
a horrible hack. Any ideas?
--Cliff
RE: [PATCH] bucket debugging.
Posted by Cliff Woolley <jw...@virginia.edu>.
On Wed, 13 Aug 2003, Sander Striker wrote:
> > compiler command lines even longer
> >
> > more work though :)
>
> You can probably reuse what I did with --enable-pool-debug in APR.
Aww crap, now I gotta figure out autoconf? :-) Sure, that sounds
like a reasonable request.
--Cliff
RE: [PATCH] bucket debugging.
Posted by Sander Striker <st...@apache.org>.
> From: Jeff Trawick [mailto:trawick@attglobal.net]
> Sent: Wednesday, August 13, 2003 12:39 PM
> Cliff Woolley wrote:
> > Any objections to the following patch? All you do is pass
> > -DAPR_BUCKET_DEBUG to ./configure and then several things happen:
>
> is --enable-bucket-debug preferable? a couple of positive aspects of that:
+1.
> a) natural place for help text (apr-util/configure --help)
> b) after introducing configure logic, it becomes natural for the
> compile-time setting to go in a header file instead of making the
> compiler command lines even longer
>
> more work though :)
You can probably reuse what I did with --enable-pool-debug in APR.
Sander
Re: [PATCH] bucket debugging.
Posted by Jeff Trawick <tr...@attglobal.net>.
Cliff Woolley wrote:
> Any objections to the following patch? All you do is pass
> -DAPR_BUCKET_DEBUG to ./configure and then several things happen:
is --enable-bucket-debug preferable? a couple of positive aspects of that:
a) natural place for help text (apr-util/configure --help)
b) after introducing configure logic, it becomes natural for the
compile-time setting to go in a header file instead of making the
compiler command lines even longer
more work though :)
Re: [PATCH] bucket debugging.
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Aug 13, 2003 at 02:08:57AM -0400, Cliff Woolley wrote:
> On Tue, 12 Aug 2003, Greg Stein wrote:
>
> > > + while (curr) {
> > > + if (node == curr) {
> > > + return 0;
> > > + }
> > > + curr = curr->next;
> > > + }
> > > + return 1;
> >
> > Forget the return code. Just stick an abort() in here.
>
> return 0 to pop the assert, abort, same difference. :) But sure, I'll do
> it that way, why not.
Sure, six of one, half-dozen of another. But the way I saw it was that the
return code was superfluous since the only caller was your macro. Thus, you
can just shift all the [abort] logic into the function rather than split
between the func and the macro.
But in any case, having a consistency check on those buggers is Goodness.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] bucket debugging.
Posted by Cliff Woolley <jw...@virginia.edu>.
On Tue, 12 Aug 2003, Greg Stein wrote:
> > + while (curr) {
> > + if (node == curr) {
> > + return 0;
> > + }
> > + curr = curr->next;
> > + }
> > + return 1;
>
> Forget the return code. Just stick an abort() in here.
return 0 to pop the assert, abort, same difference. :) But sure, I'll do
it that way, why not.
Re: [PATCH] bucket debugging.
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Aug 12, 2003 at 11:20:13PM -0400, Cliff Woolley wrote:
>
> Any objections to the following patch? All you do is pass
> -DAPR_BUCKET_DEBUG to ./configure and then several things happen:
>
> 1) the brigades get checked for consistency at various strategic points
> 2) and apr_bucket_free() calls are checked for double-free conditions
> (eg, you destroyed the same bucket twice, or destroyed one and then
> kept on using it, which causes all kinds of nasty and impossible to
> debug things to happen).
>
> If you don't define APR_BUCKET_DEBUG, it's all a no-op and is thrown away
> by the preprocessor.
+1 (in concept)
>...
> +++ buckets/apr_buckets_alloc.c 13 Aug 2003 03:14:43 -0000
> @@ -151,12 +151,33 @@
> return ((char *)node) + SIZEOF_NODE_HEADER_T;
> }
>
> +#ifdef APR_BUCKET_DEBUG
> +#include <assert.h>
> +static int not_already_free(node_header_t *node)
> +{
> + apr_bucket_alloc_t *list = node->alloc;
> + node_header_t *curr = list->freelist;
> +
> + while (curr) {
> + if (node == curr) {
> + return 0;
> + }
> + curr = curr->next;
> + }
> + return 1;
Forget the return code. Just stick an abort() in here.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] bucket debugging.
Posted by Brian Pane <br...@cnet.com>.
On Tue, 2003-08-12 at 20:20, Cliff Woolley wrote:
> Any objections to the following patch? All you do is pass
> -DAPR_BUCKET_DEBUG to ./configure and then several things happen:
>
> 1) the brigades get checked for consistency at various strategic points
> 2) and apr_bucket_free() calls are checked for double-free conditions
> (eg, you destroyed the same bucket twice, or destroyed one and then
> kept on using it, which causes all kinds of nasty and impossible to
> debug things to happen).
>
> If you don't define APR_BUCKET_DEBUG, it's all a no-op and is thrown away
> by the preprocessor.
+1
Brian