Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1884283002:
Add debug fission support. (Closed)
|
Created:
4 years, 8 months ago by yunlian Modified:
4 years, 5 months ago CC:
google-breakpad-dev_googlegroups.com Base URL:
https://chromium.googlesource.com/breakpad/breakpad.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd debug fission support.
This added debug fission support.
It tries to find the dwp file from the debug dir /usr/lib/debug/*/debug
and read symbols from them.
Most of this patch comes from
https://critique.corp.google.com/#review/52048295
and some fixes after that.
The elf_reader.cc comes from TOT google code. I just
removed some google dependency.
Current problems from this patch
1: Some type mismatch: from uint8_t * to char *.
2: Some hack to find the .dwp file. (replace .debug with .dwp)
BUG=chromium:604440
R=dehao@google.com, ivanpe@chromium.org
Committed: https://chromium.googlesource.com/breakpad/breakpad/+/764c21f7529df70a19f1e1cb33bb9ece28e0bf8f
Patch Set 1 #
Total comments: 49
Patch Set 2 : #Patch Set 3 : Add debug fission support. #Patch Set 4 : #
Total comments: 36
Patch Set 5 : Add debug fission support #
Total comments: 3
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Add debug fission support. #
Messages
Total messages: 35 (10 generated)
Description was changed from ========== Add debug fission support. This added debug fission support. It tries to find the dwp file from the debug dir /usr/lib/debug/*/debug and read symbols from them. Current problems from this patch 1: Some type mismatch: from uint8_t * to char *. 2: Some hack to find the .dwp file. (replace .debug with .dwp) BUG=chromium:559633 ========== to ========== Add debug fission support. This added debug fission support. It tries to find the dwp file from the debug dir /usr/lib/debug/*/debug and read symbols from them. Current problems from this patch 1: Some type mismatch: from uint8_t * to char *. 2: Some hack to find the .dwp file. (replace .debug with .dwp) BUG=chromium:559633 ==========
yunlian@chromium.org changed reviewers: + mseaborn@chromium.org, vapier@chromium.org
warnings will have to be fixed before we can merge the BUG= line looks incorrect ... links to an unrelated issue patch looks like it was written by someone else ? can you document its provenance ? https://codereview.chromium.org/1884283002/diff/1/src/common/common.gyp File src/common/common.gyp (right): https://codereview.chromium.org/1884283002/diff/1/src/common/common.gyp#newco... src/common/common.gyp:67: 'dwarf/elf_reader.cc', keep it sorted ? https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2enums.h File src/common/dwarf/dwarf2enums.h (right): https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2enum... src/common/dwarf/dwarf2enums.h:270: DW_AT_GNU_macros = 0x2119, ////////////needed?????????????? what's with the crazy comment ? just delete it ? https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... File src/common/dwarf/dwarf2reader.cc (right): https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:381: reader_->ReadOneByte(start)); line wrapping is broken now after this change https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:385: reader_->ReadTwoBytes(start)); line wrapping is broken now after this change https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:389: reader_->ReadFourBytes(start)); line wrapping is broken now after this change https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:393: reader_->ReadEightBytes(start)); line wrapping is broken now after this change https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:398: str); line wrapping is broken now after this change -- just unwrap it https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:403: reader_->ReadUnsignedLEB128(start, line wrapping is broken now after this change https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:409: reader_->ReadSignedLEB128(start, &len)); line wrapping is broken now after this change https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:413: reader_->ReadAddress(start)); line wrapping is broken now after this change https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:417: reader_->ReadOffset(start)); line wrapping is broken now after this change https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:498: str); line wrapping is broken now after this change -- just unwrap it https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:513: str); line wrapping here looks broken -- just unwrap it https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:515: break; pointless break -> delete https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:522: reader_->ReadAddress(addr_ptr)); line wrapping is broken https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:524: break; pointless break -> delete https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:527: only one blank line here https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:611: inline int GetElfWidth(const ElfReader& elf) { i'm not seeing where ElfReader is defined. can you point it out ? this func seems like it should be part of ElfReader instead ... https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:626: if (stat(dwp_path_.c_str(), &statbuf)) { compare it against != 0, and put a comment in the body like: // Fall back to a split .debug file in the same directory. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:666: ByteReader reader(ENDIANNESS_LITTLE); is hardcoding little endian here correct ? shouldn't it be based on the elf endian ? https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:682: static const char* section_names[] = { mark the array itself const too static const char* const section_names[] = { https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:780: const char * section_name = elf_reader_->GetSectionName(shndx); delete the space before the * https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... File src/common/dwarf/dwarf2reader.h (right): https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.h:190: just one blank line here https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.h:293: just one blank line here https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.h:522: const uint8_t * addr_buffer_; omit the space before the *
also, missing some unittest updates
version #2 is not ready for review. I will upload a review ready version later. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... File src/common/dwarf/dwarf2reader.cc (right): https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:515: break; On 2016/04/14 22:48:47, vapier wrote: > pointless break -> delete Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:626: if (stat(dwp_path_.c_str(), &statbuf)) { On 2016/04/14 22:48:47, vapier wrote: > compare it against != 0, and put a comment in the body like: > > // Fall back to a split .debug file in the same directory. Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:666: ByteReader reader(ENDIANNESS_LITTLE); On 2016/04/14 22:48:47, vapier wrote: > is hardcoding little endian here correct ? shouldn't it be based on the elf > endian ? Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:682: static const char* section_names[] = { On 2016/04/14 22:48:47, vapier wrote: > mark the array itself const too > > static const char* const section_names[] = { Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:780: const char * section_name = elf_reader_->GetSectionName(shndx); On 2016/04/14 22:48:47, vapier wrote: > delete the space before the * Done.
https://codereview.chromium.org/1884283002/diff/1/src/common/common.gyp File src/common/common.gyp (right): https://codereview.chromium.org/1884283002/diff/1/src/common/common.gyp#newco... src/common/common.gyp:67: 'dwarf/elf_reader.cc', On 2016/04/14 22:48:46, vapier wrote: > keep it sorted ? Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2enums.h File src/common/dwarf/dwarf2enums.h (right): https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2enum... src/common/dwarf/dwarf2enums.h:270: DW_AT_GNU_macros = 0x2119, ////////////needed?????????????? On 2016/04/14 22:48:47, vapier wrote: > what's with the crazy comment ? just delete it ? Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... File src/common/dwarf/dwarf2reader.cc (right): https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:381: reader_->ReadOneByte(start)); On 2016/04/14 22:48:47, vapier wrote: > line wrapping is broken now after this change Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:385: reader_->ReadTwoBytes(start)); On 2016/04/14 22:48:47, vapier wrote: > line wrapping is broken now after this change Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:389: reader_->ReadFourBytes(start)); On 2016/04/14 22:48:47, vapier wrote: > line wrapping is broken now after this change Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:393: reader_->ReadEightBytes(start)); On 2016/04/14 22:48:47, vapier wrote: > line wrapping is broken now after this change Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:398: str); On 2016/04/14 22:48:47, vapier wrote: > line wrapping is broken now after this change -- just unwrap it Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:403: reader_->ReadUnsignedLEB128(start, On 2016/04/14 22:48:47, vapier wrote: > line wrapping is broken now after this change Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:409: reader_->ReadSignedLEB128(start, &len)); On 2016/04/14 22:48:47, vapier wrote: > line wrapping is broken now after this change Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:413: reader_->ReadAddress(start)); On 2016/04/14 22:48:47, vapier wrote: > line wrapping is broken now after this change Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:417: reader_->ReadOffset(start)); On 2016/04/14 22:48:47, vapier wrote: > line wrapping is broken now after this change Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:498: str); On 2016/04/14 22:48:47, vapier wrote: > line wrapping is broken now after this change -- just unwrap it Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:522: reader_->ReadAddress(addr_ptr)); On 2016/04/14 22:48:47, vapier wrote: > line wrapping is broken Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:524: break; On 2016/04/14 22:48:47, vapier wrote: > pointless break -> delete Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:527: On 2016/04/14 22:48:47, vapier wrote: > only one blank line here Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.cc:611: inline int GetElfWidth(const ElfReader& elf) { On 2016/04/14 22:48:47, vapier wrote: > i'm not seeing where ElfReader is defined. can you point it out ? > > this func seems like it should be part of ElfReader instead ... The elf_reader.h and elf_reader.cc were missing in the first patch. Sorry for the confusion. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... File src/common/dwarf/dwarf2reader.h (right): https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.h:190: On 2016/04/14 22:48:48, vapier wrote: > just one blank line here Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.h:293: On 2016/04/14 22:48:47, vapier wrote: > just one blank line here Done. https://codereview.chromium.org/1884283002/diff/1/src/common/dwarf/dwarf2read... src/common/dwarf/dwarf2reader.h:522: const uint8_t * addr_buffer_; On 2016/04/14 22:48:47, vapier wrote: > omit the space before the * Done.
yunlian@chromium.org changed reviewers: + llozano@chromium.org
ping?
Are you sure you have the correct bug number with "BUG=chromium:559633" there? That's a private bug that doesn't appear relevant and it was closed as "WontFix".
Description was changed from ========== Add debug fission support. This added debug fission support. It tries to find the dwp file from the debug dir /usr/lib/debug/*/debug and read symbols from them. Current problems from this patch 1: Some type mismatch: from uint8_t * to char *. 2: Some hack to find the .dwp file. (replace .debug with .dwp) BUG=chromium:559633 ========== to ========== Add debug fission support. This added debug fission support. It tries to find the dwp file from the debug dir /usr/lib/debug/*/debug and read symbols from them. Current problems from this patch 1: Some type mismatch: from uint8_t * to char *. 2: Some hack to find the .dwp file. (replace .debug with .dwp) BUG=chromium:604440 ==========
On 2016/04/26 18:25:39, Mark Seaborn wrote: > Are you sure you have the correct bug number with "BUG=chromium:559633" there? > That's a private bug that doesn't appear relevant and it was closed as > "WontFix". Sorry, the bug entry is updated.
yunlian@chromium.org changed reviewers: + ivanpe@chromium.org
This code review is quite large and some of code already exists elsewhere. Can you please, add some more information to the description regarding where the code comes from and whether it has been reviewed and tested before? https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... File src/common/dwarf/dwarf2reader.cc (right): https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:603: Please, do not leave a blank line here. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:612: void CompilationUnit::ProcessSplitDwarf() { It looks like the code that follows comes from: https://github.com/google/autofdo/blame/master/symbolize/dwarf2reader.cc Who owns the code? Has it been reviewed before? Can you please, include dehao@google.com for review. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:675: SectionMap* sections) { indent is off. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:682: for (unsigned int i = 0u; i < sizeof(section_names)/sizeof(*(section_names)); ++i) { Lines should be <= 80 chars long https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:690: base_name, std::make_pair(reinterpret_cast<const uint8_t *>(section_data), section_size))); Lines should be <= 80 chars long https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:717: version_ = byte_reader_.ReadFourBytes(reinterpret_cast<const uint8_t *>(cu_index_)); Lines should be <= 80 chars long https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:720: nslots_ = byte_reader_.ReadFourBytes(reinterpret_cast<const uint8_t *>(cu_index_) Lines should be <= 80 chars long https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:729: ncolumns_ = byte_reader_.ReadFourBytes(reinterpret_cast<const uint8_t *>(cu_index_) Lines should be <= 80 chars long https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:731: nunits_ = byte_reader_.ReadFourBytes(reinterpret_cast<const uint8_t *>(cu_index_) Lines should be <= 80 chars long https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:733: nslots_ = byte_reader_.ReadFourBytes(reinterpret_cast<const uint8_t *>(cu_index_) Lines should be <= 80 chars long https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:770: unsigned int shndx = byte_reader_.ReadFourBytes(reinterpret_cast<const uint8_t *>(shndx_list)); Lines should be <= 80 chars long Please, apply to the whole file. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... File src/common/dwarf/dwarf2reader.h (right): https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.h:194: No need to leave a blank line. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.h:201: // Start to process a compilation unit at OFFSET from the beginning of the It looks like the code that follows comes from: https://github.com/google/autofdo/blame/master/symbolize/dwarf2reader.h Who owns the code? Has it been reviewed before? Can you please, include dehao@google.com for review. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.h:243: // Called when we have an attribute whose value is a reference to indent is off https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.h:407: if (attr == DW_AT_GNU_dwo_id) I would suggest to put curly braces around these. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.h:413: // TODO(ccoutant): When we add DW_AT_ranges_base from DWARF-5, Why not TODO(yunlian): ? https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/elf_re... File src/common/dwarf/elf_reader.cc (right): https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/elf_re... src/common/dwarf/elf_reader.cc:2: // Author: chatham@google.com (Andrew Chatham) Where does this file come from? Has it been reviewed before? https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/elf_re... File src/common/dwarf/elf_reader.h (right): https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/elf_re... src/common/dwarf/elf_reader.h:1: // Copyright 2005 Google Inc. All Rights Reserved. Where does this file come from? Has it been reviewed before?
yunlian@chromium.org changed reviewers: + dehao@google.com
https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... File src/common/dwarf/dwarf2reader.cc (right): https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:603: On 2016/04/27 01:11:44, ivanpe wrote: > Please, do not leave a blank line here. Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:612: void CompilationUnit::ProcessSplitDwarf() { On 2016/04/27 01:11:44, ivanpe wrote: > It looks like the code that follows comes from: > https://github.com/google/autofdo/blame/master/symbolize/dwarf2reader.cc > > Who owns the code? Has it been reviewed before? > > Can you please, include mailto:dehao@google.com for review. Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:675: SectionMap* sections) { On 2016/04/27 01:11:44, ivanpe wrote: > indent is off. Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:682: for (unsigned int i = 0u; i < sizeof(section_names)/sizeof(*(section_names)); ++i) { On 2016/04/27 01:11:44, ivanpe wrote: > Lines should be <= 80 chars long Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:690: base_name, std::make_pair(reinterpret_cast<const uint8_t *>(section_data), section_size))); On 2016/04/27 01:11:44, ivanpe wrote: > Lines should be <= 80 chars long Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:717: version_ = byte_reader_.ReadFourBytes(reinterpret_cast<const uint8_t *>(cu_index_)); On 2016/04/27 01:11:44, ivanpe wrote: > Lines should be <= 80 chars long Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:720: nslots_ = byte_reader_.ReadFourBytes(reinterpret_cast<const uint8_t *>(cu_index_) On 2016/04/27 01:11:44, ivanpe wrote: > Lines should be <= 80 chars long Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:729: ncolumns_ = byte_reader_.ReadFourBytes(reinterpret_cast<const uint8_t *>(cu_index_) On 2016/04/27 01:11:44, ivanpe wrote: > Lines should be <= 80 chars long Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:731: nunits_ = byte_reader_.ReadFourBytes(reinterpret_cast<const uint8_t *>(cu_index_) On 2016/04/27 01:11:44, ivanpe wrote: > Lines should be <= 80 chars long Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:733: nslots_ = byte_reader_.ReadFourBytes(reinterpret_cast<const uint8_t *>(cu_index_) On 2016/04/27 01:11:44, ivanpe wrote: > Lines should be <= 80 chars long Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:770: unsigned int shndx = byte_reader_.ReadFourBytes(reinterpret_cast<const uint8_t *>(shndx_list)); On 2016/04/27 01:11:44, ivanpe wrote: > Lines should be <= 80 chars long > > Please, apply to the whole file. Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... File src/common/dwarf/dwarf2reader.h (right): https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.h:194: On 2016/04/27 01:11:44, ivanpe wrote: > No need to leave a blank line. Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.h:243: // Called when we have an attribute whose value is a reference to On 2016/04/27 01:11:44, ivanpe wrote: > indent is off Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.h:407: if (attr == DW_AT_GNU_dwo_id) On 2016/04/27 01:11:44, ivanpe wrote: > I would suggest to put curly braces around these. Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.h:413: // TODO(ccoutant): When we add DW_AT_ranges_base from DWARF-5, On 2016/04/27 01:11:45, ivanpe wrote: > Why not TODO(yunlian): ? Done. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/elf_re... File src/common/dwarf/elf_reader.cc (right): https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/elf_re... src/common/dwarf/elf_reader.cc:2: // Author: chatham@google.com (Andrew Chatham) On 2016/04/27 01:11:45, ivanpe wrote: > Where does this file come from? Has it been reviewed before? It comes from Google source tree and it has been reviewed. https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/elf_re... File src/common/dwarf/elf_reader.h (right): https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/elf_re... src/common/dwarf/elf_reader.h:1: // Copyright 2005 Google Inc. All Rights Reserved. On 2016/04/27 01:11:45, ivanpe wrote: > Where does this file come from? Has it been reviewed before? It comes from internal google tree and it has been reviewed.
https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/elf_re... File src/common/dwarf/elf_reader.cc (right): https://codereview.chromium.org/1884283002/diff/60001/src/common/dwarf/elf_re... src/common/dwarf/elf_reader.cc:2: // Author: chatham@google.com (Andrew Chatham) On 2016/04/27 16:35:29, yunlian wrote: > On 2016/04/27 01:11:45, ivanpe wrote: > > Where does this file come from? Has it been reviewed before? > > It comes from Google source tree and it has been reviewed. Great. Can you please, update the CL description with information on where this code came from. Also, in the description you should clarify which parts of the code are new and must be reviewed more carefully. This is a very large CL and such information could speed up the review process and people can focus on the more important parts. https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... File src/common/dwarf/dwarf2reader.cc (right): https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:690: base_name, std::make_pair(reinterpret_cast<const uint8_t *>(section_data), Please, keep lines <= 80 chars. https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... File src/common/dwarf/dwarf2reader.h (right): https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.h:414: // TODO(yunlian): When we add DW_AT_ranges_base from DWARF-5, This comment should go after the }
lgtm
Description was changed from ========== Add debug fission support. This added debug fission support. It tries to find the dwp file from the debug dir /usr/lib/debug/*/debug and read symbols from them. Current problems from this patch 1: Some type mismatch: from uint8_t * to char *. 2: Some hack to find the .dwp file. (replace .debug with .dwp) BUG=chromium:604440 ========== to ========== Add debug fission support. This added debug fission support. It tries to find the dwp file from the debug dir /usr/lib/debug/*/debug and read symbols from them. Most of this patch comes from CL:52048295 and some fixes after that. The elf_reader.cc comes from TOT google code. I just removed some google dependency. Current problems from this patch 1: Some type mismatch: from uint8_t * to char *. 2: Some hack to find the .dwp file. (replace .debug with .dwp) BUG=chromium:604440 ==========
The description is updated. https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... File src/common/dwarf/dwarf2reader.cc (right): https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... src/common/dwarf/dwarf2reader.cc:690: base_name, std::make_pair(reinterpret_cast<const uint8_t *>(section_data), On 2016/04/27 18:02:07, ivanpe wrote: > Please, keep lines <= 80 chars. Done.
On 2016/05/02 21:17:38, yunlian wrote: > The description is updated. > > https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... > File src/common/dwarf/dwarf2reader.cc (right): > > https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... > src/common/dwarf/dwarf2reader.cc:690: base_name, > std::make_pair(reinterpret_cast<const uint8_t *>(section_data), > On 2016/04/27 18:02:07, ivanpe wrote: > > Please, keep lines <= 80 chars. > > Done. Can you please make the CL links in the description clickable (URL). LGTM otherwise.
Description was changed from ========== Add debug fission support. This added debug fission support. It tries to find the dwp file from the debug dir /usr/lib/debug/*/debug and read symbols from them. Most of this patch comes from CL:52048295 and some fixes after that. The elf_reader.cc comes from TOT google code. I just removed some google dependency. Current problems from this patch 1: Some type mismatch: from uint8_t * to char *. 2: Some hack to find the .dwp file. (replace .debug with .dwp) BUG=chromium:604440 ========== to ========== Add debug fission support. This added debug fission support. It tries to find the dwp file from the debug dir /usr/lib/debug/*/debug and read symbols from them. Most of this patch comes from https://critique.corp.google.com/#review/52048295 and some fixes after that. The elf_reader.cc comes from TOT google code. I just removed some google dependency. Current problems from this patch 1: Some type mismatch: from uint8_t * to char *. 2: Some hack to find the .dwp file. (replace .debug with .dwp) BUG=chromium:604440 ==========
On 2016/05/03 01:24:31, ivanpe wrote: > On 2016/05/02 21:17:38, yunlian wrote: > > The description is updated. > > > > > https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... > > File src/common/dwarf/dwarf2reader.cc (right): > > > > > https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... > > src/common/dwarf/dwarf2reader.cc:690: base_name, > > std::make_pair(reinterpret_cast<const uint8_t *>(section_data), > > On 2016/04/27 18:02:07, ivanpe wrote: > > > Please, keep lines <= 80 chars. > > > > Done. > > Can you please make the CL links in the description clickable (URL). LGTM > otherwise. Done, thanks.
On 2016/05/03 16:20:48, yunlian wrote: > On 2016/05/03 01:24:31, ivanpe wrote: > > On 2016/05/02 21:17:38, yunlian wrote: > > > The description is updated. > > > > > > > > > https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... > > > File src/common/dwarf/dwarf2reader.cc (right): > > > > > > > > > https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... > > > src/common/dwarf/dwarf2reader.cc:690: base_name, > > > std::make_pair(reinterpret_cast<const uint8_t *>(section_data), > > > On 2016/04/27 18:02:07, ivanpe wrote: > > > > Please, keep lines <= 80 chars. > > > > > > Done. > > > > Can you please make the CL links in the description clickable (URL). LGTM > > otherwise. > > Done, thanks. LGTM
On 2016/05/03 16:24:43, ivanpe wrote: > On 2016/05/03 16:20:48, yunlian wrote: > > On 2016/05/03 01:24:31, ivanpe wrote: > > > On 2016/05/02 21:17:38, yunlian wrote: > > > > The description is updated. > > > > > > > > > > > > > > https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... > > > > File src/common/dwarf/dwarf2reader.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1884283002/diff/80001/src/common/dwarf/dwarf2... > > > > src/common/dwarf/dwarf2reader.cc:690: base_name, > > > > std::make_pair(reinterpret_cast<const uint8_t *>(section_data), > > > > On 2016/04/27 18:02:07, ivanpe wrote: > > > > > Please, keep lines <= 80 chars. > > > > > > > > Done. > > > > > > Can you please make the CL links in the description clickable (URL). LGTM > > > otherwise. > > > > Done, thanks. > > LGTM Can some one commit it for me? I could not commit it. Thanks
Description was changed from ========== Add debug fission support. This added debug fission support. It tries to find the dwp file from the debug dir /usr/lib/debug/*/debug and read symbols from them. Most of this patch comes from https://critique.corp.google.com/#review/52048295 and some fixes after that. The elf_reader.cc comes from TOT google code. I just removed some google dependency. Current problems from this patch 1: Some type mismatch: from uint8_t * to char *. 2: Some hack to find the .dwp file. (replace .debug with .dwp) BUG=chromium:604440 ========== to ========== Add debug fission support. This added debug fission support. It tries to find the dwp file from the debug dir /usr/lib/debug/*/debug and read symbols from them. Most of this patch comes from https://critique.corp.google.com/#review/52048295 and some fixes after that. The elf_reader.cc comes from TOT google code. I just removed some google dependency. Current problems from this patch 1: Some type mismatch: from uint8_t * to char *. 2: Some hack to find the .dwp file. (replace .debug with .dwp) BUG=chromium:604440 R=dehao@google.com, ivanpe@chromium.org Committed: https://chromium.googlesource.com/breakpad/breakpad/+/764c21f7529df70a19f1e1c... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) manually as 764c21f7529df70a19f1e1cb33bb9ece28e0bf8f (presubmit successful).
Message was sent while issue was closed.
On 2016/05/04 18:09:49, yunlian wrote: > Committed patchset #8 (id:140001) manually as > 764c21f7529df70a19f1e1cb33bb9ece28e0bf8f (presubmit successful). This change appears to have broken the src/tools/mac/dump_syms/dump_syms.xcodeproj project for OSX.
Message was sent while issue was closed.
On 2016/05/06 18:25:20, thomasn wrote: > On 2016/05/04 18:09:49, yunlian wrote: > > Committed patchset #8 (id:140001) manually as > > 764c21f7529df70a19f1e1cb33bb9ece28e0bf8f (presubmit successful). > > This change appears to have broken the > src/tools/mac/dump_syms/dump_syms.xcodeproj project for OSX. How can I reproduce that?
Message was sent while issue was closed.
On 2016/05/06 20:55:04, yunlian wrote: > On 2016/05/06 18:25:20, thomasn wrote: > > On 2016/05/04 18:09:49, yunlian wrote: > > > Committed patchset #8 (id:140001) manually as > > > 764c21f7529df70a19f1e1cb33bb9ece28e0bf8f (presubmit successful). > > > > This change appears to have broken the > > src/tools/mac/dump_syms/dump_syms.xcodeproj project for OSX. > > How can I reproduce that? I'm using Xcode 7.3.1 on OSX 10.11.4. If I try to build that project with the revision just before these changes, it succeeds. If I try to build it with this revision, I first get a compiler error about ambiguous != operator. If I fix that, I then get linker errors related to ElfReader that I can't easily resolve.
Message was sent while issue was closed.
On 2016/05/07 03:50:19, thomasn wrote: > On 2016/05/06 20:55:04, yunlian wrote: > > On 2016/05/06 18:25:20, thomasn wrote: > > > On 2016/05/04 18:09:49, yunlian wrote: > > > > Committed patchset #8 (id:140001) manually as > > > > 764c21f7529df70a19f1e1cb33bb9ece28e0bf8f (presubmit successful). > > > > > > This change appears to have broken the > > > src/tools/mac/dump_syms/dump_syms.xcodeproj project for OSX. > > > > How can I reproduce that? > > I'm using Xcode 7.3.1 on OSX 10.11.4. If I try to build that project with the > revision just before these changes, it succeeds. If I try to build it with this > revision, I first get a compiler error about ambiguous != operator. If I fix > that, I then get linker errors related to ElfReader that I can't easily resolve. Could you please send me the error logs about the compiler error and linker error? Thanks
Message was sent while issue was closed.
ted.mielczarek@gmail.com changed reviewers: + ted.mielczarek@gmail.com
Message was sent while issue was closed.
I didn't notice this patch when it went in, but it seems a little silly that we
added elf_reader.{cc,h} when we already had ELF parsing code in the tree:
https://chromium.googlesource.com/breakpad/breakpad/+/master/src/common/linux...
https://chromium.googlesource.com/breakpad/breakpad/+/master/src/common/linux...
Especially since this code is used in dump_symbols, where we're already using
that other ELF parsing code.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
