Issue 586002: Try getting file-ids from memory when possible, and fallback to reading files otherwise
|
Created:
10 years, 11 months ago by glandium Modified:
10 years, 6 months ago CC:
google-breakpad-dev_googlegroups.com Visibility:
Public. |
DescriptionTry getting file-ids from memory when possible, and fallback to reading files otherwise Patch Set 1 #
Total comments: 8
Patch Set 2 : Modified the LinuxDumper::ElfFileIdentifierForMapping comment #
Total comments: 1
MessagesTotal messages: 9
Just one comment, otherwise LGTM. https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... File src/client/linux/minidump_writer/linux_dumper.cc (right): https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... src/client/linux/minidump_writer/linux_dumper.cc:106: size_t size = getpagesize(); This should probably be using sys_sysconf(_SC_PAGESIZE) to avoid glibc.
Sign in to reply to this message.
https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... File src/client/linux/minidump_writer/linux_dumper.cc (right): https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... src/client/linux/minidump_writer/linux_dumper.cc:103: // Try getting the file-id from the first page in memory, or from assuming the file-id is in the first page will probably work for most Linux ELFs, but there's no requirement that it be there. a safer route would be to parse all the PT_NOTEs in the ELF instead. https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... src/client/linux/minidump_writer/linux_dumper.cc:106: size_t size = getpagesize(); getpagesize() isn't specific to glibc. then again, it's no longer part of the last few POSIX standards (it was part of an older version), so using sysconf makes sense. did you mean sysconf() rather than sys_sysconf() ?
Sign in to reply to this message.
https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... File src/client/linux/minidump_writer/linux_dumper.cc (right): https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... src/client/linux/minidump_writer/linux_dumper.cc:103: // Try getting the file-id from the first page in memory, or from On 2013/06/13 05:13:31, vapier wrote: > assuming the file-id is in the first page will probably work for most Linux > ELFs, but there's no requirement that it be there. > > a safer route would be to parse all the PT_NOTEs in the ELF instead. ElfFileIdentifierFromMappedFile does that. I didn't feel like there was much interest in duplicating that code just to ensure there is enough data in the first page to read them, when in the vast majority of cases, it will be the case.
Sign in to reply to this message.
https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... File src/client/linux/minidump_writer/linux_dumper.cc (right): https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... src/client/linux/minidump_writer/linux_dumper.cc:106: size_t size = getpagesize(); The comments about avoiding glibc were mostly about our dynamic-linker-avoidance in the crashed process in general, so sys_sysconf would be the syscall implementation from linux_syscall_support.
Sign in to reply to this message.
https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... File src/client/linux/minidump_writer/linux_dumper.cc (right): https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... src/client/linux/minidump_writer/linux_dumper.cc:103: // Try getting the file-id from the first page in memory, or from maybe a better answer would be to extend the API so that ElfFileIdentifier can ask for memory when it's not available ? at the very least, you should document the known limitation this code is imposing so that people who run into this in the future have an easy jumping point. https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... src/client/linux/minidump_writer/linux_dumper.cc:106: size_t size = getpagesize(); i saw sysconf() being used in the tree, but i guess that's just solaris & unittests, so those don't count
Sign in to reply to this message.
https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... File src/client/linux/minidump_writer/linux_dumper.cc (right): https://breakpad.appspot.com/586002/diff/1/src/client/linux/minidump_writer/l... src/client/linux/minidump_writer/linux_dumper.cc:106: size_t size = getpagesize(); On 2013/06/06 19:55:27, Ted Mielczarek wrote: > This should probably be using sys_sysconf(_SC_PAGESIZE) to avoid glibc. getpagesize is already used in linux_dumper.h. Moreover, linux_syscall_support's implementation of sysconf calls __getpagesize from the libc anyways.
Sign in to reply to this message.
https://breakpad.appspot.com/586002/diff/8001/src/client/linux/minidump_write... File src/client/linux/minidump_writer/linux_dumper.cc (left): https://breakpad.appspot.com/586002/diff/8001/src/client/linux/minidump_write... src/client/linux/minidump_writer/linux_dumper.cc:106: if (mapping.start_addr == auxv_[AT_SYSINFO_EHDR]) { what tree is this patch made against ? i don't see this logic ever being written this way in trunk/.
Sign in to reply to this message.
On 2013/09/25 07:06:51, vapier wrote: > https://breakpad.appspot.com/586002/diff/8001/src/client/linux/minidump_write... > File src/client/linux/minidump_writer/linux_dumper.cc (left): > > https://breakpad.appspot.com/586002/diff/8001/src/client/linux/minidump_write... > src/client/linux/minidump_writer/linux_dumper.cc:106: if (mapping.start_addr == > auxv_[AT_SYSINFO_EHDR]) { > what tree is this patch made against ? i don't see this logic ever being > written this way in trunk/. There are several patches that this one depends upon. See https://bugzilla.mozilla.org/show_bug.cgi?id=689178#c7 for the ordered list of issues.
Sign in to reply to this message.
|
