Issue 393002: Rework dump_symbols.cc to handle cross-word-size dumping (Closed)
|
Created:
11 years, 10 months ago by Ted Mielczarek Modified:
11 years, 9 months ago CC:
google-breakpad-dev_googlegroups.com Visibility:
Public. |
DescriptionThis patch reworks dump_symbols.cc to use templates+traits to handle dumping cross-word-size binaries. It's kind of invasive, but the diff wound up not being as bad as I thought it would. I had to get rid of your FixAddress trick, but I replaced it with some template trickery. I ran the tests on both a 64-bit and 32-bit build and they pass, so that's nice. Patch Set 1 #
Total comments: 10
Patch Set 2 : Revised formatting #
Total comments: 12
Patch Set 3 : Comments addressed #Patch Set 4 : Comments addressed #
Total comments: 1
MessagesTotal messages: 20
Hi, Ted. I'm going to be in Japan until the end of the month, and only intermittently on-line. I probably won't be able to review this quickly. (Great work, though!) On Jun 7, 2012 6:21 AM, <ted.mielczarek@gmail.com> wrote: > Reviewers: jimb, > > Description: > This patch reworks dump_symbols.cc to use templates+traits to handle > dumping cross-word-size binaries. It's kind of invasive, but the diff > wound up not being as bad as I thought it would. I had to get rid of > your FixAddress trick, but I replaced it with some template trickery. I > ran the tests on both a 64-bit and 32-bit build and they pass, so that's > nice. > > Please review this at http://breakpad.appspot.com/**393002/<http://breakpad.appspot.com/393002/> > > Affected files: > M src/common/linux/dump_symbols.**cc > M src/common/linux/dump_symbols_**unittest.cc > A src/common/linux/elfutils-inl.**h > M src/common/linux/elfutils.cc > M src/common/linux/elfutils.h > > >
Sign in to reply to this message.
Thanks for the heads up! No rush here, this just kept annoying me trying to dump 32-bit binaries on my 64-bit Linux. -Ted On Sat, Jun 9, 2012 at 2:54 AM, Jim Blandy <jimb@red-bean.com> wrote: > Hi, Ted. I'm going to be in Japan until the end of the month, and only > intermittently on-line. I probably won't be able to review this quickly. > > (Great work, though!) > > On Jun 7, 2012 6:21 AM, <ted.mielczarek@gmail.com> wrote: >> >> Reviewers: jimb, >> >> Description: >> This patch reworks dump_symbols.cc to use templates+traits to handle >> dumping cross-word-size binaries. It's kind of invasive, but the diff >> wound up not being as bad as I thought it would. I had to get rid of >> your FixAddress trick, but I replaced it with some template trickery. I >> ran the tests on both a 64-bit and 32-bit build and they pass, so that's >> nice. >> >> Please review this at http://breakpad.appspot.com/393002/ >> >> Affected files: >> M src/common/linux/dump_symbols.cc >> M src/common/linux/dump_symbols_unittest.cc >> A src/common/linux/elfutils-inl.h >> M src/common/linux/elfutils.cc >> M src/common/linux/elfutils.h >> >> >
Sign in to reply to this message.
For the record, I confirm that this patch, when applied after 392002, builds correctly on Linux-x86_64. Moreover, I can now use dump_syms to dump the symbols 32-bit Android ARM shared libraries when running the host 64-bit program. It'd be nice if these two patches could be submitted soon. Thanks.
Sign in to reply to this message.
On Wed, Jun 27, 2012 at 11:28 AM, <digit@chromium.org> wrote: > For the record, I confirm that this patch, when applied after 392002, > builds correctly on Linux-x86_64. > > Moreover, I can now use dump_syms to dump the symbols 32-bit Android ARM > shared libraries when running the host 64-bit program. > > It'd be nice if these two patches could be submitted soon. If you want to track down another reviewer (maybe you can cajole Mark into reviewing it) that'd be fine with me. I asked jimb because he wrote most of this code, but it's not a hard requirement. -Ted
Sign in to reply to this message.
Sure, I just added Mark on cc to this reply. Mark, can we ask you to take a look at 392002<https://breakpad.appspot.com/392002/>and 392003 <https://breakpad.appspot.com/393002/>? Thanks in advance. - Digit On Wed, Jun 27, 2012 at 6:03 PM, Ted Mielczarek <ted.mielczarek@gmail.com>wrote: > On Wed, Jun 27, 2012 at 11:28 AM, <digit@chromium.org> wrote: > > For the record, I confirm that this patch, when applied after 392002, > > builds correctly on Linux-x86_64. > > > > Moreover, I can now use dump_syms to dump the symbols 32-bit Android ARM > > shared libraries when running the host 64-bit program. > > > > It'd be nice if these two patches could be submitted soon. > > If you want to track down another reviewer (maybe you can cajole Mark > into reviewing it) that'd be fine with me. I asked jimb because he > wrote most of this code, but it's not a hard requirement. > > -Ted >
Sign in to reply to this message.
This looks mostly good, except for a ton of formatting things. https://breakpad.appspot.com/393002/diff/1/src/common/linux/dump_symbols.cc File src/common/linux/dump_symbols.cc (right): https://breakpad.appspot.com/393002/diff/1/src/common/linux/dump_symbols.cc#n... src/common/linux/dump_symbols.cc:159: bool Why’s this so lonely? https://breakpad.appspot.com/393002/diff/1/src/common/linux/dump_symbols.cc#n... src/common/linux/dump_symbols.cc:172: GetOffset<ElfClass,uint8_t>(elf_header, stab_section->sh_offset); Spaces between commas, even as template arguments. Same on line 174. And elsewhere in this file. https://breakpad.appspot.com/393002/diff/1/src/common/linux/dump_symbols.cc#n... src/common/linux/dump_symbols.cc:204: bool This one is lonely too! It belongs on the next line. Throughout this file. https://breakpad.appspot.com/393002/diff/1/src/common/linux/dump_symbols.cc#n... src/common/linux/dump_symbols.cc:225: std::string name = GetOffset<ElfClass,char>(elf_header, section_names->sh_offset) + section->sh_name; Fix this and the next line, because they’re too long. https://breakpad.appspot.com/393002/diff/1/src/common/linux/dump_symbols.cc#n... src/common/linux/dump_symbols.cc:518: = FindElfSectionByName<ElfClass>(".stab", SHT_PROGBITS, You’re taking code that’s correct (four space indent on continuation lines) and making it less correct (two spaces). And you’re introducing lots of new code that uses two spaces instead of four for continuations. Fix! https://breakpad.appspot.com/393002/diff/1/src/common/linux/dump_symbols.cc#n... src/common/linux/dump_symbols.cc:607: ReadDebugLink<ElfClass>(GetOffset<ElfClass,char>(elf_header, gnu_debuglink_section->sh_offset), 80 https://breakpad.appspot.com/393002/diff/1/src/common/linux/dump_symbols.cc#n... src/common/linux/dump_symbols.cc:817: return false; This is pointless. https://breakpad.appspot.com/393002/diff/1/src/common/linux/elfutils-inl.h File src/common/linux/elfutils-inl.h (right): https://breakpad.appspot.com/393002/diff/1/src/common/linux/elfutils-inl.h#ne... src/common/linux/elfutils-inl.h:70: } } // namespace google_breakpad
Sign in to reply to this message.
Thanks, I'll clean up the formatting. https://breakpad.appspot.com/393002/diff/1/src/common/linux/dump_symbols.cc File src/common/linux/dump_symbols.cc (right): https://breakpad.appspot.com/393002/diff/1/src/common/linux/dump_symbols.cc#n... src/common/linux/dump_symbols.cc:159: bool On 2012/06/29 16:36:29, Mark Mentovai wrote: > Why’s this so lonely? I think I confused Mozilla style with Google style. Sure wish there was one true C++ style! https://breakpad.appspot.com/393002/diff/1/src/common/linux/dump_symbols.cc#n... src/common/linux/dump_symbols.cc:518: = FindElfSectionByName<ElfClass>(".stab", SHT_PROGBITS, On 2012/06/29 16:36:29, Mark Mentovai wrote: > You’re taking code that’s correct (four space indent on continuation lines) and > making it less correct (two spaces). And you’re introducing lots of new code > that uses two spaces instead of four for continuations. Fix! Ugh, I think that was emacs unhelpfully reformatting my code.
Sign in to reply to this message.
Any chance to see this patch (and the one it depends on) rebased and submitted at some point? We need something like this for the Chrome on Android upstreaming. That's not too urgent, and I'm leaving to a two-week vacation tomorrow, just would like an ETA if possible. Thanks.
Sign in to reply to this message.
On Fri, Jul 13, 2012 at 6:57 AM, <digit@chromium.org> wrote: > Any chance to see this patch (and the one it depends on) rebased and > submitted at some point? We need something like this for the Chrome on > Android upstreaming. > > That's not too urgent, and I'm leaving to a two-week vacation tomorrow, > just would like an ETA if possible. Thanks. I got sidetracked on some more pressing things, but I should be able to get these updated and landed in the next week.
Sign in to reply to this message.
I believe I have exorcised the formatting demons.
Sign in to reply to this message.
https://breakpad.appspot.com/393002/diff/13001/src/common/linux/dump_symbols.cc File src/common/linux/dump_symbols.cc (right): https://breakpad.appspot.com/393002/diff/13001/src/common/linux/dump_symbols.... src/common/linux/dump_symbols.cc:164: Module* module) { Just noticed this indent was wrong after submitting the patch. Fixed locally.
Sign in to reply to this message.
I think this is great. https://breakpad.appspot.com/393002/diff/13001/src/common/linux/dump_symbols.cc File src/common/linux/dump_symbols.cc (right): https://breakpad.appspot.com/393002/diff/13001/src/common/linux/dump_symbols.... src/common/linux/dump_symbols.cc:144: const typename ElfClass::Phdr* program_headers, Arguments that continue like this get a 4-space indent, not 2. https://breakpad.appspot.com/393002/diff/13001/src/common/linux/dump_symbols.... src/common/linux/dump_symbols.cc:226: + section->sh_name; This looks weird too. We’d normally write: string name = GetOffset<ElfClass, char>(elf_header, section_names->sh_offset) + section->sh_name; https://breakpad.appspot.com/393002/diff/13001/src/common/linux/dump_symbols.... src/common/linux/dump_symbols.cc:237: // This should never have been called if the file doesn't have a Thank you. :) https://breakpad.appspot.com/393002/diff/13001/src/common/linux/dump_symbols.... src/common/linux/dump_symbols.cc:756: if (!LoadELF(debuglink_file, &debug_map_wrapper, (void**)&debug_elf_header)) Avoid (c_style)casts and prefer cplusplus<style>(casts). https://breakpad.appspot.com/393002/diff/13001/src/common/linux/elfutils-inl.h File src/common/linux/elfutils-inl.h (right): https://breakpad.appspot.com/393002/diff/13001/src/common/linux/elfutils-inl.... src/common/linux/elfutils-inl.h:29: This file needs #include guards. https://breakpad.appspot.com/393002/diff/13001/src/common/linux/elfutils-inl.... src/common/linux/elfutils-inl.h:40: + offset); You put other operators before the line break (like the chain of &&s below), be consistent. https://breakpad.appspot.com/393002/diff/13001/src/common/linux/elfutils-inl.... src/common/linux/elfutils-inl.h:55: int name_len = strlen(name); Are you supposed to use my_strlen for this? You already have linux_libc_support.h.
Sign in to reply to this message.
https://breakpad.appspot.com/393002/diff/13001/src/common/linux/dump_symbols.cc File src/common/linux/dump_symbols.cc (right): https://breakpad.appspot.com/393002/diff/13001/src/common/linux/dump_symbols.... src/common/linux/dump_symbols.cc:144: const typename ElfClass::Phdr* program_headers, On 2012/07/18 15:35:13, Mark Mentovai wrote: > Arguments that continue like this get a 4-space indent, not 2. Ugh, thought I got all of these! https://breakpad.appspot.com/393002/diff/13001/src/common/linux/dump_symbols.... src/common/linux/dump_symbols.cc:226: + section->sh_name; On 2012/07/18 15:35:13, Mark Mentovai wrote: > This looks weird too. We’d normally write: > > string name = GetOffset<ElfClass, char>(elf_header, > section_names->sh_offset) + > section->sh_name; Thanks, I wasn't sure what the best way to format this was. https://breakpad.appspot.com/393002/diff/13001/src/common/linux/dump_symbols.... src/common/linux/dump_symbols.cc:237: // This should never have been called if the file doesn't have a On 2012/07/18 15:35:13, Mark Mentovai wrote: > Thank you. :) I figured if I was going to touch half of this file, I might as well clean up some comments. https://breakpad.appspot.com/393002/diff/13001/src/common/linux/elfutils-inl.h File src/common/linux/elfutils-inl.h (right): https://breakpad.appspot.com/393002/diff/13001/src/common/linux/elfutils-inl.... src/common/linux/elfutils-inl.h:55: int name_len = strlen(name); On 2012/07/18 15:35:13, Mark Mentovai wrote: > Are you supposed to use my_strlen for this? You already have > linux_libc_support.h. Good catch.
Sign in to reply to this message.
I don’t see any new changes here. Just poke this again by e-mail when the next version (should be final version) is up.
Sign in to reply to this message.
Okay, this should be good to go.
Sign in to reply to this message.
Awesome. LGTM. http://breakpad.appspot.com/393002/diff/24001/src/common/linux/dump_symbols_u... File src/common/linux/dump_symbols_unittest.cc (right): http://breakpad.appspot.com/393002/diff/24001/src/common/linux/dump_symbols_u... src/common/linux/dump_symbols_unittest.cc:161: Remove the blank line at the end of the file.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
