Code Review: A Solaris version of minidump generator
Alfred Peng
There is a patch for the minidump generator on Solaris. Could you please help review it?
The issue: http://code.google.com/p/google-breakpad/issues/detail?id=171
The patch: http://code.google.com/p/google-breakpad/issues/attachment?aid=6410182470975882103&name=solaris-minidumpv1.patch
To build it on Solaris, an extra patch is also needed: http://code.google.com/p/google-breakpad/issues/attachment?aid=8417120717348824537&name=solaris-miscv1.patch
The extra patch will result in the 0-sized array issue which need to be fixed.
exception_handler and dump_symbols are still needed for Solaris. Anything else?
Thanks,
-Alfred
Mark Mentovai
+// Copyright (c) 2006, Google Inc.
Should be 2007 now.
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+//
+// file_id.h: Return a unique identifier for a file
And leave the line between the license and the new comment blank, to
make it obvious that they're set off from each other.
These two comments apply elsewhere, too.
Index: src/common/solaris/file_id.cc
+#include <cassert>
+#include <cstdio>
+#include <elf.h>
Our style is to include system C headers like <elf.h> before system
C++ ones like <cassert> and <cstdio>, and to put local headers last.
No points docked, because our style guide isn't publicly available.
We're trying to do something about that. I agree that it sucks to
code to conventions that aren't specified.
(Specifically, of the local headers included by foo.cc, they should be
ordered foo-inl.h, *-inl.h, foo.h, *.h.)
+static bool FindElfTextSection(int fd, const void *elf_base,
+ const void **text_start,
+ int *text_size) {
Can you provide a brief comment explaining what this function does, so
that readers know how to use it and what to expect it to do?
+ elf_end(elf);
+ return false;
Seeing this pattern as frequently as we see it here usually means that
you'd be better off using some stack object to free the resource,
like:
class AutoElfEnder {
public:
AutoElfEnder(Elf *elf) : elf_(elf) {}
~AutoElfEnder() { if (elf_) elf_end(elf_); }
Elf* release() { Elf* elf = elf_; elf_ = NULL; return elf; }
private:
Elf *elf_;
};
+ if (elf_header.e_ident[EI_MAG0] != ELFMAG0 ||
+ elf_header.e_ident[EI_MAG1] != ELFMAG1 ||
+ elf_header.e_ident[EI_MAG2] != ELFMAG2 ||
+ elf_header.e_ident[EI_MAG3] != ELFMAG3) {
Are we going to need to think about byte-swapping here? If this code
is used for an eventual Solaris version of dump_syms, it probably
should be able to dump symbols for other-endian architectures too.
+ printf("Header magic doens't match\n");
Check spelling.
Should these messages go to stderr? Should there be a message printed
for the |elf_version(EV_CURRENT) == EV_NONE| early return check too?
+ *text_start = NULL;
+ *text_size = 0;
I'd move these way up to the top of the function body, right after the asserts.
+ const char *text_section_name = ".text";
+ int name_len = strlen(text_section_name);
static const char kTextSectionName[] = ".text";
static const int kNameLen = sizeof(kTextSectionName) - 1;
+ const char *section_name = elf_strptr(elf,
elf_header.e_shstrndx, shdr.sh_name);
We have an 80-character line limit, this may come up elsewhere too.
+ if (!strncmp(section_name, text_section_name, name_len)) {
For str*cmp, it's easier to follow if you use |!= 0|.
strncmp is wrong here, it'll match when section_name = ".textual".
+ return true;
I think it makes more sense to return true only if you found a .text
section. That simplifies calls to this function, so callers won't
need to check the out-values of text_section and text_size.
+FileID::FileID(const char *path) {
+ strncpy(path_, path, sizeof(path_));
+}
But path_ is of size PATH_MAX. Oops!
+bool FileID::ElfFileIdentifier(unsigned char identifier[16]) {
+ if (freopen(path_, "r", stdin) == NULL)
+ return false;
+ if (fstat(fd, &st) != 0 && st.st_size <= 0) {
That should be ||, not &&.
Why are you (ab)using stdin here?
+ close(fd);
Same comment as above about using an auto-closer - since you've got
stdio hooked up too, do you want to use fclose?
+ void *base = mmap(NULL, st.st_size,
+ PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
Oh my, why PROT_WRITE?
+// static
+void FileID::ConvertIdentifierToString(const unsigned char identifier[16],
+ char *buffer, int buffer_length) {
+ for (int idx = 0; (buffer_idx < buffer_length) && (idx < 16); ++idx) {
Instead of going to the trouble of doing both index checks and
allowing truncated data, why not just specify that the buffer you pass
to this method must have room for at least 37 chars? When will the
truncated data be useful?
We've mostly switched to dashless identifiers, if you're planning on
using the string that this returns in the same way that we usually use
identifiers with MDCVInfoPDB70, you should drop the dashes too and
append a '0' corresponding to the age field (making the minimum buffer
size 34).
(My original plan was that we'd only use MDCVInfoPDB70 on Windows
only, and use some other struct type[s] on other platforms, but that
never seemed to happen - if you did want to define your own struct
type to use instead, you'd be free to choose whatever format you wish
for the identifier's string representation.)
Index: src/client/solaris/handler/Makefile
Makefiles should have license junk too.
+CC=CC
Is that right?
You can probably just leave it out and let make pluck $CC from the
environment, or pick a sane default if $CC isn't defined there.
Anyway, shouldn't you be using $CXX? This is C++ source.
Index: src/client/solaris/handler/minidump_generator.cc
+// Author: Alfred Peng
That should go after the license junk - see most of the files in
src/processor for an example. (It's a good idea, you can put this in
your other source files, too.)
+// This unnamed namespace contains helper functions.
Your helpers should still be marked |static| even though they're in an
unnamed namespace - if they're not static, they'll still be linked as
global symbols.
(Stopping here for now to take a break, I'll pick this up again at
this point later.)
Mark Mentovai
Index: src/client/solaris/handler/minidump_generator.cc
+bool WriteLwpStack(uintptr_t last_ebp,
+ uintptr_t last_esp,
+ const SolarisLwp *ps_lister,
+ UntypedMDRVA *memory,
+ MDMemoryDescriptor *loc) {
Fix indentation. Same comment applies to other function definitions
and function calls.
+ uintptr_t stack_bottom = ps_lister->GetLwpStackBottom(last_ebp);
So I looked for this...
+ // Get the bottom of the stack from ebp.
+ uintptr_t GetLwpStackBottom(uintptr_t current_esp) const;
and then the implementation:
+uintptr_t SolarisLwp::GetLwpStackBottom(uintptr_t current_ebp) const {
Hmm. Do you want ebp or esp?
The whole thing comes with this proviso:
+// TODO: Improve it.
I think we should improve it by looking at the memory map now, if it's
possible. Eventually, we want to be able to figure the stack
dimensions by referencing only %esp, but I understand that the walking
that you've got in place temporarily uses %ebp instead. As you
probably know, we want to avoid doing stackwalking in the client, but
at least you've got the IsAddressMapped check.
+ loc->start_of_memory_range = 0 | last_esp;
Direct assignment won't work? Cast won't work?
+bool WriteCPUInformation(MDRawSystemInfo *sys_info) {
+ // Match i*86 and x86* as X86 architecture.
+ if ((strstr(uts.machine, "x86") == uts.machine) ||
+ (strlen(uts.machine) == 5 &&
+ uts.machine[0] == 'i' &&
+ uts.machine[1] == '8' &&
+ uts.machine[2] == '6'))
This will match x86* but not i*86 - what's up here?
+ else if (!strcpy(uts.machine, "sparc"))
+ sys_info->processor_architecture = MD_CPU_ARCHITECTURE_ARM;
This is just wrong.
+bool WriteOSInformation(MinidumpFileWriter *minidump_writer,
+ MDRawSystemInfo *sys_info) {
+ cur_os_info++) {
Again, it sucks to write to a spec you can't see, but we prefer
preincrement/decrement, all else being equal. This comes up
elsewhere, including a few lines down:
+ space_left--;
+ sys_info->platform_id = MD_OS_UNIX;
Let's define MD_OS_SOLARIS = 0x8202 so you don't need to be so generic.
+bool LwpInformationCallback(lwpstatus_t *lsp, void *context) {
+ if (lsp->pr_lwpid != pthread_self()) {
You should comment on why you don't do this for the current thread.
+bool WriteCVRecord(MinidumpFileWriter *minidump_writer,
+ MDRawModule *module,
+ const char *module_path) {
+ snprintf(path, sizeof(path), "/proc/self/object/%s", module_name);
You've gone to great lengths to avoid sprintf so far in other
locations, but you use it here. If you've come to the conclusion that
sprintf is safe on your platform at exception time, then you can
change a lot of more cumbersome string manipulations. Otherwise, you
shouldn't use sprintf here, either.
Also consider the use of printf for error messages.
+bool WriteModuleListStream(MinidumpFileWriter *minidump_writer,
+ const WriterArgument *writer_args,
+ MDRawDirectory *dir) {
+ int module_count = writer_args->lwp_lister->GetModuleCount();
Extra space.
+bool ModuleInfoCallback(const ModuleInfo &module_info,
+ void *context) {
+ if (!callback_context->minidump_writer->WriteString(module_info.name, 0,
+ &loc))
+ return false;
This conditional should have {braces} because the whole thing covers
more than two lines. This happens elsewhere in this function, and in
other functions, too.
+bool WriteSystemInfoStream(MinidumpFileWriter *minidump_writer,
+ const WriterArgument *writer_args,
+ MDRawDirectory *dir) {
Hmm, it occurs to me that you're passing around these three arguments
a whole lot. Would it make more sense to make each of these functions
into nonstatic private methods of MinidumpFileWriter, so that they can
have access to what are now arguments as private data? I think that
would improve things.
+// Function table to writer a full minidump.
+WriteStringFN writers[] = {
My previous comment about making the functions in this unnamed
namespace |static| applies to data like this too - in fact, this can
also be |const|.
+void* Write(void *argument) {
+ WriterArgument *writer_args =
+ static_cast<WriterArgument *>(argument);
I'm not looking too closely at the caller, but I bet there's a better
way to do this than a static cast.
+ if (!writer_args->lwp_lister->SuspendAllLwps())
+ return NULL;
You want to guarantee that all LWPs will resume when this function
exits, but you've got a couple of early returns that don't call
ResumeAllLwps. Suggest an auto-resume-on-destroy stack object? I see
that SolarisLwp will resume them if they haven't already been when
it's destroyed, but is that soon enough?
+void MinidumpGenerator::AllocateStack() {
+ stack_.reset(new char[kStackSize]);
Heap allocation isn't safe at exception time, |new| needs to go.
+ if (pthread_create(&tid, &attr, Write, (void *)&argument)) {
+ printf("Error create thread\n");
+ return 127;
+ } else
+ printf("write thread %d\n", tid);
If one conditional branch has {braces}, the rest need them too.
Oh, so you're spawning a thread at exception time? That's not
obviously exception-safe unless you can prove its safety on Solaris.
On our other platforms, we have the thread prespawned and trigger it
in a platform-dependent manner. On the Mac, for example, the
exception is automatically delivered to our waiting thread, but on
Windows, we need to trigger a mutex manually to transition control
from the exception thread to the dump thread. I strongly urge you to
preallocate as many resources as possible prior to exception time,
like when the handler is installed. See
http://code.google.com/p/google-breakpad/wiki/ClientDesign .
Index: src/client/solaris/handler/minidump_generator.h
+struct sigcontext;
Looks like this is left over from the Linux handler.
+// Write a minidump to file based on the signo and sig_ctx.
This one, too.
Index: src/client/solaris/handler/minidump_test.cc
+static void CreateThread(int num) {
+// pthread_join(h, NULL);
+// pthread_detach(h);
What's with the commented-out code in this file?
Index: src/client/solaris/handler/solaris_lwp.cc
+#include <alloca.h>
Uh-oh - alloca and other variable-sized stack usage is also very
heavily discouraged because it can frustrate debugging. (Actually,
alloca is forbidden - but because we have to forbid malloc in
exception-time code, I'm going to be slightly lenient and say "very
heavily discouraged" instead.)
+int fstat(int, struct stat *);
You guys don't have fstat declared by <sys/stat.h>? That surprises
me. (I don't have a Solaris installation readily available to check.)
+bool LocalAtoi(char *s, int *r) {
Same comment about making these file-only functions static applies here.
+ if (endptr == s)
+ return false;
Check indentation.
+#if defined(__i386__) && !defined(NO_FRAME_POINTER)
The fact that the handler is compiled with or without frame pointers
has limited bearing on whether other code it's linked with is compiled
the same way. If we're not ready to handle %ebp-less stuff on Solaris
yet, we should just ignore it and call it unsupported.
Similarly, none of the rest of the code here will work on non-x86, so
I'd just as soon call other CPUs unsupported and leave the whole #else
branch here out altogether.
+static int
+IterateLwpAll(int pid, CallbackParam<LwpidCallback> *callback_param) {
+ snprintf(lwp_path, sizeof(lwp_path), "/proc/%d/lwp", (int)pid);
As above.
+ if (strcmp(entry->d_name, ".") &&
+ strcmp(entry->d_name, "..")) {
Prefer explicit |!= 0| checks.
+static bool SuspendLwp(int lwpid, void *context) {
[...]
+static bool ResumeLwp(int lwpid, void *context) {
These are pretty similar, can they be combined into one function that
takes a suspend/resume boolean argument?
+int SolarisLwp::SuspendAllLwps() {
[...]
+void SolarisLwp::ResumeAllLwps() {
Same.
+static prheader_t* read_lfile(int pid, const char *lname) {
Document what this is supposed to do, how to call it, what it returns,
who owns the pointer it returns, etc.
+ if ((Lhp = (prheader *)malloc(size)) == NULL)
Can't malloc at exception time.
+int
+SolarisLwp::Lwp_iter_all(int pid,
+ CallbackParam<LwpCallback> *callback_param) const {
Return type goes on the same line, and fix the indentation on the
continuation line.
+ if (Lhp != NULL)
+ free(Lhp);
+ if (Lphp != NULL)
+ free(Lphp);
free(NULL) is safe, so the checks aren't needed - but we can't be
messing with the heap like this anyway.
+ if (nstat != 0)
+ goto retry;
When does this happen? Is there anything to safeguard against hitting
this case each time we check for it?
+int SolarisLwp::ListModules(
+ CallbackParam<ModuleCallback> *callback_param) const {
+ printf("alloca %d error: %s\n", size, strerror(ENOMEM));
...and if you have to alloca, when you're printing the error, it
probably doesn't make much sense to include the constant
strerror(ENOMEM).
Index: src/client/solaris/handler/solaris_lwp.h
+#define _KERNEL
+#include <sys/procfs.h>
+#undef _KERNEL
What if someone else had wanted _KERNEL defined?
#ifndef _KERNEL
#define _KERNEL
#define MUST_UNDEF_KERNEL
#endif // _KERNEL
#include <sys/procfs.h>
#ifdef MUST_UNDEF_KERNEL
#undef _KERNEL
#undef MUST_UNDEF_KERNEL
#endif // MUST_UNDEF_KERNEL
+// Max module path name length.
+#define kMaxModuleNameLength 256
Much better as a constant than a macro.
+ typedef bool (*LwpCallback)(lwpstatus_t* lsp, void *context);
Unindent.
+ private:
+ // Check if the address is a valid virtual address.
+ bool IsAddressMapped(uintptr_t address) const;
+
+ private:
Don't need multiple private sections, it's OK to mix methods with data.
Sorry there are so many comments, but there was so much code! Thanks
for the contribution.
Mark
Mark Mentovai
- MDRawThread threads[0];
+ MDRawThread threads[1];
We use sizeof to take the size of a bunch of these structures in
minidump.cc, and we expect the size to only include the header portion
and zero, not one, variable-sized element that follows.
If we change these, we also need to comment (in minidump_format.h)
that we're diverging from Microsoft's structure definitions to provide
better language conformance.
+#if defined(__SUNPRO_CC)
How about gcc on your platform? Should we be looking for a macro that
identifies Solarisness instead of one that identifies the specific
compiler? (gcc -E -dM - < /dev/null will show you predefined macros
if you don't know what's already predefined by gcc.)
Mark
Alfred Peng
Thanks for the careful review. I've updated the patches and posted them here:
http://code.google.com/p/google-breakpad/issues/attachment?aid=8575836088288585552&name=solaris-handlerv2-2.patch
http://code.google.com/p/google-breakpad/issues/attachment?aid=3736019298878462905&name=solaris-miscv2-2.patch
The first patch also includes the exception handler.
If you want to try out the patch by yourself, the VMWare image for Solaris or the install DVD can be provided:-)
Please see my comments below:
Index: src/common/solaris/file_id.cc
+ if (elf_header.e_ident[EI_MAG0] != ELFMAG0 ||
+ elf_header.e_ident[EI_MAG1] != ELFMAG1 ||
+ elf_header.e_ident[EI_MAG2] != ELFMAG2 ||
+ elf_header.e_ident[EI_MAG3] != ELFMAG3) {
Are we going to need to think about byte-swapping here? If this code
is used for an eventual Solaris version of dump_syms, it probably
should be able to dump symbols for other-endian architectures too.
e_ident is an array for unsigned char. The endian issue shouldn't have impact on it.
+bool FileID::ElfFileIdentifier(unsigned char identifier[16]) {
+ if (freopen(path_, "r", stdin) == NULL)
+ return false;
Why are you (ab)using stdin here?
No need to use stdin. Get rid of it.
+// static
+void FileID::ConvertIdentifierToString(const unsigned char identifier[16],
+ char *buffer, int buffer_length) {
+ for (int idx = 0; (buffer_idx < buffer_length) && (idx < 16); ++idx) {
Instead of going to the trouble of doing both index checks and
allowing truncated data, why not just specify that the buffer you pass
to this method must have room for at least 37 chars? When will the
truncated data be useful?
We've mostly switched to dashless identifiers, if you're planning on
using the string that this returns in the same way that we usually use
identifiers with MDCVInfoPDB70, you should drop the dashes too and
append a '0' corresponding to the age field (making the minimum buffer
size 34).
(My original plan was that we'd only use MDCVInfoPDB70 on Windows
only, and use some other struct type[s] on other platforms, but that
never seemed to happen - if you did want to define your own struct
type to use instead, you'd be free to choose whatever format you wish
for the identifier's string representation.)
I define another struct for the identifiers: MDCVInfoELF. Not sure whether it's the proper way to go. The minimum buffer size is 34. Shorter buffer will cause false return.
Index: src/client/solaris/handler/Makefile
Makefiles should have license junk too.
+CC=CC
Is that right?
You can probably just leave it out and let make pluck $CC from the
environment, or pick a sane default if $CC isn't defined there.
Anyway, shouldn't you be using $CXX? This is C++ source.
CC is the sun C++ compiler, and cc is for C. We highly recommend the users to use cc/CC on Solaris instead of gcc. It's already bundled in SXDE(Solaris Express Developer Edition). All our desktop applications are built by cc/CC. I also use a CC specific compiling option: -features=extensions. It allows non-standard code that is commonly accepted by other C++ compilers. BTW, gcc can build the code successfully and the option needs to be removed.
Index: src/client/solaris/handler/minidump_generator.cc
+ uintptr_t stack_bottom = ps_lister->GetLwpStackBottom(last_ebp);
So I looked for this...
+ // Get the bottom of the stack from ebp.
+ uintptr_t GetLwpStackBottom(uintptr_t current_esp) const;
and then the implementation:
+uintptr_t SolarisLwp::GetLwpStackBottom(uintptr_t current_ebp) const {
Hmm. Do you want ebp or esp?
The whole thing comes with this proviso:
+// TODO: Improve it.
I think we should improve it by looking at the memory map now, if it's
possible. Eventually, we want to be able to figure the stack
dimensions by referencing only %esp, but I understand that the walking
that you've got in place temporarily uses %ebp instead. As you
probably know, we want to avoid doing stackwalking in the client, but
at least you've got the IsAddressMapped check.
I change the code here by only using %esp. The logic is to search in the address map provided by the proc file system. If the value of %esp lies in one of the map, I'll take the map as the stack for the current process. And the end boundary of the map is the stack bottom. One unresolved issue is that I think the environment variable at the bottom of the stack should be removed. But I didn't find out the way to calculate the size of this environment array... Any suggestions is appreciated.
+bool WriteCPUInformation(MDRawSystemInfo *sys_info) {
+ // Match i*86 and x86* as X86 architecture.
+ if ((strstr(uts.machine, "x86") == uts.machine) ||
+ (strlen(uts.machine) == 5 &&
+ uts.machine[0] == 'i' &&
+ uts.machine[1] == '8' &&
+ uts.machine[2] == '6'))
This will match x86* but not i*86 - what's up here?
The machine value on Solaris platform for x86 is "i86pc".
+ else if (!strcpy(uts.machine, "sparc"))
+ sys_info->processor_architecture = MD_CPU_ARCHITECTURE_ARM;
This is just wrong.
And "sun4u" on SPARC. I define MD_CPU_ARCHITECTURE_SPARC for the value here.
+ if (lsp->pr_lwpid != pthread_self()) {
You should comment on why you don't do this for the current thread.
The problem here is if we register several exception handler on the process, extra information about other handler threads will still be printed out.
+bool WriteCVRecord(MinidumpFileWriter *minidump_writer,
+ MDRawModule *module,
+ const char *module_path) {
+ snprintf(path, sizeof(path), "/proc/self/object/%s", module_name);
You've gone to great lengths to avoid sprintf so far in other
locations, but you use it here. If you've come to the conclusion that
sprintf is safe on your platform at exception time, then you can
change a lot of more cumbersome string manipulations. Otherwise, you
shouldn't use sprintf here, either.
Also consider the use of printf for error messages.
One guy in our team told me that snprintf won't operate on heap on Solaris. But printf and fprintf are not. So I thin that it's safe to use snprintf in the code. The new file message_output.h is for this.
+bool WriteSystemInfoStream(MinidumpFileWriter *minidump_writer,
+ const WriterArgument *writer_args,
+ MDRawDirectory *dir) {
Hmm, it occurs to me that you're passing around these three arguments
a whole lot. Would it make more sense to make each of these functions
into nonstatic private methods of MinidumpFileWriter, so that they can
have access to what are now arguments as private data? I think that
would improve things.
The "Write*" functions have been moved to class MinidumpGenerator and the arguments are shared between them.
+ if (!writer_args->lwp_lister->SuspendAllLwps())
+ return NULL;
You want to guarantee that all LWPs will resume when this function
exits, but you've got a couple of early returns that don't call
ResumeAllLwps. Suggest an auto-resume-on-destroy stack object? I see
that SolarisLwp will resume them if they haven't already been when
it's destroyed, but is that soon enough?
The auto destroy class rocks:-)
+ if (pthread_create(&tid, &attr, Write, (void *)&argument)) {
+ printf("Error create thread\n");
+ return 127;
+ } else
+ printf("write thread %d\n", tid);
If one conditional branch has {braces}, the rest need them too.
Oh, so you're spawning a thread at exception time? That's not
obviously exception-safe unless you can prove its safety on Solaris.
On our other platforms, we have the thread prespawned and trigger it
in a platform-dependent manner. On the Mac, for example, the
exception is automatically delivered to our waiting thread, but on
Windows, we need to trigger a mutex manually to transition control
from the exception thread to the dump thread. I strongly urge you to
preallocate as many resources as possible prior to exception time,
like when the handler is installed. See
http://code.google.com/p/google-breakpad/wiki/ClientDesign .
The pthread will be created at the constructor of class ExceptionHandler. That could avoid the heap allocation during the exception phase. Great!
Index: src/client/solaris/handler/solaris_lwp.cc
+#include <alloca.h >
Uh-oh - alloca and other variable-sized stack usage is also very
heavily discouraged because it can frustrate debugging. (Actually,
alloca is forbidden - but because we have to forbid malloc in
exception-time code, I'm going to be slightly lenient and say "very
heavily discouraged" instead.)
The current solution is to use the memory on the stack. Is there any way to resolve the problem that the array size isn't enough for the headers? A memory sub-system for breakpad might make things a little bit complicated.
+#if defined(__i386__) && !defined(NO_FRAME_POINTER)
The fact that the handler is compiled with or without frame pointers
has limited bearing on whether other code it's linked with is compiled
the same way. If we're not ready to handle %ebp-less stuff on Solaris
yet, we should just ignore it and call it unsupported.
Similarly, none of the rest of the code here will work on non-x86, so
I'd just as soon call other CPUs unsupported and leave the whole #else
branch here out altogether.
The current SolarisLwp only supports x86. I add the comments in the header file. And SPARC support needs to be added in the future.
+ if (nstat != 0)
+ goto retry;
When does this happen? Is there anything to safeguard against hitting
this case each time we check for it?
As all the threads are suspended, we don't need to retry. Get rid of it.
Index: src/google_breakpad/common/minidump_format.h
- MDRawThread threads[0];
+ MDRawThread threads[1];
We use sizeof to take the size of a bunch of these structures in
minidump.cc, and we expect the size to only include the header portion
and zero, not one, variable-sized element that follows.
If we change these, we also need to comment (in minidump_format.h)
that we're diverging from Microsoft's structure definitions to provide
better language conformance.
Thanks for the fix for this. That's really helpful!
+#if defined(__SUNPRO_CC)
How about gcc on your platform? Should we be looking for a macro that
identifies Solarisness instead of one that identifies the specific
compiler? (gcc -E -dM - < /dev/null will show you predefined macros
if you don't know what's already predefined by gcc.)
Thanks & Best Regards,
-Alfred
Alfred Peng
Thanks,
-Alfred
Mark Mentovai
>Index: src/google_breakpad/common/minidump_format.h
>+ MD_CPU_ARCHITECTURE_SPARC = 11, /* PROCESSOR_ARCHITECTURE_SPARC */
Microsoft defines these values, and I'd like to avoid conflicting with
them. I suggest 0x8001 for MD_CPU_ARCHITECTURE_SPARC. The comment
should identify the analogous constant in Microsoft's headers, but
since they don't define anything for SPARC, the comment here should
state that this value and SPARC support is a Breakpad extension.
>+} MDIdentifier;
>+} MDCVInfoELF;
Interesting - why the two separate structs?
Mark Mentovai
>Index: src/client/solaris/handler/exception_handler.cc
>+int SigTable[] = {
Can be static?
Can be const? If so, call it kSigTable. Otherwise, call it
sig_table. We've almost got a public style guide, I think, I know
it's really unfair to have to write to a standard you can't see...
>+#if defined(SIGSEGV)
#if defined or #ifdef - be consistent through here. Will these
signals ever not be defined?
>+ SIGBUS,
>+#endif
>+};
gcc -pedantic won't like the trailing comma - given the #ifdefs (if
needed), maybe this needs to have a NULL last element?
>+pthread_mutex_t ExceptionHandler::handler_stack_mutex_ =
>+PTHREAD_MUTEX_INITIALIZER;
And indent continuation lines too...
>+ print_message1(2, "warning: removing Breakpad handler out of order\n");
What's the deal with print_message1 and print_message2? The comments
around their declarations should at least say when to use them. (I
know the answer to this question because I read your e-mail, but the
answer should be in the source code as well.)
>+void ExceptionHandler::TeardownAllHandler() {
TeardownAllHandlers?
>Index: src/client/solaris/handler/exception_handler.h
>+ // file is <dump_path>\<minidump_id>.dmp. context is the parameter supplied
In a Solaris-specific file, a Solaris path separator would look better.
>Index: src/client/solaris/handler/minidump_generator.cc
>+ else if (strcpy(uts.machine, "sun4u") == 0)
I'm pretty sure you still don't want strcpy here. strcmp? strcmp is
probably better for the case above ("i86pc") too.
>I change this to
>"#if defined(__SUNPRO_CC) || (defined(__GNUC__) && defined(__sun__))"
>to include gcc. Could we have any MACRO generated from configure?
Sure, feel free to add it. :)
This patch looks pretty good, we can take it with the above things
fixed. Thanks again for your contribution!
Alfred Peng
Thanks for the nice review. See my comments below.
solaris-miscv2-2.patch
>Index: src/google_breakpad/common/minidump_format.h
>+} MDIdentifier;
>+} MDCVInfoELF;
Interesting - why the two separate structs?
These two structs are patterned from MDCVInfoPDB70 and MDGUID. Combine them into one on Solaris.
solaris-handlerv2-2.patch
>Index: src/client/solaris/handler/exception_handler.cc
>+int SigTable[] = {
Can be static?
Can be const? If so, call it kSigTable. Otherwise, call it
sig_table. We've almost got a public style guide, I think, I know
it's really unfair to have to write to a standard you can't see...
Glad to have a public guide. That should be a good news for all the developers:-)
>+#if defined(SIGSEGV)
#if defined or #ifdef - be consistent through here. Will these
signals ever not be defined?
I think all the signals are defined on Solaris. Get rid of all the defines here.
>I change this to
>"#if defined(__SUNPRO_CC) || (defined(__GNUC__) && defined(__sun__))"
>to include gcc. Could we have any MACRO generated from configure?
Sure, feel free to add it. :)
Actually I'm a little bit confused about the build system. For the client code, I don't need to do the configure. To "make" in the handler directory is just fine.
The updated patches have been put here:
http://code.google.com/p/google-breakpad/issues/attachment?aid=-2064340959640391395&name=solaris-handlerv3.patch
http://code.google.com/p/google-breakpad/issues/attachment?aid=2217133082202632824&name=solaris-miscv3.patch
If the patches above look fine, could you please help check in them?