[Crash-utility] [PATCH 1/2] libgcore: fix get notesize calculation

[Lei Wen]

Hi HATAYAMA,

> -----Original Message-----
> From: HATAYAMA Daisuke [mailto:d.hatayama at jp.fujitsu.com]
> Sent: Wednesday, March 14, 2012 10:45 AM
> To: Lei Wen
> Cc: crash-utility at redhat.com
> Subject: Re: [PATCH 1/2] libgcore: fix get notesize calculation
> 
> Hello Lei,
> 
> Sorry for very late response.
> 
> I'm now reviewing your patches.

Thanks for reviewing!

> 
> From: Lei Wen <leiwen at marvell.com>
> Subject: [PATCH 1/2] libgcore: fix get notesize calculation
> Date: Sat, 11 Feb 2012 22:44:14 -0800
> 
> > The logic in the kernel is if regset get method return not zero value,
> > which means error in that case, then it should skip this notesize calculation.
> > Align this logic in the libgcore with kernel.
> >
> > Signed-off-by: Lei Wen <leiwen at marvell.com>
> > ---
> >  extensions/libgcore/gcore_coredump.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/extensions/libgcore/gcore_coredump.c
> b/extensions/libgcore/gcore_coredump.c
> > index 8a8855d..e2d9941 100644
> > --- a/extensions/libgcore/gcore_coredump.c
> > +++ b/extensions/libgcore/gcore_coredump.c
> > @@ -454,7 +454,7 @@ fill_thread_core_info(struct elf_thread_core_info *t,
> >  		    !regset->active(task_to_context(t->task), regset))
> >  			continue;
> >  		data = (void *)GETBUF(regset->size);
> > -		if (!regset->get(task_to_context(t->task), regset, regset->size,
> > +		if (regset->get(task_to_context(t->task), regset, regset->size,
> >  				 data))
> >  			continue;
> >  		if (regset->callback)
> > --
> > 1.7.5.4
> >
> 
> I borrowed most of logic in gcore from kernel source code for ease of
> development and for ease of porting each architecture's implementation
> following regset interface.
> 
> The interface for get method in gcore command is different from
> kernel's. Originally, it returns 0 in a successfull case, and
> otherwise returns non-zero values. But in gcore, such get method
> returns TRUE in a successfull case, and otherwise FALSE.
> 
> I cannot recall why I made this change at that time precisely. But OK,
> I'll revert it into the original one. Based on the original motivation
> of why I adapted regset interface, it's better to keep it original.
> 
> BTW, looking at your 2nd patch, fpa_get() and vfp_get() bahaves
> differently w.r.t. the above mentioned one: fpa_get() returns 0 in a
> sucessfull case, but vfp_get() TRUE.
> 
> Is it OK that I'll modify vfp_get() so that it returns 0 in a
> successfull state?

Opps, that is my fault, thanks for correcting.
I'm ok with the change.

> 
> Furthermore, these days I'm beginning with the current design of gcore
> command is too large to maintain. I guess most users of this command
> requires only a small part of the features currently gcore command
> provides. In other words, they are satisfied only with seeing memory
> contents and backtrace using gdb. They don't need acurate restoration
> of register values. For the other direction, porting kernel source
> code directly, such as elf_core_dump() and regset interface, itself
> might be too large. I will perhaps rewrite gcore command entirely
> compactly. Please keep it mind.

Actually we think it is vital to also keep the register set extract out to analyze the user space application, since combining the registers and assembler code, we could get full view of what is happening when the fatal signal happens, like what the local variable content is then. Just backtrace is not enough...

Could we keep such feature?

> 
> Thanks.
> HATAYAMA, Daisuke

Thanks,
Lei