You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Sander Striker <st...@samba-tng.org> on 2001/05/20 15:09:33 UTC
[PATCH] APR Stackable Memory System
Hi,
David is gone for a week, but I already wrote some stuff
that might be worth reviewing/commiting.
Attached are a few patches:
sms-bugfix.patch
Fixes a bug in apr_sms_free() that snuck in.
sms-style.patch.1
More general cleanup so the code is more conformant
to the coding style guidelines as found on:
http://dev.apache.org:/styleguide.html.
sms-abort.patch.2
Adds an abort_fn member to the apr_sms_t. This
includes code calling the abort_fn, if present, on
APR_ENOMEM. The code to set the abort function is
not present yet. I added this to get sms one step
closer to apr pools.
sms-create.patch.3
Cleans up apr_sms_create() [only to be called by
memory system implementors]. It now returns a status
code which will be needed when adding lock creation
and other things that could fail in this function.
Thusfar the patches. I have been thinking about making
the code thread safe, but this will need some thought.
As someone pointed out earlier, when only one thread
is accessing the code locking is useless overhead.
Then there is the problem that apr_lock_create() takes
a pool (which we don't have).
And finally, when do we lock?
In case of the sms framework, we only need to lock when
modifying/reading the child memory system lists and the
cleanup function lists.
In case of the tracking sms we need to lock on malloc/free/
etc. I suppose this is specific to every sms and should be
implemented using a seperate lock.
Sander
Re: [PATCH] APR Stackable Memory System
Posted by Cliff Woolley <cl...@yahoo.com>.
On Sun, 20 May 2001, Sander Striker wrote:
> David is gone for a week, but I already wrote some stuff
> that might be worth reviewing/commiting.
>
> Attached are a few patches:
I'll take a look at these later today and commit if nobody beats me to it.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
RE: [PATCH] APR Stackable Memory System
Posted by Greg Marr <gr...@alum.wpi.edu>.
At 12:16 PM 05/21/2001, Cliff Woolley wrote:
>On Mon, 21 May 2001, Sander Striker wrote:
> > dreid> Why not stick to the standard apr format of
> > dreid>
> > dreid> apr_status_t abort_fn(char *sourcefile, int lineno);
> >
> > striker> Ahh, there is such a function? I need to do a lot of
> digging into
> > APR.
>
>Hmm... yeah, that's a point. Then again, if the abort_fn is
>basically just abort(), then it's pretty easy to do a debugger
>backtrace and get all the sourcefile/lineno information for the
>entire call stack that way.
Assuming you have that debugging info available. If you've stripped
the executable, or didn't compile with any debug info, that won't
work terribly well.
> I really don't think we should be adding sourcefile/lineno
> arguments to ALL of the SMS functions...
If you do this, then for those memory systems where you do have to
call free, you can have a debug build where you track the filename
and line number of each allocation, and when exiting, report the ones
that were never freed. I'm not sure if this would be at all useful
in APR, though. Do any of the memory systems require explicit calls
to free, or is everything done through cleanups now?
--
Greg Marr
gregm@alum.wpi.edu
"We thought you were dead."
"I was, but I'm better now." - Sheridan, "The Summoning"
RE: [PATCH] APR Stackable Memory System
Posted by Cliff Woolley <cl...@yahoo.com>.
On Mon, 21 May 2001, Sander Striker wrote:
> dreid> Why not stick to the standard apr format of
> dreid>
> dreid> apr_status_t abort_fn(char *sourcefile, int lineno);
>
> striker> Ahh, there is such a function? I need to do a lot of digging into
> APR.
Hmm... yeah, that's a point. Then again, if the abort_fn is basically
just abort(), then it's pretty easy to do a debugger backtrace and get all
the sourcefile/lineno information for the entire call stack that way. I
really don't think we should be adding sourcefile/lineno arguments to ALL
of the SMS functions...
Well, whatever. Other opinions?
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
RE: [PATCH] APR Stackable Memory System
Posted by Sander Striker <st...@samba-tng.org>.
> I've committed the first two patches. While I was at it, I threw in a
> couple more styleguide cleanups that I spotted with the second commit.
Thx.
>> sms-abort.patch.2
>> Adds an abort_fn member to the apr_sms_t. This
>> includes code calling the abort_fn, if present, on
>> APR_ENOMEM. The code to set the abort function is
>> not present yet. I added this to get sms one step
>> closer to apr pools.
>
> I'm wondering on this one if the abort function should have a prototype
> more like the one used by pools (ie, it should take just a status code
> rather than filename/linenum).
Ah, ok, I asked David about that before he left. I hope he doesn't mind
me quoting snippets here from what he said:
"
dreid> Why not stick to the standard apr format of
dreid>
dreid> apr_status_t abort_fn(char *sourcefile, int lineno);
striker> Ahh, there is such a function? I need to do a lot of digging into
APR.
[...]
striker> Btw, in apr_pools.h there is this:
striker> typedef int (*apr_abortfunc_t)(int retcode);
dreid> I'll have to review the code further to give more optinions, but if
you
dreid> try to stick with the standard format it'll make things better
dreid> and also allows for stacking of layers with errors not being lost
:)
striker> Uhuh, agreed. I'll see what I can do. By the way, my intention was
striker> to reflect the offending caller of the apr_sms_malloc() call in
striker> sourcefile and lineno by doing something like:
striker>
striker> #define apr_sms_malloc(sms, size) \
striker> apr_sms_malloc(sms, size, __FILE__, __LINE__)
striker>
striker> And to modify the apr_sms_malloc implementation accordingly. Would
this
striker> be usefull? I mean, this way you know which part of the code caused
the
striker> last memory allocation that made it abort.
striker>
striker> This ofcourse also goes for apr_sms_realloc and apr_sms_calloc.
"
> I'm really only hesitant because I don't
> know for sure whether or not __FILE__ and __LINE__ are universally
> available symbols across all preprocessors. Besides, knowing which file
> and line it aborted in isn't much help since it will always be within one
> of the memory functions, when we're more interested in which file and line
> *called* the memory allocation function that failed (I think).
>
> Thoughts?
Yes, I wanted to do something like this:
#define apr_sms_malloc(mem_sys, size) \
apr_sms_malloc(mem_sys, size, __FILE__, __LINE__)
And change the prototype to:
APR_DECLARE(void *) apr_sms_malloc(apr_sms_t *sms, apr_size_t size,
char *sourcefile, int lineno);
The only problem is that the memory systems themselves will need the
extra parameters too, otherwise it won't bubble up. So that's why I
didn't want to do this yet, because I'm not sure this is what is
wanted.
And ofcourse apr_sms_malloc is just the first, calloc and realloc
would have to be done too.
And then I start to wonder if we don't just want to do this to
all memory system functions (like free, reset, destroy), so someone
can implement a decent profiling/logging memory system.
As for the __FILE__ and __LINE__ symbols, I have yet to meet a
compiler package that doesn't support them. On the other hand I have
only seen a few: gcc (on Linux), VC++ (Winxxx), and a series of small
(mostly) unknow ones these and other platforms (Amiga). It is ANSI
standard... (I did not bring this up by the way :-).
Someone out there know of one that doesn't support them, please speak
up.
[...]
> Thanks,
> --Cliff
Sander
Re: [PATCH] APR Stackable Memory System
Posted by Cliff Woolley <cl...@yahoo.com>.
I've committed the first two patches. While I was at it, I threw in a
couple more styleguide cleanups that I spotted with the second commit.
On Sun, 20 May 2001, Sander Striker wrote:
> sms-abort.patch.2
> Adds an abort_fn member to the apr_sms_t. This
> includes code calling the abort_fn, if present, on
> APR_ENOMEM. The code to set the abort function is
> not present yet. I added this to get sms one step
> closer to apr pools.
I'm wondering on this one if the abort function should have a prototype
more like the one used by pools (ie, it should take just a status code
rather than filename/linenum). I'm really only hesitant because I don't
know for sure whether or not __FILE__ and __LINE__ are universally
available symbols across all preprocessors. Besides, knowing which file
and line it aborted in isn't much help since it will always be within one
of the memory functions, when we're more interested in which file and line
*called* the memory allocation function that failed (I think).
Thoughts?
> sms-create.patch.3
> Cleans up apr_sms_create() [only to be called by
> memory system implementors]. It now returns a status
> code which will be needed when adding lock creation
> and other things that could fail in this function.
I'll get to this one after the abort patch is taken care of.
Thanks,
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA