Re: Patch for PR 10728

[Chris Moller <cmoller at redhat dot com>]

On 02/02/10 18:57, Tom Tromey wrote:
> There are other calls to val_print in this function.
> It seems like they should all be wrapped.
>    

I was basically applying the minimum-tinkering rule, but I'll wrap the 
other instances.

> If cp_free_statmem_stack must be called, then you need a cleanup.
> However, does it really need to be called?  IMO it is ok to keep a
> little data on this obstack in between calls as long as it is cleared at
> the next call (so that we don't leak an unbounded amount over time).
>    

It's not a matter of leaving a little data around, it's that the obstack 
has to be guaranteed to have been initialised (obstack_init()) and that 
it's empty.

I guess the obstack_init could by moved to _initialize_cp_valprint() as 
part of and obstack_begin(), and, actually that would likely be a good 
idea.  I'd mis-remembered that obstack_init doesn't allocate anything 
until the first grow--I just checked and it actually does allocate space 
which, so far as I can tell, can only be recovered with an 
obstack_finish.  But there still has to be a means of guaranteeing the 
obstack is empty.

Is there a cleanup for stuff that gets done in _initialize_cp_valprint()?

> It isn't clear to me from this patch or the explanation that
> cp_initialise_statmem_stack is called at every relevant entry point.
>    

The problem is I don't know which eps are relevant.  Does anything other 
than C/C++ have the statics problem?

> In fact it would be better not to have to do that, because auditing
> this call hierarchy is tricky.
>
> E.g., m2-valprint.c has a call to cp_print_value_fields.  Does that
> matter?
>    

I honestly don't know--all this arm-waving is explicitly to fix statics 
in C, and possibly exclusively C++.   I haven't much of a clue what the 
m2-* code is for (Modular 2?) and less of an idea if it needs to deal 
with statics.

> I didn't read the old code in depth; is there a way to make this
> self-initializing?
>    

I tried to find a way to do that, but just a couple of steps downstream 
from where I have the initialisation (at cp_print_value_fields) the code 
starts recursing.  The obstack stuff /has/ to be initialised and/or 
guaranteed clear, and that has to happen before the recursion starts.   
I could have stuck the initialisation in c_val_print, but that gets 
called from scm-valprint.c and jv-valprint.c, but I didn't want to 
tinker with otherwise unaffected code paths.

> Chris>  -      first_dont_print
> Chris>  -	= (CORE_ADDR *) obstack_base (&dont_print_statmem_obstack);
> Chris>  -      i = (CORE_ADDR *) obstack_next_free (&dont_print_statmem_obstack)
> Chris>  -	- first_dont_print;
> Chris>  +      first_dont_print =
> Chris>  +	(CORE_ADDR *)obstack_base (&dont_print_statmem_obstack);
> Chris>  +      i	=
> Chris>  +	obstack_object_size (&dont_print_statmem_obstack) / sizeof(CORE_ADDR);
>
> The formatting is weird here; the first statement doesn't appear to have
> changed, but has gratuitous formatting changes,

I just thought it made the stmts more readable.