Fix base, ipc, mojo build with ENABLE_BASE_TRACING disabled [chromium/src : main]
Grace Cham (Gerrit)
Attention is currently required from: Hidehiko Abe, Eric Seckler, Ken Rockot, Alex Moshchuk.
Grace Cham would like Hidehiko Abe, Eric Seckler, Ken Rockot and Alex Moshchuk to review this change.
Fix base, ipc, mojo build with ENABLE_BASE_TRACING disabled
trace_event is not supported in libchrome, and local patches are
currently applied to replace trace_event headers by stub or commenting
out relevant lines manually, although there is a build flag
enable_base_tracing which could be used for disabling it.
Fix the build in libchrome components on chromium upstream when the flag
is set to false, so that libchrome does not need to maintain the
divergence.
Most of the compile errors are caused by redefinition of classes or
functions when both the stub and real headers are included, and can be
fixed by including the proxy header base/trace_event/base_tracing.h
instead.
Bug: b:204714883
Test: autoninja -C out/Default chrome (ENABLE_BASE_TRACING=true)
Test: autoninja -C out/Default base ipc mojo/{core/embedder,public} \
(ENABLE_BASE_TRACING=false)
Change-Id: I67803deb50350f449632416d27c65cb7d2b07f41
---
M mojo/core/message_pipe_dispatcher.cc
M mojo/public/tools/bindings/generators/cpp_templates/wrapper_class_definition.tmpl
M mojo/core/handle_table.cc
M ipc/ipc_sync_message_filter.cc
M base/trace_event/trace_event_stub.h
M mojo/public/cpp/bindings/generic_pending_receiver.h
M mojo/core/channel.cc
M mojo/public/tools/bindings/generators/cpp_templates/module-shared.cc.tmpl
M mojo/public/cpp/bindings/message.h
M mojo/core/user_message_impl.cc
M mojo/public/cpp/bindings/struct_ptr.h
M mojo/public/cpp/system/simple_watcher.cc
M ipc/ipc_message.cc
M ipc/ipc_sync_channel.cc
M mojo/core/data_pipe_consumer_dispatcher.cc
M mojo/public/tools/bindings/generators/cpp_templates/module-shared.h.tmpl
M ipc/BUILD.gn
M content/browser/renderer_host/render_process_host_impl.cc
M mojo/core/handle_table.h
M mojo/public/cpp/bindings/generic_pending_receiver.cc
M mojo/public/cpp/bindings/lib/connector.cc
M ipc/ipc_message_templates.h
M ipc/ipc_channel_mojo.cc
M mojo/public/cpp/bindings/tracing_helpers.h
M mojo/public/cpp/bindings/lib/message.cc
M components/tracing/common/trace_startup_config.cc
M ipc/trace_ipc_message.h
M ipc/ipc_mojo_bootstrap.cc
M mojo/core/core.cc
M mojo/core/data_pipe_producer_dispatcher.cc
M mojo/public/cpp/bindings/lib/send_message_helper.cc
M mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl
M mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl
M base/trace_event/base_tracing.h
M ipc/ipc_message_pipe_reader.cc
35 files changed, 176 insertions(+), 55 deletions(-)
To view, visit change 3251806. To unsubscribe, or for help writing mail filters, visit settings.
Grace Cham (Gerrit)
Attention is currently required from: Hidehiko Abe, Eric Seckler, Ken Rockot, Alex Moshchuk.
Grace Cham (Gerrit)
Attention is currently required from: Hidehiko Abe, Eric Seckler, Ken Rockot, Hans Wennborg, Alex Moshchuk.
Grace Cham would like Hans Wennborg to review this change.
To view, visit change 3251806. To unsubscribe, or for help writing mail filters, visit settings.
Grace Cham (Gerrit)
Attention is currently required from: Hidehiko Abe, Eric Seckler, Ken Rockot, Hans Wennborg, Alex Moshchuk.
1 comment:
Patchset:
Hi Hans, this change will cause the header size increases to over the limit in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/events/input_event.cc;l=10;bpv=1;bpt=0
May I know if this within the acceptable range to raise the limit? If yes, what would be an appropriate new limit (say 1300000)? Thanks!
To view, visit change 3251806. To unsubscribe, or for help writing mail filters, visit settings.
Hans Wennborg (Gerrit)
Attention is currently required from: Hidehiko Abe, Eric Seckler, Grace Cham, Ken Rockot, Alex Moshchuk.
5 comments:
Patchset:
Hi Hans, this change will cause the header size increases to over the limit in https://source. […]
Sure, if the growth is necessary, just bump the limit.
But I also wonder if it would be possible to make this change without adding the new includes?
File base/trace_event/base_tracing.h:
Patch Set #7, Line 26:
#include "base/trace_event/trace_id_helper.h" // nogncheckThis declares GetNextGlobalTraceId(), but I don't see it being used in base_tracing.h, so why is this include needed?
Actually I don't really understand how this header works at all. Why does it have to pull in so many headers when it doesn't implement anything?
Patch Set #7, Line 30:
#include "third_party/perfetto/include/perfetto/tracing/traced_value_forward.h" // nogncheckIt seems unusual to include both the real header (above) and the _forward.h header. Is there a reason for this?
Patch Set #7, Line 31:
#include "third_party/perfetto/protos/perfetto/trace/track_event/chrome_mojo_event_info.pbzero.h" // nogncheckWhat's this needed for? Could you use forward declarations instead?
File base/trace_event/trace_event_stub.h:
Patch Set #7, Line 15:
#include "base/task/single_thread_task_runner.h"Could you use a forward declaration instead?
To view, visit change 3251806. To unsubscribe, or for help writing mail filters, visit settings.
Eric Seckler (Gerrit)
Attention is currently required from: Hidehiko Abe, Grace Cham, Ken Rockot, Alex Moshchuk.
Grace Cham has uploaded this change for review.
To view, visit change 3251806. To unsubscribe, or for help writing mail filters, visit settings.
Eric Seckler (Gerrit)
Attention is currently required from: Hidehiko Abe, Grace Cham, Ken Rockot, Alex Moshchuk.
3 comments:
Patchset:
Hm... enable_base_tracing / the base_tracing.h headers aren't really intended to be used anywhere outside //base.
Fix the build in libchrome components on chromium upstream when the flag
is set to false, so that libchrome does not need to maintain the
divergence.
I'm not sure if imposing this maintenance on chromium is feasible. There's already concern that supporting enable_base_tracing within //base only doesn't align with //base/README ("base is written for the Chromium project and is not intended to be used outside it").
See also crbug/1183347 with some related discussion.
There's currently no automated testing of enable_base_tracing=false due to it not being officially supported by //base. If we don't add such testing, this config is bound to be broken frequently, especially if we now extend it beyond //base. Not sure if that's much better than your current local patches ;)
Which components is libchrome using, apart from //base? Would this be limited to //ipc and //mojo?
+thakis and +gab for opinions as well.
File components/tracing/common/trace_startup_config.cc:
Patch Set #7, Line 18:
#include "base/trace_event/base_tracing.h"Does libchrome use trace_startup_config.cc, or can we revert this?
File content/browser/renderer_host/render_process_host_impl.cc:
Patch Set #7, Line 3726:
msg->type());Why is this change needed?
To view, visit change 3251806. To unsubscribe, or for help writing mail filters, visit settings.
Grace Cham (Gerrit)
Attention is currently required from: Hidehiko Abe, Eric Seckler, Ken Rockot, Hans Wennborg, Alex Moshchuk.
Grace Cham has uploaded this change for review.
Fix base, ipc, mojo build with ENABLE_BASE_TRACING disabled
trace_event is not supported in libchrome, and local patches are
currently applied to replace trace_event headers by stub or commenting
out relevant lines manually, although there is a build flag
enable_base_tracing which could be used for disabling it.
Fix the build in libchrome components on chromium upstream when the flag
is set to false, so that libchrome does not need to maintain the
divergence.
M ipc/trace_ipc_message.h
M ipc/ipc_mojo_bootstrap.cc
M mojo/core/core.cc
M mojo/core/data_pipe_producer_dispatcher.cc
M mojo/public/cpp/bindings/lib/send_message_helper.cc
M mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl
M mojo/public/tools/bindings/generators/cpp_templates/module.h.tmpl
M base/trace_event/base_tracing.h
M ipc/ipc_message_pipe_reader.cc
34 files changed, 179 insertions(+), 51 deletions(-)
Grace Cham (Gerrit)
Attention is currently required from: Hidehiko Abe, Eric Seckler, Ken Rockot, Hans Wennborg, Alex Moshchuk.
7 comments:
Patchset:
There's currently no automated testing of enable_base_tracing=false due to it not
being officially supported by //base. If we don't add such testing, this config
is bound to be broken frequently, especially if we now extend it beyond //base.
What you said is very true, we (libchrome team) in fact were thinking of enforcing new change to build with enable_base_tracing=false, using e.g. CQ, so that we can be free of it in downstream.
We weren't aware of crbug.com/1183347 (thanks for the pointer!), and IIUC base owners are against this, and the support of enable_base_tracing might even be stopped some time.
This is certainly a bad news to us.That said, it is still desirable to have this CL submitted since it could at least reduce the divergence and necessary maintenance for a while.
Which components is libchrome using, apart from //base? Would this be limited
to //ipc and //mojo?
Yes, as far as trace_event is concerned.
File base/trace_event/base_tracing.h:
Patch Set #7, Line 26:
#include "base/trace_event/trace_id_helper.h" // nogncheck
This declares GetNextGlobalTraceId(), but I don't see it being used in base_tracing. […]
As commented in line 8 of this file, this is a `Proxy header that provides tracing instrumentation for //base code` to allow switching between base tracing on and off.
When base tracing is enabled (with the gn flag enable_base_tracing, setting value of the build flag ENABLE_BASE_TRACING), code that uses trace event will include the "real" trace_event/ headers.
When it is disabled, they should include instead base/trace_event/trace_event_stub.h which is a stub header that provides mock implementations for trace event macros and others.User code can't include both at the same time or else there'll be redefinitions.
The way it switches between the real headers and stub header is to always include this base/trace_event/base_tracing.h in other code, so that there is no need to have the
```
#if BUILDFLAG(ENABLE_BASE_TRACING)
#include ...
#else
#include "base/trace_event/trace_event_stub.h"
#endif
```
to determine which header(s) to include in each and every user code.
Patch Set #7, Line 30:
#include "third_party/perfetto/include/perfetto/tracing/traced_value_forward.h" // nogncheck
It seems unusual to include both the real header (above) and the _forward.h header. […]
Thanks for catching this, this is indeed not needed.
Patch Set #7, Line 31:
#include "third_party/perfetto/protos/perfetto/trace/track_event/chrome_mojo_event_info.pbzero.h" // nogncheckWhat's this needed for? Could you use forward declarations instead?
Thanks for asking. This is originally used in mojo/public/cpp/{bindings/lib/connector.cc,system/simple_watcher.cc}. I moved them to this proxy header file as I do with other trace_event-only headers. But code searching again suggested that they are usually kept in the original file but included optionally with the `BUILDFLAG(ENABLE_BASE_TRACING)`.
File base/trace_event/trace_event_stub.h:
Patch Set #7, Line 15:
#include "base/task/single_thread_task_runner.h"Could you use a forward declaration instead?
Use of base::SingleThreadTaskRunner with scoped_refptr (line 234, 238) will complain `member access into incomplete type` if forward declaration is given only.
File components/tracing/common/trace_startup_config.cc:
Patch Set #7, Line 18:
#include "base/trace_event/base_tracing.h"
Does libchrome use trace_startup_config. […]
This is not included in libchrome and reverting it should be good, thanks!
File content/browser/renderer_host/render_process_host_impl.cc:
Patch Set #7, Line 3726:
msg->type());Why is this change needed?
This is a bit convoluted.
Primarily I have to fix a compile error in ChannelMojo::OnMessageReceived in ipc/ipc_channel_mojo.cc#310[1].
If the code is left as is without the change, there'll be a unused variable warning (i.e. error) on `message_ptr` for the tracing disabled build because TRACE_IPC_MESSAGE_SEND is empty in that case.
If we change
```
const Message* message_ptr = &message;
TRACE_IPC_MESSAGE_SEND("ipc,toplevel", "ChannelMojo::OnMessageReceived",
message_ptr);
```
to
```
TRACE_IPC_MESSAGE_SEND("ipc,toplevel", "ChannelMojo::OnMessageReceived", &message);
```
there will be `cannot take the address of an rvalue...` in the tracing enabled build. I can't think of a way to pass &message to the lambda in the macro TRACE_IPC_MESSAGE_SEND. But since all it uses is the message's type [2] which is an uint32_t, I changed the macro to take the type directly (and refactored a few other usages including this here).[1]: https://chromium-review.googlesource.com/c/chromium/src/+/3251806/7/ipc/ipc_channel_mojo.cc#310
[2]: https://source.chromium.org/chromium/chromium/src/+/main:ipc/trace_ipc_message.h;l=19
[3]: https://chromium-review.googlesource.com/c/chromium/src/+/3251806/7/ipc/trace_ipc_message.h#22
To view, visit change 3251806. To unsubscribe, or for help writing mail filters, visit settings.
Sami Kyöstilä (Gerrit)
Attention is currently required from: Hidehiko Abe, Eric Seckler, Grace Cham, Ken Rockot, Hans Wennborg, Alex Moshchuk.
1 comment:
Patchset:
> There's currently no automated testing of enable_base_tracing=false due to it not […]
Just to offer another alternative to reduce the long term maintenance cost, now that Perfetto is available as an automatically rolled ebuild in Chrome OS[1], maybe we could flip enable_base_tracing=true in libchrome too? I think it might be pretty useful to get get trace events out of CrOS system services too.
To view, visit change 3251806. To unsubscribe, or for help writing mail filters, visit settings.
Hans Wennborg (Gerrit)
Attention is currently required from: Hidehiko Abe, Eric Seckler, Grace Cham, Ken Rockot, Alex Moshchuk.
2 comments:
File base/trace_event/base_tracing.h:
Patch Set #7, Line 26:
#include "base/trace_event/trace_id_helper.h" // nogncheck
As commented in line 8 of this file, this is a `Proxy header that provides tracing instrumentation f […]
Thanks for the explanation.
File base/trace_event/trace_event_stub.h:
Patch Set #7, Line 15:
#include "base/task/single_thread_task_runner.h"
Use of base::SingleThreadTaskRunner with scoped_refptr (line 234, 238) will complain `member access […]
If the RegisterDumpProvider() definitions were moved to the .cc file, a forward declaration in the .h file should be sufficient.
To view, visit change 3251806. To unsubscribe, or for help writing mail filters, visit settings.
Grace Cham (Gerrit)
Attention is currently required from: Hidehiko Abe, Eric Seckler, Ken Rockot, Hans Wennborg, Alex Moshchuk, Sami Kyöstilä.
3 comments:
Patchset:
Just to offer another alternative to reduce the long term maintenance cost, now that Perfetto is ava […]
Thank you for the suggestion. I talked with the team about it and we are also considering the direction of supporting tracing, possibly start the investigation some time next year.
Patchset:
Thanks everyone for reviewing.
Since this CL is quite large I'll split it into multiple CLs and mark this as abandoned.
File base/trace_event/trace_event_stub.h:
Patch Set #7, Line 15:
#include "base/task/single_thread_task_runner.h"
If the RegisterDumpProvider() definitions were moved to the .cc file, a forward declaration in the . […]
Thanks! Yes that works.
To view, visit change 3251806. To unsubscribe, or for help writing mail filters, visit settings.
Grace Cham (Gerrit)
Grace Cham abandoned this change.
To view, visit change 3251806. To unsubscribe, or for help writing mail filters, visit settings.