Proposal: Allow absl::optional and migrate base::Optional to it

[Peter Kasting]

I propose that we allow absl::optional via the following steps:

(1) In base/optional.h, #include "third_party/abseil-cpp/absl/types/optional.h", and make base::Optional and base::nullopt_t aliases to the absl equivalents.

(2) Remove base/optional_unittest.*.

(3) Send a message to chromium-dev@ saying that absl::optional is legal and preferred.

(4) Write an LSC proposal for the remaining steps below.

(5) Rewrite all uses of base::Optional to absl::optional, and base::nullopt_t to absl::nullopt_t.

(6) Rewrite all #includes of base/optional.h to  third_party/abseil-cpp/absl/types/optional.h.

(7) Remove base/optional.h.

I volunteer to do these steps if this meets with general approval.

PK

[Jeremy Roman]

Makes sense to me. Do we know whether there are any API differences? (They would hopefully be minor but it wouldn't surprise me if base::Optional has grown non-standard bits at some point.)

--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDsJMQWbQO-iYWz4tooXzcY%2BFK34Eajsavg5KwAPmNq7g%40mail.gmail.com.

[dan...@chromium.org]

On Wed, Apr 14, 2021 at 2:08 PM Jeremy Roman <jbr...@chromium.org> wrote:

Makes sense to me. Do we know whether there are any API differences? (They would hopefully be minor but it wouldn't surprise me if base::Optional has grown non-standard bits at some point.)

If it did it was not intentional, it should match std::optional.

 

On Wed, Apr 14, 2021 at 1:44 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

I propose that we allow absl::optional via the following steps:

(1) In base/optional.h, #include "third_party/abseil-cpp/absl/types/optional.h", and make base::Optional and base::nullopt_t aliases to the absl equivalents.

(2) Remove base/optional_unittest.*.

(3) Send a message to chromium-dev@ saying that absl::optional is legal and preferred.

(4) Write an LSC proposal for the remaining steps below.

(5) Rewrite all uses of base::Optional to absl::optional, and base::nullopt_t to absl::nullopt_t.

(6) Rewrite all #includes of base/optional.h to  third_party/abseil-cpp/absl/types/optional.h.

(7) Remove base/optional.h.

I volunteer to do these steps if this meets with general approval.

PK

--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDsJMQWbQO-iYWz4tooXzcY%2BFK34Eajsavg5KwAPmNq7g%40mail.gmail.com.

--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13c24gppNVPM5GVTkk7P22MXFh98ivmAjCyH1fyZiULpyA%40mail.gmail.com.

[Daniel Cheng]

Two questions:

- making sure we don't backslide on our stricter enforcement of validity? std::optional::operator* does not runtime enforce validity (instead, it's UB to use this when there's no contained value), while absl::optional::operator* and std::optional::operator* currently do

- is it possible for there to be any binary size effects from this? It looks like absl uses abort() to terminate execution when accessing an unpopulated optional using value(). 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaSjY6ZmGqyziGFEuSt7m2K%2Bw6JG9z84%3DU%2Bu%2Be8SDYqEow%40mail.gmail.com.

[Daniel Cheng]

2021年4月14日(水) 11:18 Daniel Cheng <dch...@chromium.org>:

Two questions:

- making sure we don't backslide on our stricter enforcement of validity? std::optional::operator* does not runtime enforce validity (instead, it's UB to use this when there's no contained value), while absl::optional::operator* and std::optional::operator* currently do

Sorry for the typo. absl::optional and *base::Optional* do, but not std::optional.

[dan...@chromium.org]

On Wed, Apr 14, 2021 at 2:37 PM Daniel Cheng <dch...@chromium.org> wrote:

2021年4月14日(水) 11:18 Daniel Cheng <dch...@chromium.org>:

Two questions:

- making sure we don't backslide on our stricter enforcement of validity? std::optional::operator* does not runtime enforce validity (instead, it's UB to use this when there's no contained value), while absl::optional::operator* and std::optional::operator* currently do

Sorry for the typo. absl::optional and *base::Optional* do, but not std::optional.

I think this point means maintaining our own optional_unittests with death tests for the properties we want to maintain regarding avoiding UB.

I thought we had conversation with absl about providing our own impl instead of abort() but I guess that didn't happen or I am misremembering.

[Peter Kasting]

On Wed, Apr 14, 2021 at 11:18 AM Daniel Cheng <dch...@chromium.org> wrote:

- making sure we don't backslide on our stricter enforcement of validity? std::optional::operator* does not runtime enforce validity (instead, it's UB to use this when there's no contained value), while absl::optional::operator* and std::optional::operator* currently do

We could prevent a fallback to the std:: version once C++17 becomes available via #define ABSL_OPTION_USE_STD_OPTIONAL 0.  That would be harmless to do now.

I don't think we need to maintain our own unittests beyond absl's, but if consensus is the other way I'm happy to keep them around.

- is it possible for there to be any binary size effects from this? It looks like absl uses abort() to terminate execution when accessing an unpopulated optional using value().

It's possible; I can at least look locally when writing the patch.

PK 

[dan...@chromium.org]

On Wed, Apr 14, 2021 at 2:41 PM Peter Kasting <pkas...@google.com> wrote:

On Wed, Apr 14, 2021 at 11:18 AM Daniel Cheng <dch...@chromium.org> wrote:

- making sure we don't backslide on our stricter enforcement of validity? std::optional::operator* does not runtime enforce validity (instead, it's UB to use this when there's no contained value), while absl::optional::operator* and std::optional::operator* currently do

We could prevent a fallback to the std:: version once C++17 becomes available via #define ABSL_OPTION_USE_STD_OPTIONAL 0.  That would be harmless to do now.

I don't think we need to maintain our own unittests beyond absl's, but if consensus is the other way I'm happy to keep them around.

There's no real way to feel safe that we maintain this without tests, I'm afraid. One day absl might remove that ifdef or something.

[Jan Wilken Dörrie]

The overall proposal LGTM.

Re keeping the DEATH checks: I tend to agree that we should. Then we also don't need to fear to silently regress on this when absl::optional becomes std::optional, and we don't need to touch ABSL_OPTION_USE_STD_OPTIONAL. 

Re binary size: It looks like they use abort() in value(), and try to do something similar to IMMEDIATE_CRASH in the hardened operator->() and operator*(). In addition to checking locally the android-binary-size trybot should hopefully give us a good idea about the impact here as well.

--

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaQdsEQTpCR1yXQtT44ygEnoN0SRtjuPU1W2x2esSa%2B9MA%40mail.gmail.com.

[Peter Kasting]

What I'm hearing so far:

* No opposition to the overall idea

* Keep the death tests in optional_unittest.cc (forever)

* Check binary size locally and on the android-binary-size trybot to see how this impacts it

* No need to #define ABSL_OPTION_USE_STD_OPTIONAL 0 (for now) -- can always use this later if attempting to migrate to C++17 causes our death tests to fail

Please correct misimpressions and add further input as necessary.  Otherwise I can probably start working on this early next week.

PK

[K. Moon]

SGTM. Although given jdoerrie@'s comment about possible code size differences between value() and operator->/*(), I'm wondering if this would lead to guidance at some point to prefer one over the other?

--

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAwJnrMFg_zK0X0c%2BE7rB2UNAuCC-Z3JF5eaks7PE3vbA%40mail.gmail.com.

[Daniel Cheng]

I think if we find that there are binary size differences, we should follow up with absl to see if we can eliminate them: developers should not be choosing between value() / operator-> / operator* for the purpose of trying to minimize runtime overhead / binary size.

(We've previously experienced this with base::Optional, since it was not always the case that all ways to unwrap the value CHECKed that the value was actually populated)

The overall plan SGTM.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-5QZRRpaAuS3h2rDpyZJWqF5TjLiFn0N6OsE9B7boKVXw%40mail.gmail.com.

[Anton Bikineev]

I had some free time this weekend to play around with rewriting of Optional. I've done the following:

1. sed for all chromium, except base. Names that needed to be changed:
• include "base/optional.h" -> include "third_party/abseil-cpp/absl/types/optional.h"
• base::Optional -> absl::optional
• base::nullopt -> absl::nullopt
• base::make_optional -> absl::make_optional
2. For base/, I've applied a simple clang-tidy checker. This is because inside the base namespace the names can be both qualified and unqualified. This still required to manually change some platform specific files.

Some technical observations:
1) abseil's optional.h has includes that are not relative to the chromium's rootdir (i.e. includes inside abseil are not transformed with copybara). //base publicly depends on //third_party/abseil-cpp, which pulls the cflag -Ithird_party/abseil-cpp. Targets that use base::Optional but forget to depend on //base will break (I ran into about 9 such targets).

2) base::in_place can't be easily replaced with sed, because mojo's CheckedPtr also uses base::in_place.

3) Oilpan's clang plugin (blink_gc_plugin) has checks for the correct use of base::Optional for GCed classes. The check and the corresponding tests should be updated.

4) gdbscript and windbg scripts that have pretty-printers for base::Optional should be updated (tools/gdb/gdb_chrome.py and tools/win/DebugVisualizers/chrome.natvis).

5) Some files that use the base's CHECKS forget to include base/check.h, which didn't fail because the header was transitively included from base/optional.h.

6) Calls to the function base::OptionalOrNullptr() must always be fully qualified outside of the base namespace, because ADL no longer works for absl::optional.

Here is a huge CL that replaces all uses of base::Optional. I don't think it's a right way to proceed though, since it's too hard to rebase (about 12k changed files). Maybe a more incremental approach would work better.

The binary size effect of absl::optional seems to be not too bad: android-binary-size bot has the regression of 16.2KB regression, which is below the allowed limit of 16.3KB :).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKqEFn8A6wMpOUW2B9N1QfkZM4Bx2YSHrJKhs6ms_jU%3DoQ%40mail.gmail.com.

[Anton Bikineev]

Regarding the rewrite, I would propose the following course of action:

1) have typedefs in base/optional.h that point to absl::optional and friends. This will flush out dependency issues in GN, which can be fixed in the same CL (i.e. targets that depend on base::Optional (directly or transitively) should have deps += [ "//base" ]);

2) apply rewrites to components (maybe a CL per component), fixing other issues I mentioned in the previous email;

3) update the rest: the Oilpan plugin and gdbscripts.

[Peter Kasting]

On Sun, Apr 18, 2021 at 3:43 PM Anton Bikineev <biki...@google.com> wrote:

Regarding the rewrite, I would propose the following course of action:

1) have typedefs in base/optional.h that point to absl::optional and friends. This will flush out dependency issues in GN, which can be fixed in the same CL (i.e. targets that depend on base::Optional (directly or transitively) should have deps += [ "//base" ]);

2) apply rewrites to components (maybe a CL per component), fixing other issues I mentioned in the previous email;

3) update the rest: the Oilpan plugin and gdbscripts.

I can write up an LSC proposal for this work.

Would you be willing to manually split your giant CL into smaller chunks and send them to me for review?  I can start reviewing and landing once the LSC proposal gets approval.

PK 

[Anton Bikineev]

1) have typedefs in base/optional.h that point to absl::optional and friends. This will flush out dependency issues in GN, which can be fixed in the same CL (i.e. targets that depend on base::Optional (directly or transitively) should have deps += [ "//base" ]);

 

Btw, I just realized that this will also require replacing base::in_place with absl::in_place, which will mix the use of base::optional and absl::in_place.

 

Would you be willing to manually split your giant CL into smaller chunks and send them to me for review?  I can start reviewing and landing once the LSC proposal gets approval.

 

Sure, however, I'm not sure I'll have time during the week. I guess, since the proposal will anyway await the approval, this should be fine.

[Hans Wennborg]

I didn't see build speed mentioned yet, but it seems base/optional and
absl/optional are roughly the same size so hopefully it's not an
issue:

https://commondatastorage.googleapis.com/chromium-browser-clang/include-analysis.html#filter=%5Ebase%2Foptional%5C.h%7Cthird_party%2Fabseil-cpp%2Fabsl%2Ftypes%2Foptional%5C.h%24&sort=tsize

[Peter Kasting]

Can someone with more knowledge of the libc++ hardening plans (if any) speak to that here?

The LSC proposal for this work was approved, but Nico (CCed) noted on that thread that we might get C++17 support before EOY.  This raises two questions:

1. How soon/likely is it that we get hardening in libc++'s optional (and elsewhere)?  Who on our side is driving this?
2. Given the answer to #1, is there any desire to delay this switchover and go straight to std::optional?  Or should we proceed with absl:: for now?

PK

[Chris Palmer]

I've had the libc++ hardening thing on my plate for a while now, but it has been hard for me to move it forward for a few reasons (one being a big re-org of libcxx in llvm after I had gotten a working patch +1'd :) ). I'll either finish it or hand it off ASAP, but in any case Abseil should be good to go now, and should pass our existing base/optional_unittest.* tests.

--

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFAeTynfrobNfif2Wb8iaKEQgAAmxTw%3DkLCQTgtYBS4ZOA%40mail.gmail.com.

[Daniel Cheng]

I think it makes sense to still go ahead and do this conversion, even if we might switch over to C++17 soon. It's fairly straightforward and mechanical, and it eliminates the need to write weird conversion code at the boundary layers.

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAOuvq20f209VV-gQG-j0%2BqMXnrZ-Ej%3DiX4KePz%2BPHkuOyVz9qA%40mail.gmail.com.