spanification: automatically spanify .../tracing/perfetto/privacy_filtering_check.cc etc. [chromium/src : main]
Daniel Angulo (Gerrit)
Daniel Angulo voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 10 comments![]( "Open in Gerrit")
size_t acceptedFieldIdSize(const int* accepted_field_ids) {
size_t size = 0;
while (UNSAFE_TODO(accepted_field_ids[size]) != -1) {
size++;
}
return size;
}This is highly performance sensitive code, I think we shouldn't be computing the size. As I said I think we should keep this as unsafe until we update the generator to use bounds checked objects.
// SAFETY: proto is a pointer to the serialized trace proto which is a
// kProtoZeroMessageSet, which is a repeated field of
// TracePacket. TracePacket has a maximum size of 256 MiB - far below the
// threshold where we would need to use unsafe buffers.What does this mean about "far below the threshold"? This comment doesn't explain why this is safe? I think the fact is ProtoDecoder is a span like type, and it doesn't do bounds checking, at the moment it has 1 unsafe constructor and 2 safe-ish constructors, we can't claim UNSAFE_BUFFERS here and wash our hands of it. Lets leave it as UNSAFE_TODO.
auto proto_span = UNSAFE_BUFFERS(base::span(Also this type is non-obvious type so auto hides the information.
```
base::span<uint8_t> proto_span = UNSAFE_TODO(base:span(proto->begin(), proto->bytes_left()));
```
proto->begin(), static_cast<size_t>(proto->end() - proto->begin())));use bytes_left()
// SAFETY: MessageInfo states that the accepted_field_id is terminated byCan you link to this?
root->accepted_field_ids,It feels like we should spanify this, but this is coming from generated code see (go/how-do-i-chrometto), so for now this needs to remain UNSAFE_TODO.
output.append(proto_span.subspan(current_field_start).data(),
proto_span.subspan(next_field_start).data());This doesn't look great (also two subspans being created)
```
const std::string_view msg_data =
base::as_string_view(proto_span.subspan(current_field_start, next_field_start - current_field_start));
output.append(msg_data.begin(), msg_data.end());
```
Perhaps?
Actually thinking about this a bit more we are just looping over `proto` and reading one by one, we can instead of creating a bunch of subspans just use SpanReader?
I.E.
```
base::SpanReader reader (proto_span);
size_t current_field_start = 0;
size_t next_field_start = 0;
for (auto f = proto->ReadField(); f.valid();
f = proto->ReadField(), current_field_start = next_field_start) {
next_field_start = proto->read_offset();
CHECK_EQ(reader.num_read(), current_field_start);
...
maybe_data = reader.Read(next_field_start - current_field_start);
output.append(maybe_data->begin(), maybe_data->end());
```
.copy_from(base::as_byte_span(output).subspan(out_msg_start_offset,note: this has to remain copy_from (since it is potentially overlapping).
.copy_from(base::span(field_id_buf).first(field_id_length));copy_from_nonoverlapping
.copy_from(base::span(payload_size_buf).first(payload_size_length));copy_from_nonoverlapping
Related details
- Arthur Sonzogni
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
.copy_from(base::span(field_id_buf).first(field_id_length));copy_from_nonoverlapping
Hi @nus...@chromium.org, I have a different opinion about `copy_from`/`copy_from_nonoverlapping`. I don't really believe there are meaningful performance difference and we should use the short/safe version by default.
I added some arguments below:
https://chromium-review.googlesource.com/c/chromium/src/+/7541348/comment/7892b7db_f5eb5c23/
WDYT?
Related details
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 8 comments![]( "Open in Gerrit")
size_t acceptedFieldIdSize(const int* accepted_field_ids) {
size_t size = 0;
while (UNSAFE_TODO(accepted_field_ids[size]) != -1) {
size++;
}
return size;
}This is highly performance sensitive code, I think we shouldn't be computing the size. As I said I think we should keep this as unsafe until we update the generator to use bounds checked objects.
Done
// SAFETY: proto is a pointer to the serialized trace proto which is a
// kProtoZeroMessageSet, which is a repeated field of
// TracePacket. TracePacket has a maximum size of 256 MiB - far below the
// threshold where we would need to use unsafe buffers.What does this mean about "far below the threshold"? This comment doesn't explain why this is safe? I think the fact is ProtoDecoder is a span like type, and it doesn't do bounds checking, at the moment it has 1 unsafe constructor and 2 safe-ish constructors, we can't claim UNSAFE_BUFFERS here and wash our hands of it. Lets leave it as UNSAFE_TODO.
I leave it as UNSAFE_TODO for now.
Also this type is non-obvious type so auto hides the information.
```
base::span<uint8_t> proto_span = UNSAFE_TODO(base:span(proto->begin(), proto->bytes_left()));
```
Done
proto->begin(), static_cast<size_t>(proto->end() - proto->begin())));Daniel Angulouse bytes_left()
Done
// SAFETY: MessageInfo states that the accepted_field_id is terminated byCan you link to this?
Done
It feels like we should spanify this, but this is coming from generated code see (go/how-do-i-chrometto), so for now this needs to remain UNSAFE_TODO.
Done
output.append(proto_span.subspan(current_field_start).data(),
proto_span.subspan(next_field_start).data());This doesn't look great (also two subspans being created)
```
const std::string_view msg_data =
base::as_string_view(proto_span.subspan(current_field_start, next_field_start - current_field_start));
output.append(msg_data.begin(), msg_data.end());
```Perhaps?
Actually thinking about this a bit more we are just looping over `proto` and reading one by one, we can instead of creating a bunch of subspans just use SpanReader?
I.E.
```
base::SpanReader reader (proto_span);
size_t current_field_start = 0;
size_t next_field_start = 0;
for (auto f = proto->ReadField(); f.valid();
f = proto->ReadField(), current_field_start = next_field_start) {
next_field_start = proto->read_offset();
CHECK_EQ(reader.num_read(), current_field_start);
...
maybe_data = reader.Read(next_field_start - current_field_start);
output.append(maybe_data->begin(), maybe_data->end());
```
Done
.copy_from(base::span(payload_size_buf).first(payload_size_length));Daniel Angulocopy_from_nonoverlapping
Done
Related details
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 2 comments![]( "Open in Gerrit")
Let me know when the tests are passing.
.copy_from(base::span(field_id_buf).first(field_id_length));copy_from_nonoverlapping
Hi @nus...@chromium.org, I have a different opinion about `copy_from`/`copy_from_nonoverlapping`. I don't really believe there are meaningful performance difference and we should use the short/safe version by default.
I added some arguments below:
https://chromium-review.googlesource.com/c/chromium/src/+/7541348/comment/7892b7db_f5eb5c23/WDYT?
Interesting let me reply there, I'm happy to go with copy_from as a default, I had been operating as a clearly different buffers should be done with non_overlapping to ensure maximum performance to avoid impact due to our rewrites.
In this code we're finishing a trace which isn't super performance critical happy to go with the safe versions here.
Related details
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 1 comment![]( "Open in Gerrit")
Let me know when the tests are passing.
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 3 comments![]( "Open in Gerrit")
proto->begin(), static_cast<size_t>(proto->end() - proto->begin())));we don't need to use the size do we? Can't we just do `base::span(proto->begin(), proto->end())` I think it reads clearer?
Update the safety comment similarly.
auto data = proto_span.subspan(current_field_start, field_size);
output.append(reinterpret_cast<const char*>(data.data()), data.size());if the only time we use data is as a char should we make `proto_span` be a char array?
I.E.
```
auto proto_span = base::as_chars(UNSAFE_BUFFERS(base::span(proto->begin(), proto->end())));
```
Then we don't need the cast?
base::as_writable_byte_span(output)We use output as a bytes_span repeatedly can we factor it out
```
auto output_bytes = base::as_writable_byte_span(output);
output_bytes.subspan(..., ...).copy_from(output_bytes.subspan(..., ...));
```
Related details
- Arthur Sonzogni
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 5 comments![]( "Open in Gerrit")
BUG=439964610
Change-Id: Id28596d051629e66bec29f70a4fe700a121137e9nit: Ideally, the git footer line aren't separated by an empty line.
Moreover the syntax is "Bug: 439964610"
Right now we have:
```
cat description | git footers
Change-Id: Id28596d051629e66bec29f70a4fe700a121137e9
```
If you remove the new line and update the format, you get:
```
cat description | git footers
Change-Id: Id28596d051629e66bec29f70a4fe700a121137e9
Bug: 439964610
```
#include "base/containers/span_reader.h"Is this used?
proto->begin(), static_cast<size_t>(proto->end() - proto->begin())));we don't need to use the size do we? Can't we just do `base::span(proto->begin(), proto->end())` I think it reads clearer?
Update the safety comment similarly.
I think span has a constructor from a pair of iterator.
```
auto proto_span = base::span(proto)
```
This allows not having a SAFETY/UNSAFE_BUFFERS
auto data = proto_span.subspan(current_field_start, field_size);
output.append(reinterpret_cast<const char*>(data.data()), data.size());if the only time we use data is as a char should we make `proto_span` be a char array?
I.E.
```
auto proto_span = base::as_chars(UNSAFE_BUFFERS(base::span(proto->begin(), proto->end())));
```Then we don't need the cast?
Or:
```
output.append(base::as_string_view(data));
```
.copy_from_nonoverlapping(`copy_from` ? (That is the safer variant, and I don't believe it is slower)
Related details
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 6 comments![]( "Open in Gerrit")
BUG=439964610
Change-Id: Id28596d051629e66bec29f70a4fe700a121137e9nit: Ideally, the git footer line aren't separated by an empty line.
Moreover the syntax is "Bug: 439964610"Right now we have:
```
cat description | git footers
Change-Id: Id28596d051629e66bec29f70a4fe700a121137e9
```If you remove the new line and update the format, you get:
```
cat description | git footers
Change-Id: Id28596d051629e66bec29f70a4fe700a121137e9
Bug: 439964610
```
Done
#include "base/containers/span_reader.h"Daniel AnguloIs this used?
Done
proto->begin(), static_cast<size_t>(proto->end() - proto->begin())));Arthur Sonzogniwe don't need to use the size do we? Can't we just do `base::span(proto->begin(), proto->end())` I think it reads clearer?
Update the safety comment similarly.
I think span has a constructor from a pair of iterator.
```
auto proto_span = base::span(proto)
```This allows not having a SAFETY/UNSAFE_BUFFERS
converting directly proto to span leads to this error: no viable constructor or deduction guide for deduction of template arguments of 'base::span'
using begin() and end() requires UNSAFE_BUFFERS
auto data = proto_span.subspan(current_field_start, field_size);
output.append(reinterpret_cast<const char*>(data.data()), data.size());Arthur Sonzogniif the only time we use data is as a char should we make `proto_span` be a char array?
I.E.
```
auto proto_span = base::as_chars(UNSAFE_BUFFERS(base::span(proto->begin(), proto->end())));
```Then we don't need the cast?
Or:
```
output.append(base::as_string_view(data));
```
output.append(base::as_string_view(data));
looks nicer
We use output as a bytes_span repeatedly can we factor it out
```
auto output_bytes = base::as_writable_byte_span(output);output_bytes.subspan(..., ...).copy_from(output_bytes.subspan(..., ...));
```
Done
`copy_from` ? (That is the safer variant, and I don't believe it is slower)
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 3 comments![]( "Open in Gerrit")
proto->begin(), static_cast<size_t>(proto->end() - proto->begin())));Arthur Sonzogniwe don't need to use the size do we? Can't we just do `base::span(proto->begin(), proto->end())` I think it reads clearer?
Update the safety comment similarly.
Daniel AnguloI think span has a constructor from a pair of iterator.
```
auto proto_span = base::span(proto)
```This allows not having a SAFETY/UNSAFE_BUFFERS
converting directly proto to span leads to this error: no viable constructor or deduction guide for deduction of template arguments of 'base::span'
using begin() and end() requires UNSAFE_BUFFERS
deduction is a guide to what type should the template values take, you can override it. Can you try
```
base::span<uint8_t>(proto)
```
?
// SAFETY: proto is a pointer to the serialized trace proto which is a
// kProtoZeroMessageSet, which is a repeated field of
// TracePacket. TracePacket has a maximum size of 256 MiB - far below the
// threshold where we would need to use unsafe buffers.Daniel AnguloWhat does this mean about "far below the threshold"? This comment doesn't explain why this is safe? I think the fact is ProtoDecoder is a span like type, and it doesn't do bounds checking, at the moment it has 1 unsafe constructor and 2 safe-ish constructors, we can't claim UNSAFE_BUFFERS here and wash our hands of it. Lets leave it as UNSAFE_TODO.
I leave it as UNSAFE_TODO for now.
Also this looks to still be a UNSAFE_BUFFERS not UNSAFE_TODO as requested since `proto` isn't hardened if we keep the constructor that needs to be wrapped it should be UNSAFE_TODO (if the other comment manages to remove this UNSAFE_TODO I guess that is fine).
.copy_from(base::as_byte_span(output).subspan(out_msg_start_offset,Isn't this just
`output_bytes`
?
Related details
- Arthur Sonzogni
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 3 comments![]( "Open in Gerrit")
proto->begin(), static_cast<size_t>(proto->end() - proto->begin())));Arthur Sonzogniwe don't need to use the size do we? Can't we just do `base::span(proto->begin(), proto->end())` I think it reads clearer?
Update the safety comment similarly.
Daniel AnguloI think span has a constructor from a pair of iterator.
```
auto proto_span = base::span(proto)
```This allows not having a SAFETY/UNSAFE_BUFFERS
Stephen Nuskoconverting directly proto to span leads to this error: no viable constructor or deduction guide for deduction of template arguments of 'base::span'
using begin() and end() requires UNSAFE_BUFFERS
deduction is a guide to what type should the template values take, you can override it. Can you try
```
base::span<uint8_t>(proto)
```?
I tried. This work:
```
base::span<uint8_t>(*proto)
```
}A nice thing about `base::span` is that we can "peel" them using `split_at` or `take_first`. When there are disjoint union, we can name the separate parts and we don't need to repeat the different offset?
I have generated this example using Gemini:
```
// Recursively copies the |proto|'s accepted field IDs including all sub
// messages, over to |output|. Keeps track of |parent_ids| - field id in parent
// message, in order of most recent child last.
bool FilterProtoRecursively(const MessageInfo* root,
ProtoDecoder* proto,
std::vector<uint32_t>* parent_ids,
std::string& output) {
// Write any allowed fields of the message (the message's "payload") into
// |output| at the |out_msg_start_offset|. This will not include the field ID
// or size of the current message yet. We add those back below once we know
// the final message size. Emitting the message payload into |output| saves
// allocations for extra buffer, but will still require a memmove below. Other
// alternative is to just use the max length bytes like protozero does.
bool has_blocked_fields = false;
const size_t out_msg_start_offset = output.size();
proto->Reset();
base::span<const uint8_t> remaining_proto = base::span(*proto);
for (auto f = proto->ReadField(); f.valid(); f = proto->ReadField()) {
// ReadField() advanced the internal decoder pointer. The field we just read
// is the difference between the previous remaining bytes and the current.
base::span<const uint8_t> field_span = remaining_proto.take_first(
remaining_proto.size() - proto->bytes_left());
// If the field is not available in the accepted fields, then skip copying.
int index = FindIndexOfValue(root->accepted_field_ids, f.id());
if (index == -1) {
#if DCHECK_IS_ON()
LogDisallowedField(parent_ids, f.id());
#endif
has_blocked_fields = true;
continue;
}
// If the field is allowed, then either the field is a nested message, or a
// POD. If it's a nested message, then the message description will be
// part of |sub_messages| list. If the message description is nullptr, then
// assume it is POD.
if (!root->sub_messages ||
UNSAFE_TODO(root->sub_messages[index]) == nullptr) {
// PODs can just be copied over to output. Packed fields can be treated
// just like primitive fields, by just copying over the full data. Note
// that there cannot be packed nested messages. Note that we cannot use
// |f.data()| here since it does not include the preamble (field id and
// possibly length), so we need to use |field_span|.
output.append(base::as_string_view(field_span));
} else {
// Make recursive call to filter the nested message.
ProtoDecoder decoder(f.data(), f.size());
parent_ids->push_back(f.id());
has_blocked_fields |= FilterProtoRecursively(
UNSAFE_TODO(root->sub_messages[index]), &decoder, parent_ids, output);
parent_ids->pop_back();
}
}
const uint32_t payload_size = output.size() - out_msg_start_offset;
// The format is <field id><payload size><message data>.
// This function wrote the payload of the current message starting from the
// end of output. We need to insert the preamble (<field id><payload size>),
// after moving the payload by the size of the preamble.
const uint32_t field_id =
protozero::proto_utils::MakeTagLengthDelimited(parent_ids->back());
std::array<uint8_t, protozero::proto_utils::kMaxTagEncodedSize> field_id_buf =
{};
std::array<uint8_t, protozero::proto_utils::kMessageLengthFieldSize>
payload_size_buf = {};
uint8_t* field_id_end =
protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());
const size_t field_id_length = field_id_end - field_id_buf.data();
uint8_t* payload_size_end = protozero::proto_utils::WriteVarInt(
payload_size, payload_size_buf.data());
const size_t payload_size_length = payload_size_end - payload_size_buf.data();
const size_t preamble_length = field_id_length + payload_size_length;
output.append(preamble_length, 0);
auto message_span = base::as_writable_byte_span(output);
message_span.take_first(out_msg_start_offset);
if (payload_size != 0) {
// Move the payload to make room for the preamble at the beginning.
// message_span is [original_payload][empty_space]
// destination is [empty_space][final_payload_position]
message_span.subspan(preamble_length).copy_from(
message_span.first(payload_size));
}base::span field_id_span = message_span.take_first(field_id_length);
base::span payload_size_span = message_span.take_first(payload_size_length);
field_id_span.copy_from(base::span(field_id_buf).first(field_id_length));
payload_size_span.copy_from(base::span(payload_size_buf).first(payload_size_length));
return has_blocked_fields;
}
```
(I only verified it compile and pass the PrivacyFilteringTest, no general guarantee)
WDYT? Maybe we can try to use `take_first` more to me the code more idiomatic?
Related details
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 3 comments![]( "Open in Gerrit")
proto->begin(), static_cast<size_t>(proto->end() - proto->begin())));Arthur Sonzogniwe don't need to use the size do we? Can't we just do `base::span(proto->begin(), proto->end())` I think it reads clearer?
Update the safety comment similarly.
Daniel AnguloI think span has a constructor from a pair of iterator.
```
auto proto_span = base::span(proto)
```This allows not having a SAFETY/UNSAFE_BUFFERS
Stephen Nuskoconverting directly proto to span leads to this error: no viable constructor or deduction guide for deduction of template arguments of 'base::span'
using begin() and end() requires UNSAFE_BUFFERS
Arthur Sonzognideduction is a guide to what type should the template values take, you can override it. Can you try
```
base::span<uint8_t>(proto)
```?
I tried. This work:
```
base::span<uint8_t>(*proto)
```
it didn't work for me, I got error: services/tracing/perfetto/privacy_filtering_check.cc:77:21: error: no matching conversion for functional-style cast from 'ProtoDecoder' to 'base::span<uint8_t>' (aka 'base::span<unsigned char>')
77 | auto proto_span = base::span<uint8_t>(*proto);
but figured out that needs to be const uint8_t
`auto proto_span = base::span<const uint8_t>(*proto);`
// SAFETY: proto is a pointer to the serialized trace proto which is a
// kProtoZeroMessageSet, which is a repeated field of
// TracePacket. TracePacket has a maximum size of 256 MiB - far below the
// threshold where we would need to use unsafe buffers.Daniel AnguloWhat does this mean about "far below the threshold"? This comment doesn't explain why this is safe? I think the fact is ProtoDecoder is a span like type, and it doesn't do bounds checking, at the moment it has 1 unsafe constructor and 2 safe-ish constructors, we can't claim UNSAFE_BUFFERS here and wash our hands of it. Lets leave it as UNSAFE_TODO.
Stephen NuskoI leave it as UNSAFE_TODO for now.
Also this looks to still be a UNSAFE_BUFFERS not UNSAFE_TODO as requested since `proto` isn't hardened if we keep the constructor that needs to be wrapped it should be UNSAFE_TODO (if the other comment manages to remove this UNSAFE_TODO I guess that is fine).
Done
.copy_from(base::as_byte_span(output).subspan(out_msg_start_offset,Isn't this just
`output_bytes`
?
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 2 comments![]( "Open in Gerrit")
// SAFETY: proto is a pointer to a buffer which is guaranteed to have at least
// end() - begin() bytes, which is the size of the proto.
// auto proto_span = UNSAFE_TODO(base::span(proto->begin(), proto->end()));nit: looks like this safety comment isn't needed anymore?
I like it in general, naming the parts rather than a bunch of offsets seems pretty nice, but I do have some comments especially once I see it fully diffed.
For example could we merge the two arrays into one and create a split span for the two locations, then we could .first from them to get the spans of the real data, making it a bit clearer.
```
std::array<uint8_t, protozero::proto_utils::kMaxTagEncodedSize + protozero::proto_utils::kMessageLengthFieldSize> storage =
{};
auto [field_id_buf, payload_size_buf] = base::span(storage).split_at<protozero::proto_utils::kMaxTagEncodedSize>();
uint8_t* const field_id_end =
protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());
field_id_buf = field_id_buf.first(field_id_end - field_id_buf.data());
uint8_t* const payload_size_end = protozero::proto_utils::WriteVarInt(
payload_size, payload_size_buf.data());
payload_size_buf = payload_size_buf.first(payload_size_end - payload_size_buf.data());
const size_t preamble_size = payload_size_buf.size() + field_id_buf.size();
....
message_span.take_first(field_id_buf.size()).copy_from(field_id_buf);
message_span.take_first(payload_size_buf.size()).copy_from(payload_size_buf);
```
Also
```
auto message_span = base::as_writable_byte_span(output);
message_span.take_first(out_msg_start_offset);
```
is just
```
auto message_span = base::as_writable_byte_span(output).subspan(out_msg_start_offset);
```
So please take a look for other optimizations and readability if you go this route.
Related details
- Arthur Sonzogni
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 2 comments![]( "Open in Gerrit")
// SAFETY: proto is a pointer to a buffer which is guaranteed to have at least
// end() - begin() bytes, which is the size of the proto.
// auto proto_span = UNSAFE_TODO(base::span(proto->begin(), proto->end()));nit: looks like this safety comment isn't needed anymore?
Done
for the first snippet, where could it be placed? I don't see obvious substituition
Related details
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
The first snippet from arthur or from me? for Arthur his snippet is just the whole `FilterProtoRecursively` for me my snippet replaced
```
// The format is <field id><payload size><message data>.
// This function wrote the payload of the current message starting from the
// end of output. We need to insert the preamble (<field id><payload size>),
// after moving the payload by the size of the preamble.
const uint32_t field_id =
protozero::proto_utils::MakeTagLengthDelimited(parent_ids->back());
std::array<uint8_t, protozero::proto_utils::kMaxTagEncodedSize> field_id_buf =
{};
std::array<uint8_t, protozero::proto_utils::kMessageLengthFieldSize>
payload_size_buf = {};
uint8_t* field_id_end =
protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());
const size_t field_id_length = field_id_end - field_id_buf.data();
uint8_t* payload_size_end = protozero::proto_utils::WriteVarInt(
payload_size, payload_size_buf.data());
const size_t payload_size_length = payload_size_end - payload_size_buf.data();
const size_t preamble_length = field_id_length + payload_size_length;
output.append(preamble_length, 0);
- Arthur Sonzogni
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Angulo
| Auto-Submit | +1 |
yeah I was talking about the snippet you first commented, thanks for the clarification. I've made the substituition but tests are not passing, it was throwing a lot of compile errors because undefined variables, I renamed the variables to their corresponding equivalent, but now a lot of tests just crash. I'll keep previous version if you don't mind.
Related details
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
Which tests and why do they crash? It should be functionally the same? if it isn't then it likely is just a typo or something minor? It is good if you give concrete examples in situations like this so I can understand if there is a fundamental issue I didn't think about or if a minor tweak would make it work.
Related details
- Arthur Sonzogni
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 1 comment![]( "Open in Gerrit")
check patchset 16, there is the substituited code (I commented original subsstituited code for clear comparison, will delete once new code is working).
I had to add field_id_buf_first and payload_size_buf_first variables because split don't allow reasign. Also renaming preamble_length to preamble_size.
this patchset fails with
8 tests crashed:
PrivacyFilteringTest.NestedSafeAndUnsafeFields (../../services/tracing/perfetto/privacy_filtering_check_unittest.cc:123)
PrivacyFilteringTest.RepeatedFields (../../services/tracing/perfetto/privacy_filtering_check_unittest.cc:57)
PrivacyFilteringTest.SafeAndUnsafeFields (../../services/tracing/perfetto/privacy_filtering_check_unittest.cc:97)
PrivacyFilteringTest.SafeAndUnsafePackets (../../services/tracing/perfetto/privacy_filtering_check_unittest.cc:110)
PrivacyFilteringTest.SafeMessageWithOnlyUnsafeFields (../../services/tracing/perfetto/privacy_filtering_check_unittest.cc:83)
PrivacyFilteringTest.SafeToplevelField (../../services/tracing/perfetto/privacy_filtering_check_unittest.cc:39)
PrivacyFilteringTest.SafeToplevelMessageField (../../services/tracing/perfetto/privacy_filtering_check_unittest.cc:48)
PrivacyFilteringTest.UnsafeToplevelField (../../services/tracing/perfetto/privacy_filtering_check_unittest.cc:75)
Reasons:
Check failed: size_type{count} <= size().
and
Check failed: size() == other.size().
Related details
- Arthur Sonzogni
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
Isn't the crash because you made `field_id_buf_first` but did the `copy_from` using `field_id_buf` which isn't the right size? and similarly for `payload_size_buf`?
It is hard to tell because you don't have the full crash stacks but that seems like the issue?
For the `payload_size_buf_first` issue can you use `payload_size_buf.take_first(payload_size)`?
Related details
- Arthur Sonzogni
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Angulo
| Auto-Submit | +1 |
thanks for pointing that out. I've made the changes but still crashing, now without obvious reason.
[0318/174659.488245:INFO:base/test/launcher/test_launcher.cc:1209] Starting [PrivacyFilteringTest.RepeatedFields, PrivacyFilteringTest.UnsafeToplevelField, Priva
cyFilteringTest.SafeMessageWithOnlyUnsafeFields, PrivacyFilteringTest.SafeAndUnsafeFields, PrivacyFilteringTest.SafeAndUnsafePackets, PrivacyFilteringTest.Nested
SafeAndUnsafeFields, AdaptPerfettoConfigForChromeTest.PrivacyFiltering, CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_0_BlockSource_0, CookieSettingsTestP
.IsPrivacyModeEnabled/GrantSource_0_BlockSource_1, CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_1_BlockSource_0]
[0318/174659.488265:INFO:base/test/launcher/test_launcher.cc:1209] Starting [CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_1_BlockSource_1, CookieSettings
TestP.IsPrivacyModeEnabled/GrantSource_2_BlockSource_0, CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_2_BlockSource_1, CookieSettingsTestP.IsPrivacyModeEn
abled/GrantSource_3_BlockSource_0, CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_3_BlockSource_1]
[0318/174659.488251:INFO:base/test/launcher/test_launcher.cc:1209] Starting [NetworkContextTest.PrivacyModeDisabledByDefault, NetworkContextTest.PrivacyModeEnabl
edIfCookiesBlocked, NetworkContextTest.PrivacyModeDisabledIfCookiesAllowed, NetworkContextTest.PrivacyModeDisabledIfCookiesSettingForOtherURL, NetworkContextTest
.PrivacyModeEnabledIfThirdPartyCookiesBlocked, TracingConsumerTest.PrivacyFilterConfig, TracingConsumerTest.NoPrivacyFilterWithJsonConversion, PrivacyFilteringTe
st.EmptyTrace, PrivacyFilteringTest.SafeToplevelField, PrivacyFilteringTest.SafeToplevelMessageField]
[1/25] CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_1_BlockSource_1 (0 ms)
[2/25] CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_2_BlockSource_0 (0 ms)
[3/25] CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_2_BlockSource_1 (0 ms)
[4/25] CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_3_BlockSource_0 (0 ms)
[5/25] CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_3_BlockSource_1 (0 ms)
[0318/174701.064017:ERROR:base/test/launcher/test_launcher.cc:1313] no test result for PrivacyFilteringTest.UnsafeToplevelField
[0318/174701.064073:ERROR:base/test/launcher/test_launcher.cc:1313] no test result for PrivacyFilteringTest.SafeMessageWithOnlyUnsafeFields
[0318/174701.064081:ERROR:base/test/launcher/test_launcher.cc:1313] no test result for PrivacyFilteringTest.SafeAndUnsafeFields
[0318/174701.064088:ERROR:base/test/launcher/test_launcher.cc:1313] no test result for PrivacyFilteringTest.SafeAndUnsafePackets
[0318/174701.064109:ERROR:base/test/launcher/test_launcher.cc:1313] no test result for PrivacyFilteringTest.NestedSafeAndUnsafeFields
[0318/174701.064118:ERROR:base/test/launcher/test_launcher.cc:1313] no test result for AdaptPerfettoConfigForChromeTest.PrivacyFiltering
[0318/174701.064130:ERROR:base/test/launcher/test_launcher.cc:1313] no test result for CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_0_BlockSource_0
[0318/174701.064137:ERROR:base/test/launcher/test_launcher.cc:1313] no test result for CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_0_BlockSource_1
[0318/174701.064144:ERROR:base/test/launcher/test_launcher.cc:1313] no test result for CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_1_BlockSource_0
[ RUN ] PrivacyFilteringTest.RepeatedFields
[6/25] PrivacyFilteringTest.RepeatedFields (CRASHED)
[7/25] PrivacyFilteringTest.UnsafeToplevelField (SKIPPED)
[8/25] PrivacyFilteringTest.SafeMessageWithOnlyUnsafeFields (SKIPPED)
[9/25] PrivacyFilteringTest.SafeAndUnsafeFields (SKIPPED)
[10/25] PrivacyFilteringTest.SafeAndUnsafePackets (SKIPPED)
[11/25] PrivacyFilteringTest.NestedSafeAndUnsafeFields (SKIPPED)
[12/25] AdaptPerfettoConfigForChromeTest.PrivacyFiltering (SKIPPED)
[13/25] CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_0_BlockSource_0 (SKIPPED)
[14/25] CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_0_BlockSource_1 (SKIPPED)
[15/25] CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_1_BlockSource_0 (SKIPPED)
[0318/174701.064789:ERROR:base/test/launcher/test_launcher.cc:1313] no test result for PrivacyFilteringTest.SafeToplevelMessageField
[16/25] NetworkContextTest.PrivacyModeDisabledByDefault (4 ms)
[17/25] NetworkContextTest.PrivacyModeEnabledIfCookiesBlocked (3 ms)
[18/25] NetworkContextTest.PrivacyModeDisabledIfCookiesAllowed (3 ms)
[19/25] NetworkContextTest.PrivacyModeDisabledIfCookiesSettingForOtherURL (3 ms)
[20/25] NetworkContextTest.PrivacyModeEnabledIfThirdPartyCookiesBlocked (3 ms)
[21/25] TracingConsumerTest.PrivacyFilterConfig (2 ms)
[22/25] TracingConsumerTest.NoPrivacyFilterWithJsonConversion (1 ms)
[23/25] PrivacyFilteringTest.EmptyTrace (0 ms)
[ RUN ] PrivacyFilteringTest.SafeToplevelField
[24/25] PrivacyFilteringTest.SafeToplevelField (CRASHED)
[25/25] PrivacyFilteringTest.SafeToplevelMessageField (SKIPPED)
Retrying 12 tests (retry #0)
[0318/174701.066809:INFO:base/test/launcher/test_launcher.cc:1209] Starting [PrivacyFilteringTest.SafeToplevelField]
[ RUN ] PrivacyFilteringTest.SafeToplevelField
[26/37] PrivacyFilteringTest.SafeToplevelField (CRASHED)
[21/25] TracingConsumerTest.PrivacyFilterConfig (2 ms)
[22/25] TracingConsumerTest.NoPrivacyFilterWithJsonConversion (1 ms)
[23/25] PrivacyFilteringTest.EmptyTrace (0 ms)
[ RUN ] PrivacyFilteringTest.SafeToplevelField
[24/25] PrivacyFilteringTest.SafeToplevelField (CRASHED)
[25/25] PrivacyFilteringTest.SafeToplevelMessageField (SKIPPED)
Retrying 12 tests (retry #0)
[0318/174701.066809:INFO:base/test/launcher/test_launcher.cc:1209] Starting [PrivacyFilteringTest
[ RUN ] PrivacyFilteringTest.SafeToplevelField
[26/37] PrivacyFilteringTest.SafeToplevelField (CRASHED)
[0318/174702.899558:INFO:base/test/launcher/test_launcher.cc:1209] Starting [PrivacyFilteringTest
[ RUN ] PrivacyFilteringTest.SafeToplevelMessageField
[0318/174704.470050:INFO:base/test/launcher/test_launcher.cc:1209] Starting [PrivacyFilteringTest
[27/37] PrivacyFilteringTest.SafeToplevelMessageField (CRASHED)
[31/37] PrivacyFilteringTest.SafeAndUnsafeFields (CRASHED)
[0318/174712.325657:INFO:base/test/launcher/test_launcher.cc:1209] Starting [PrivacyFilteringTest
.SafeAndUnsafePackets]
[ RUN ] PrivacyFilteringTest.SafeAndUnsafePackets
[32/37] PrivacyFilteringTest.SafeAndUnsafePackets (CRASHED)
[0318/174713.896112:INFO:base/test/launcher/test_launcher.cc:1209] Starting [PrivacyFilteringTest
.NestedSafeAndUnsafeFields]
[ RUN ] PrivacyFilteringTest.NestedSafeAndUnsafeFields
[0318/174715.466556:INFO:base/test/launcher/test_launcher.cc:1209] Starting [AdaptPerfettoConfigF
orChromeTest.PrivacyFiltering]
[33/37] PrivacyFilteringTest.NestedSafeAndUnsafeFields (CRASHED)
[34/37] AdaptPerfettoConfigForChromeTest.PrivacyFiltering (6 ms)
[0318/174715.527858:INFO:base/test/launcher/test_launcher.cc:1209] Starting [CookieSettingsTestP.
IsPrivacyModeEnabled/GrantSource_0_BlockSource_0]
[35/37] CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_0_BlockSource_0 (0 ms)
[0318/174715.580932:INFO:base/test/launcher/test_launcher.cc:1209] Starting [CookieSettingsTestP.
IsPrivacyModeEnabled/GrantSource_0_BlockSource_1]
[0318/174715.633840:INFO:base/test/launcher/test_launcher.cc:1209] Starting [CookieSettingsTestP.
IsPrivacyModeEnabled/GrantSource_1_BlockSource_0]
[36/37] CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_0_BlockSource_1 (0 ms)
[37/37] CookieSettingsTestP.IsPrivacyModeEnabled/GrantSource_1_BlockSource_0 (0 ms)
8 tests crashed:
PrivacyFilteringTest.NestedSafeAndUnsafeFields (../../services/tracing/perfetto/privacy_filte
ring_check_unittest.cc:123)
PrivacyFilteringTest.RepeatedFields (../../services/tracing/perfetto/privacy_filtering_check_
unittest.cc:57)
PrivacyFilteringTest.SafeAndUnsafeFields (../../services/tracing/perfetto/privacy_filtering_c
heck_unittest.cc:97)
PrivacyFilteringTest.SafeAndUnsafePackets (../../services/tracing/perfetto/privacy_filtering_
check_unittest.cc:110)
PrivacyFilteringTest.SafeMessageWithOnlyUnsafeFields (../../services/tracing/perfetto/privacy
_filtering_check_unittest.cc:83)
PrivacyFilteringTest.SafeToplevelField (../../services/tracing/perfetto/privacy_filtering_che
ck_unittest.cc:39)
PrivacyFilteringTest.SafeToplevelMessageField (../../services/tracing/perfetto/privacy_filter
ing_check_unittest.cc:48)
PrivacyFilteringTest.UnsafeToplevelField (../../services/tracing/perfetto/privacy_filtering_c
heck_unittest.cc:75)
Tests took 16 seconds.
(II) Server terminated successfully (0). Closing log file.
a
Related details
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 4 comments![]( "Open in Gerrit")
output.append(preamble_length, 0);You've deleted this line in the substitution, but it was supposed to remain the same.
`output.append(preamble_length, 0);`
Replied elsewhere, there was a line you deleted that is likely causing it to immediately OOBs. Also you should have stack traces in the logs if you build with debug.
auto field_id_buf_first = field_id_buf.first(
static_cast<size_t>(field_id_end - field_id_buf.data()));rather than `first` and assigning to a new variable can you use `field_id_buf.take_first()`?
message_span.take_first(field_id_buf_first.size())
.copy_from(field_id_buf_first);
message_span.take_first(payload_size_buf_first.size())
.copy_from(payload_size_buf_first);better
```
auto [field_id_dest, payload_size_dest] = message_span.first_first(preamble_size).split_at(field_id_buf.size());
field_id_dest.copy_from(field_id_buf);
payload_size_dest.copy_from(payload_size_buf);
```
Related details
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
auto field_id_buf_first = field_id_buf.first(
static_cast<size_t>(field_id_end - field_id_buf.data()));rather than `first` and assigning to a new variable can you use `field_id_buf.take_first()`?
Ah sorry this won't work because it removes the first part. We'd need a `take_last` to modify it in place.
You would need to do the following I think
```
base::span<uint8_t> field_id_buf, payload_size_buf;
std::tie(field_id_buf, payload_size_buf) = base::span(storage).split_at<protozero::proto_utils::kMaxTagEncodedSize>
```
Then you could do
```
field_id_buf = field_id_buf.first(field_id_end - field_id_buf.data());
```
But I think it is fine to leave it as a new variable if you name it a bit better. I.E.
```
auto [field_id_max_storage, payload_size_max_storage] = ...;
auto field_id_buf = field_id_max_storage.first(...);
```
Any of these I think is fine.
Or maybe actually I like this the best:
```
auto storage_span = base::span(storage);
uint8_t* const field_id_end =
protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());
auto field_id_buf = storage_span.take_first(field_id_end - storage_span.size());
uint8_t* const payload_size_end = protozero::proto_utils::WriteVarInt(
payload_size, payload_size_buf.data());
auto payload_size_buf = storage_span.take_first(payload_size_end - storage_span.size());
```
Then we don't need the split, but this is fine since we are just writing into it and then taking the data as long as the backing storage is large enough this will work.
Daniel Angulo (Gerrit)
Daniel Angulo added 4 comments![]( "Open in Gerrit")
output.append(preamble_length, 0);You've deleted this line in the substitution, but it was supposed to remain the same.
`output.append(preamble_length, 0);`
Done
uint8_t* field_id_end =
protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());
const size_t field_id_length = field_id_end - field_id_buf.data();
uint8_t* payload_size_end = protozero::proto_utils::WriteVarInt(
payload_size, payload_size_buf.data());
const size_t payload_size_length = payload_size_end - payload_size_buf.data();
const size_t preamble_length = field_id_length + payload_size_length;
output.append(preamble_length, 0);
auto message_span = base::as_writable_byte_span(output);
message_span.take_first(out_msg_start_offset);if (payload_size != 0) {
// Move the payload to make room for the preamble at the beginning.
// message_span is [original_payload][empty_space]
// destination is [empty_space][final_payload_position]
message_span.subspan(preamble_length).copy_from(
message_span.first(payload_size));
}base::span field_id_span = message_span.take_first(field_id_length);
base::span payload_size_span = message_span.take_first(payload_size_length);field_id_span.copy_from(base::span(field_id_buf).first(field_id_length));
payload_size_span.copy_from(base::span(payload_size_buf).first(payload_size_length));return has_blocked_fields;
}
```
(I only verified it compile and pass the PrivacyFilteringTest, no general guarantee)
WDYT? Maybe we can try to use `take_first` more to me the code more idiomatic?
I like it in general, naming the parts rather than a bunch of offsets seems pretty nice, but I do have some comments especially once I see it fully diffed.
For example could we merge the two arrays into one and create a split span for the two locations, then we could .first from them to get the spans of the real data, making it a bit clearer.
```
std::array<uint8_t, protozero::proto_utils::kMaxTagEncodedSize + protozero::proto_utils::kMessageLengthFieldSize> storage =
{};
auto [field_id_buf, payload_size_buf] = base::span(storage).split_at<protozero::proto_utils::kMaxTagEncodedSize>();
uint8_t* const field_id_end =
protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());
field_id_buf = field_id_buf.first(field_id_end - field_id_buf.data());
uint8_t* const payload_size_end = protozero::proto_utils::WriteVarInt(
payload_size, payload_size_buf.data());
payload_size_buf = payload_size_buf.first(payload_size_end - payload_size_buf.data());
const size_t preamble_size = payload_size_buf.size() + field_id_buf.size();
....
message_span.take_first(field_id_buf.size()).copy_from(field_id_buf);
message_span.take_first(payload_size_buf.size()).copy_from(payload_size_buf);
```Also
```
auto message_span = base::as_writable_byte_span(output);
message_span.take_first(out_msg_start_offset);
```is just
```
auto message_span = base::as_writable_byte_span(output).subspan(out_msg_start_offset);
```So please take a look for other optimizations and readability if you go this route.
Stephen Nuskofor the first snippet, where could it be placed? I don't see obvious substituition
The first snippet from arthur or from me? for Arthur his snippet is just the whole `FilterProtoRecursively` for me my snippet replaced
```
// The format is <field id><payload size><message data>.
// This function wrote the payload of the current message starting from the
// end of output. We need to insert the preamble (<field id><payload size>),
// after moving the payload by the size of the preamble.
const uint32_t field_id =
protozero::proto_utils::MakeTagLengthDelimited(parent_ids->back());
std::array<uint8_t, protozero::proto_utils::kMaxTagEncodedSize> field_id_buf =
{};
std::array<uint8_t, protozero::proto_utils::kMessageLengthFieldSize>
payload_size_buf = {};
uint8_t* field_id_end =
protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());
const size_t field_id_length = field_id_end - field_id_buf.data();
uint8_t* payload_size_end = protozero::proto_utils::WriteVarInt(
payload_size, payload_size_buf.data());
from Arthur's.
Done
auto field_id_buf_first = field_id_buf.first(
static_cast<size_t>(field_id_end - field_id_buf.data()));Stephen Nuskorather than `first` and assigning to a new variable can you use `field_id_buf.take_first()`?
Ah sorry this won't work because it removes the first part. We'd need a `take_last` to modify it in place.
You would need to do the following I think
```
base::span<uint8_t> field_id_buf, payload_size_buf;
std::tie(field_id_buf, payload_size_buf) = base::span(storage).split_at<protozero::proto_utils::kMaxTagEncodedSize>
```Then you could do
```
field_id_buf = field_id_buf.first(field_id_end - field_id_buf.data());
```But I think it is fine to leave it as a new variable if you name it a bit better. I.E.
```
auto [field_id_max_storage, payload_size_max_storage] = ...;
auto field_id_buf = field_id_max_storage.first(...);
```Any of these I think is fine.
Or maybe actually I like this the best:
```
auto storage_span = base::span(storage);
uint8_t* const field_id_end =
protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());
auto field_id_buf = storage_span.take_first(field_id_end - storage_span.size());
uint8_t* const payload_size_end = protozero::proto_utils::WriteVarInt(
payload_size, payload_size_buf.data());
auto payload_size_buf = storage_span.take_first(payload_size_end - storage_span.size());
```Then we don't need the split, but this is fine since we are just writing into it and then taking the data as long as the backing storage is large enough this will work.
Done
message_span.take_first(field_id_buf_first.size())
.copy_from(field_id_buf_first);
message_span.take_first(payload_size_buf_first.size())
.copy_from(payload_size_buf_first);better
```
auto [field_id_dest, payload_size_dest] = message_span.first_first(preamble_size).split_at(field_id_buf.size());
field_id_dest.copy_from(field_id_buf);
payload_size_dest.copy_from(payload_size_buf);
```
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 4 comments![]( "Open in Gerrit")
/// START SUBSTITUTION
/*
std::array<uint8_t, protozero::proto_utils::kMaxTagEncodedSize> field_id_buf =
{};
std::array<uint8_t, protozero::proto_utils::kMessageLengthFieldSize>
payload_size_buf = {};
uint8_t* field_id_end =
protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());
const size_t field_id_length = field_id_end - field_id_buf.data();
uint8_t* payload_size_end = protozero::proto_utils::WriteVarInt(
payload_size, payload_size_buf.data());
const size_t payload_size_length = payload_size_end - payload_size_buf.data();
const size_t preamble_length = field_id_length + payload_size_length;
output.append(preamble_length, 0);
*/
// END SUBSTITUTION
// uint8_t* field_id_end =
// protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());Delete this once done.
auto field_id_buf_first = field_id_buf.first(
static_cast<size_t>(field_id_end - field_id_buf.data()));Stephen Nuskorather than `first` and assigning to a new variable can you use `field_id_buf.take_first()`?
Daniel AnguloAh sorry this won't work because it removes the first part. We'd need a `take_last` to modify it in place.
You would need to do the following I think
```
base::span<uint8_t> field_id_buf, payload_size_buf;
std::tie(field_id_buf, payload_size_buf) = base::span(storage).split_at<protozero::proto_utils::kMaxTagEncodedSize>
```Then you could do
```
field_id_buf = field_id_buf.first(field_id_end - field_id_buf.data());
```But I think it is fine to leave it as a new variable if you name it a bit better. I.E.
```
auto [field_id_max_storage, payload_size_max_storage] = ...;
auto field_id_buf = field_id_max_storage.first(...);
```Any of these I think is fine.
Or maybe actually I like this the best:
```
auto storage_span = base::span(storage);
uint8_t* const field_id_end =
protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());
auto field_id_buf = storage_span.take_first(field_id_end - storage_span.size());
uint8_t* const payload_size_end = protozero::proto_utils::WriteVarInt(
payload_size, payload_size_buf.data());
auto payload_size_buf = storage_span.take_first(payload_size_end - storage_span.size());
```Then we don't need the split, but this is fine since we are just writing into it and then taking the data as long as the backing storage is large enough this will work.
Done
In this sort of scenario where multiple options were mentioned it is helpful to say which you did.
Because to me it looks like you did none of them? Perhaps it didn't upload given the other one also doesn't look done?
// START SUBSTITUTION
/*
field_id_span.copy_from(base::span(field_id_buf).first(field_id_length));
payload_size_span.copy_from(
base::span(payload_size_buf).first(payload_size_length));
*/
// END SUBSTITUTIONRemove this.
message_span.take_first(field_id_buf_first.size())
.copy_from(field_id_buf_first);
message_span.take_first(payload_size_buf_first.size())
.copy_from(payload_size_buf_first);Daniel Angulobetter
```
auto [field_id_dest, payload_size_dest] = message_span.first_first(preamble_size).split_at(field_id_buf.size());
field_id_dest.copy_from(field_id_buf);
payload_size_dest.copy_from(payload_size_buf);
```
Done
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo added 4 comments![]( "Open in Gerrit")
/// START SUBSTITUTION
/*
std::array<uint8_t, protozero::proto_utils::kMaxTagEncodedSize> field_id_buf =
{};
std::array<uint8_t, protozero::proto_utils::kMessageLengthFieldSize>
payload_size_buf = {};
uint8_t* field_id_end =
protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());
const size_t field_id_length = field_id_end - field_id_buf.data();
uint8_t* payload_size_end = protozero::proto_utils::WriteVarInt(
payload_size, payload_size_buf.data());
const size_t payload_size_length = payload_size_end - payload_size_buf.data();
const size_t preamble_length = field_id_length + payload_size_length;
output.append(preamble_length, 0);
*/
// END SUBSTITUTION
// uint8_t* field_id_end =
// protozero::proto_utils::WriteVarInt(field_id, field_id_buf.data());Daniel AnguloDelete this once done.
Done
auto field_id_buf_first = field_id_buf.first(
static_cast<size_t>(field_id_end - field_id_buf.data()));sorry, my changes didn't upload last time, I end up renaming the variables. FYI: I also tried to do the third option, but field_id_buf was used before declared, not 100% sure what was the intent, maybe use storage_span instead?
// START SUBSTITUTION
/*
field_id_span.copy_from(base::span(field_id_buf).first(field_id_length));
payload_size_span.copy_from(
base::span(payload_size_buf).first(payload_size_length));
*/
// END SUBSTITUTIONDaniel AnguloRemove this.
Done
message_span.take_first(field_id_buf_first.size())
.copy_from(field_id_buf_first);
message_span.take_first(payload_size_buf_first.size())
.copy_from(payload_size_buf_first);Daniel Angulobetter
```
auto [field_id_dest, payload_size_dest] = message_span.first_first(preamble_size).split_at(field_id_buf.size());
field_id_dest.copy_from(field_id_buf);
payload_size_dest.copy_from(payload_size_buf);
```
Stephen NuskoDone
This doesn't appear to be done. Did you miss a upload?
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
1 comment
I mean it is just the pointer to the first element (conceptually), so yes `storage.data()` or `storage_span.data()` would have worked.
Related details
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 2 comments![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
2 comments
std::array<uint8_t, protozero::proto_utils::kMaxTagEncodedSize +
protozero::proto_utils::kMessageLengthFieldSize>
storage = {};
auto [field_id_max_storage, payload_size_max_storage] =
base::span(storage)
.split_at<protozero::proto_utils::kMaxTagEncodedSize>();
uint8_t* const field_id_end = protozero::proto_utils::WriteVarInt(
field_id, field_id_max_storage.data());
auto field_id_buf = field_id_max_storage.first(
static_cast<size_t>(field_id_end - field_id_max_storage.data()));
uint8_t* const payload_size_end = protozero::proto_utils::WriteVarInt(
payload_size, payload_size_max_storage.data());
auto payload_size_buf = payload_size_max_storage.first(
static_cast<size_t>(payload_size_end - payload_size_max_storage.data()));
const size_t preamble_size = payload_size_buf.size() + field_id_buf.size();
output.append(preamble_size, 0);
auto message_span =
base::as_writable_byte_span(output).subspan(out_msg_start_offset);
if (payload_size != 0) {
// Move the payload to make room for the preamble at the beginning.
// message_span is [original_payload][empty_space]
// destination is [empty_space][final_payload_position]
message_span.subspan(preamble_size)
.copy_from(message_span.first(payload_size));
}
auto [field_id_dest, payload_size_dest] =
message_span.take_first(preamble_size).split_at(field_id_buf.size());
field_id_dest.copy_from(field_id_buf);
payload_size_dest.copy_from(payload_size_buf);
return has_blocked_fields;
}That's quite difficult to understand (previous code included).
I don't have a good suggestion, but maybe the owner will ;-)
Related details
- Daniel Angulo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Angulo
| Auto-Submit | +1 |
1 comment
That's quite difficult to understand (previous code included).
I don't have a good suggestion, but maybe the owner will ;-)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Angulo (Gerrit)
Daniel Angulo voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
spanification: automatically spanify .../tracing/perfetto/privacy_filtering_check.cc etc.
This is the result of running the automatic spanification on linux and
updating code to use and pass spans where size is known.
The original patch was fully automated using script:
//tools/clang/spanify/rewrite-multiple-platforms.sh -platforms=linux
Then refined with gemini-cli and lastly manually refined
gemini-run/batch-run-1761614304/group_162
- M services/tracing/perfetto/privacy_filtering_check.cc
Code-Review: +1 by Arthur Sonzogni, +1 by Stephen Nusko
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |