Extract DumpContext base class from MinidumpContext
This is a pure refactoring in preparation of the Microdump(s)
(which will be introduced by later CL). No behavioral change is
intended.
This change removes the MinidumpContext -> MinidumpStream
inheritance (which is not actually needed as MinidumpContexts
are never stored to streams) and extracts a base class
(DumpContext) out of it.
Similarly, ProcessResult is moved out to its own class.
BUG=https://code.google.com/p/chromium/issues/detail?id=410294
Primiano,
This is the refactoring CL, as suggested in
https://breakpad.appspot.com/9674002/
I replied to your comments in the original CL, let me know if you'd rather have
me copy them here.
thanks,
Maria
On 2014/08/29 15:18:52, mmandlis wrote:
> Primiano,
>
> This is the refactoring CL, as suggested in
> https://breakpad.appspot.com/9674002/
>
> I replied to your comments in the original CL, let me know if you'd rather
have
> me copy them here.
>
> thanks,
> Maria
Maria in I had an offline chat on this. This change, as it is, is changing the
MinidumpContext name and interface. There seem to be at least one external
project (v8/tools/oom_dump/oom_dump.cc) which relies on that API.
There seem to be two possible ways to get around this problem without changing
the MinidumpContext API:
1. It looks like MinidumpContext doesn't really need to inherit from
MinidumpStream. If that is really the case, we could just remove that dependency
and make a linear inheritance chain MinidumpContext -> DumpContext and live
happy (and replace it with Maria's proposed MinidumpContext -> DumpContext
inheritance).
2. If 1. is not the case (i.e. there is some code relying on MinidumpContext
being a MinidumpStream) we can go through delegation as suggested in my original
comments in https://breakpad.appspot.com/9674002/
In any of the two cases MinidumpContext will keep its name and API interface, so
this CL will look less scary anyways :-)
I was optimistic and did option 1. I think it is even cleaner now, when I got
rid of the weird context-reader thing..
Thank you! PTAL
On 2014/09/01 10:58:36, primiano wrote:
> On 2014/08/29 15:18:52, mmandlis wrote:
> > Primiano,
> >
> > This is the refactoring CL, as suggested in
> > https://breakpad.appspot.com/9674002/
> >
> > I replied to your comments in the original CL, let me know if you'd rather
> have
> > me copy them here.
> >
> > thanks,
> > Maria
>
> Maria in I had an offline chat on this. This change, as it is, is changing the
> MinidumpContext name and interface. There seem to be at least one external
> project (v8/tools/oom_dump/oom_dump.cc) which relies on that API.
> There seem to be two possible ways to get around this problem without changing
> the MinidumpContext API:
>
> 1. It looks like MinidumpContext doesn't really need to inherit from
> MinidumpStream. If that is really the case, we could just remove that
dependency
> and make a linear inheritance chain MinidumpContext -> DumpContext and live
> happy (and replace it with Maria's proposed MinidumpContext -> DumpContext
> inheritance).
>
> 2. If 1. is not the case (i.e. there is some code relying on MinidumpContext
> being a MinidumpStream) we can go through delegation as suggested in my
original
> comments in https://breakpad.appspot.com/9674002/
>
> In any of the two cases MinidumpContext will keep its name and API interface,
so
> this CL will look less scary anyways :-)
Still LGTM % micro-nits
You seem to have added some extra spacing between namespace { and the statements
below in your last patchsets (2 blank lines instead of 1).
The http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces
seem to suggest just one. So the previous version should be fine.
If you want you can avoid uploading a new patchset and address these whitespace
changes after Mark's review.
Many thanks.
https://breakpad.appspot.com/5684002/diff/680008/src/google_breakpad/processo...
File src/google_breakpad/processor/dump_object.h (right):
https://breakpad.appspot.com/5684002/diff/680008/src/google_breakpad/processo...
src/google_breakpad/processor/dump_object.h:43: protected:
On 2014/09/03 10:43:58, mmandlis wrote:
> On 2014/09/02 13:13:30, primiano wrote:
> > I think you need an explicit empty protected DumpObject constructor here,
and
> a
> > corresponding .cc file where the constructor initializes valid_ to false (as
> > minidump.cc was used to do).
> > Otherwise the value of valid_ here will be undefined and very subtle things
> will
> > happen :)
>
> This is the moment when I really miss Java.
Well, in Chromium we have clang plugins which check for these things.
Unfortunately, I don't think there is any configuration for breakpad (well, it
even lacks a CI)
https://breakpad.appspot.com/5684002/diff/920002/Makefile.am
File Makefile.am (right):
https://breakpad.appspot.com/5684002/diff/920002/Makefile.am#newcode687
Makefile.am:687: src/processor/dump_object.o \
out of curiosity, how did you figure out that you need dump_object here in
processor_exploitability_unittest? Is it "add missing dependencies till it
compiles" strategy right?
https://breakpad.appspot.com/5684002/diff/920002/Makefile.am#newcode1086
Makefile.am:1086: src/processor/dump_context.o \
nit: alpha-sorting: these two (dump_...) should go after disassembler
CC: +benm
If you want to take a look to the bigger picture, the original bigger CL (which
includes this refactor, at an earlier stage, + the microdump classes) was this
one. https://breakpad.appspot.com/9674002/
Ah, in addition, I think you should also regenerate the Makefile.in here.
IIRC you are running Ubuntu Precise. In order to get up to date with automake
and autoconf you need do the following:
- Download these two files:
http://packages.ubuntu.com/trusty/all/automake/downloadhttp://packages.ubuntu.com/trusty/all/autoconf/download
- Install the two deb packages (using the Ubuntu UI, just open them).
- Once installed, you'll get the proper version of automake. At this point chdir
to the breakpad/src folder (where Makefile.am lives) and run automake with no
arguments. That should regenerate the Makefile.in.
- Add the file to this cl and upload another patchset.
Thanks
Hmm this has landed as
https://code.google.com/p/google-breakpad/source/detail?r=1370
Please mark this CL as closed.
Note: something went slightly wrong while committing to SVN. The commit message
in SVN is just
"Refactoring in preparation for microdump processing" and lacks the reference to
the codereview URL (and all your beautiful commit message that shows here).
Not a great deal (also I think there is no way to rewrite SVN history) but we
should be a bit more careful next time
P.S. I doublechecked that the content of the CL is identical (i.e. r1369 +
Patchset9 of this CL === r1370) so the content is fine.
I can edit svn commit messages. Unlike git, this can be done without screwing
history up for people who have synced past the changed version.
I’ve added a link to the code review. If you’d like more information in the
commit message, give it to me and I can add that too.
On 2014/09/09 13:08:58, Mark Mentovai wrote:
> I can edit svn commit messages. Unlike git, this can be done without screwing
> history up for people who have synced past the changed version.
Ah right.
It would be lovely if you could just set the full description of this CL to be
the SVN message.
Thanks Mark!
On 2014/09/09 13:44:50, Mark Mentovai wrote:
> OK, fixed it up.
Mark, thanks a lot for fixing the commit message! Just wondering, what is the
command to do the edit, in case I screw things again next time)
thanks!
Issue 5684002: Refactoring in preparation for microdump processing
(Closed)
Created 9 years, 8 months ago by mmandlis
Modified 9 years, 8 months ago
Reviewers: primiano, Mark Mentovai
Base URL: http://google-breakpad.googlecode.com/svn/trunk/
Comments: 29
