[client] New class LengthDelimitedRingBuffer [crashpad/crashpad : main]
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen, Mark Mentovai, Robert Sesek.
To view, visit change 4023618. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Fowles Kulukundis (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Mark Mentovai, Robert Sesek.
4 comments:
File client/length_delimited_ring_buffer.h:
Patch Set #25, Line 17:
constexpr uint32_t kDefaultRingBufferCapacity = 8192;you need to use `inline constexpr` in headers to avoid ODR violations
generally memcpy is more common to see then std::copy for things like this
Patch Set #25, Line 368:
ReadBase128VarintUint32FromRingBufferinstead of encoding the lengths using varint, I would just declare a fixed size of length and use that consistently
Patch Set #25, Line 436:
buffer_lengthprefer explicit comparison with zero to implicit promotion to bool
To view, visit change 4023618. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen, Mark Mentovai, Matthew Fowles Kulukundis, Robert Sesek.
4 comments:
File client/length_delimited_ring_buffer.h:
Patch Set #25, Line 17:
constexpr uint32_t kDefaultRingBufferCapacity = 8192;you need to use `inline constexpr` in headers to avoid ODR violations
Ah, I always miss that. Thanks, done.
generally memcpy is more common to see then std::copy for things like this
OK, done.
Patch Set #25, Line 368:
ReadBase128VarintUint32FromRingBuffer
instead of encoding the lengths using varint, I would just declare a fixed size of length and use th […]
My idea here was to be compatible with existing protobuf logic which knows how to deserialize a list of varint-delimited fields from an array. That makes things a lot simpler on the iOS side, at least.
If it's OK, I'd like to keep this as-is. But if you feel strongly, I'm happy to change it.
Patch Set #25, Line 436:
buffer_lengthprefer explicit comparison with zero to implicit promotion to bool
Thanks, done.
To view, visit change 4023618. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen, Mark Mentovai, Matthew Fowles Kulukundis, Robert Sesek.
1 comment:
File client/length_delimited_ring_buffer.h:
Patch Set #25, Line 368:
ReadBase128VarintUint32FromRingBufferMy idea here was to be compatible with existing protobuf logic which knows how to deserialize a list […]
Discussed offline. I added documentation explaining why this uses Base128 varint-delimited encoding and links to the relevant protobuf APIs as an example.
Matthew suggested returning an error if the Varint128-encoded length would take up more than 2 bytes (2^14 == 16 KiB), but the logic already returns an error if the client invokes `Push()` with a length larger than the capacity of the ringbuffer, so I left that logic as-is in case clients want to push payloads larger than 16 KiB onto the ringbuffer.
To view, visit change 4023618. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Fowles Kulukundis (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Mark Mentovai, Robert Sesek.
Patch set 30:Commit-Queue +1
Matthew Fowles Kulukundis (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Mark Mentovai, Robert Sesek.
1 comment:
Patchset:
LGTM
To view, visit change 4023618. To unsubscribe, or for help writing mail filters, visit settings.
Robert Sesek (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Mark Mentovai.
1 comment:
File client/length_delimited_ring_buffer.h:
Patch Set #36, Line 7:
#include <optional>Same comment on prior CL re: optional. I do think it could be nice to have optional in Crashpad, so I would maybe go with one of the first two suggestsions.
To view, visit change 4023618. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen, Mark Mentovai, Robert Sesek.
1 comment:
File client/length_delimited_ring_buffer.h:
Patch Set #36, Line 7:
#include <optional>
Same comment on prior CL re: optional. […]
Looks like Mark is OK with `std::optional` (as long as we don't trigger UB by using operator*/operator-> on an empty optional).
To view, visit change 4023618. To unsubscribe, or for help writing mail filters, visit settings.
Robert Sesek (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Mark Mentovai.
2 comments:
File client/length_delimited_ring_buffer.h:
Needs the boilerplate file header.
File client/length_delimited_ring_buffer_test.cc:
Ditto.
To view, visit change 4023618. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen, Mark Mentovai, Robert Sesek.
2 comments:
File client/length_delimited_ring_buffer.h:
Needs the boilerplate file header.
Argh, so sorry about that. Thanks, fixed.
File client/length_delimited_ring_buffer_test.cc:
Ditto.
Done
To view, visit change 4023618. To unsubscribe, or for help writing mail filters, visit settings.
Robert Sesek (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Mark Mentovai.
Patch set 38:Code-Review +1
1 comment:
Patchset:
Approving based on kfm@'s review.
To view, visit change 4023618. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Mark Mentovai.
Patch set 38:Commit-Queue +2
Crashpad LUCI CQ (Gerrit)
Crashpad LUCI CQ submitted this change.
[client] New class LengthDelimitedRingBuffer
This CL implements LengthDelimitedRingBuffer, a general-purpose
ringbuffer suitable for use as a Crashpad Annotation.
This ringbuffer supports writing variably-sized data delimited by a Base
128 varint-encoded length separator.
LengthDelimitedRingBuffer is backed by a std::array, so it has a fixed
maximum size. It supports reading via RingBufferReader as well as
writing via RingBufferWriter.
Change-Id: I23ecb4a85ee8e846e1efc6937a5cb089a494d50a
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4023618
Reviewed-by: Robert Sesek <rse...@chromium.org>
Commit-Queue: Ben Hamilton <benha...@google.com>
---
M client/BUILD.gn
A client/length_delimited_ring_buffer.h
A client/length_delimited_ring_buffer_test.cc
3 files changed, 853 insertions(+), 0 deletions(-)
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton.
14 comments:
File client/length_delimited_ring_buffer.h:
#include <algorithm>
#include <array>
#include <limits>
#include <optional>
#include <vector>
#include <stdint.h>
#include <string.h>C system headers come before C++ system headers.
Patch Set #39, Line 32:
inline constexpr uint32_t kDefaultRingBufferCapacity = 8192;Want a type alias for this?
static_assert(sizeof(Range) == 8,
"struct Range is not packed on this platform");Why is this important?
Patch Set #39, Line 160:
<< (length * 7);Handle overflow? You’re limited to a `uint32_t` result by your own design, but if the buffer’s been trashed, this will keep reading corrupted garbage and subjecting them to comically enormous shifts.
Patch Set #39, Line 234:
// Every varint encodes to at least 1 byte of data.Refer to the discussion on vertical whitespace in later patches in this series. If it’s hard to know when it’s good to stop and take a breath with a blank line, you can use comments as a good hint that “before” and “after” are sufficiently different.
Patch Set #39, Line 235:
int length = 1;This example carries additional value: you’d want a blank line after this, too, because the comment above only applies to this line, not really to anything that follows.
Patch Set #39, Line 293:
const uint8_t* other_ring_buffer_bytes = reinterpret_cast<const uint8_t*>(buffer) + sizeof(*other_header);This went beyond 80 columns. Didn’t run `git cl format`? Now you’ll want to `clang-format` the whole file.
Patch Set #39, Line 311:
static constexpr uint32_t kMagic = 0xcab00d1e;cute ;)
Patch Set #39, Line 344:
// using CTAD.Consider your audience (or audiences!) If your readers know what CTAD is, this comment isn’t necessary. If they don’t know what CTAD is, the comment is helpful, but you just threw a bunch of unpronounceable capital letters in their faces. So as written, this comment doesn’t really address any audience well.
Don’t send your readers to the dictionary. Spell it out or leave it out.
Lines 423 and 538 too.
File client/length_delimited_ring_buffer_test.cc:
#include <array>
#include <string>
#include <vector>
#include <stdint.h>C system headers before C++ system headers.
Patch Set #39, Line 26:
namespace {Tests go in `crashpad::test::(unnamed)`.
using ::crashpad::LengthDelimitedRingBufferReader;
using ::crashpad::LengthDelimitedRingBufferWriter;
using ::crashpad::RingBufferData;You won’t need these if you’re properly within `namespace crashpad` or a child thereof.
using ::crashpad::LengthDelimitedRingBufferReader;
using ::crashpad::LengthDelimitedRingBufferWriter;
using ::crashpad::RingBufferData;
using ::testing::Eq;
using ::testing::IsFalse;
using ::testing::IsTrue;The full-qualification leading `::` is almost never done in Crashpad. I guess a couple others have slipped through the cracks, but you’ve added more in this one file than we had to begin with.
\xAB"
"\xCD";Not a big deal, but you wrote a bunch of pretty hex digits with lowercase letters, and then suddenly STARTED SHOUTING.
To view, visit change 4023618. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
14 comments:
Patchset:
Thanks for the review. Sent http://crrev.com/c/4199705 to fix everything but the static_assert, question below.
File client/length_delimited_ring_buffer.h:
#include <algorithm>
#include <array>
#include <limits>
#include <optional>
#include <vector>
#include <stdint.h>
#include <string.h>C system headers come before C++ system headers.
Done
Patch Set #39, Line 32:
inline constexpr uint32_t kDefaultRingBufferCapacity = 8192;Want a type alias for this?
Done
static_assert(sizeof(Range) == 8,
"struct Range is not packed on this platform");Why is this important?
Since this struct is directly persisted to disk via Annotation, I wanted to make sure its alignment and size doesn't change for any reason (compiler changes, etc.).
Is there a better way to ensure this? I can take out the static_asserts here and leave them to the RingBufferAnnotation itself, but I figured the error messages would be easier to understand with a few smaller static_asserts.
Patch Set #39, Line 160:
<< (length * 7);
Handle overflow? You’re limited to a `uint32_t` result by your own design, but if the buffer’s been […]
Done
Patch Set #39, Line 234:
// Every varint encodes to at least 1 byte of data.
Refer to the discussion on vertical whitespace in later patches in this series. […]
Done
Patch Set #39, Line 235:
int length = 1;
This example carries additional value: you’d want a blank line after this, too, because the comment […]
Done
Patch Set #39, Line 293:
const uint8_t* other_ring_buffer_bytes = reinterpret_cast<const uint8_t*>(buffer) + sizeof(*other_header);
This went beyond 80 columns. […]
Done
Patch Set #39, Line 344:
// using CTAD.
Consider your audience (or audiences!) If your readers know what CTAD is, this comment isn’t necessa […]
Done
File client/length_delimited_ring_buffer_test.cc:
#include <array>
#include <string>
#include <vector>
#include <stdint.h>C system headers before C++ system headers.
Done
Patch Set #39, Line 26:
namespace {Tests go in `crashpad::test::(unnamed)`.
Done
using ::crashpad::LengthDelimitedRingBufferReader;
using ::crashpad::LengthDelimitedRingBufferWriter;
using ::crashpad::RingBufferData;You won’t need these if you’re properly within `namespace crashpad` or a child thereof.
Done
using ::crashpad::LengthDelimitedRingBufferReader;
using ::crashpad::LengthDelimitedRingBufferWriter;
using ::crashpad::RingBufferData;
using ::testing::Eq;
using ::testing::IsFalse;
using ::testing::IsTrue;
The full-qualification leading `::` is almost never done in Crashpad. […]
Done
\xAB"
"\xCD";
Not a big deal, but you wrote a bunch of pretty hex digits with lowercase letters, and then suddenly […]
Done
To view, visit change 4023618. To unsubscribe, or for help writing mail filters, visit settings.