Added 2 patches waiting for review

[michael shang]

Hi Mark and all,

I just created 2 issues, please kindly find the patch and give a review.
issue 200 try to fix several small bugs when writing the dump file.
issue 201 add sparc cpu support into breakpad, some testdata also attached
as well, for there may be some faults in he code I did not realized.

BTW, I think the '}' at minidump_processor.cc:835 should be at 734
to close the switch for WIN32. Please also have a check.

Thanks,
Michael

[Mark Mentovai]

This looks good to me, with a few minor changes. I've also got a
couple of questions. I'd like Alfred to take a look, too.

>BTW, I think the '}' at minidump_processor.cc:835 should be at 734
>to close the switch for WIN32. Please also have a check.

I think you're right. The WIN32_NT/WIN32_WINDOWS case should have a
closing brace at 734, and LINUX should have an opening brace at 735.

>Index: src/client/solaris/handler/solaris_lwp.cc

>+ * Determine number of mappings, this value gotta be
>+ * larger than the actual module count

"must" instead of "gotta".

>Index: src/client/solaris/handler/minidump_generator.cc

>+ if (!list.AllocateObjectAndArray(lwp_count-1, sizeof(MDRawThread)))

Style: spaces around the new -.

>Index: src/processor/stackwalker.cc

>+ cpu_stackwalker = new StackwalkerSPARC(system_info,
>+ context->GetContextSPARC(),

This doesn't line up properly.

>Index: src/processor/minidump.cc

>+//add inline symbol, for cc compiler on Solaris
>+static inline void Swap(u_int64_t* value) {

Space between // and your comment text.

I don't see why this should be necessary. I purposely didn't make
this function inline because of code size concerns.

>+ printf(" float_save.regs[%2d] = 0x%llx\n",

Probably don't need all that whitespace in there - let's make this

+ printf(" float_save.regs[%2d] = 0x%llx\n",

since it's the longest line, and make it so that all of the other
lines' = characters in this method line up.

>+ // may be padded with 4 bytes on 64bit ABIs for alignment

(MinidumpThreadList) This looks like it accounts for a problem that
occurs if a 32-bit processor handles a 64-bit dump. If so, there are
probably problems going the other way too: if a 64-bit processor
handles a 32-bit dump, it'll already have the padding in this struct,
so the size reported in the 32-bit dump that doesn't have the padding
will be 4 bytes less. Right?

>+ // may be padded with 4 bytes on 64bit ABIs for alignment

(MinidumpModuleList, MinidumpMemoryList) Same here. Since this
pattern comes up more than once, we probably need a more elegant
solution.

>Index: src/processor/stackwalker_sparc.cc

>+StackwalkerSPARC::StackwalkerSPARC(const SystemInfo *system_info,
>+ const MDRawContextSPARC *context,

Doesn't line up here either. (I bet you used the x86 or ppc file as a
template!)

>+ // %sp_new = %fp_old %fp=%i6=g_r[30] %sp=%o6=g_r[14]

Put each assignment on a new line, I think that makes it clearer.

>+ frame->context.pc = instruction+8;

Put space on either side of the +. Good job with getting context.pc
and instruction right here.

>Index: src/processor/stackwalker_sparc.h

>+ StackwalkerSPARC(const SystemInfo *system_info,
>+ const MDRawContextSPARC *context,

Line up, as above.

>Index: src/processor/minidump_processor.cc

>+ case MD_OS_SOLARIS:

Wrap the whole case in its own {}, as was supposed to be the
convention here. (See the comment way up at the top of this e-mail.)
Looks like things got a little wonky when Linux was added, no biggie.

>Index: src/processor/stackwalker_selftest.cc

Cool!

>+ StackwalkerSPARC stackwalker = StackwalkerSPARC(NULL, &context,
&memory, NULL,
>+ NULL, &resolver);

Line up.

>Index: Makefile.am

- src/processor/stackwalker_x86.h
+ src/processor/stackwalker_x86.h \
+ src/processor/stackwalker_sparc.cc \
+ src/processor/stackwalker_sparc.h

I like to keep things sorted alphabetically in these sections - sparc
stuff between ppc and x86.

[satis...@gmail.com]

Thanks Mark, Alfred have already checked it once and I will revise the
code according to your suggestion.
Michael

[Alfred]

Hi Michael,

Thanks for the patches. Some questions listed below and maybe need
some help from Mento.

> memset(&module, 0, sizeof (module));
> module.start_addr = _maps->pr_vaddr;
> module.size = _maps->pr_size;
> - if (name && (strcmp(name, "a.out") != 0))
> + if ((strlen(name) > 0) && (strcmp(name, "a.out") != 0)) {
> strncpy(module.name, name, sizeof (module.name) - 1);
> + ++module_count;
> + }

Do we need to find a way to get rid of local variable "module" when
calling "GetModuleCount"? It's redundant in this case.

- lwp->thread_id = lwp_lister_->getpid();
+ // should be the thread_id
+ lwp->thread_id = lsp->pr_lwpid;

I'm a little bit confused about thread_id. For the Linux code,
thread_id comes from getpid(). So I think it's also the case for
Solaris. What's it for? If it's changed here, some other places also
need to be updated, such as here:

> dir->location = exception.location();
> exception.get()->thread_id = requester_pid_;
> exception.get()->exception_record.exception_code = signo_;

Apart from above and mento's review, I'm fine with this Solaris
handler patch. Will give the opinion on the SPARC patch later on.

Thanks,
-Alfred

[satis...@gmail.com]

Hi Alfred,

>
> > memset(&module, 0, sizeof (module));
> > module.start_addr = _maps->pr_vaddr;
> > module.size = _maps->pr_size;
> > - if (name && (strcmp(name, "a.out") != 0))
> > + if ((strlen(name) > 0) && (strcmp(name, "a.out") != 0)) {
> > strncpy(module.name, name, sizeof (module.name) - 1);
> > + ++module_count;
> > + }
>
> Do we need to find a way to get rid of local variable "module" when
> calling "GetModuleCount"? It's redundant in this case.

A simple resolution came into my mind is use some "if":

ModuleInfo module;

if (callback_param) {

memset(&module, 0, sizeof (module));
module.start_addr = _maps->pr_vaddr;
module.size = _maps->pr_size;
}

if ((strlen(name) > 0) && (strcmp(name, "a.out") != 0)) {
if (callback_param)

strncpy(module.name, name, sizeof (module.name) - 1);

++module_count;
}
if (callback_param &&
(!callback_param->call_back(module, callback_param->context)))
{
break;
}

How do you think?

> - lwp->thread_id = lwp_lister_->getpid();
> + // should be the thread_id
> + lwp->thread_id = lsp->pr_lwpid;
>
> I'm a little bit confused about thread_id. For the Linux code,
> thread_id comes from getpid(). So I think it's also the case for
> Solaris. What's it for?

It is the thread_id, Linux code reads the /proc/$pid/task dir and get
all the thread_ids

> If it's changed here, some other places also
> need to be updated, such as here:
>
> > dir->location = exception.location();
> > exception.get()->thread_id = requester_pid_;
> > exception.get()->exception_record.exception_code = signo_;
>

Yeah, you are right. it should be the crashed thread id or just a zero
which
means we have no info about it. I will try to revise this part later.

> Apart from above and mento's review, I'm fine with this Solaris
> handler patch. Will give the opinion on the SPARC patch later on.

Many thanks for the review and suggestions.
Michael

[michael shang]

Hi Mark,

Thanks a lot for the suggestion, I've already made some changes with
the patch following your advice. And I updated the
stackwalker_selftest.cc to let it
work with Sun cc compiler, during which a bug in stackwalker_sparc has been
found and fixed.
But still I have some questions, please find the attached patch and
give some advice.

>
> >Index: src/processor/minidump.cc

> >+static inline void Swap(u_int64_t* value) {
>

> I don't see why this should be necessary. I purposely didn't make
> this function inline because of code size concerns.
>

Yes, it's wired, without "inline" compiler would give errors about
cannot find this function.
I think it is may be a bug of Sun cc. Anyway, I will report a bug. But for now,
it can not compile using cc.

> >+ // may be padded with 4 bytes on 64bit ABIs for alignment
>
> (MinidumpThreadList) This looks like it accounts for a problem that
> occurs if a 32-bit processor handles a 64-bit dump. If so, there are
> probably problems going the other way too: if a 64-bit processor
> handles a 32-bit dump, it'll already have the padding in this struct,
> so the size reported in the 32-bit dump that doesn't have the padding
> will be 4 bytes less. Right?
>

There are no problems when 64bit processor handling a 32bit dump.
Because when comparing:
expected_size != sizeof(thread_count) + thread_count * sizeof(MDRawThread)
the left value expected_size will be 4 bytes bigger in 64bit dump than
32 bit ones,
but the right value will always be the same, for sizeof(thread_count)
will always be
4bytes and the definition of MDRawThread do not have padding problem.
After all, this way we can fix the problem.

> >+ // may be padded with 4 bytes on 64bit ABIs for alignment
>
> (MinidumpModuleList, MinidumpMemoryList) Same here. Since this
> pattern comes up more than once, we probably need a more elegant
> solution.
>

Yes, MinidumpModuleList, MinidumpMemoryList and MinidumpThreadList
have the same problem, so it's third times. When I wanted to change here
I think modify the struct definition and add if when processing may both be OK,
but this way is simple. As in a more elegant solution, do you mean put these
lines in a global method to use?

>
> >Index: src/processor/stackwalker_selftest.cc
>
Updated again, stackwalker_selftest_sol.s added to let stackwalker_selftest
work with Sun cc, now stackwalk_sparc can pass the test

Index: src/google_breakpad/processor/stack_frame_cpu.h
+struct StackFrameSPARC : public StackFrame {
+ // to be confirmed
+ enum ContextValidity {
+ CONTEXT_VALID_NONE = 0,
+ CONTEXT_VALID_PC = 0 << 0,
+ CONTEXT_VALID_SP = 0 << 1,
+ CONTEXT_VALID_FP = 0 << 2,
+ CONTEXT_VALID_ALL = -1
+ };
Here I defined the struct according to the x86 and ppc definition, in
my view, at lease there should be pc, sp and fp register, I am
worrying about lack of something, is that enough?

BTW, I have signed the agreement
http://code.google.com/legal/individual-cla-v1.0.html and
send to by fax to 001 650 649-1732, please have a check too.

Thanks,
Michael