Intent to Add //base dependency on absl for unavailable-but-approved STL types - Invitation to comment

[Gabriel Charette (via Google Docs)]

g...@chromium.org has invited you to comment on the following document:

Intent to Add //base dependency on absl for unavailable-but-approved STL types

Hello everyone, after recent threads around importing a few absl types in //base to avoid reinventing the wheel, seems we were only lacking a concrete proposal, so here it is.

I think this will save many SWE-hours now and in the future.

Thanks,
Gab

Open in Docs

Snapshot of the item below:

Intent to Add //base dependency on absl for unavailable-but-approved STL types

Status: In Review

Authors: g...@chromium.org 

Reviewers: c...@chromium.org 

Last Updated: 2019-03-08

PUBLIC

Objective

Use third_party/abseil-cpp/absl/types to avoid reinventing the wheel for types already accepted in the STL versions of C++ Chromium doesn’t yet support (e.g. C++17 types like std::optional, std::variant, etc.).

Ban all absl:: usage outside of src/base/ via presubmit errors (enforced by CQ unless NOPRESUBMIT=True). Expose allowed absl:: types in base:: through using declarations in base headers.

Background

jdoerrie@ has already attempted to do this for base/optional.h and base/thread_annotations.h (CL). A few custom modifications are required on abseil-cpp, i.e.:

1. ABSL_TYPES_EXPORT on exported types for component build
2. Port NO_THREAD_SAFETY_ANALYSIS from base/optional.h to absl/types/optional.h (required for ~5 callsites), a simple one liner to maintain.
3. Tweak a few internal templates (i.e. s/get_assign_copy_traits()/ctor_copy_traits::trait) for ObjC++ compat (could upstream to absl but also lightweight to maintain).

alexclarke@ proposed doing this for base/variant.h (cxx email thread); no decision taken but general feeling that if we did this we should do it for all relevant types, not just one-off for base::variant (hence this doc)

LON interpreted the above email thread as a no-go on importing absl and alexclarke@ implemented base::variant from scratch in 8 CLs totalling >5000LOCs (inc. tests):

• [promises] Add support for std::hash and base::variant_size(Indirect ancestor)
• [promises] Add support for swap to base::variant(Indirect ancestor)
• [promises] Add base::visit to base::variant(Indirect ancestor)
• [promises] Add comparison operators to base::variant(Indirect ancestor)
• [promises] Add support for emplace to base::variant(Indirect ancestor)
• [Promises] Introduce base::variant(Indirect ancestor)
• [promises] Core base::variant functionality(Indirect ancestor)
• Add base/parameter_pack.h a collection of parameter pack utils()

gab@ still thinks strongly that reinventing the wheel is the wrong thing and spurred a side-discussion on slack to coalesce thoughts : seems that besides a few minor concerns needing a doc (this), nothing is really blocking importing absl

It’s unfortunate that alexclarke@ ended up writing all this code but if anything it reinforces the need to import absl as reinventing 5000LOCs (and reviewing + maintaining it) doesn’t seem like the right move.

Proposal

Enforce that only** //base can use absl:: via presubmits errors.

** webrtc/ is the only other exception per already being a user (and we can drop unfortunate conversion utils like ToBaseOptional)

Only expose absl types which are STL types documented on cppreference.com but merely not available to us yet in our supported C++ version (e.g. C++17 constructs). This will be made explicit in third_party/abseil-cpp/README.chromium.

Users of, e.g., base::optional would include base/optional.h and rely on it forwarding the absl type via:

namespace base {

using optional = absl::optional;

}

Concerns

Concerns mentioned on the various threads.

• (nil). Documentation no longer being available directly in //base: restricting to STL types means we can rely (and even link to) cppreference.com’s excellent documentation
• (nil w/ unittests). Our implementations have security CHECKs and correctness DCHECKs (e.g. base::optional and base::span): absl’s implementation also does (FATAL log + abort() instead of CHECKs and assert() instead of DCHECKs): we should add EXPECT_(D)CHECK_DEATH unittests in //base to verify and maintain this critical aspect of the absl impl

• Maybe we need to ! define NDEBUG in DCHECK_IS_ON() builds to get absl assert()’s -- but we might just want to do that anyways.

• (nil). C++ exceptions: absl doesn’t use them unless ABSL_HAVE_EXCEPTIONS is defined (results in FATAL log + abort() when not)

• See ABSL_HAVE_EXCEPTIONS definition. Basically it can’t be on unless one-of __EXCEPTIONS, __cpp_exceptions, or _CPPUNWIND is defined.

• (nil) absl types leaking outside of base from absl includes at the top of base includes: won’t be possible to use them per the presubmit error against absl:: .

Code size: allegedly absl::variant results in more generated code^[a]. Is it that substantial? Can we fix it upstream?

Security Considerations

We need to make sure absl has the same level of security guarantees as Chromium as far out-of-bound/invalid-memory-access CHECKs go. It looks like they do, but base unittests will be added to verify and maintain.

Testing Plan

1. Write EXPECT_(D)CHECK_DEATH tests in existing unittests.
2. Migrate existing types without touching existing unittests.
3. Delete redundant unittests (i.e. all but the death tests).

Work Estimates

1. Rebase and land https://chromium-review.googlesource.com/c/chromium/src/+/1277105 (jdoerrie@?)

1. With additional presubmit error check
2. Without modifying DEPS outside of //base?

1. (maybe) alexclarke@ to fix code size generation issues from base::variant in absl upstream.
2. alexclarke@ to land base/variant.h using absl::variant

This seems like much less work than reviewing/iterating-on and then maintaining 5000LOCs of base::variant code (and it gets more beneficial every new desired STL type we import this way).

Additional Benefits

Immediate compliance with the STL when Chromium begins supporting new versions of C++.

[a]+alexc...@chromium.org to document this further

_Assigned to Alex Clarke_

Google Docs: Create and edit documents online. Google LLC, 1600 Amphitheatre Parkway, Mountain View, CA 94043, USA You have received this email because someone shared a document with you from Google Docs.

[Gabriel Charette]

Monday morning bump :)

This is a courtesy copy of an email for your record only. It's not the same email your collaborators received. Click here to learn more.

[Nico Weber]

I left a few comments on the document. The high level comment is that the document doesn't really say what problem it's attempting to solve, or even what exactly it's proposing.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7L%2BqytPKVyFOL23fFfvEUvpG12gtOxLa7h29TrhB_k_ATQ%40mail.gmail.com.

[Gabriel Charette]

On Mon, Mar 11, 2019 at 9:37 AM Nico Weber <tha...@chromium.org> wrote:

I left a few comments on the document. The high level comment is that the document doesn't really say what problem it's attempting to solve, or even what exactly it's proposing.

Isn't this the very first line of the doc...? "Objective : Use third_party/abseil-cpp/absl/types to avoid reinventing the wheel for types already accepted in the STL versions of C++ Chromium doesn’t yet support (e.g. C++17 types like std::optional, std::variant, etc.)."

Then it goes into details about the Background and the Proposal... So beyond an explicit list of Relevant types (which I just added), I don't see what's not clear about this doc..