Add exc_server_variants including UniversalMachExcServer and its test

Add exc_server_variants including UniversalMachExcServer and its test.

TEST=util_test ExcServerVariants.*
R=rsesek@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/177f5dcddce4a38b0769b3d843f3d4d5c4dfd240

[Mark Mentovai, 2014-09-09 17:11:33]

mark@chromium.org changed reviewers:
+ rsesek@chromium.org

[Mark Mentovai, 2014-09-09 20:27:54]

This now depends on https://codereview.chromium.org/553153002/.

[Robert Sesek, 2014-09-10 16:44:42]

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
File util/mach/exc_server_variants.cc (right):

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants.cc:173: out_reply->RetCode =
Is out_reply->msgh_size correct here? (Right now it's the size of
mig_reply_error_t).

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants.cc:188: Request* in_request_1;
Don't really know what in_request_1 is.

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants.cc:628: std::vector<mach_exception_data_type_t>
mach_codes;
Reserve code_count space?

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
File util/mach/exc_server_variants.h (right):

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants.h:67: mach_msg_type_number_t* new_state_count) =
0;
No destroy_request?

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants.h:278: class SimplifiedMachExcServer : public
MachExcServer,
Do you really need to present all of these interfaces in a public header? Are
they all meant to be used? Could you move some to an internal namespace/header?

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
File util/mach/exc_server_variants_test.cc (right):

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants_test.cc:538: DefaultValue<T>::Set(default_value);
Where's DefaultValue defined?

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants_test.cc:545: ScopedDefaultValue<kern_return_t>
default_kern_return_t(KERN_FAILURE);
… I don't understand how this is used.

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants_test.cc:902:
EXPECT_NE(static_cast<mach_port_t>(MACH_PORT_NULL), thread);
You write this cast enough that it could be a handy constant to have in a common
header, e.g.:

const mach_port_t kMachPortNull = MACH_PORT_NULL.

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants_test.cc:945: 
nit: double blank line

[Mark Mentovai, 2014-09-10 20:12:56]

All feedback addressed.

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
File util/mach/exc_server_variants.cc (right):

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants.cc:173: out_reply->RetCode =
rsesek wrote:
> Is out_reply->msgh_size correct here? (Right now it's the size of
> mig_reply_error_t).

It’s correct because the two structures are defined identically, but you’re
right, I should be explicit here.

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
File util/mach/exc_server_variants.h (right):

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants.h:67: mach_msg_type_number_t* new_state_count) =
0;
rsesek wrote:
> No destroy_request?

No, because this request is not complex. Documented.

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants.h:278: class SimplifiedMachExcServer : public
MachExcServer,
rsesek wrote:
> Do you really need to present all of these interfaces in a public header? Are
> they all meant to be used? Could you move some to an internal
namespace/header?

I only need to expose the Universal interface. I’ll stick the others in the
internal namespace. I can’t hide them in the .cc file because the Universal one
needs to see the declarations.

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
File util/mach/exc_server_variants_test.cc (right):

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants_test.cc:538: DefaultValue<T>::Set(default_value);
rsesek wrote:
> Where's DefaultValue defined?

gmock:
https://code.google.com/p/googlemock/wiki/CheatSheet#Setting_Default_Actions

https://codereview.chromium.org/545053003/diff/20001/util/mach/exc_server_var...
util/mach/exc_server_variants_test.cc:902:
EXPECT_NE(static_cast<mach_port_t>(MACH_PORT_NULL), thread);
rsesek wrote:
> You write this cast enough that it could be a handy constant to have in a
common
> header, e.g.:
> 
> const mach_port_t kMachPortNull = MACH_PORT_NULL.

I have a decent header to put this in, it’s got one unrelated constant but that
shouldn’t be a problem.

[Robert Sesek, 2014-09-10 21:10:21]

LGTM

https://codereview.chromium.org/545053003/diff/40001/util/util.gyp
File util/util.gyp (right):

https://codereview.chromium.org/545053003/diff/40001/util/util.gyp#newcode64
util/util.gyp:64: 'mach/exc_server_variants.cc',
Need to add the new header here.

[Mark Mentovai, 2014-09-10 21:29:10]

Committed patchset #4 (id:60001) manually as 177f5dc (presubmit successful).