problem compiling with is_component_build

[Roger Tawa]

Hi all,

tl;dr: code that compiles with is_component_build=true does not compile with is_component_build=false.  Fixing for the latter breaks the former.  Suggestions?

Long version

I wrote this code:

struct LastWriterInfo {

  raw_ptr<ClipboardHostImpl> writer;

  ui::ClipboardSequenceNumberToken sequence_number;

};

LastWriterInfo& GetLastWriterInfo() {

  static LastWriterInfo info;

  return info;

}

which fails to compile when `is_component_build=false` with:

../../content/browser/renderer_host/clipboard_host_impl.cc:75:25: error: declaration requires an exit-time destructor [-Werror,-Wexit-time-destructors]

  static LastWriterInfo info;

                        ^

1 error generated.

However it compiles just fine with `is_component_build=true`.  This is true for windows and linux.

If I change the function to:

LastWriterInfo& GetLastWriterInfo() {

  static base::NoDestructor<LastWriterInfo> info;

  return *info;

}

it no longer compiles when `is_component_build=true`:

../../base/no_destructor.h:79:3: error: static assertion failed due to requirement '!std::is_trivially_destructible_v<content::(anonymous namespace)::LastWriterInfo>': T is trivially destructible; please use a function-local static of type T directly instead

  static_assert(

  ^

../../content/browser/renderer_host/clipboard_host_impl.cc:73:45: note: in instantiation of template class 'base::NoDestructor<content::(anonymous namespace)::LastWriterInfo>' requested here

  static base::NoDestructor<LastWriterInfo> info;

                                            ^

However it compiles just fine with `is_component_build=false`.  Again, the same for windows and linux.

Changing the struct to be:

struct LastWriterInfo {

  ClipboardHostImpl* writer = nullptr;

  ui::ClipboardSequenceNumberToken sequence_number;

};

allows this code to compile in both cases.  Is there a solution that works that also includes raw_ptr<>?

Thanks,
Roger

-

[dan...@chromium.org]

raw_ptr is a weak recounting pointer basically, as such it requires a destructor. We don't allow it in NoDestructor then presumably because we are worried that the destructor may be meaningful? +Daniel Cheng 

Any solution that I can imagine here would be relaxing NoDestructor.

 

Thanks,
Roger

-

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAFv%2B6Uogb5tzsdQd%3DPBdacb%2Bf3O3O7sqV6vN2AyPJ%3Ds9__-67w%40mail.gmail.com.

[Joe Mason]

If I read the comments in base/no_destructor.h right, NoDestructor isn't necessary when the destructor is trivial because it can't have any order-of-destruction dependencies:

// - If `T` is trivially destructible, do not use `base::NoDestructor<T>`:

//

// const uint64_t GetUnstableSessionSeed() {

// // No need to use `base::NoDestructor<T>` as `uint64_t` is trivially

// // destructible and does not require a global destructor.

// static const uint64_t kSessionSeed = base::RandUint64();

So the static_assert is there because the destructor may NOT be meaningful, and is trying to stop people from using NoDestructor when not needed.

raw_ptr has a meaningful destructor in some build configurations (eg. with ENABLE_BACKUP_REF_PTR_SUPPORT) and not others (https://source.chromium.org/chromium/chromium/src/+/main:base/allocator/partition_allocator/pointers/raw_ptr.h;l=903;drc=792356842a46bf11d6db111d205a151b3ab0b467)

And... yeah, I haven't traced through all the layers of build flags, but I think on Windows `enable_backup_ref_ptr_support` has a different value in the component and non-component builds (https://source.chromium.org/chromium/chromium/src/+/main:build_overrides/partition_alloc.gni;l=81;drc=7ef6a4bcbee053167d4a79f24890d63e559adfda)

If developer-only component builds are pretty much the only place that doesn't enable_backup_ref_ptr_support, a simple fix would be to give raw_ptr a non-trivial destructor there, just to make it the same in all build configs. That might be justified even if there are release configs without enable_backup_ref_ptr_support.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHtyhaTpJJJ9%2BSfnUwqJV35FZa_Z7yw2C_B3tSdhjeKD47JoZA%40mail.gmail.com.

[Joe Mason]

In the mean time you could use some template magic to add base::NoDestructor only when needed, but it's annoying and really shouldn't be necessary:

// Uses NoDestructor by default. Enabled unless overridden by a partial specialization.

template<typename T, typename Enable=void>

struct NoDestructorWrapper {

  T& get() { return *t.get(); }

  private:

   base::NoDestructor<T> t;

};

// Partial specialization for trivially destructible types.

template<typename T>

struct NoDestructorWrapper<T, typename std::enable_if_t<std::is_trivially_destructible_v<T>>> {

  T& get() { return t; }

  private:

   T t;

};

LastWriterInfo& GetLastWriterInfo() {

    static NoDestructorWrapper<LastWriterInfo> info;

    return info.get();

}

[dan...@chromium.org]

backup_ref_ptr_support is not enabled in 3p repos that consume PA, and raw_ptr is meant to be a trivial no-op there.

On Mon, Mar 13, 2023 at 5:10 PM Joe Mason <joenot...@google.com> wrote:

[Roger Tawa]

Thanks all for the tips and info.

I think the best solution is to not use raw_ptr<> here but I was looking to see if there was something I missed.  The advantage of raw_ptr<> is that it nulls on creation and destruction.  In this case, since this is a global, it's only created once (so the `= nullptr` in the declaration solves that) and we don't want a destructor, so that's a moot point.  I think this is better than hacking raw_ptr<> or doing fancy template programming.

Roger

-

[Alex Ilin]

> The advantage of raw_ptr<> is that it nulls on creation

I'm not sure that it's always true: 

  // raw_ptr can be trivially default constructed (leaving |wrapped_ptr_|
  // uninitialized).  This is needed for compatibility with raw pointers.
  //
  // TODO(lukasza): Always initialize |wrapped_ptr_|.  Fix resulting build
  // errors.  Analyze performance impact.

https://crsrc.org/c/base/allocator/partition_allocator/pointers/raw_ptr.h;drc=0e9a0b6e9bb6ec59521977eec805f5d0bca833e0;l=891

Is it actually safe to leave raw_ptr<> uninitialized?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAFv%2B6UqiV4Mmi6UOYtM4HpcmK%3D1F7%3DqAzPqef4zrYV39V-Mvrw%40mail.gmail.com.

[dan...@chromium.org]

On Thu, Mar 16, 2023 at 5:05 AM Alex Ilin <alex...@chromium.org> wrote:

> The advantage of raw_ptr<> is that it nulls on creation

I'm not sure that it's always true: 

  // raw_ptr can be trivially default constructed (leaving |wrapped_ptr_|
  // uninitialized).  This is needed for compatibility with raw pointers.
  //
  // TODO(lukasza): Always initialize |wrapped_ptr_|.  Fix resulting build
  // errors.  Analyze performance impact.

https://crsrc.org/c/base/allocator/partition_allocator/pointers/raw_ptr.h;drc=0e9a0b6e9bb6ec59521977eec805f5d0bca833e0;l=891

Is it actually safe to leave raw_ptr<> uninitialized?

Correct, it is initialized on some platforms but not all platforms until BRP is enabled everywhere.