Commit bad5128d authored by Piotr Tworek's avatar Piotr Tworek Committed by Commit Bot

Fix TracingCategory::kNavigation GOLD linking problems.

Patch ae49e297, "Add tracing to
ClassifyNavigation to make debugging easier." introduced the following
code:

struct TracingCategory {
  static constexpr const char kNavigation[] = "navigation";
};

This looks fine at first glance, but has one subtle problem that the
author of the code might not have been aware of. In C++14 (which is
currently used by chomium) a static constexpr member variable (unlike
other global constexpr variables) has an external linkage. This is not
really intuitive and is probably why it was fixed in C++17, where such
static constexpr member is inline.

The interesting part is this subtle difference in linking semantics
breaks component builds when using GOLD linker instead of LLD.

  obj/content/browser/browser/navigation_controller_impl.o(.debug_addr+0x858):
  error: undefined reference to 'content::TracingCategory::kNavigation'

To fix this lets just convert TracingCategory from struct into a
namespace. This way it should be inlined no matter which version of the
C++ standard the compiler is instructed to adhere to.

Ref: https://en.cppreference.com/w/cpp/language/constexpr

Change-Id: Ibf0471eaf20805cc7af8236386aa7c5318d26f4c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436824
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Reviewed-by: default avatarNasko Oskov <nasko@chromium.org>
Reviewed-by: default avatarAlexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812257}
parent 5e4d1603
......@@ -1248,7 +1248,7 @@ bool NavigationControllerImpl::RendererDidNavigate(
NavigationType NavigationControllerImpl::ClassifyNavigation(
RenderFrameHostImpl* rfh,
const FrameHostMsg_DidCommitProvisionalLoad_Params& params) {
TraceReturnReason<TracingCategory::kNavigation> trace_return(
TraceReturnReason<tracing_category::kNavigation> trace_return(
"ClassifyNavigation");
if (params.did_create_new_entry) {
......
......@@ -11,9 +11,11 @@
namespace content {
struct TracingCategory {
static constexpr const char kNavigation[] = "navigation";
};
// This can't be a struct since in C++14 static constexpr structure members
// have external linkage. This has been fixed in C++17.
namespace tracing_category {
static constexpr const char kNavigation[] = "navigation";
}
// Class which facilitates annotating with traces all possible return paths
// from a function or a method. Setting the return reason is enforced by a
......@@ -24,7 +26,7 @@ struct TracingCategory {
// Example usage:
//
// void SomeMethod() {
// TraceReturnReason<TracingCategory::kNavigation> trace_return("Method");
// TraceReturnReason<tracing_category::kNavigation> trace_return("Method");
//
// if (condition) {
// trace_return.set_return_reason("foo");
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment