refactor: third_party/blink/renderer/core/html/parser/literal_buffer.h [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 4 comments![]( "Open in Gerrit")
size_t size_ = 0;Blink Style Guide: Prefer blink:: types over STL and base types. Consider using 'wtf_size_t' instead of 'size_t' for the member variable 'size_' to match the return type of 'size()' and reduce memory usage.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
for (char c : std::string_view(expected_characters)) {Blink Style Guide: Prefer blink:: types over STL and base types. Please use 'StringView' (WTF::StringView) instead of 'std::string_view'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
for (char c : std::string_view(expected_characters)) {Blink Style Guide: Prefer blink:: types over STL and base types. Please use 'StringView' (WTF::StringView) instead of 'std::string_view'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
int /*nbDefaulted*/,Blink Style Guide: Prefer blink:: types over STL and base types. Please use 'StringView' (WTF::StringView) instead of 'std::string_view'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
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. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
Preemptively: Please run relevant benchmarks before sending this for review. This is the fifth AI-generated spanification CL we've seen only this week (!) that is a probable performance regression, and it's starting to be disruptive for team load. Thank you :-)
Related details
- Sergio Solano
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1023ec81b10000
Related details
- Sergio Solano
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. |
Sergio Solano (Gerrit)
Sergio Solano added 4 comments![]( "Open in Gerrit")
Blink Style Guide: Prefer blink:: types over STL and base types. Consider using 'wtf_size_t' instead of 'size_t' for the member variable 'size_' to match the return type of 'size()' and reduce memory usage.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
for (char c : std::string_view(expected_characters)) {Blink Style Guide: Prefer blink:: types over STL and base types. Please use 'StringView' (WTF::StringView) instead of 'std::string_view'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
for (char c : std::string_view(expected_characters)) {Blink Style Guide: Prefer blink:: types over STL and base types. Please use 'StringView' (WTF::StringView) instead of 'std::string_view'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Blink Style Guide: Prefer blink:: types over STL and base types. Please use 'StringView' (WTF::StringView) instead of 'std::string_view'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
- 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 8 comments![]( "Open in Gerrit")
for (unsigned i = 0; i < expected_characters.length(); ++i) {
source.AdvanceAndASSERT(expected_characters[i]);
}Same here?
for (unsigned i = 0; i < expected_characters.length(); ++i) {
source.AdvanceAndASSERTIgnoringCase(expected_characters[i]);
}This could just be a for each loop right?
```
for (const auto& expected : expected_characters) {
source.AdvanceAndASSERTIgnoringCase(expected);
}
```
// It is legal to copy Member<> with memcpy() here, since we are in a
// constructor. (We could use an initializer, but Clang doesn't
// manage to combine the loads and stores into larger parts.)
UNSAFE_TODO(memcpy(&values_, &other.values_, sizeof(values_)));
UNSAFE_TODO(memcpy(&children_, &other.children_, sizeof(children_)));This comment is very clear that this is heavily optimized code.
blink code should be spanified with extreme care.
Can you do some due diligence here and
1) check that speedometer3/jetstream2 doesn't experience a regression
2) Look for any micro benchmarks around this css parser?
3) That if you look for the assembly code in particular here it generates similar optimized code similar or better than before?
If we can't do 3) we should perhaps leave this until an expert with the skill to do so has a chance to look.
// SAFETY: The loop iterates over a WTF::Vector within the bounds of the
// container, from a valid iterator to the end.a WTF::Vector is not hardened I don't think we should promote this to UNSAFE_BUFFERS leave as UNSAFE_TODO, we can't meaningfully verify the bounds here by any API promise.
search_start != begin_times_.end(); UNSAFE_BUFFERS(++search_start)) {Same here (SMILInstanceTimeList is just a blink::Vector)
char buffer[1024];
// SAFETY: vsnprintf ensures not to write more than 1024 bytes.
UNSAFE_BUFFERS(vsnprintf(buffer, sizeof(buffer), message, args));Can we use `base::SafeSPrintf`?
// `GetError` is safe as it immediately converts the arguments into a
// `std::string` using `base::StringPrintV`.
UNSAFE_BUFFERS(GetParser(closure)->GetError(XMLErrors::kErrorTypeWarning,
message, args));I don't understand what is the buffers usage here we are suppressing? This just looks like we are reinterpreting casting and calling a function on the result? Can you let me know the error? looks like we shouldn't need a UNSAFE_BUFFERS?
UNSAFE_BUFFERS(GetParser(closure)->GetError(XMLErrors::kErrorTypeNonFatal,Same here?
Related details
- Sergio Solano
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")
// It is legal to copy Member<> with memcpy() here, since we are in a
// constructor. (We could use an initializer, but Clang doesn't
// manage to combine the loads and stores into larger parts.)
UNSAFE_TODO(memcpy(&values_, &other.values_, sizeof(values_)));
UNSAFE_TODO(memcpy(&children_, &other.children_, sizeof(children_)));This comment is very clear that this is heavily optimized code.
blink code should be spanified with extreme care.
Can you do some due diligence here and
1) check that speedometer3/jetstream2 doesn't experience a regression
2) Look for any micro benchmarks around this css parser?
3) That if you look for the assembly code in particular here it generates similar optimized code similar or better than before?If we can't do 3) we should perhaps leave this until an expert with the skill to do so has a chance to look.
And I see you already did speedometer 😄
https://pinpoint-dot-chromeperf.appspot.com/job/1023ec81b10000
But can you run it on mac-m1_mini_2020-perf with 150 tries (it should default) as described here: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/benchmark_performance_regressions.md
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14ee0c64f10000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17ddebc4f10000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1757328ef10000
Related details
- Sergio Solano
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
- TodoMVC-JavaScript-ES6-Webpack-Complex-DOM: base median = 37.985000000474976 -> patched median = 38.195000000018624
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/131061e9f10000
Sergio Solano (Gerrit)
Sergio Solano added 8 comments![]( "Open in Gerrit")
for (unsigned i = 0; i < expected_characters.length(); ++i) {
source.AdvanceAndASSERT(expected_characters[i]);
}Sergio SolanoSame here?
Done
for (unsigned i = 0; i < expected_characters.length(); ++i) {
source.AdvanceAndASSERTIgnoringCase(expected_characters[i]);
}This could just be a for each loop right?
```
for (const auto& expected : expected_characters) {
source.AdvanceAndASSERTIgnoringCase(expected);
}
```
Done
// It is legal to copy Member<> with memcpy() here, since we are in a
// constructor. (We could use an initializer, but Clang doesn't
// manage to combine the loads and stores into larger parts.)
UNSAFE_TODO(memcpy(&values_, &other.values_, sizeof(values_)));
UNSAFE_TODO(memcpy(&children_, &other.children_, sizeof(children_)));Stephen NuskoThis comment is very clear that this is heavily optimized code.
blink code should be spanified with extreme care.
Can you do some due diligence here and
1) check that speedometer3/jetstream2 doesn't experience a regression
2) Look for any micro benchmarks around this css parser?
3) That if you look for the assembly code in particular here it generates similar optimized code similar or better than before?If we can't do 3) we should perhaps leave this until an expert with the skill to do so has a chance to look.
And I see you already did speedometer 😄
https://pinpoint-dot-chromeperf.appspot.com/job/1023ec81b10000
But can you run it on mac-m1_mini_2020-perf with 150 tries (it should default) as described here: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/benchmark_performance_regressions.md
I'm postponing this file to compare the original vs. proposed assembly.
// SAFETY: The loop iterates over a WTF::Vector within the bounds of the
// container, from a valid iterator to the end.a WTF::Vector is not hardened I don't think we should promote this to UNSAFE_BUFFERS leave as UNSAFE_TODO, we can't meaningfully verify the bounds here by any API promise.
Done
search_start != begin_times_.end(); UNSAFE_BUFFERS(++search_start)) {Same here (SMILInstanceTimeList is just a blink::Vector)
Done
char buffer[1024];
// SAFETY: vsnprintf ensures not to write more than 1024 bytes.
UNSAFE_BUFFERS(vsnprintf(buffer, sizeof(buffer), message, args));Can we use `base::SafeSPrintf`?
First i discarded `base::SafeSPrintf` beacause it is a C++ variadic function and it cannot handle va_list. (Is my understanding correct?)
Then i tried with `base::StringPrintV` which worked wrapped in UNSAFE_BUFFERS but raised presubmit error: `.../xml_document_parser.cc:1503 uses disallowed identifier base::StringPrintV`.
I found file `third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py`, StringPrintv and SafeSPrintf are not listed there, I assume both will fail with `disallowed identifier`.
I think this is the best option. Is the UNSAFE_BUFFERS justified here? I think so because GetError is only called from within InitializeParserContext.
// `GetError` is safe as it immediately converts the arguments into a
// `std::string` using `base::StringPrintV`.
UNSAFE_BUFFERS(GetParser(closure)->GetError(XMLErrors::kErrorTypeWarning,
message, args));I don't understand what is the buffers usage here we are suppressing? This just looks like we are reinterpreting casting and calling a function on the result? Can you let me know the error? looks like we shouldn't need a UNSAFE_BUFFERS?
Done
UNSAFE_BUFFERS(GetParser(closure)->GetError(XMLErrors::kErrorTypeNonFatal,Sergio SolanoSame here?
Done
Related details
- 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 5 comments![]( "Open in Gerrit")
base::span<const LChar> ptr =nit: ptr isn't a good name anymore. `chars`?
for (unsigned i = 1; i < end - start; ++i) {nit: `ptr.size()`
char buffer[1024];
// SAFETY: vsnprintf ensures not to write more than 1024 bytes.
UNSAFE_BUFFERS(vsnprintf(buffer, sizeof(buffer), message, args));Sergio SolanoCan we use `base::SafeSPrintf`?
First i discarded `base::SafeSPrintf` beacause it is a C++ variadic function and it cannot handle va_list. (Is my understanding correct?)
Then i tried with `base::StringPrintV` which worked wrapped in UNSAFE_BUFFERS but raised presubmit error: `.../xml_document_parser.cc:1503 uses disallowed identifier base::StringPrintV`.
I found file `third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py`, StringPrintv and SafeSPrintf are not listed there, I assume both will fail with `disallowed identifier`.
I think this is the best option. Is the UNSAFE_BUFFERS justified here? I think so because GetError is only called from within InitializeParserContext.
Can we use VSpanPrintf then at least: https://source.chromium.org/chromium/chromium/src/+/main:base/strings/span_printf.h;l=25;drc=542eedeaf4606aec2003451a8365ac5a0269e33e
Looks like it is already used in blink?: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/text/wtf_string.cc;l=249;drc=9234a13fa1fafcb70e754de57ebd40dcdd092a11
But also StringPrintV returns a string (which is an allocation, close but not quite what we want).
std::string formatted_message = buffer;This is a no-go, this is creating a copy of the buffer where no copy existed before. Why do you need this?
Likely what you wanted is `base::as_string_view(buffer)`?, but we don't really use it as a span so probably just `&buffer[0]` is fine?
// `GetError` is safe as it immediately converts the arguments into a
// `std::string` using `base::StringPrintV`.
UNSAFE_BUFFERS(GetParser(closure)->GetError(XMLErrors::kErrorTypeWarning,
message, args));Sergio SolanoI don't understand what is the buffers usage here we are suppressing? This just looks like we are reinterpreting casting and calling a function on the result? Can you let me know the error? looks like we shouldn't need a UNSAFE_BUFFERS?
Done
Sorry what is done? I still don't understand what this is wrapping since GetError is not UNSAFE_BUFFER_USAGE function, if it was then that makes sense but we should speak to how we ensure the invariants.
Related details
- Sergio Solano
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. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 5 comments![]( "Open in Gerrit")
Votes added by Sergio Solano
| Commit-Queue | +1 |
5 comments
nit: ptr isn't a good name anymore. `chars`?
Done
for (unsigned i = 1; i < end - start; ++i) {Sergio Solanonit: `ptr.size()`
Done
char buffer[1024];
// SAFETY: vsnprintf ensures not to write more than 1024 bytes.
UNSAFE_BUFFERS(vsnprintf(buffer, sizeof(buffer), message, args));Sergio SolanoCan we use `base::SafeSPrintf`?
Stephen NuskoFirst i discarded `base::SafeSPrintf` beacause it is a C++ variadic function and it cannot handle va_list. (Is my understanding correct?)
Then i tried with `base::StringPrintV` which worked wrapped in UNSAFE_BUFFERS but raised presubmit error: `.../xml_document_parser.cc:1503 uses disallowed identifier base::StringPrintV`.
I found file `third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py`, StringPrintv and SafeSPrintf are not listed there, I assume both will fail with `disallowed identifier`.
I think this is the best option. Is the UNSAFE_BUFFERS justified here? I think so because GetError is only called from within InitializeParserContext.
Can we use VSpanPrintf then at least: https://source.chromium.org/chromium/chromium/src/+/main:base/strings/span_printf.h;l=25;drc=542eedeaf4606aec2003451a8365ac5a0269e33e
Looks like it is already used in blink?: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/text/wtf_string.cc;l=249;drc=9234a13fa1fafcb70e754de57ebd40dcdd092a11
But also StringPrintV returns a string (which is an allocation, close but not quite what we want).
When uploading I get: `third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1285 uses disallowed identifier base::VSpanPrintf`
It only works if I add `"+base/strings/span_printf.h",` to file third_party/blink/renderer/core/xml/DEPS and `'base::VSpanPrintf',
` to file third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py. Do you have a different alternative? Seems hard to approve but it also seems safer.
std::string formatted_message = buffer;This is a no-go, this is creating a copy of the buffer where no copy existed before. Why do you need this?
Likely what you wanted is `base::as_string_view(buffer)`?, but we don't really use it as a span so probably just `&buffer[0]` is fine?
Done
// `GetError` is safe as it immediately converts the arguments into a
// `std::string` using `base::StringPrintV`.
UNSAFE_BUFFERS(GetParser(closure)->GetError(XMLErrors::kErrorTypeWarning,
message, args));Sergio SolanoI don't understand what is the buffers usage here we are suppressing? This just looks like we are reinterpreting casting and calling a function on the result? Can you let me know the error? looks like we shouldn't need a UNSAFE_BUFFERS?
Stephen NuskoDone
Sorry what is done? I still don't understand what this is wrapping since GetError is not UNSAFE_BUFFER_USAGE function, if it was then that makes sense but we should speak to how we ensure the invariants.
Sorry, this are the errors without UNSAFE_BUFEERS:
```stderr:
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1498:3: error: formatting function 'GetError' is unsafe [-Werror,-Wunsafe-buffer-usage-in-format-attr-call]
1498 | GetParser(closure)->GetError(XMLErrors::kErrorTypeWarning, message, args);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1498:3: note: See //docs/unsafe_buffers.md for help.
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1498:62: error: string argument is not guaranteed to be null-terminated
1498 | GetParser(closure)->GetError(XMLErrors::kErrorTypeWarning, message, args);
| ^~~~~~~
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1498:62: note: See //docs/unsafe_buffers.md for help.
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1506:3: error: formatting function 'GetError' is unsafe [-Werror,-Wunsafe-buffer-usage-in-format-attr-call]
1506 | GetParser(closure)->GetError(XMLErrors::kErrorTypeNonFatal, message, args);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1506:3: note: See //docs/unsafe_buffers.md for help.
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1506:63: error: string argument is not guaranteed to be null-terminated
1506 | GetParser(closure)->GetError(XMLErrors::kErrorTypeNonFatal, message, args);
| ^~~~~~~
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1506:63: note: See //docs/unsafe_buffers.md for help.
4 errors generated.
build failed```
Related details
- 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")
char buffer[1024];
// SAFETY: vsnprintf ensures not to write more than 1024 bytes.
UNSAFE_BUFFERS(vsnprintf(buffer, sizeof(buffer), message, args));Sergio SolanoCan we use `base::SafeSPrintf`?
Stephen NuskoFirst i discarded `base::SafeSPrintf` beacause it is a C++ variadic function and it cannot handle va_list. (Is my understanding correct?)
Then i tried with `base::StringPrintV` which worked wrapped in UNSAFE_BUFFERS but raised presubmit error: `.../xml_document_parser.cc:1503 uses disallowed identifier base::StringPrintV`.
I found file `third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py`, StringPrintv and SafeSPrintf are not listed there, I assume both will fail with `disallowed identifier`.
I think this is the best option. Is the UNSAFE_BUFFERS justified here? I think so because GetError is only called from within InitializeParserContext.
Sergio SolanoCan we use VSpanPrintf then at least: https://source.chromium.org/chromium/chromium/src/+/main:base/strings/span_printf.h;l=25;drc=542eedeaf4606aec2003451a8365ac5a0269e33e
Looks like it is already used in blink?: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/text/wtf_string.cc;l=249;drc=9234a13fa1fafcb70e754de57ebd40dcdd092a11
But also StringPrintV returns a string (which is an allocation, close but not quite what we want).
When uploading I get: `third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1285 uses disallowed identifier base::VSpanPrintf`
It only works if I add `"+base/strings/span_printf.h",` to file third_party/blink/renderer/core/xml/DEPS and `'base::VSpanPrintf',
` to file third_party/blink/tools/blinkpy/presubmit/audit_non_blink_usage.py. Do you have a different alternative? Seems hard to approve but it also seems safer.
I think the correct way would be in [third_party/blink/renderer/core/DEPS](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/DEPS;l=16;drc=d7e6a942b1eaa978ab30fc4b7cec7587862864b7) right beside `"+base/strings/stringprintf.h"` add a `+base/strings/span_printf.h`
And then probably add it to the allowed for every directory in core, but you are right we should probably do this as a follow up. Thanks for investigating, we can send a follow up and ask blink owners if they want to allow this span wrapped function and do a round of updates to show usage, but lets leave it as is.
Okay thank you for updating the safety comment it now makes sense, before it sounded like we were saying the conversion made it safe, but it does not because if it isn't null terminated the conversion with trigger an OOB.
Does libxml have an API doc we can link to? I couldn't find one so I guess not...
'base::VSpanPrintf',Don't need to add it twice.
(but as discussed lets remove it for now.
Related details
- Sergio Solano
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. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sergio Solano
| Commit-Queue | +1 |
1 comment
Don't need to add it twice.
(but as discussed lets remove it for now.
- 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 5 comments![]( "Open in Gerrit")
As a final check can you run some of the microbenchmarks
third_party/blink/renderer/core/css/style_perftest.cc
and
third_party/blink/renderer/core/css/parser/css_parser_fast_paths_perftest.cc
?
And see the results?
double result = chars[0] - '0';
for (unsigned i = 1; i < chars.size(); ++i) {
result = result * 10 + (chars[i] - '0');
}could this be a range based for?
```
double result = 0;
for (char c : chars) {
result = result * 10 + (c - '0');
}
```
On the first iteration `result == zero` so `result * 10` cancels out and is the same as the original `result = chars[0] - '-';`, and then for the rest it is exactly the same?
// SAFETY: message and args are provided by libxml and VSpanPrintf ensures not
// to write more than 1024 bytes.VSpanPrintf isn't marked as an unsafe function is it? Do we need the UNSAFE_BUFFERS? or is it because it is a format printing function?
Should we leave it as UNSAFE_TODO if there isn't one.
// to be null-terminated. However, the compiler triggers
// -Wunsafe-buffer-usage without the UNSAFE_BUFFERS macro.nit: don't need this part, we just need to explain why GetError is safe.
I.E. GetError is unsafe if |message| or any values in |args| are not null terminated. But `message` is provided by libxml...
Related details
- Sergio Solano
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. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 5 comments![]( "Open in Gerrit")
Votes added by Sergio Solano
| Commit-Queue | +1 |
As a final check can you run some of the microbenchmarks
third_party/blink/renderer/core/css/style_perftest.cc
and
third_party/blink/renderer/core/css/parser/css_parser_fast_paths_perftest.cc
?
And see the results?
StyleCalcPerfTest
```
RUN, avg_diff (ms), avg_baseline (ms), diff_percentage
.Alexa1000, -0.07, 106.83, -0.06%
.ECommerce, -0.17, 97.03, -0.17%
.Encyclopedia, -10.63, 728.37, -1.46%
.Extension, 0.20, 438.60, 0.05%
.News, -0.83, 172.20, -0.48%
.Search, 0.07, 111.47, 0.06%
.Social1, -0.23, 145.10, -0.16%
.Social2, 0.13, 63.67, 0.21%
.Sports, 1.93, 311.43, 0.62%
.Video, 2.20, 226.40, 0.97%
```
StyleFastPathPerfTest
```
RUN, avg_diff (ms), avg_baseline (ms), diff_percentage
.MotionMarkMultiply, -0.60, 360.40, -0.17%
```
Hi Stephen, I've added the results of the microbenchmarks. The results are the average of 30 runs.
double result = chars[0] - '0';
for (unsigned i = 1; i < chars.size(); ++i) {
result = result * 10 + (chars[i] - '0');
}could this be a range based for?
```
double result = 0;
for (char c : chars) {
result = result * 10 + (c - '0');
}
```On the first iteration `result == zero` so `result * 10` cancels out and is the same as the original `result = chars[0] - '-';`, and then for the rest it is exactly the same?
Done
// SAFETY: message and args are provided by libxml and VSpanPrintf ensures not
// to write more than 1024 bytes.VSpanPrintf isn't marked as an unsafe function is it? Do we need the UNSAFE_BUFFERS? or is it because it is a format printing function?
Kind of, to get it to work without presubmit warninig I edited file third_party/blink/renderer/core/xml/DEPS. That is a way of marking it unsafe, isn't it?
Without the UNSAFE_BUFFERS I get ```stderr:
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1284:3: error: formatting function 'VSpanPrintf' is unsafe [-Werror,-Wunsafe-buffer-usage-in-format-attr-call]
1284 | base::VSpanPrintf(base::span(formatted_message), message, args);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1284:3: note: See //docs/unsafe_buffers.md for help.
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1284:52: error: string argument is not guaranteed to be null-terminated
1284 | base::VSpanPrintf(base::span(formatted_message), message, args);
| ^~~~~~~
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1284:52: note: See //docs/unsafe_buffers.md for help.
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1499:3: error: formatting function 'GetError' is unsafe [-Werror,-Wunsafe-buffer-usage-in-format-attr-call]
1499 | GetParser(closure)->GetError(XMLErrors::kErrorTypeWarning,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1500 | message, args);
| ~~~~~~~~~~~~~~
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1499:3: note: See //docs/unsafe_buffers.md for help.
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1500:32: error: string argument is not guaranteed to be null-terminated
1500 | message, args);
| ^~~~~~~
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1500:32: note: See //docs/unsafe_buffers.md for help.
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1510:3: error: formatting function 'GetError' is unsafe [-Werror,-Wunsafe-buffer-usage-in-format-attr-call]
1510 | GetParser(closure)->GetError(XMLErrors::kErrorTypeNonFatal, message, args);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1510:3: note: See //docs/unsafe_buffers.md for help.
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1510:63: error: string argument is not guaranteed to be null-terminated
1510 | GetParser(closure)->GetError(XMLErrors::kErrorTypeNonFatal, message, args);
| ^~~~~~~
../../third_party/blink/renderer/core/xml/parser/xml_document_parser.cc:1510:63: note: See //docs/unsafe_buffers.md for help.
6 errors generated.
build failed```
About docs that backup that UNSAFE_BUFFERS is needed i found: https://clang.llvm.org/docs/DiagnosticsReference.html. Gemini tells me it's related to VSpanPrintf being print and va_list related, but I could not found manuals/docs.
// to be null-terminated. However, the compiler triggers
// -Wunsafe-buffer-usage without the UNSAFE_BUFFERS macro.nit: don't need this part, we just need to explain why GetError is safe.
I.E. GetError is unsafe if |message| or any values in |args| are not null terminated. But `message` is provided by libxml...
- 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")
Sergio SolanoAs a final check can you run some of the microbenchmarks
third_party/blink/renderer/core/css/style_perftest.cc
and
third_party/blink/renderer/core/css/parser/css_parser_fast_paths_perftest.cc
?
And see the results?
StyleCalcPerfTest
```
RUN, avg_diff (ms), avg_baseline (ms), diff_percentage
.Alexa1000, -0.07, 106.83, -0.06%
.ECommerce, -0.17, 97.03, -0.17%
.Encyclopedia, -10.63, 728.37, -1.46%
.Extension, 0.20, 438.60, 0.05%
.News, -0.83, 172.20, -0.48%
.Search, 0.07, 111.47, 0.06%
.Social1, -0.23, 145.10, -0.16%
.Social2, 0.13, 63.67, 0.21%
.Sports, 1.93, 311.43, 0.62%
.Video, 2.20, 226.40, 0.97%
```StyleFastPathPerfTest
```
RUN, avg_diff (ms), avg_baseline (ms), diff_percentage
.MotionMarkMultiply, -0.60, 360.40, -0.17%
```
Just to make sure
baseline is HEAD and diff_percentage is `(current_cl_results - HEAD_results)/HEAD_results` I.E. the relative difference?
Can you give me a feel for what the std is? I'm trying to understand if the .97% and .62% regressions in `Sports` and `Video` are "real" or if everything is basically zero?
They look like they are about 2ms slower which might be within the noise for something that takes 226ms in the baseline, but it might also be detectable if it is a very stable test.
while ((offset_ + offset) < string_length_ &&I also wonder if you rewrite these to be range based for if the optimizer would do a better job here (I.E.
```
for (const LChar& lchar : string_.Span8().subspan(offset_)) {
if (!characterPredicate(lchar)) {
break;
}
++offset;
}
```
base::span<const LChar> characters8 = string_.Span8();Should we `subspan(offset_)` to avoid having to do the math repeatedly?
```suggestion
base::span<const LChar> characters8 = string_.Span8().subspan(offset_);
```
Then remove offset_ from all the math below.
base::span<const LChar> characters = string_.Span8();Similar questions about these while loops.
Related details
- Sergio Solano
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. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
compare_blink_perf.cc tells you how to run and make a report out of these (with confidence intervals).
Related details
- Sergio Solano
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. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 5 comments![]( "Open in Gerrit")
Votes added by Sergio Solano
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Hi Stephen, I've calculated the std across 30 runs to check for regressions.
For Video, the difference is +0.43ms but the std is 8.05ms.
For Sports, the difference is -4.3ms with a std of 9.77ms.
Only for MotionMarkMultiply the difference (+3.97ms) approaches the standard deviation (4.68ms), but it is still less than 1 std.
In all cases, the difference between MAIN and CL is less than one standard deviation. This indicates the results are within the noise floor of the test environment and do not represent a real regression.
```
mean std
branch cl main cl main
test
StyleCalcPerfTest.Alexa1000 110.200000 109.566667 2.496895 3.339248
StyleCalcPerfTest.ECommerce 99.433333 99.133333 2.555364 2.473770
StyleCalcPerfTest.Encyclopedia 723.600000 733.333333 11.906533 11.043280
StyleCalcPerfTest.Extension 448.200000 450.933333 9.939125 13.970248
StyleCalcPerfTest.News 174.900000 178.133333 3.862731 5.829375
StyleCalcPerfTest.Search 117.233333 119.433333 6.404111 6.709146
StyleCalcPerfTest.Social1 148.800000 150.433333 2.187740 3.616851
StyleCalcPerfTest.Social2 64.900000 65.733333 1.422722 2.242741
StyleCalcPerfTest.Sports 319.700000 324.000000 9.979807 9.773292
StyleCalcPerfTest.Video 235.300000 234.866667 8.971795 8.046131
StyleFastPathPerfTest.MotionMarkMultiply 363.400000 359.433333 3.558380 4.680726```
Hi, Stephen. I've added the std and applied your suggestions.
I also wonder if you rewrite these to be range based for if the optimizer would do a better job here (I.E.
```
for (const LChar& lchar : string_.Span8().subspan(offset_)) {
if (!characterPredicate(lchar)) {
break;
}
++offset;
}
```
Done
base::span<const LChar> characters8 = string_.Span8();Should we `subspan(offset_)` to avoid having to do the math repeatedly?
```suggestion
base::span<const LChar> characters8 = string_.Span8().subspan(offset_);
```Then remove offset_ from all the math below.
Done
Similar questions about these while loops.
- Steinar H Gunderson
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
Please again see and use compare_blink_perf.cc, not a home-grown z-test.
Related details
- Sergio Solano
- 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. |