Issue 5714003: Microdump processing implementation (Closed)
|
Created:
10 years, 2 months ago by mmandlis Modified:
10 years ago CC:
google-breakpad-dev_googlegroups.com Base URL:
http://google-breakpad.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionMicrodump processing implementation According to design document: http://goo.gl/B3wIRN This is an initial implementation version, support ARM architecture only. BUG=chromium:410294 R=primiano@chromium.org Committed: https://code.google.com/p/google-breakpad/source/detail?r=1403 Patch Set 1 #Patch Set 2 : Add microdump.h/cc files #Patch Set 3 : Microdump processing implmentation and unit tests. #Patch Set 4 : Minor cleanups. #Patch Set 5 : Add test symbols file. #
Total comments: 47
Patch Set 6 : Address Primiano's comments: Derive MicrodumpModuleList from BasicCodeModules, parse the OS info to… #Patch Set 7 : Add test case for unsupported architecture. #
Total comments: 47
Patch Set 8 : Address Primiano's comments for ps7: update types, comments, whitespaces etc. #
Total comments: 8
Patch Set 9 : Address final comments. Update minidump to skip stack header line. #
Created: 10 years ago
(Patch set is too large to download)
MessagesTotal messages: 16
In general looks good an make sense. Just a note. I thing I'm going to revisit slightly the text output of microdump (the CPU = ... parts). Starting from today I am re-taking over the work on the client side, so everything should look more clear to me in a few days.
Sign in to reply to this message.
... and, finally! Can you, please, start reviewing? (Hopefully, the review will take shorter than putting the CL together, I think it is one of my most prolonged ))) PS: I did include the symbols file under testdata/symbols, but while uploading it says: "Not uploading the patch for src/processor/testdata/symbols/microdump/libchromeshell.so/76304586D0CD2C8FF899C602BF1756A20/libchromeshell.so.sym because the file is too large." I'm not sure if it means that the file only doesn't show here, or it won't be submitted either. Ideas? PPS: another thing I couldn't figure out yet is regex matching, so I did just straight-forward strings comparison. On 2014/09/18 10:24:29, primiano wrote: > In general looks good an make sense. > Just a note. I thing I'm going to revisit slightly the text output of microdump > (the CPU = ... parts). > Starting from today I am re-taking over the work on the client side, so > everything should look more clear to me in a few days.
Sign in to reply to this message.
Thanks you very much for putting this together. Also many thanks for the test!!! it's awesome. I didn't have time for a full review (will continue later after meetings / tomorrow) but I have a first batch of comments. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... File src/google_breakpad/processor/microdump.h (right): https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:34: // - the crashing thread stack + System information (OS type / version) https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:57: void Add(const CodeModule* module); I think it's preferrable to have a virtual dtor here (a trivial inline one is fine). Rationale: if you omit it, you will still get a virtual dtor (so it will be fine) just because your base class (CodeModules) has a virtual dtor itself. But if somebody tomorrow removes the virtual dtor from the CodeModules and your MicrodumpModuleList is used via a base pointer (e.g., CodeModules* cm = new MicrodumpModuleList()), you will leak the modules_ vector here. I created a smaller repro case here: http://pastebin.com/DiMAvz3C https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:58: I think these extra \n spacing is unnecessary, at least by looking at similar other headers. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:70: private: Nit: you need an extra \n here, i.e. class Foo { public: aa(); bb(); private: cc(); } https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:72: MicrodumpModules modules_; Hmm I wonder if instead of reimplementing all this stuff here you could extend BasicCodeModules, which already implements most of the things you need (%add). Perhaps you could make a small cange to BasicCodeModules and turn private -> protected, and extend the rest here. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:80: Nit: I think you can remove this extra \n between ctor and dtor. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:81: virtual ~MicrodumpContext(); Eventually, instead, you could safely omit the dtor here because you don't have any state (field) in this class. However, I'm fine both ways (i.e. it you want t keep it for sake of consistency). If you decide to keep it, I think you can just make it inline {} and avoid the bolierplate in the .cc file. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:100: MicrodumpMemoryRegion(); Same rationale as MicrodumpModuleList above, I think you want a virtual dtor here (to correctly destroy the contents_ string). https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:107: uint64_t GetBase() const; Nit: All these methods should be virtual, as they are such in MemoryRegion. I know that minidump.h doesn't have that, but I think that is just because we don't have a clang checker plugin there. P.S. As a matter of facts it will work anyways as intended, as IIRC non virtual methods which extend virtual ones of a base class become implicitly virtual themselves. But it's nice to make it evident that they are virtual (instead of having to walk the class hieararchy back to find out) and is mandated by our code style: "(In C++ 11) Use eitehr override, virtual, final... Older (pre-C++11) code will use the virtual keyword as an inferior alternative annotation." https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:117: private: nit: add \n before private: https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:131: static const char kMicrodumpBegin[]; Do you need these constants to be accessed from the outside? if not it would be probably better to move them to the .cc file into an anonymous namespace, as they sound much more an implementation detail. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:140: virtual DumpContext* GetContext() { return context_.get(); } Nit: I think you want a \n between the dtor and the GetContext. Refer to: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Format https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc File src/processor/microdump.cc (right): https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:51: uint64_t HexStrToUL(const string& str) { I think you want these three helper functions, which are used only in this translation unit, to be in an anonymous namespace. https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:99: return (module_count() > sequence) ? modules_.at(sequence) : NULL; Hmm I think that GetModuleAtSequence expects the modules to be ordered by address, not by insertion order. Either you replicate the behavior of BasicCodeModules, which uses a RangeMap or this sounds another argument in favor of extending it. https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:116: MicrodumpContext::MicrodumpContext() { See my comments in the other file. I think you can remove the ctor and make the virtual dtor inline https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:122: void MicrodumpContext::SetContextX86(MDRawContextX86* x86) { Hmm do you really need all these SetContext functions? And do they need to be public? I wonder whether it would be just more compact and readable copying these lines below into the ::SetContext fn, and remove them from here and the .h https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:271: printf("%s\n", contents_.c_str()); Since it's a .cc file you probably want cout << contents_ << endl ? https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:329: stack_region_->Init(stack_start, If you move this below (after all the if-s and after the while) you can avoid re-init'ing the stack_region at every chunk and do it in one pass below (it's not super nice to see something beign re-initialized at every loop, even if in this case it works anyway) https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:332: string cpu_state_str = line; you could abbreviate and make it more efficient by doing: string cpu_statestr(line, found + strlen(kCpuKey)); instead of creating a copy and erasing it. https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:337: string mmap_line = line; same thing here
Sign in to reply to this message.
Thanks you very much for putting this together. Also many thanks for the test!!! it's awesome. I didn't have time for a full review (will continue later after meetings / tomorrow) but I have a first batch of comments.
Sign in to reply to this message.
Some extra comments on microdump.cc, more on the architectural side. https://breakpad.appspot.com/5714003/diff/360001/Makefile.am File Makefile.am (right): https://breakpad.appspot.com/5714003/diff/360001/Makefile.am#newcode171 Makefile.am:171: src/google_breakpad/processor/microdump.h \ ahha! The spaces are tricking you again again :) https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc File src/processor/microdump.cc (right): https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:308: } I would also add a check to skip a line if it doesn't contain the "google-breakpad" string. Remember that there is no guarantee that the microdump is atomic as whole (each individual line is, but not the all set of lines). Another process could print a line in the middle so you might end up erroneously matching a line such as "AndroidProcessBlah: Foo: O Bar" https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:311: // This is for future use. I had a second thought, and I think this translation unit could be slightly reworded and abbreviated at the same time. I don't like a lot the idea that we rely on the size fo the context to figure out which architecture it is for. What I would do is something like the following: 1) Tokenize the "O:" os line here. You just need to keep the first field (e.g. arm7l) that will tell you the architecture (you can throw away the rest for the moment). 2) Have MicrodumpContext being a simple derivate class of DumpContext, and make Microdump a friend of that, so it can access the SetContext... methods which are already there. 3) In the kCpuKey case below, you can then do something like: if (arch == "arm7l") { MDRawContextARM* arm = new MDRawContextARM(); memcpy.... context_->SetContextArm(arm) } I would prefer that you don't write the code for the remaining architectures (i.e. if architecture != arm7l just bail out). We can add it later as we start supporting new platforms. For the moment it would be extremely untested and misleading if somebody else tries to use it.
Sign in to reply to this message.
Address comments, PTAL https://breakpad.appspot.com/5714003/diff/360001/Makefile.am File Makefile.am (right): https://breakpad.appspot.com/5714003/diff/360001/Makefile.am#newcode171 Makefile.am:171: src/google_breakpad/processor/microdump.h \ On 2014/11/14 02:43:39, primiano wrote: > ahha! The spaces are tricking you again again :) It's not the spaces, it's the tabs that are tricking me)) Do you see what I see, the weird red characters? I can't get rid of it (it only shows here, in all my editors is looks alright) What editor do you use for editing this file? Done. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... File src/google_breakpad/processor/microdump.h (right): https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:34: // - the crashing thread stack On 2014/11/13 22:59:33, primiano wrote: > + System information (OS type / version) Done. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:57: void Add(const CodeModule* module); On 2014/11/13 22:59:33, primiano wrote: > I think it's preferrable to have a virtual dtor here (a trivial inline one is > fine). Rationale: > if you omit it, you will still get a virtual dtor (so it will be fine) just > because your base class (CodeModules) has a virtual dtor itself. > But if somebody tomorrow removes the virtual dtor from the CodeModules and your > MicrodumpModuleList is used via a base pointer (e.g., CodeModules* cm = new > MicrodumpModuleList()), you will leak the modules_ vector here. > > I created a smaller repro case here: > http://pastebin.com/DiMAvz3C Done. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:58: On 2014/11/13 22:59:33, primiano wrote: > I think these extra \n spacing is unnecessary, at least by looking at similar > other headers. Done. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:70: private: On 2014/11/13 22:59:33, primiano wrote: > Nit: you need an extra \n here, i.e. > > class Foo { > public: > aa(); > bb(); > > private: > cc(); > } Done. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:72: MicrodumpModules modules_; On 2014/11/13 22:59:33, primiano wrote: > Hmm I wonder if instead of reimplementing all this stuff here you could extend > BasicCodeModules, which already implements most of the things you need (%add). > Perhaps you could make a small cange to BasicCodeModules and turn private -> > protected, and extend the rest here. Awesome idea, thanks a lot, it's much cleaner now. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:80: On 2014/11/13 22:59:33, primiano wrote: > Nit: I think you can remove this extra \n between ctor and dtor. Done. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:81: virtual ~MicrodumpContext(); On 2014/11/13 22:59:33, primiano wrote: > Eventually, instead, you could safely omit the dtor here because you don't have > any state (field) in this class. However, I'm fine both ways (i.e. it you want t > keep it for sake of consistency). > If you decide to keep it, I think you can just make it inline {} and avoid the > bolierplate in the .cc file. Done. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:100: MicrodumpMemoryRegion(); On 2014/11/13 22:59:33, primiano wrote: > Same rationale as MicrodumpModuleList above, I think you want a virtual dtor > here (to correctly destroy the contents_ string). Done. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:107: uint64_t GetBase() const; On 2014/11/13 22:59:33, primiano wrote: > Nit: All these methods should be virtual, as they are such in MemoryRegion. > I know that minidump.h doesn't have that, but I think that is just because we > don't have a clang checker plugin there. > > P.S. As a matter of facts it will work anyways as intended, as IIRC non virtual > methods which extend virtual ones of a base class become implicitly virtual > themselves. But it's nice to make it evident that they are virtual (instead of > having to walk the class hieararchy back to find out) and is mandated by our > code style: "(In C++ 11) Use eitehr override, virtual, final... Older > (pre-C++11) code will use the virtual keyword as an inferior alternative > annotation." Done. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:117: private: On 2014/11/13 22:59:33, primiano wrote: > nit: add \n before private: Done. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:131: static const char kMicrodumpBegin[]; On 2014/11/13 22:59:33, primiano wrote: > Do you need these constants to be accessed from the outside? if not it would be > probably better to move them to the .cc file into an anonymous namespace, as > they sound much more an implementation detail. Done. https://breakpad.appspot.com/5714003/diff/360001/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:140: virtual DumpContext* GetContext() { return context_.get(); } On 2014/11/13 22:59:33, primiano wrote: > Nit: I think you want a \n between the dtor and the GetContext. Refer to: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Class_Format Done. https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc File src/processor/microdump.cc (right): https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:51: uint64_t HexStrToUL(const string& str) { On 2014/11/13 22:59:33, primiano wrote: > I think you want these three helper functions, which are used only in this > translation unit, to be in an anonymous namespace. Done. https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:99: return (module_count() > sequence) ? modules_.at(sequence) : NULL; On 2014/11/13 22:59:33, primiano wrote: > Hmm I think that GetModuleAtSequence expects the modules to be ordered by > address, not by insertion order. Either you replicate the behavior of > BasicCodeModules, which uses a RangeMap or this sounds another argument in favor > of extending it. Done. https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:116: MicrodumpContext::MicrodumpContext() { On 2014/11/13 22:59:33, primiano wrote: > See my comments in the other file. I think you can remove the ctor and make the > virtual dtor inline Done. https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:122: void MicrodumpContext::SetContextX86(MDRawContextX86* x86) { I did a bit of an opposite - I got rid of the SetContext(..) function in favor of the if {} blocks in the MicrodumpConstructor (as I check the architecture there). The SetContextXXX(..) methods (I only left the single one that I'm using for now) are overriding the base classes' methods (made them virtual). How does it look to you now? On 2014/11/13 22:59:33, primiano wrote: > Hmm do you really need all these SetContext functions? And do they need to be > public? > I wonder whether it would be just more compact and readable copying these lines > below into the ::SetContext fn, and remove them from here and the .h https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:271: printf("%s\n", contents_.c_str()); On 2014/11/13 22:59:33, primiano wrote: > Since it's a .cc file you probably want cout << contents_ << endl ? Done. https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:308: } On 2014/11/14 02:43:39, primiano wrote: > I would also add a check to skip a line if it doesn't contain the > "google-breakpad" string. > Remember that there is no guarantee that the microdump is atomic as whole (each > individual line is, but not the all set of lines). > Another process could print a line in the middle so you might end up erroneously > matching a line such as "AndroidProcessBlah: Foo: O Bar" Done. https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:311: // This is for future use. On 2014/11/14 02:43:39, primiano wrote: > I had a second thought, and I think this translation unit could be slightly > reworded and abbreviated at the same time. I don't like a lot the idea that we > rely on the size fo the context to figure out which architecture it is for. > What I would do is something like the following: > > 1) Tokenize the "O:" os line here. You just need to keep the first field (e.g. > arm7l) that will tell you the architecture (you can throw away the rest for the > moment). > > 2) Have MicrodumpContext being a simple derivate class of DumpContext, and make > Microdump a friend of that, so it can access the SetContext... methods which are > already there. > > 3) In the kCpuKey case below, you can then do something like: > if (arch == "arm7l") { > MDRawContextARM* arm = new MDRawContextARM(); > memcpy.... > context_->SetContextArm(arm) > } > > I would prefer that you don't write the code for the remaining architectures > (i.e. if architecture != arm7l just bail out). We can add it later as we start > supporting new platforms. > For the moment it would be extremely untested and misleading if somebody else > tries to use it. Done. https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:329: stack_region_->Init(stack_start, On 2014/11/13 22:59:33, primiano wrote: > If you move this below (after all the if-s and after the while) you can avoid > re-init'ing the stack_region at every chunk and do it in one pass below (it's > not super nice to see something beign re-initialized at every loop, even if in > this case it works anyway) Done. https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:332: string cpu_state_str = line; On 2014/11/13 22:59:33, primiano wrote: > you could abbreviate and make it more efficient by doing: > string cpu_statestr(line, found + strlen(kCpuKey)); > instead of creating a copy and erasing it. Done. https://breakpad.appspot.com/5714003/diff/360001/src/processor/microdump.cc#n... src/processor/microdump.cc:337: string mmap_line = line; On 2014/11/13 22:59:33, primiano wrote: > same thing here Done.
Sign in to reply to this message.
Almost there. I haven't reviewed the unittest yet (I am going to do that now) but the rest seems good % some comment here and there. https://breakpad.appspot.com/5714003/diff/360001/Makefile.am File Makefile.am (right): https://breakpad.appspot.com/5714003/diff/360001/Makefile.am#newcode171 Makefile.am:171: src/google_breakpad/processor/microdump.h \ > It's not the spaces, it's the tabs that are tricking me)) > Do you see what I see, the weird red characters? I can't get rid of it (it only > shows here, in all my editors is looks alright) I think that what's happening is that your editor replaces tabs with 4 spaces (which is a good thing to do, with the only exception of Makefiles, where tabs have a semantic, sigh). That situation indistinguishible to the human eye when opening the file with most editors (today is kind of a de-facto standard rendering a trailing tab with 4 spaces). But when you open the file with an editor where tab=8 chars (which is the traditional value from the times of DOS), or which explicitly shows tabs in a different color, you see it. codereview.chromium.org does both (shows the tab in a different color, and has tab=8cols). >What editor do you use for editing this file? Oh, I limit myself to make changes in the .cc files, and let you do the changes which involve makefile changes :P A way can be: 1) vim Makefile.am 2) :set notabexpand 3) add the line using a tab to indent 4) :set tabsize=8 At this point, you should see everything indenting more on the right. If your newly added line moves as well, you're fine. just :wq the file > Done. I think your editor has some options to convert tabs to spaces automatically. https://breakpad.appspot.com/5714003/diff/560003/src/google_breakpad/processo... File src/google_breakpad/processor/microdump.h (right): https://breakpad.appspot.com/5714003/diff/560003/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:58: void Add(const CodeModule* module); Can you add a comment saying that this takes ownership of |module|. (In chromium you would make this explicit using scoped_ptr, however I think that the breakpad's implementation of scoped_ptr doesn't implement the required Pass() semantic) https://breakpad.appspot.com/5714003/diff/560003/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:92: // Fetch a little-endian value from ADDRESS in contents_ whose size Nit: double space https://breakpad.appspot.com/5714003/diff/560003/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:101: // Microdump is the user's interface to a microdump file. It provides access to Nit: double space (between . and It) https://breakpad.appspot.com/5714003/diff/560003/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:108: virtual DumpContext* GetContext() { return context_.get(); } Do these methods really need to be virtual? https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc File src/processor/microdump.cc (right): https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:41: #include <sstream> Nit: alpha order: move this before stdio https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:61: return strtoul(str.c_str(), NULL, 16); Hmm I think that this as it is might create some portability issues (strtoul returns unsigned long). Instead of thinking how to cast between generic long and int_XX, I'd just use the STL and eventually downcast: template<typename T> T HexStrTo(const string& str) { uint64_t res = 0; std::istringstream ss(str); ss >> std::hex >> res; return static_cast<T>(res); } then you should be able to use HexStrTo<uint8_t>(...) HexStrTo<uint32_t>(...) HexStrTo<uint64_t>(...) https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:108: MicrodumpMemoryRegion::MicrodumpMemoryRegion(): base_address_(0) { } Nit: add a space before the ":" https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:111: const string &contents) { to be honest, I'd probably turn contents_ into a vector<uint8_t>, and make this argument a const vector<uint8t>& contents, unless there is some convenience reason I missed. It would be cleaner to read, and should avoid you to use a copy constructor below on line 241, without having to use the .begin(), .end() trick. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:116: uint64_t MicrodumpMemoryRegion::GetBase() const { return base_address_; } Nit: please leave one extra line between these (same below). https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:120: uint8_t *value) const { Nit: here and below, the * should go next to the type, i.e. uint8_t* value https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:143: int start = address - base_address_; I think you want uint64_t here or some compilers might barf https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:146: v = (v << 8) | static_cast<unsigned char>(contents_[start + i]); nit: unsigned_char -> uint8_t https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:152: std::cout << contents_ << std::endl; Hmm this is going to print potentially garbage (as the region contains bytes and not a human readable string). I understand why you have to implement this (because the base class declares it). On the other side, if you don't really ned the Print I'm fine with just a assert(false); // Not reached, just needed due to honor the base class contract. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:166: uintptr_t stack_start = 0; uint64_t (see my comment below) https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:172: size_t found; nit: I'd probably name this something like "pos". Found smeels too much like a bool https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:194: arch = arch.substr(1, arch.length() - 2); // remove quotes Nit: just two spaces between ; and // https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:199: uintptr_t start_addr; Considering that at the end you want a uint64_t, you might want to declare this as a uint64_t. Also, this won't work if you try to process a 64-bit dump (e.g. arm64) on a 32-bit host machine (x86). https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:205: assert(start_addr == stack_start + stack_content.size()); Can you please add a comment here to explain that this is to check that stack chunks are contiguous?
Sign in to reply to this message.
And the last bits. Many thanks again for the test. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... File src/processor/microdump_processor.cc (right): https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... src/processor/microdump_processor.cc:37: Late nit, I guess you don't need this newline here? https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... src/processor/microdump_processor.cc:41: #include "google_breakpad/processor/microdump.h" Nit: alpha sorting https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... File src/processor/microdump_processor_unittest.cc (right): https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... src/processor/microdump_processor_unittest.cc:33: #include <fstream> Nit: alpha sorting https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... src/processor/microdump_processor_unittest.cc:56: using std::ifstream; considering that you are using that only in one place, probably it's cleaner to just use std:: on line 70. Ditto for vector https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... src/processor/microdump_processor_unittest.cc:93: void PrintStack(const ProcessState& state) { Nobody seems to call this function.
Sign in to reply to this message.
Addressed comments, PTAL. Three issues still remain unsolved: 1) The Makefile.am: I tried to follow your instruction, but it didn't quite work. Anyway, I see the indentation is right in all 3 editors I try, but it's only the '>>' red character in the diffs that is weird. Do you see it too? I just noticed that it is not showing in the unified diffs, so maybe it's alright after all? 2) The first line of the stack (4th line of the microdump) in the test file - I sent you an email earlier about it. I'm currently ignoring it while parsing, because the address it contains is not in order - was this intentional? what special handling should I add? 3) The test symbols file is not showing as part of the CL (I get a message that it is too large) - is it just not showing in the UI, or do I need to do smth special to include it in the CL? thanks. https://breakpad.appspot.com/5714003/diff/560003/src/google_breakpad/processo... File src/google_breakpad/processor/microdump.h (right): https://breakpad.appspot.com/5714003/diff/560003/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:58: void Add(const CodeModule* module); On 2014/11/18 01:02:11, primiano wrote: > Can you add a comment saying that this takes ownership of |module|. > (In chromium you would make this explicit using scoped_ptr, however I think that > the breakpad's implementation of scoped_ptr doesn't implement the required > Pass() semantic) Done. https://breakpad.appspot.com/5714003/diff/560003/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:92: // Fetch a little-endian value from ADDRESS in contents_ whose size On 2014/11/18 01:02:11, primiano wrote: > Nit: double space Done. https://breakpad.appspot.com/5714003/diff/560003/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:101: // Microdump is the user's interface to a microdump file. It provides access to On 2014/11/18 01:02:11, primiano wrote: > Nit: double space (between . and It) I think there should be double space between sentences in function comments (I checked other .h files and they have them). Otherwise, it is inconsistent with your previous comment, isn't it? https://breakpad.appspot.com/5714003/diff/560003/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:108: virtual DumpContext* GetContext() { return context_.get(); } On 2014/11/18 01:02:11, primiano wrote: > Do these methods really need to be virtual? Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc File src/processor/microdump.cc (right): https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:41: #include <sstream> On 2014/11/18 01:02:11, primiano wrote: > Nit: alpha order: move this before stdio Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:61: return strtoul(str.c_str(), NULL, 16); On 2014/11/18 01:02:11, primiano wrote: > Hmm I think that this as it is might create some portability issues (strtoul > returns unsigned long). > Instead of thinking how to cast between generic long and int_XX, I'd just use > the STL and eventually downcast: > > template<typename T> > T HexStrTo(const string& str) { > uint64_t res = 0; > std::istringstream ss(str); > ss >> std::hex >> res; > return static_cast<T>(res); > } > > then you should be able to use > HexStrTo<uint8_t>(...) > HexStrTo<uint32_t>(...) > HexStrTo<uint64_t>(...) Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:108: MicrodumpMemoryRegion::MicrodumpMemoryRegion(): base_address_(0) { } On 2014/11/18 01:02:11, primiano wrote: > Nit: add a space before the ":" Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:111: const string &contents) { On 2014/11/18 01:02:11, primiano wrote: > to be honest, I'd probably turn contents_ into a vector<uint8_t>, and make this > argument a const vector<uint8t>& contents, unless there is some convenience > reason I missed. > It would be cleaner to read, and should avoid you to use a copy constructor > below on line 241, without having to use the .begin(), .end() trick. Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:116: uint64_t MicrodumpMemoryRegion::GetBase() const { return base_address_; } On 2014/11/18 01:02:11, primiano wrote: > Nit: please leave one extra line between these (same below). Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:120: uint8_t *value) const { On 2014/11/18 01:02:11, primiano wrote: > Nit: here and below, the * should go next to the type, i.e. uint8_t* value I do prefer this writing too, but it will be inconsistent with rest of the breakpad code. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:143: int start = address - base_address_; On 2014/11/18 01:02:11, primiano wrote: > I think you want uint64_t here or some compilers might barf Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:146: v = (v << 8) | static_cast<unsigned char>(contents_[start + i]); On 2014/11/18 01:02:11, primiano wrote: > nit: unsigned_char -> uint8_t Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:152: std::cout << contents_ << std::endl; On 2014/11/18 01:02:11, primiano wrote: > Hmm this is going to print potentially garbage (as the region contains bytes and > not a human readable string). > I understand why you have to implement this (because the base class declares > it). On the other side, if you don't really ned the Print I'm fine with just a > assert(false); // Not reached, just needed due to honor the base class contract. Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:166: uintptr_t stack_start = 0; On 2014/11/18 01:02:11, primiano wrote: > uint64_t (see my comment below) Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:172: size_t found; On 2014/11/18 01:02:11, primiano wrote: > nit: I'd probably name this something like "pos". Found smeels too much like a > bool Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:194: arch = arch.substr(1, arch.length() - 2); // remove quotes On 2014/11/18 01:02:11, primiano wrote: > Nit: just two spaces between ; and // Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:199: uintptr_t start_addr; On 2014/11/18 01:02:11, primiano wrote: > Considering that at the end you want a uint64_t, you might want to declare this > as a uint64_t. > Also, this won't work if you try to process a 64-bit dump (e.g. arm64) on a > 32-bit host machine (x86). Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump.cc#n... src/processor/microdump.cc:205: assert(start_addr == stack_start + stack_content.size()); On 2014/11/18 01:02:11, primiano wrote: > Can you please add a comment here to explain that this is to check that stack > chunks are contiguous? Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... File src/processor/microdump_processor.cc (right): https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... src/processor/microdump_processor.cc:37: On 2014/11/18 01:56:16, primiano wrote: > Late nit, I guess you don't need this newline here? I think, I do - looking at other files, the .h system includes are separated with blank line from the following includes. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... src/processor/microdump_processor.cc:41: #include "google_breakpad/processor/microdump.h" On 2014/11/18 01:56:16, primiano wrote: > Nit: alpha sorting Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... File src/processor/microdump_processor_unittest.cc (right): https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... src/processor/microdump_processor_unittest.cc:33: #include <fstream> On 2014/11/18 01:56:16, primiano wrote: > Nit: alpha sorting Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... src/processor/microdump_processor_unittest.cc:56: using std::ifstream; On 2014/11/18 01:56:16, primiano wrote: > considering that you are using that only in one place, probably it's cleaner to > just use std:: on line 70. > Ditto for vector Done. https://breakpad.appspot.com/5714003/diff/560003/src/processor/microdump_proc... src/processor/microdump_processor_unittest.cc:93: void PrintStack(const ProcessState& state) { On 2014/11/18 01:56:16, primiano wrote: > Nobody seems to call this function. Done.
Sign in to reply to this message.
Thanks! Finally LGTM with some remaining small nits. As regards your questions: 1) I think that what you uploaded here in the last PS is right. Just rietveld is showing you (with the >>) that the character being inserted is a tab and not a serie of spaces. Which is precisely what you intend to achieve. 3) It's a rietveld known issue, dont' worry. Everything should be fine when you will dcommit (the file will be committed properly to svn) and I have no interest in reviewing a binary blob :) 2) That initial line (S 0) has a slightly different meaning. It means: here the stack starts (S 0). Then it provides the value of the stack pointer (A48BD840), The start address of the stack being dumped (A48BD000) and the lenght of the stack. To be honest, I now realize that most of its content is redundant and useful only to double-check that the following stack chunks are correct. I don't think there is a compelling need to do that in the current CL. What I would do at the moment is removing the exra "x" in the .dmp file and just a line in the processor which just skips "S 0", with a TODO like "we could use the S 0 stack header in future to double check that we received all the stack as expected". https://breakpad.appspot.com/5714003/diff/590004/src/google_breakpad/processo... File src/google_breakpad/processor/microdump.h (right): https://breakpad.appspot.com/5714003/diff/590004/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:84: virtual bool GetMemoryAtAddress(uint64_t address, uint8_t *value) const; I might have missed this in the previous reviews sorry. Just small nit: the star should go next to the type (uint8_t*) https://breakpad.appspot.com/5714003/diff/590004/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:96: bool GetMemoryLittleEndian(uint64_t address, ValueType *value) const; ditto here https://breakpad.appspot.com/5714003/diff/590004/src/processor/microdump.cc File src/processor/microdump.cc (right): https://breakpad.appspot.com/5714003/diff/590004/src/processor/microdump.cc#n... src/processor/microdump.cc:52: static const char kGoogleBreakpadKey[] = "/google-breakpad("; Nit (lines 52-70): stuff in a namespace should not be indented (i.e. ParseHexBuf is fine, the rest above should look like that) https://breakpad.appspot.com/5714003/diff/590004/src/processor/microdump.cc#n... src/processor/microdump.cc:208: stack_tokens >> std::hex >> start_addr; For sake of consistency below, I'd prefer that you put the token into a start_addr_str string and then use the helper function HexStrToL<uint64_t> like you did below on line 234. Rationale: I am not 100% sure that otherwise istringstream is going to do the right thing if the token is not exactly a 64 bit wide hex str.
Sign in to reply to this message.
Addressed comments, submitting shortly.. Thanks. https://breakpad.appspot.com/5714003/diff/590004/src/google_breakpad/processo... File src/google_breakpad/processor/microdump.h (right): https://breakpad.appspot.com/5714003/diff/590004/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:84: virtual bool GetMemoryAtAddress(uint64_t address, uint8_t *value) const; On 2014/11/18 23:00:58, primiano wrote: > I might have missed this in the previous reviews sorry. Just small nit: the star > should go next to the type (uint8_t*) Done. https://breakpad.appspot.com/5714003/diff/590004/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:96: bool GetMemoryLittleEndian(uint64_t address, ValueType *value) const; On 2014/11/18 23:00:58, primiano wrote: > ditto here Done. https://breakpad.appspot.com/5714003/diff/590004/src/processor/microdump.cc File src/processor/microdump.cc (right): https://breakpad.appspot.com/5714003/diff/590004/src/processor/microdump.cc#n... src/processor/microdump.cc:52: static const char kGoogleBreakpadKey[] = "/google-breakpad("; On 2014/11/18 23:00:58, primiano wrote: > Nit (lines 52-70): stuff in a namespace should not be indented (i.e. ParseHexBuf > is fine, the rest above should look like that) Done. https://breakpad.appspot.com/5714003/diff/590004/src/processor/microdump.cc#n... src/processor/microdump.cc:208: stack_tokens >> std::hex >> start_addr; On 2014/11/18 23:00:58, primiano wrote: > For sake of consistency below, I'd prefer that you put the token into a > start_addr_str string and then use the helper function HexStrToL<uint64_t> like > you did below on line 234. > Rationale: I am not 100% sure that otherwise istringstream is going to do the > right thing if the token is not exactly a 64 bit wide hex str. Done.
Sign in to reply to this message.
Sign in to reply to this message.
>https://support.mozilla.org/vi/search?q=https%3A%... ????
Sign in to reply to this message.
Message was sent while issue was closed.
Committed patchset #9 (id:760002) manually as 1403 (presubmit successful).
Sign in to reply to this message.
Message was sent while issue was closed.
https://mb.b8ag.com/%28S%281grqpxn0g3gxmsa1u5qqwids%29%29/index.aspx
Sign in to reply to this message.
Message was sent while issue was closed.
https://mb.b8ag.com/(S(rfsss0gmso4sfoxhksygowov))/index.aspx https://breakpad.appspot.com/5714003/diff/560003/src/google_breakpad/processo... File src/google_breakpad/processor/microdump.h (right): https://breakpad.appspot.com/5714003/diff/560003/src/google_breakpad/processo... src/google_breakpad/processor/microdump.h:58: void Add(const CodeModule* module); On 2014/11/18 19:44:10, mmandlis wrote: > <font><font class="">Ngày 2014/11/18 01:02:11, primiano đã viết:> Bạn có thể thêm một bình luận nói rằng điều này có quyền sở hữu của | mô-đun |. </font><font>> (Bằng crôm bạn sẽ thực hiện điều này rõ ràng sử dụng scoped_ptr, tuy nhiên tôi nghĩ rằng> thực hiện của breakpad của scoped_ptr không thực hiện được yêu cầu> Pass () ngữ nghĩa) > > </font></font> > <font><font>Xong. > </font></font> https://mb.b8ag.com/(S(rfsss0gmso4sfoxhksygowov))/index.aspx
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
