Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11761030:
Create the crash key registration system and register some Mac-specific keys. (Closed)
|
Created:
7 years, 11 months ago by Robert Sesek Modified:
7 years, 11 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, sail+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCreate the crash key registration system and register some Mac-specific keys.
BUG=77656
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177015
Patch Set 1 #
Total comments: 11
Patch Set 2 : Address easy comments #Patch Set 3 : Init callback -> out param #
Total comments: 12
Patch Set 4 : Address comments #
Total comments: 14
Patch Set 5 : Address comments #
Total comments: 2
Patch Set 6 : '' #
Total comments: 4
Patch Set 7 : Remove CrashKey.max_length #Patch Set 8 : arraysize compile #Patch Set 9 : Android/Win compile #
Total comments: 10
Patch Set 10 : Comments #
Messages
Total messages: 29 (0 generated)
This is open to feedback regarding the CrashKey structure. I tried to combine your guys' comments from the first CL, and this is what I came up with. Right now Windows is limited to 64 bytes for a crash value, but I see no reason why we can't standardize on 255 across all platforms. I've left a TODO regarding that.
Specifically, I'm internally debating the following: * Should a maximum value length be enforced at this API level? I think yes, and I think it should be standard across all platforms. * If so, then do clients care about the number of chunks, the maximum length (which could be inferred from number of chunks if we have a standard chunk max), or both as I've done here?
On 2013/01/04 17:17:17, rsesek wrote: > Specifically, I'm internally debating the following: > > * Should a maximum value length be enforced at this API level? I think yes, and > I think it should be standard across all platforms. > * If so, then do clients care about the number of chunks, the maximum length > (which could be inferred from number of chunks if we have a standard chunk max), > or both as I've done here? I think that the number of chunks may be something reasonable for key registration (because it feeds into the processing commitment at the head end), but the length of each chunk seems odd to expose here, because it might be capped. Maybe it would be reasonable to phrase a maximum amount of data to chunk up, though in that case the caller could just call substr() on the passed-in value. Do we have more than one user of chunking, though? If we had this function: void SetCrashKeyVector(const StringPiece& base_key, const StringPiece& count_key, std::vector<std::string>& values, size_t max_values); would that caller be materially impacted? Hmm, maybe, we don't have base::SplitStringIntoChunks(). Anyhow, we'll need something to log a vector for other reasons, so maybe having a chunker isn't worth that much. Part of the goal would be similar to having names for switches, it prevents typos and makes it easier to find the set being processed, and also provides a little OWNERS feedback. OWNERS feedback can cover to some extent for also managing the limited storage space (like if someone decides to store a base64-encoded data structure). I'm not uncomfortable with specifying the vector/chunk nature of the key at the point of logging, rather than trying to hide it somehow. I'm not feeling super enthusiastic right now. I +100 like the notion of making the key stuff more dynamic rather than all of the hard-coded wiring. I'm not sure how much of the notion of registration stems from being a control freak, and how much stems from my mental model of why the existing Windows code is the way it is. https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc File base/debug/crash_logging.cc (right): https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc#n... base/debug/crash_logging.cc:19: std::map<std::string, CrashKey>* g_crash_keys_; Would prefer =NULL, if it's the default, the compiler can optimize it away. https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc#n... base/debug/crash_logging.cc:26: } // namespace This should be outside the base::debug::, I think? https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc#n... base/debug/crash_logging.cc:127: g_crash_keys_ = new std::map<std::string, CrashKey>; This won't work great for modules which want to manage their own crash keys. If chrome/ is the only embedder, it probably doesn't matter that much, and the entire point is to prevent people from larding things up, so it's possible it shouldn't be made generic. Thinking. If it's one-shot, DCHECK(!g_crash_keys_). And maybe a thread assertion of some sort would make sense. https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc#n... base/debug/crash_logging.cc:149: g_crash_keys_->find(key.as_string()); Once we're calling as_string(), StringPiece isn't much of a gain any longer, just lets you pass char* or std::string. Sigh. Can the keys be const char* and forced to be static storage? Hmm. https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.h#ne... base/debug/crash_logging.h:69: typedef void (*InitCrashKeysCallbackFuncT)(const std::vector<std::string>&); I don't understand why this is present. Is the callback going to be coming from somewhere distant from the caller? Why isn't is a base::Callback?
On 2013/01/04 18:59:38, shess wrote: > On 2013/01/04 17:17:17, rsesek wrote: > > Specifically, I'm internally debating the following: > > > > * Should a maximum value length be enforced at this API level? I think yes, > and > > I think it should be standard across all platforms. > > * If so, then do clients care about the number of chunks, the maximum length > > (which could be inferred from number of chunks if we have a standard chunk > max), > > or both as I've done here? > > I think that the number of chunks may be something reasonable for key > registration (because it feeds into the processing commitment at the head end), > but the length of each chunk seems odd to expose here, because it might be > capped. Maybe it would be reasonable to phrase a maximum amount of data to > chunk up, though in that case the caller could just call substr() on the > passed-in value. OK. I think then maybe we should just specify the number of chunks at registration, and then the standardize the length of a chunk across all platforms and document that in the API. > Do we have more than one user of chunking, though? Yes. > If we had this function: > void SetCrashKeyVector(const StringPiece& base_key, const StringPiece& > count_key, std::vector<std::string>& values, size_t max_values); I'll look at what the requirements of a vector variant of SetCrashKey are when I convert child_process_logging. > Part of the goal would be similar to having names for switches, it prevents > typos and makes it easier to find the set being processed, and also provides a > little OWNERS feedback. OWNERS feedback can cover to some extent for also > managing the limited storage space (like if someone decides to store a > base64-encoded data structure). I'm not uncomfortable with specifying the > vector/chunk nature of the key at the point of logging, rather than trying to > hide it somehow. I'm not sure what you're getting at here. > I'm not feeling super enthusiastic right now. I +100 like the notion of making > the key stuff more dynamic rather than all of the hard-coded wiring. I'm not > sure how much of the notion of registration stems from being a control freak, > and how much stems from my mental model of why the existing Windows code is the > way it is. Yes, the issue is Windows. Linux and Mac could do unregistered, ad-hoc crash logging without issue. Windows, however, requires fixed storage for all the crash keys at the time of Breakpad initialization. This really ties our hands and requires us to know the size of the array. That really means knowing the number of keys and the number of chunks for each key, which is why I've architected things this way. https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc File base/debug/crash_logging.cc (right): https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc#n... base/debug/crash_logging.cc:19: std::map<std::string, CrashKey>* g_crash_keys_; On 2013/01/04 18:59:39, shess wrote: > Would prefer =NULL, if it's the default, the compiler can optimize it away. Done. https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc#n... base/debug/crash_logging.cc:26: } // namespace On 2013/01/04 18:59:39, shess wrote: > This should be outside the base::debug::, I think? I don't think it matters. https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc#n... base/debug/crash_logging.cc:127: g_crash_keys_ = new std::map<std::string, CrashKey>; On 2013/01/04 18:59:39, shess wrote: > This won't work great for modules which want to manage their own crash keys. If > chrome/ is the only embedder, it probably doesn't matter that much, and the > entire point is to prevent people from larding things up, so it's possible it > shouldn't be made generic. Thinking. > > If it's one-shot, DCHECK(!g_crash_keys_). And maybe a thread assertion of some > sort would make sense. Because Windows enforces the requirement of a fixed-size storage, yes this can only be called once. Having modules manage their own crash keys sort of implies that each module then has its own crash reporter. https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc#n... base/debug/crash_logging.cc:149: g_crash_keys_->find(key.as_string()); On 2013/01/04 18:59:39, shess wrote: > Once we're calling as_string(), StringPiece isn't much of a gain any longer, > just lets you pass char* or std::string. Sigh. Can the keys be const char* and > forced to be static storage? Hmm. If we do it by const char* though, won't using them as keys in a map perform pointer equality rather than string equality? That seems wonky. https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.h#ne... base/debug/crash_logging.h:69: typedef void (*InitCrashKeysCallbackFuncT)(const std::vector<std::string>&); On 2013/01/04 18:59:39, shess wrote: > I don't understand why this is present. Is the callback going to be coming from > somewhere distant from the caller? Why isn't is a base::Callback? Windows needs to know the amount of storage to reserve in its breakpad impl, so this is a way of getting that along with all the key names that could be used. You'll see this getting used in the next CL for the Windows impl. Changed to a base::Callback though.
On 2013/01/04 19:25:38, rsesek wrote:
> OK. I think then maybe we should just specify the number of chunks at
> registration, and then the standardize the length of a chunk across all
> platforms and document that in the API.
I was going to say "maybe max-chunks and max-total", but then realized that
max-total could just be substr() in the caller (unless we want to log the total
independently).
> > Part of the goal would be similar to having names for switches, it prevents
> > typos and makes it easier to find the set being processed, and also provides
a
> > little OWNERS feedback. OWNERS feedback can cover to some extent for also
> > managing the limited storage space (like if someone decides to store a
> > base64-encoded data structure). I'm not uncomfortable with specifying the
> > vector/chunk nature of the key at the point of logging, rather than trying
to
> > hide it somehow.
>
> I'm not sure what you're getting at here.
Mostly I mean that I think it would be reasonable to have the code setting the
value express the chunk-ness. So SetCrashKeyValue() for short/unchunked, and
SetCrashKeyChunkedValue() for long values which could be chunked. But not great
if Windows limits things.
> > I'm not feeling super enthusiastic right now. I +100 like the notion of
> making
> > the key stuff more dynamic rather than all of the hard-coded wiring. I'm
not
> > sure how much of the notion of registration stems from being a control
freak,
> > and how much stems from my mental model of why the existing Windows code is
> the
> > way it is.
>
> Yes, the issue is Windows. Linux and Mac could do unregistered, ad-hoc crash
> logging without issue. Windows, however, requires fixed storage for all the
> crash keys at the time of Breakpad initialization. This really ties our hands
> and requires us to know the size of the array. That really means knowing the
> number of keys and the number of chunks for each key, which is why I've
> architected things this way.
Does it require that the keys and values all be setup at startup, or does it
require that the space for the keys and values all be set? My understanding was
that it was the latter, so instead of creating a structure capable of containing
the union of all possible keys, you could allocate a "big enough" structure,
dynamically populate it with keys as you see them, and if it overflows steal one
of the keys to indicate that, so that you know to increase the size. Don't keep
a bunch of unused slots for keys which aren't set (or where the URLs are short),
instead keep stuff tightly packed (which shouldn't be too expensive for the
amounts of data we're looking at).
To be clear, I'm saying something like:
const size_t kMaxKeyValuePairs = 64;
static struct {
char key[kMaxKeyLength];
char value[kMaxValueLength];
} g_key_value_pairs[kMaxKeyValuePairs];
Breakpad would point at this, but the crash-key code might have some separate
data structures to index into things for purposes of making updates easier.
A subtle bit is doing management in a safe way WRT threading and crashing. I
believe the current code clears keys to indicate "not present", and doesn't do
much-if-anything about threading. So it might be that real management wouldn't
be good, but that doesn't mean something like a circular buffer wouldn't work,
or first-come-first-served to register wouldn't work.
https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc
File base/debug/crash_logging.cc (right):
https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc#n...
base/debug/crash_logging.cc:149: g_crash_keys_->find(key.as_string());
On 2013/01/04 19:25:38, rsesek wrote:
> On 2013/01/04 18:59:39, shess wrote:
> > Once we're calling as_string(), StringPiece isn't much of a gain any longer,
> > just lets you pass char* or std::string. Sigh. Can the keys be const char*
> and
> > forced to be static storage? Hmm.
>
> If we do it by const char* though, won't using them as keys in a map perform
> pointer equality rather than string equality? That seems wonky.
That's actually what I meant - kKeyName would be the literal key, which just
happened to point to a string.
When I originally suggested StringPiece, I was still thinking in terms of a
flexible API - now you're locking it down so that the keys could potentially be
exactly the keys registered, no more, no less. Keying off the contents rather
than the pointer might still be easier, though, since we'll have module-boundary
issues around using kKeyName (unless the module defines and chrome/ uses the
module's variable in registering?).
On 2013/01/05 00:22:05, shess wrote:
> On 2013/01/04 19:25:38, rsesek wrote:
> > OK. I think then maybe we should just specify the number of chunks at
> > registration, and then the standardize the length of a chunk across all
> > platforms and document that in the API.
>
> I was going to say "maybe max-chunks and max-total", but then realized that
> max-total could just be substr() in the caller (unless we want to log the
total
> independently).
>
> > > Part of the goal would be similar to having names for switches, it
prevents
> > > typos and makes it easier to find the set being processed, and also
provides
> a
> > > little OWNERS feedback. OWNERS feedback can cover to some extent for also
> > > managing the limited storage space (like if someone decides to store a
> > > base64-encoded data structure). I'm not uncomfortable with specifying the
> > > vector/chunk nature of the key at the point of logging, rather than trying
> to
> > > hide it somehow.
> >
> > I'm not sure what you're getting at here.
>
> Mostly I mean that I think it would be reasonable to have the code setting the
> value express the chunk-ness. So SetCrashKeyValue() for short/unchunked, and
> SetCrashKeyChunkedValue() for long values which could be chunked. But not
great
> if Windows limits things.
I agree that it'd be preferable to set the number of chunks at Set-time, but
because of the Windows limitations I think we need to do it at registration
time. It does sound like we can drop the length parameter in the registration,
though. I'll do that in the next patch set.
> > > I'm not feeling super enthusiastic right now. I +100 like the notion of
> > making
> > > the key stuff more dynamic rather than all of the hard-coded wiring. I'm
> not
> > > sure how much of the notion of registration stems from being a control
> freak,
> > > and how much stems from my mental model of why the existing Windows code
is
> > the
> > > way it is.
> >
> > Yes, the issue is Windows. Linux and Mac could do unregistered, ad-hoc crash
> > logging without issue. Windows, however, requires fixed storage for all the
> > crash keys at the time of Breakpad initialization. This really ties our
hands
> > and requires us to know the size of the array. That really means knowing the
> > number of keys and the number of chunks for each key, which is why I've
> > architected things this way.
>
> Does it require that the keys and values all be setup at startup, or does it
> require that the space for the keys and values all be set? My understanding
was
> that it was the latter, so instead of creating a structure capable of
containing
> the union of all possible keys, you could allocate a "big enough" structure,
> dynamically populate it with keys as you see them, and if it overflows steal
one
> of the keys to indicate that, so that you know to increase the size. Don't
keep
> a bunch of unused slots for keys which aren't set (or where the URLs are
short),
> instead keep stuff tightly packed (which shouldn't be too expensive for the
> amounts of data we're looking at).
>
> To be clear, I'm saying something like:
>
> const size_t kMaxKeyValuePairs = 64;
> static struct {
> char key[kMaxKeyLength];
> char value[kMaxValueLength];
> } g_key_value_pairs[kMaxKeyValuePairs];
On Windows, the storage for crash keys is fixed after crash reporting is
initialized, and the size of the array is immutable after that point. While we
could create space enough for all potential keys, it cannot be grown
dynamically, and it would always require that space even for keys unused. That's
ultimately what led me to require all keys to be registered at Breakpad-init
time, so that the precise amount of space could be allocated.
> Breakpad would point at this, but the crash-key code might have some separate
> data structures to index into things for purposes of making updates easier.
Yes, my intention was to have the Breakpad impl in Chrome maintain a map from
key to the storage struct inside the Breakpad library to make the updates easy.
> A subtle bit is doing management in a safe way WRT threading and crashing. I
> believe the current code clears keys to indicate "not present", and doesn't do
> much-if-anything about threading. So it might be that real management
wouldn't
> be good, but that doesn't mean something like a circular buffer wouldn't work,
> or first-come-first-served to register wouldn't work.
I think the thread safety concerns are valid wrt the API we're introducing, so
that's something to look at. Breakpad does appear to be designed to do the right
thing at crash time, though.
>
> https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc
> File base/debug/crash_logging.cc (right):
>
>
https://codereview.chromium.org/11761030/diff/1/base/debug/crash_logging.cc#n...
> base/debug/crash_logging.cc:149: g_crash_keys_->find(key.as_string());
> On 2013/01/04 19:25:38, rsesek wrote:
> > On 2013/01/04 18:59:39, shess wrote:
> > > Once we're calling as_string(), StringPiece isn't much of a gain any
longer,
> > > just lets you pass char* or std::string. Sigh. Can the keys be const
char*
> > and
> > > forced to be static storage? Hmm.
> >
> > If we do it by const char* though, won't using them as keys in a map perform
> > pointer equality rather than string equality? That seems wonky.
>
> That's actually what I meant - kKeyName would be the literal key, which just
> happened to point to a string.
>
> When I originally suggested StringPiece, I was still thinking in terms of a
> flexible API - now you're locking it down so that the keys could potentially
be
> exactly the keys registered, no more, no less. Keying off the contents rather
> than the pointer might still be easier, though, since we'll have
module-boundary
> issues around using kKeyName (unless the module defines and chrome/ uses the
> module's variable in registering?).
Since we don't ship a component build (and probably never will due to load times
of dynamically linked programs), I'm not sure that keying of pointers is a
problem in practice, but I do think it'd be safer to key off string contents.
WDYT of this? I've removed the init callback and just have an out param for the total number of keys for which the impl should allocate space.
https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.cc File base/debug/crash_logging.cc (right): https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.c... base/debug/crash_logging.cc:64: // Handle the un-checked case. un-checked, or un-chunked? https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.c... base/debug/crash_logging.cc:70: for (size_t i = 1; i <= crash_key->num_chunks; ++i) { I had to stare at this for a couple seconds to see that here you're doing [1,num_chunks], whereas above you're doing [0,num_chunks)+1. I could see making these consistent, either zero-based with a +1, or 1-based without, though I can see the above really wants zero-based. Another option would be to make the set version like: for (size_t i = 0; i < chunks.size(); ++i) { const std::string key = StringPrintf(..., i+1); if (i < num_chunks) set(...); else clear(...); } then the clear version looks like that, without the if(). https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.c... base/debug/crash_logging.cc:160: for (size_t i = 0; Since you're breaking lines anyhow, might as well be: for (size_t i = 0, offset = 0; https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.h... base/debug/crash_logging.h:12: #include "base/callback.h" I assume this was here for using base::Callback, which is now gone? https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.h... base/debug/crash_logging.h:74: size_t* out_key_storage_size); I guess there must be something I'm missing, here. The crash keys are being initialized by some caller who presumably already knows the out_key_storage_size info, more-or-less? My main thought about the num_chunks/max_length in CrashKey is that they are useful for when a caller calls Set/Clear, not for calculating the implementation space requirements. If you just need to return a count, you could always have it be the return value :-).
https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.cc File base/debug/crash_logging.cc (right): https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.c... base/debug/crash_logging.cc:64: // Handle the un-checked case. On 2013/01/08 00:34:37, shess wrote: > un-checked, or un-chunked? Done. https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.c... base/debug/crash_logging.cc:64: // Handle the un-checked case. On 2013/01/08 00:34:37, shess wrote: > un-checked, or un-chunked? Done. https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.c... base/debug/crash_logging.cc:70: for (size_t i = 1; i <= crash_key->num_chunks; ++i) { On 2013/01/08 00:34:37, shess wrote: > I had to stare at this for a couple seconds to see that here you're doing > [1,num_chunks], whereas above you're doing [0,num_chunks)+1. I could see making > these consistent, either zero-based with a +1, or 1-based without, though I can > see the above really wants zero-based. > > Another option would be to make the set version like: > for (size_t i = 0; i < chunks.size(); ++i) { > const std::string key = StringPrintf(..., i+1); > if (i < num_chunks) > set(...); > else > clear(...); > } > > then the clear version looks like that, without the if(). Switched to the [0,num_chunks)+1 approach. https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.c... base/debug/crash_logging.cc:160: for (size_t i = 0; On 2013/01/08 00:34:37, shess wrote: > Since you're breaking lines anyhow, might as well be: > for (size_t i = 0, offset = 0; Done. https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.h... base/debug/crash_logging.h:12: #include "base/callback.h" On 2013/01/08 00:34:37, shess wrote: > I assume this was here for using base::Callback, which is now gone? Done. https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.h... base/debug/crash_logging.h:74: size_t* out_key_storage_size); On 2013/01/08 00:34:37, shess wrote: > I guess there must be something I'm missing, here. The crash keys are being > initialized by some caller who presumably already knows the out_key_storage_size > info, more-or-less? My main thought about the num_chunks/max_length in CrashKey > is that they are useful for when a caller calls Set/Clear, not for calculating > the implementation space requirements. > > If you just need to return a count, you could always have it be the return value > :-). My assumption is that when the caller registers the key, they register the space for it and should know that. Windows requires all space for crash keys be allocated at Breakpad initialization. I went the route of calculating the exact amount of space required. The other option is to allocate a "safe" number. But how do you determine that safe number? Ideally you don't register crash keys at all, I think (like how Mac worked before), but I did it because of the Windows limitation. So I guess that leaves these options: 1) Calculate exact space requirements as done here. 2) Remove key registration requirement and guess at a "safe" number of keys (64, 128, 256?). 3) Keep the key registration requirement and use the size of the registration list * some factor (1.25?) to guess the number of keys for which space should be allocated. I implemented (1) and chunking/length properties are determined at registration. (2) and (3) could push chunking/length down to the Set() callsite.
I'm going home, maybe my brain is not thinkie write. https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.h... base/debug/crash_logging.h:74: size_t* out_key_storage_size); On 2013/01/08 00:56:23, rsesek wrote: > On 2013/01/08 00:34:37, shess wrote: > > I guess there must be something I'm missing, here. > > My assumption is that when the caller registers the key, they register the space > for it and should know that. I think you're over-analyzing. Mostly, I'm thinking "The caller passed me a wad of crap, and I'm passing back a count of the number of items in your wad of crap". But I would have assumed that the caller already knew how much crap it had. Is that right? I can see why it might be right. I'm just wondering if it is right. It kind of assumes that the caller can be sure it can allocate the space after calling this function, which may-or-may-not be true (it could have to allocate before calling, for instance). I guess maybe you could decouple implementation from this API, and just say "returns the number of crash keys needed to handle |keys|". Then I don't feel like the function is instructing me to do things a certain way. The caller can do whatever it wants with that information, either allocate the space now, or verify that previously-allocated space was big enough, or on Mac/Linux ignore it completely.
On 2013/01/08 01:06:05, shess wrote: > I'm going home, maybe my brain is not thinkie write. > > https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.h > File base/debug/crash_logging.h (right): > > https://codereview.chromium.org/11761030/diff/8002/base/debug/crash_logging.h... > base/debug/crash_logging.h:74: size_t* out_key_storage_size); > On 2013/01/08 00:56:23, rsesek wrote: > > On 2013/01/08 00:34:37, shess wrote: > > > I guess there must be something I'm missing, here. > > > > My assumption is that when the caller registers the key, they register the > space > > for it and should know that. > > I think you're over-analyzing. Mostly, I'm thinking "The caller passed me a wad > of crap, and I'm passing back a count of the number of items in your wad of > crap". But I would have assumed that the caller already knew how much crap it > had. Is that right? Yes. Instead of returning the count, the caller could count the number of chunks itself. I thought it'd be beneficial to not have that code site-specific, but I guess since Windows is the only platform that needs fixed-size storage, that doesn't matter too much. > I can see why it might be right. I'm just wondering if it is right. It kind of > assumes that the caller can be sure it can allocate the space after calling this > function, which may-or-may-not be true (it could have to allocate before > calling, for instance). > > I guess maybe you could decouple implementation from this API, and just say > "returns the number of crash keys needed to handle |keys|". Then I don't feel > like the function is instructing me to do things a certain way. The caller can > do whatever it wants with that information, either allocate the space now, or > verify that previously-allocated space was big enough, or on Mac/Linux ignore it > completely. I could see doing that. Something like TotalSizeNeededForKeys(const CrashKey* const, size_t count);
This may help clarify things: the initial (i.e. untested) attempt at the Windows impl: https://codereview.chromium.org/11776040
ping! Trying to get this usable this week.
On 2013/01/08 15:49:15, rsesek wrote: > I could see doing that. Something like TotalSizeNeededForKeys(const CrashKey* > const, size_t count); As noted, I was just thinking that you could return it, rather than changing the name. Unless you're pretty certain you'll need an *out or callback to propagate the value. https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging.cc File base/debug/crash_logging.cc (right): https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging.... base/debug/crash_logging.cc:19: std::map<std::string, CrashKey>* g_crash_keys_ = NULL; Just occurred to me - the key is redundant with the value's key_name, you could use StringPiece to duplicate. Or not, really. https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging.... base/debug/crash_logging.h:73: size_t* out_key_storage_size); I'm still -1 on having a size_t* out parameter when your return value is void. Might as well just return the value, the caller can ignore it. https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... File base/debug/crash_logging_unittest.cc (right): https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... base/debug/crash_logging_unittest.cc:25: key_values_ = new std::map<std::string, std::string>; Set this before setting the reporting functions, just in case. https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... base/debug/crash_logging_unittest.cc:94: EXPECT_EQ(values.end(), values.find(kChunk3)); It might make sense to have this up by the all-three case, to make sure clear doesn't have an off-by-one error (maybe the last value wouldn't be cleared right, but the 2-item case masked it). https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... base/debug/crash_logging_unittest.cc:107: EXPECT_EQ(key_values_->end(), key_values_->find(kTestKey)); Assertions about key_values_->size() might be reasonable. https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... base/debug/crash_logging_unittest.cc:114: { "chunked-6", 6, 200 }, I'd have been perfectly happy forbidding -N from the base key :-).
Thanks. Addressed all comments. Only outstanding issue is whether we should get rid of num_chunks, max_length, or neither. https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging.cc File base/debug/crash_logging.cc (right): https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging.... base/debug/crash_logging.cc:19: std::map<std::string, CrashKey>* g_crash_keys_ = NULL; On 2013/01/09 22:34:20, shess wrote: > Just occurred to me - the key is redundant with the value's key_name, you could > use StringPiece to duplicate. Or not, really. Clever. Done. https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging.h File base/debug/crash_logging.h (right): https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging.... base/debug/crash_logging.h:73: size_t* out_key_storage_size); On 2013/01/09 22:34:20, shess wrote: > I'm still -1 on having a size_t* out parameter when your return value is void. > Might as well just return the value, the caller can ignore it. OK, I don't feel strongly enough to argue ;). Done. https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... File base/debug/crash_logging_unittest.cc (right): https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... base/debug/crash_logging_unittest.cc:25: key_values_ = new std::map<std::string, std::string>; On 2013/01/09 22:34:20, shess wrote: > Set this before setting the reporting functions, just in case. Done. https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... base/debug/crash_logging_unittest.cc:94: EXPECT_EQ(values.end(), values.find(kChunk3)); On 2013/01/09 22:34:20, shess wrote: > It might make sense to have this up by the all-three case, to make sure clear > doesn't have an off-by-one error (maybe the last value wouldn't be cleared > right, but the 2-item case masked it). I want to test that under-filling it clears the last chunk. That's what line 87 does. https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... base/debug/crash_logging_unittest.cc:107: EXPECT_EQ(key_values_->end(), key_values_->find(kTestKey)); On 2013/01/09 22:34:20, shess wrote: > Assertions about key_values_->size() might be reasonable. Done. https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... base/debug/crash_logging_unittest.cc:114: { "chunked-6", 6, 200 }, On 2013/01/09 22:34:20, shess wrote: > I'd have been perfectly happy forbidding -N from the base key :-). Seems like it's more trouble than it's worth.
On 2013/01/09 23:06:29, rsesek wrote: > Thanks. Addressed all comments. Only outstanding issue is whether we should get > rid of num_chunks, max_length, or neither. I don't know, and it's probably not worth worrying about. Let's just watch what people do with it and if it seems like they are contorting themselves to do inappropriate things, we can change things. Though it maybe would make sense to pull max_length up to a parameter for the struct. Is there any case where it would be something other than the maximum implementation-defined length for a key? https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... File base/debug/crash_logging_unittest.cc (right): https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... base/debug/crash_logging_unittest.cc:94: EXPECT_EQ(values.end(), values.find(kChunk3)); On 2013/01/09 23:06:30, rsesek wrote: > On 2013/01/09 22:34:20, shess wrote: > > It might make sense to have this up by the all-three case, to make sure clear > > doesn't have an off-by-one error (maybe the last value wouldn't be cleared > > right, but the 2-item case masked it). > > I want to test that under-filling it clears the last chunk. That's what line 87 > does. Yeah, I agree that that needs tested. What I mean is that clear and set are distinct code paths, so if there were an off-by-one in clear, then this case wouldn't test for it because the value was already previously cleared. So I think it's worth testing that the last chunk gets cleared for both a short set and for a clear. https://codereview.chromium.org/11761030/diff/27001/base/debug/crash_logging.cc File base/debug/crash_logging.cc (right): https://codereview.chromium.org/11761030/diff/27001/base/debug/crash_logging.... base/debug/crash_logging.cc:133: } Doing DCHECK_EQ(count, g_crash_keys_->size()) would provide a check that none of the keys were duplicates.
On 2013/01/09 23:40:18, shess wrote: > On 2013/01/09 23:06:29, rsesek wrote: > > Thanks. Addressed all comments. Only outstanding issue is whether we should > get > > rid of num_chunks, max_length, or neither. > > I don't know, and it's probably not worth worrying about. Let's just watch what > people do with it and if it seems like they are contorting themselves to do > inappropriate things, we can change things. > > Though it maybe would make sense to pull max_length up to a parameter for the > struct. Is there any case where it would be something other than the maximum > implementation-defined length for a key? I agree that this is probably fine for now. What's registered right now doesn't make use of max_length, but I haven't yet looked earnestly at converting all the platforms' different child_process_logging yet, so maybe something will need it. At the very least, it makes chunking easier to test. Not that that's a good reason, though. What do you mean "pull max_length up to a paramater for the struct"? https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... File base/debug/crash_logging_unittest.cc (right): https://codereview.chromium.org/11761030/diff/16002/base/debug/crash_logging_... base/debug/crash_logging_unittest.cc:94: EXPECT_EQ(values.end(), values.find(kChunk3)); On 2013/01/09 23:40:18, shess wrote: > On 2013/01/09 23:06:30, rsesek wrote: > > On 2013/01/09 22:34:20, shess wrote: > > > It might make sense to have this up by the all-three case, to make sure > clear > > > doesn't have an off-by-one error (maybe the last value wouldn't be cleared > > > right, but the 2-item case masked it). > > > > I want to test that under-filling it clears the last chunk. That's what line > 87 > > does. > > Yeah, I agree that that needs tested. What I mean is that clear and set are > distinct code paths, so if there were an off-by-one in clear, then this case > wouldn't test for it because the value was already previously cleared. So I > think it's worth testing that the last chunk gets cleared for both a short set > and for a clear. Got it. Done. https://codereview.chromium.org/11761030/diff/27001/base/debug/crash_logging.cc File base/debug/crash_logging.cc (right): https://codereview.chromium.org/11761030/diff/27001/base/debug/crash_logging.... base/debug/crash_logging.cc:133: } On 2013/01/09 23:40:19, shess wrote: > Doing DCHECK_EQ(count, g_crash_keys_->size()) would provide a check that none of > the keys were duplicates. Good idea. Done.
lgtm. https://codereview.chromium.org/11761030/diff/22002/base/debug/crash_logging_... File base/debug/crash_logging_unittest.cc (right): https://codereview.chromium.org/11761030/diff/22002/base/debug/crash_logging_... base/debug/crash_logging_unittest.cc:98: // Clear everything. Your call as to whether this needs re-tested or not. IMHO the earlier clear test hits the edges, I'm confident that the interior items are tested. https://codereview.chromium.org/11761030/diff/22002/chrome/common/crash_keys.cc File chrome/common/crash_keys.cc (right): https://codereview.chromium.org/11761030/diff/22002/chrome/common/crash_keys.... chrome/common/crash_keys.cc:10: static const size_t kSingleChunkLength = 255; My max_length comment was that if all of the max_length are going to be kSingleChunkLength, then you might as well pass it to InitCrashKeys(), rather than putting it in every element of keys[]. I can't really think of a reason to ever have it be something other than "maximum per-key length the system allows" for this. Also, seeing all of those 1's makes me ponder whether the signal for "unchunked" should be 0, which would allow the stupid edge case of just having a single chunk. Like I said, stupid edge case, even thinking about it makes me short of breath.
https://codereview.chromium.org/11761030/diff/22002/chrome/common/crash_keys.cc File chrome/common/crash_keys.cc (right): https://codereview.chromium.org/11761030/diff/22002/chrome/common/crash_keys.... chrome/common/crash_keys.cc:10: static const size_t kSingleChunkLength = 255; On 2013/01/10 18:43:54, shess wrote: > My max_length comment was that if all of the max_length are going to be > kSingleChunkLength, then you might as well pass it to InitCrashKeys(), rather > than putting it in every element of keys[]. I can't really think of a reason to > ever have it be something other than "maximum per-key length the system allows" > for this. Ah, I like it. Done. > Also, seeing all of those 1's makes me ponder whether the signal for "unchunked" > should be 0, which would allow the stupid edge case of just having a single > chunk. Like I said, stupid edge case, even thinking about it makes me short of > breath. Hmmm. I see why that could work, but I think that just creates more complication.
LGTM. Long-term, might keep in mind whether the chunking factor is bimodal. If it's only ever going to be 1 or 10, then another option would be to have Init() take two arrays, one for regular keys and one for chunked keys. I don't think it's worth doing anything at all in that area right now, as that kind of thing would benefit from some field experience to guide it. https://codereview.chromium.org/11761030/diff/22002/chrome/common/crash_keys.cc File chrome/common/crash_keys.cc (right): https://codereview.chromium.org/11761030/diff/22002/chrome/common/crash_keys.... chrome/common/crash_keys.cc:10: static const size_t kSingleChunkLength = 255; On 2013/01/10 19:46:56, rsesek wrote: > On 2013/01/10 18:43:54, shess wrote: > > Also, seeing all of those 1's makes me ponder whether the signal for > "unchunked" > > should be 0, which would allow the stupid edge case of just having a single > > chunk. Like I said, stupid edge case, even thinking about it makes me short > of > > breath. > > Hmmm. I see why that could work, but I think that just creates more > complication. Yeah. I think it would be more justified if this were a heavily-used API of some sort, but here if the problem ever came up we could just fix it and not worry about tracking down a bunch of developers and retraining them.
Thanks for all the feedback, Scott. +brettw for OWNERS
brettw: ping
Brett is MIA, so let's try Mark.
LGTM rubberstamp based on scott's review. https://codereview.chromium.org/11761030/diff/41001/chrome/common/crash_keys.cc File chrome/common/crash_keys.cc (right): https://codereview.chromium.org/11761030/diff/41001/chrome/common/crash_keys.... chrome/common/crash_keys.cc:37: // Crash Key Name Constants //////////////////////////////////////////////////// I wouldn't do this comment since the file is small enough for it to be easy to understand. For complex cases where a divider really helps, most code generally uses hyphens.
https://codereview.chromium.org/11761030/diff/41001/base/debug/crash_logging.cc File base/debug/crash_logging.cc (right): https://codereview.chromium.org/11761030/diff/41001/base/debug/crash_logging.... base/debug/crash_logging.cc:19: std::map<base::StringPiece, CrashKey>* g_crash_keys_ = NULL; Typedef for this type, to reduce ugliness below? https://codereview.chromium.org/11761030/diff/41001/base/debug/crash_logging.... base/debug/crash_logging.cc:24: const char kChunkFormatString[] = "%s-%Iu"; Use PRIuS instead of "Iu" and "zu". #include "base/format_macros.h" to make sure you have the definition. https://codereview.chromium.org/11761030/diff/41001/chrome/app/breakpad_mac.mm File chrome/app/breakpad_mac.mm (right): https://codereview.chromium.org/11761030/diff/41001/chrome/app/breakpad_mac.m... chrome/app/breakpad_mac.mm:255: // Initialize the new scoped crash key system. It won’t be new three months from now, but nobody’s gonna revise the comment. https://codereview.chromium.org/11761030/diff/41001/chrome/common/crash_keys.cc File chrome/common/crash_keys.cc (right): https://codereview.chromium.org/11761030/diff/41001/chrome/common/crash_keys.... chrome/common/crash_keys.cc:14: // TODO(rsesek): Remove when done testing. Needed so arraysize > 0. If the arraysize > 0 bit is really important, put a COMPILE_ASSERT here so that it holds even after you fix this TODO.
https://codereview.chromium.org/11761030/diff/41001/base/debug/crash_logging.cc File base/debug/crash_logging.cc (right): https://codereview.chromium.org/11761030/diff/41001/base/debug/crash_logging.... base/debug/crash_logging.cc:19: std::map<base::StringPiece, CrashKey>* g_crash_keys_ = NULL; On 2013/01/15 21:31:03, Mark Mentovai wrote: > Typedef for this type, to reduce ugliness below? Done. https://codereview.chromium.org/11761030/diff/41001/base/debug/crash_logging.... base/debug/crash_logging.cc:24: const char kChunkFormatString[] = "%s-%Iu"; On 2013/01/15 21:31:03, Mark Mentovai wrote: > Use PRIuS instead of "Iu" and "zu". #include "base/format_macros.h" to make sure > you have the definition. Had no idea. Done. https://codereview.chromium.org/11761030/diff/41001/chrome/app/breakpad_mac.mm File chrome/app/breakpad_mac.mm (right): https://codereview.chromium.org/11761030/diff/41001/chrome/app/breakpad_mac.m... chrome/app/breakpad_mac.mm:255: // Initialize the new scoped crash key system. On 2013/01/15 21:31:03, Mark Mentovai wrote: > It won’t be new three months from now, but nobody’s gonna revise the comment. Done. https://codereview.chromium.org/11761030/diff/41001/chrome/common/crash_keys.cc File chrome/common/crash_keys.cc (right): https://codereview.chromium.org/11761030/diff/41001/chrome/common/crash_keys.... chrome/common/crash_keys.cc:14: // TODO(rsesek): Remove when done testing. Needed so arraysize > 0. On 2013/01/15 21:31:03, Mark Mentovai wrote: > If the arraysize > 0 bit is really important, put a COMPILE_ASSERT here so that > it holds even after you fix this TODO. This is get around a compiler error from ArraySizeHelper so I don't think that's necessary. https://codereview.chromium.org/11761030/diff/41001/chrome/common/crash_keys.... chrome/common/crash_keys.cc:37: // Crash Key Name Constants //////////////////////////////////////////////////// On 2013/01/15 21:19:17, brettw wrote: > I wouldn't do this comment since the file is small enough for it to be easy to > understand. For complex cases where a divider really helps, most code generally > uses hyphens. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rsesek@chromium.org/11761030/42002
Message was sent while issue was closed.
Change committed as 177015 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
