Allowing first-party dependencies on Abseil

[Peter Kasting]

Hello cxx@,

TLDR: Several of us think we should allow first-party code to depend on Abseil, and want to plan how to roll it out.  Reply if you have opinions.

Longer version: An internal thread raised several instances of requests for Abseil, ranging in necessity.  Chrome Eng Review is on board in principle.  danakj@, dcheng@, thakis@, and I discussed the technical blockers here, including getting sufficient support from Abseil for Chromium's needs, and not risking being stranded if Abseil wants to drop C++14 support before Chromium is ready.  We believe those blockers are being addressed sufficiently.

The immediate technical blocker is http://crbug.com/1046390 , which is being addressed.  Once that is fixed, I believe that in principle we could begin allowing first-party use of absl:: code.  To do this we need three things:

1. Agreement-in-principle that we do, in fact, want to allow this.
• Proposal: Consider this agreed if there is no unresolved negative feedback after a week.
2. A high-level idea of how to do the roll-out.
• Proposal: Treat this similarly to a new C++ version, in that we start with most things default-banned, allow on a by-request basis, and aim to default-allow within two years.  Track in the Chromium style guide and chromium-cpp.appspot.com.
3. Specific details for (2), particularly obviously-safe things to allow from the beginning or obviously-concerning constructs to look at more closely.
• Proposal: Allow absl::any and absl::variant to start with, as these polyfill C++17 types, have no base:: implementation, and have consumer interest .  Allow other types and functions to begin with only as necessary to compile against third-party code using Abseil and convert to non-absl:: types.  Create a list of overlaps between base:: and absl:: (e.g. base::Optional vs. absl::optional, base::StringPiece vs. absl::string_view, base::Time and related classes vs. absl::Time and related classes, etc.) and decide a plan for deduplication going forward.  Bias towards std:: types, then absl:: types, then base:: types.

PK

[Jeremy Roman]

Generally like the idea of not duplicating Abseil more than necessary, especially since it means easier interop with other subprojects that cannot depend on base/.

Variant has been a persistent thing that we've wanted (though I've heard std::variant can blow up in binary size, so we might want to be a little cautious). Does absl::any have a concrete use case in Chromium?

 

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/CAAHOzFDTh%3DxPDb--Z%3DPEq06Gh11J0AtbakZ1NFMymGFUTCoTgA%40mail.gmail.com.

[Peter Kasting]

On Tue, Jun 16, 2020 at 6:48 PM Jeremy Roman <jbr...@chromium.org> wrote:

On Tue, Jun 16, 2020 at 9:26 PM Peter Kasting <pkas...@chromium.org> wrote:

• Proposal: Allow absl::any and absl::variant to start with, as these polyfill C++17 types, have no base:: implementation, and have consumer interest .

Variant has been a persistent thing that we've wanted (though I've heard std::variant can blow up in binary size, so we might want to be a little cautious). Does absl::any have a concrete use case in Chromium?

https://chromium-review.googlesource.com/c/chromium/src/+/2228543 uses it somewhat heavily (mainly because of the interface-with-third-party-code thing), and that project was one of the consumers that prompted this discussion.

::variant is definitely the obvious one we've asked for, but given the CL above and the fact that absl::any is basically just C++17's std::any, I put it in the list.  Feel free to disagree :)

PK 

[Victor Vasiliev]

Does Abseil still have no plan to support basic_string_view<T> for any T other than char?  I assume that would mean we'd have to keep StringPiece16 even if we switch from StringPiece to absl::string_view.

--

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/CAAHOzFANC0-mmgXS4MGqYUubr50_OG5mevb_mp9uBpkfBA8jJA%40mail.gmail.com.

[Chris Palmer]

To me the key thing is that we not lose safety and correctness by preferring absl::foo to base::foo. We've put a lot of work into making base more resilient and secure — recently confirmed by Project Zero to have stopped some real attacks, by the way — and I *really* don't want for Chromium to regress.

Toward that end, keep in mind that we always need to build Abseil with its hardening mode turned on in Chromium. That's a minimum requirement.

In my dream world, we'd 'productize' as much of base as we could, re-designing it as we go and as/if necessary for ergonomic safety, and make it as easy as possible for any callers (inside or outside of Chromium) to call it. The world needs a safety-oriented C++ library that remains as efficient as possible, and base is low-key succeeding at that, IMO. AFAICT nobody else is attempting to do this, so there's an unmet need in the world.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAZdMacERRu%2BnpyWmF4owcuaseuUMoPSm5-gYn6c2bwt_HfJ2Q%40mail.gmail.com.

[Gabriel Charette]

On Tue, Jun 16, 2020 at 9:26 PM Peter Kasting <pkas...@chromium.org> wrote:

Hello cxx@,

TLDR: Several of us think we should allow first-party code to depend on Abseil, and want to plan how to roll it out.  Reply if you have opinions.

Longer version: An internal thread raised several instances of requests for Abseil, ranging in necessity.  Chrome Eng Review is on board in principle.  danakj@, dcheng@, thakis@, and I discussed the technical blockers here, including getting sufficient support from Abseil for Chromium's needs, and not risking being stranded if Abseil wants to drop C++14 support before Chromium is ready.  We believe those blockers are being addressed sufficiently.

The immediate technical blocker is http://crbug.com/1046390 , which is being addressed.  Once that is fixed, I believe that in principle we could begin allowing first-party use of absl:: code.  To do this we need three things:

1. Agreement-in-principle that we do, in fact, want to allow this.
• Proposal: Consider this agreed if there is no unresolved negative feedback after a week.
2. A high-level idea of how to do the roll-out.
• Proposal: Treat this similarly to a new C++ version, in that we start with most things default-banned, allow on a by-request basis, and aim to default-allow within two years.  Track in the Chromium style guide and chromium-cpp.appspot.com.

A key difference with C++ style I think is that absl also brings in a library of utils. Hence, some folks not tracking cxx@ likely need a say in whether we let some utilities in. e.g. I'd want //base/task/OWNERS to be looped in if threading utils are considered.

 

1. Specific details for (2), particularly obviously-safe things to allow from the beginning or obviously-concerning constructs to look at more closely.
• Proposal: Allow absl::any and absl::variant to start with, as these polyfill C++17 types, have no base:: implementation, and have consumer interest .  Allow other types and functions to begin with only as necessary to compile against third-party code using Abseil and convert to non-absl:: types.  Create a list of overlaps between base:: and absl:: (e.g. base::Optional vs. absl::optional, base::StringPiece vs. absl::string_view, base::Time and related classes vs. absl::Time and related classes, etc.) and decide a plan for deduplication going forward.  Bias towards std:: types, then absl:: types, then base:: types.

This will be very useful and was the reason I'd proposed this last year (component build support and security concerns stopped it then). I just hate seeing us re-implement STL well-known types. Glad to see this being given some attention again.

 

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/CAAHOzFDTh%3DxPDb--Z%3DPEq06Gh11J0AtbakZ1NFMymGFUTCoTgA%40mail.gmail.com.

[Daniel Cheng]

On Tue, Jun 16, 2020 at 9:14 PM Chris Palmer <pal...@chromium.org> wrote:

To me the key thing is that we not lose safety and correctness by preferring absl::foo to base::foo. We've put a lot of work into making base more resilient and secure — recently confirmed by Project Zero to have stopped some real attacks, by the way — and I *really* don't want for Chromium to regress.

Toward that end, keep in mind that we always need to build Abseil with its hardening mode turned on in Chromium. That's a minimum requirement.

In my dream world, we'd 'productize' as much of base as we could, re-designing it as we go and as/if necessary for ergonomic safety, and make it as easy as possible for any callers (inside or outside of Chromium) to call it. The world needs a safety-oriented C++ library that remains as efficient as possible, and base is low-key succeeding at that, IMO. AFAICT nobody else is attempting to do this, so there's an unmet need in the world.

I'd like to second this point: for absl things we do start using in //base, I'd want to make sure we have some smoke tests to ensure that the appropriate CHECKs are enabled. To me, this is especially important because my understanding is absl transparently maps through to the standard library version if it's recent enough (e.g. support for optional, string_view, variant). I want to make sure we don't end up in a situation where we had security hardening and then lost it by upgrading a new C++ version.

Proposal: Treat this similarly to a new C++ version, in that we start with most things default-banned, allow on a by-request basis, and aim to default-allow within two years.  Track in the Chromium style guide and chromium-cpp.appspot.com.

How often does absl change/add new things? I'm a bit hesitant to say that we should just default-allow in two years, depending on how often new concepts/functionality is introduced. It seems to me the most value comes from the container types, especially for interop with other third-party libraries that are using absl at library boundaries.

Proposal: Allow absl::any and absl::variant to start with, as these polyfill C++17 types, have no base:: implementation, and have consumer interest .  Allow other types and functions to begin with only as necessary to compile against third-party code using Abseil and convert to non-absl:: types.  Create a list of overlaps between base:: and absl:: (e.g. base::Optional vs. absl::optional, base::StringPiece vs. absl::string_view, base::Time and related classes vs. absl::Time and related classes, etc.) and decide a plan for deduplication going forward.  Bias towards std:: types, then absl:: types, then base:: types.

I can't say that I personally care for any, but maybe I don't understand the use cases. I know there's a CL that wants to use absl::any, but it's hard to tell exactly what the intended usage is (since it's a support library for third-party code).

Finally, I'd like to add that span and absl's hash containers should probably be on this list: in particular, I'd really like to see absl's hash containers be the default recommendation (though we'll likely need to benchmark ARM).

Daniel

 

On Tue, Jun 16, 2020 at 7:04 PM Victor Vasiliev <vas...@chromium.org> wrote:

Does Abseil still have no plan to support basic_string_view<T> for any T other than char?  I assume that would mean we'd have to keep StringPiece16 even if we switch from StringPiece to absl::string_view.

On Tue, Jun 16, 2020 at 9:52 PM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Jun 16, 2020 at 6:48 PM Jeremy Roman <jbr...@chromium.org> wrote:

On Tue, Jun 16, 2020 at 9:26 PM Peter Kasting <pkas...@chromium.org> wrote:

• Proposal: Allow absl::any and absl::variant to start with, as these polyfill C++17 types, have no base:: implementation, and have consumer interest .

Variant has been a persistent thing that we've wanted (though I've heard std::variant can blow up in binary size, so we might want to be a little cautious). Does absl::any have a concrete use case in Chromium?

https://chromium-review.googlesource.com/c/chromium/src/+/2228543 uses it somewhat heavily (mainly because of the interface-with-third-party-code thing), and that project was one of the consumers that prompted this discussion.

::variant is definitely the obvious one we've asked for, but given the CL above and the fact that absl::any is basically just C++17's std::any, I put it in the list.  Feel free to disagree :)

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/CAAHOzFANC0-mmgXS4MGqYUubr50_OG5mevb_mp9uBpkfBA8jJA%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/CAAZdMacERRu%2BnpyWmF4owcuaseuUMoPSm5-gYn6c2bwt_HfJ2Q%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/CAOuvq22KYgih5TQvMQUviC4c%3Djs-g9wbgBk%3DDmHg2L1zLpMy0w%40mail.gmail.com.

[John Abd-El-Malek]

Can you provide more background on what is the motivation? I haven't seen a discussion of this on eng-review (only on allowing more third party dependencies to use it).

Specifically, I'm wondering what's to gain from removing the things in base/ that have equivalents in abseil. Presumably what we have works, so is converting them to use something equivalent a good use of our limited resources?

--

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/CAAHOzFDTh%3DxPDb--Z%3DPEq06Gh11J0AtbakZ1NFMymGFUTCoTgA%40mail.gmail.com.

[Daniel Cheng]

For types that are redundant between //base and //absl, interoperating with third-party libraries that use absl is somewhat painful. An example of boilerplate that's needed is

https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/platform/peerconnection/webrtc_util.h;l=15;drc=8a030dd3c631e68b67926fff0839ff085d81bdd7, which is pretty unfortunate because it forces a bunch of copies.

I'm not sure if we should replace /everything/ (in particular, I'm skeptical of changing things like Time representations). However, the //base implementations of optional and span were intentionally designed to match the upstream standard library API as closely as possible to facilitate switching to the standard library (or absl) as appropriate.

Does this concern also apply to types which have no equivalent in base (absl's hash tables, variant, et cetera)?

Daniel

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

[Peter Kasting]

On Tue, Jun 16, 2020 at 7:03 PM Victor Vasiliev <vas...@chromium.org> wrote:

Does Abseil still have no plan to support basic_string_view<T> for any T other than char?  I assume that would mean we'd have to keep StringPiece16 even if we switch from StringPiece to absl::string_view.

I believe this is https://github.com/abseil/abseil-cpp/issues/124 , which seems to be "open to this in principle, not a priority for us, big worry would be ensuring it doesn't break google3 to change string_view from a class to a type-alias for a template".  I don't see any evidence in the source tree that anything has changed on that front in the last two years.

I suspect trying to change absl to basic_string_view (and ensure google3 doesn't break) is not as good a use of our time as "ban absl::string_view except at library borders, continue to use StringPiece[16] until C++17 arrives, then switch to std::string_view and friends".  Though maybe I'm too pessimistic!

PK

[Peter Kasting]

On Wed, Jun 17, 2020 at 1:27 PM Gabriel Charette <g...@chromium.org> wrote:

On Tue, Jun 16, 2020 at 9:26 PM Peter Kasting <pkas...@chromium.org> wrote:

1. A high-level idea of how to do the roll-out.
• Proposal: Treat this similarly to a new C++ version, in that we start with most things default-banned, allow on a by-request basis, and aim to default-allow within two years.  Track in the Chromium style guide and chromium-cpp.appspot.com.

A key difference with C++ style I think is that absl also brings in a library of utils. Hence, some folks not tracking cxx@ likely need a say in whether we let some utilities in. e.g. I'd want //base/task/OWNERS to be looped in if threading utils are considered.

I think this is similar-in-concept to new C++ versions -- when either language or library features come in that interact with us, we have to determine how to accommodate them, and it's prudent to rope in area experts when doing so.  Certainly time-, threading-, and atomics-related stuff would all be areas I'd immediately want to rope in relevant owners.

PK 

[Peter Kasting]

On Wed, Jun 17, 2020 at 3:30 PM Daniel Cheng <dch...@chromium.org> wrote:

On Tue, Jun 16, 2020 at 9:14 PM Chris Palmer <pal...@chromium.org> wrote:

To me the key thing is that we not lose safety and correctness by preferring absl::foo to base::foo. We've put a lot of work into making base more resilient and secure — recently confirmed by Project Zero to have stopped some real attacks, by the way — and I *really* don't want for Chromium to regress.

Toward that end, keep in mind that we always need to build Abseil with its hardening mode turned on in Chromium. That's a minimum requirement.

In my dream world, we'd 'productize' as much of base as we could, re-designing it as we go and as/if necessary for ergonomic safety, and make it as easy as possible for any callers (inside or outside of Chromium) to call it. The world needs a safety-oriented C++ library that remains as efficient as possible, and base is low-key succeeding at that, IMO. AFAICT nobody else is attempting to do this, so there's an unmet need in the world.

I'd like to second this point: for absl things we do start using in //base, I'd want to make sure we have some smoke tests to ensure that the appropriate CHECKs are enabled. To me, this is especially important because my understanding is absl transparently maps through to the standard library version if it's recent enough (e.g. support for optional, string_view, variant). I want to make sure we don't end up in a situation where we had security hardening and then lost it by upgrading a new C++ version.

This is a good point.  @palmer: Do we currently have death tests that verify our hardening of different things?  If not, it seems like we should add them.

Regarding mapping through, you can toggle between "force std/force absl/auto-select"; see https://abseil.io/blog/201901115-options#read-more .  Thus I don't think this concern should gate, but I do think we need the tests so that we don't lose security absent-mindedly.

Proposal: Treat this similarly to a new C++ version, in that we start with most things default-banned, allow on a by-request basis, and aim to default-allow within two years.  Track in the Chromium style guide and chromium-cpp.appspot.com.

How often does absl change/add new things? I'm a bit hesitant to say that we should just default-allow in two years, depending on how often new concepts/functionality is introduced. It seems to me the most value comes from the container types, especially for interop with other third-party libraries that are using absl at library boundaries.

Looking at this more, I share your core concern.  It's a big concerning new functionality upstream adds gains significant usage in Chromium before we've had a chance to review-in-principle.  But since Abseil is less "versioned" than C++ itself, it's harder to guarantee that from both a linguistic perspective ("Abseil v3 is allowed" is not very meaningful) or a technical one (we can live on LTS branches instead of HEAD, but those are just basically cut-every-six-months).

For a steady-state goal, we have several choices I can think of:

1. "Laissez faire": Since Abseil is guided by principles and values closer to Chromium's than C++ is, assume that reactively disallowing problematic functionality as needed will be sufficient
2. "Proactive consideration": We monitor what goes into Abseil, discuss new additions periodically, and rule on them reasonably quickly 
3. "Constant vigilance": Only explicitly allowed functionality may ever be used (enforced by e.g. PRESUBMIT); using more requires an explicit request

Personally I'm OK with either 1 or 2; I don't think 3 is warranted.  If we do 2, we'll need to figure out who/what is responsible for said monitoring.

Proposal: Allow absl::any and absl::variant to start with, as these polyfill C++17 types, have no base:: implementation, and have consumer interest .  Allow other types and functions to begin with only as necessary to compile against third-party code using Abseil and convert to non-absl:: types.  Create a list of overlaps between base:: and absl:: (e.g. base::Optional vs. absl::optional, base::StringPiece vs. absl::string_view, base::Time and related classes vs. absl::Time and related classes, etc.) and decide a plan for deduplication going forward.  Bias towards std:: types, then absl:: types, then base:: types.

I can't say that I personally care for any, but maybe I don't understand the use cases. I know there's a CL that wants to use absl::any, but it's hard to tell exactly what the intended usage is (since it's a support library for third-party code).

In Chromium itself, usage would be rare, basically for type-erased stuff and "arbitrary property" sorts of cases (View class properties, WebContents user data, etc.).  It's convenient but not critical for these and I'm not wedded to having it early or at all, but any objections we'd make to absl::any will also apply to std::any in C++17, so it's probably less about discussing whether to take absl's version as whether we think the feature is risky enough that we should permanently ban it.

 

Finally, I'd like to add that span and absl's hash containers should probably be on this list: in particular, I'd really like to see absl's hash containers be the default recommendation (though we'll likely need to benchmark ARM).

I didn't put ::span on the initial list since base::span exists and I didn't want to start off with a list that overlaps at all (since any overlapping thing should have a long-term plan about where we're converging to), but yes, with things like ::span and ::optional where base, absl, and std all aim to implement the identical API, I think there's a solid case for using first absl and then std, modulo security check support.

Hash containers are one where I'd like to see the simplest, clearest guidance possible, and we already have enough containers that I'd want us to understand precisely how they should slot into that guidance before we allow them.  So they wouldn't be on my initial list either, but certainly worth the time to look at in more detail.

PK

[John Abd-El-Malek]

On Wed, Jun 17, 2020 at 4:45 PM Daniel Cheng <dch...@chromium.org> wrote:

For types that are redundant between //base and //absl, interoperating with third-party libraries that use absl is somewhat painful. An example of boilerplate that's needed is

https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/platform/peerconnection/webrtc_util.h;l=15;drc=8a030dd3c631e68b67926fff0839ff085d81bdd7, which is pretty unfortunate because it forces a bunch of copies.

I see, so a motivator is as more code that's rolled in via DEPS uses abseil, us not using it leads to more work at the edges.

 

I'm not sure if we should replace /everything/ (in particular, I'm skeptical of changing things like Time representations). However, the //base implementations of optional and span were intentionally designed to match the upstream standard library API as closely as possible to facilitate switching to the standard library (or absl) as appropriate.

Does this concern also apply to types which have no equivalent in base (absl's hash tables, variant, et cetera)?

I was wondering about things that have equivalents. If there's no equivalent, then it seems like less work to use the abseil one than to create a new one (as long as we can add bounds checks etc per Chris' email).

[Peter Kasting]

On Wed, Jun 17, 2020 at 4:34 PM John Abd-El-Malek <j...@chromium.org> wrote:

Can you provide more background on what is the motivation? I haven't seen a discussion of this on eng-review (only on allowing more third party dependencies to use it).

It's possible I misconstrued what I was told about eng-review's support for this!

Specifically, I'm wondering what's to gain from removing the things in base/ that have equivalents in abseil. Presumably what we have works, so is converting them to use something equivalent a good use of our limited resources?

To clarify, I think we should make case-by-case decisions, and conversion costs are one of the costs to consider.  (This is one of the reasons why, at least to date, we haven't pursued the STL time-related functions much.)  My "bias toward" comment was meant more as a tiebreaking function or "if reasonable to do so" rather than "unless it's completely impossible, do X".

In addition to saving work at the edges as Daniel mentioned, there are other potential benefits of changing to absl:: types:

• Binary size savings (details vary wildly); this may include followon effects for libraries like WebRTC that use or copy some/all of base:: but would like to use absl:: instead
• More likely familiar to external or google3 engineers who wish to contribute
• Saves ongoing maintenance cost for the replaced types (again, varies wildly, but for types meant to mirror the standard like ::span, matching the standard precisely requires nontrivial work, especially with changes in subsequent versions)
• Abseil is the Abseil team's primary responsibility; //base is worked on where necessary as a means to an end for us

However, for any given case, these may not outweigh the reasons to avoid conversion.  I'm skeptical offhand that using absl's Time and Duration would be a good choice, for example.

PK

[dan...@chromium.org]

On Wed, Jun 17, 2020 at 8:59 PM Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Jun 17, 2020 at 3:30 PM Daniel Cheng <dch...@chromium.org> wrote:

On Tue, Jun 16, 2020 at 9:14 PM Chris Palmer <pal...@chromium.org> wrote:

To me the key thing is that we not lose safety and correctness by preferring absl::foo to base::foo. We've put a lot of work into making base more resilient and secure — recently confirmed by Project Zero to have stopped some real attacks, by the way — and I *really* don't want for Chromium to regress.

Toward that end, keep in mind that we always need to build Abseil with its hardening mode turned on in Chromium. That's a minimum requirement.

In my dream world, we'd 'productize' as much of base as we could, re-designing it as we go and as/if necessary for ergonomic safety, and make it as easy as possible for any callers (inside or outside of Chromium) to call it. The world needs a safety-oriented C++ library that remains as efficient as possible, and base is low-key succeeding at that, IMO. AFAICT nobody else is attempting to do this, so there's an unmet need in the world.

I'd like to second this point: for absl things we do start using in //base, I'd want to make sure we have some smoke tests to ensure that the appropriate CHECKs are enabled. To me, this is especially important because my understanding is absl transparently maps through to the standard library version if it's recent enough (e.g. support for optional, string_view, variant). I want to make sure we don't end up in a situation where we had security hardening and then lost it by upgrading a new C++ version.

This is a good point.  @palmer: Do we currently have death tests that verify our hardening of different things?  If not, it seems like we should add them.

Regarding mapping through, you can toggle between "force std/force absl/auto-select"; see https://abseil.io/blog/201901115-options#read-more .  Thus I don't think this concern should gate, but I do think we need the tests so that we don't lose security absent-mindedly.

Proposal: Treat this similarly to a new C++ version, in that we start with most things default-banned, allow on a by-request basis, and aim to default-allow within two years.  Track in the Chromium style guide and chromium-cpp.appspot.com.

How often does absl change/add new things? I'm a bit hesitant to say that we should just default-allow in two years, depending on how often new concepts/functionality is introduced. It seems to me the most value comes from the container types, especially for interop with other third-party libraries that are using absl at library boundaries.

Looking at this more, I share your core concern.  It's a big concerning new functionality upstream adds gains significant usage in Chromium before we've had a chance to review-in-principle.  But since Abseil is less "versioned" than C++ itself, it's harder to guarantee that from both a linguistic perspective ("Abseil v3 is allowed" is not very meaningful) or a technical one (we can live on LTS branches instead of HEAD, but those are just basically cut-every-six-months).

For a steady-state goal, we have several choices I can think of:

1. "Laissez faire": Since Abseil is guided by principles and values closer to Chromium's than C++ is, assume that reactively disallowing problematic functionality as needed will be sufficient
2. "Proactive consideration": We monitor what goes into Abseil, discuss new additions periodically, and rule on them reasonably quickly 
3. "Constant vigilance": Only explicitly allowed functionality may ever be used (enforced by e.g. PRESUBMIT); using more requires an explicit request

For C++11 we used a website to determine what was allowed and many people today don't even know that the website exists. I would choose to crowdsource this monitoring and encode the allowlist directly into our codebase with PRESUBMIT, i.e. #3 here.

I would also not have any initial list of allowed things, I would set up the PRESUBMIT etc, then start allowing things via discussion threads similar to how we did the C++11 rollout. As pre-req to allow things I would consider this algorithm:

Is there a base equivalent?

If yes:

1. Can we alias it to absl at the time that it is allowed? Can we get to that point before allowing it then? If we can't then err toward disallowing absl's version.

2. Are there CHECKs in the base implementation? If so, are there death tests? Ensure they are covered and that when aliasing the base version to absl, the death tests continue to pass.

 

Personally I'm OK with either 1 or 2; I don't think 3 is warranted.  If we do 2, we'll need to figure out who/what is responsible for said monitoring.

Proposal: Allow absl::any and absl::variant to start with, as these polyfill C++17 types, have no base:: implementation, and have consumer interest .  Allow other types and functions to begin with only as necessary to compile against third-party code using Abseil and convert to non-absl:: types.  Create a list of overlaps between base:: and absl:: (e.g. base::Optional vs. absl::optional, base::StringPiece vs. absl::string_view, base::Time and related classes vs. absl::Time and related classes, etc.) and decide a plan for deduplication going forward.  Bias towards std:: types, then absl:: types, then base:: types.

I can't say that I personally care for any, but maybe I don't understand the use cases. I know there's a CL that wants to use absl::any, but it's hard to tell exactly what the intended usage is (since it's a support library for third-party code).

In Chromium itself, usage would be rare,

I have found statements like this to be too optimistic in the past. If we want it to be rare then we should allowlist specific places to use it IMO.

 

basically for type-erased stuff and "arbitrary property" sorts of cases (View class properties, WebContents user data, etc.).  It's convenient but not critical for these and I'm not wedded to having it early or at all, but any objections we'd make to absl::any will also apply to std::any in C++17, so it's probably less about discussing whether to take absl's version as whether we think the feature is risky enough that we should permanently ban it.

 

Finally, I'd like to add that span and absl's hash containers should probably be on this list: in particular, I'd really like to see absl's hash containers be the default recommendation (though we'll likely need to benchmark ARM).

I didn't put ::span on the initial list since base::span exists and I didn't want to start off with a list that overlaps at all (since any overlapping thing should have a long-term plan about where we're converging to), but yes, with things like ::span and ::optional where base, absl, and std all aim to implement the identical API, I think there's a solid case for using first absl and then std, modulo security check support.

Hash containers are one where I'd like to see the simplest, clearest guidance possible, and we already have enough containers that I'd want us to understand precisely how they should slot into that guidance before we allow them.  So they wouldn't be on my initial list either, but certainly worth the time to look at in more detail.

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/CAAHOzFCY3BEOtBa2CO%2Brtm7WpxxHJiQtxfF1d_HAQehuH2ijxw%40mail.gmail.com.

[dan...@chromium.org]

On Wed, Jun 17, 2020 at 9:02 PM John Abd-El-Malek <j...@chromium.org> wrote:

On Wed, Jun 17, 2020 at 4:45 PM Daniel Cheng <dch...@chromium.org> wrote:

For types that are redundant between //base and //absl, interoperating with third-party libraries that use absl is somewhat painful. An example of boilerplate that's needed is

https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/platform/peerconnection/webrtc_util.h;l=15;drc=8a030dd3c631e68b67926fff0839ff085d81bdd7, which is pretty unfortunate because it forces a bunch of copies.

I see, so a motivator is as more code that's rolled in via DEPS uses abseil, us not using it leads to more work at the edges.

Speaking specifically for optional, keeping our implementation up to date with std::experimental::optional has been a fair bit of work, and it'd be nice to outsource that to absl. :)

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

[Mirko Bonadei]

On Thursday, June 18, 2020 at 2:58:58 AM UTC+2 Peter Kasting wrote:

On Wed, Jun 17, 2020 at 3:30 PM Daniel Cheng <dch...@chromium.org> wrote:

On Tue, Jun 16, 2020 at 9:14 PM Chris Palmer <pal...@chromium.org> wrote:

To me the key thing is that we not lose safety and correctness by preferring absl::foo to base::foo. We've put a lot of work into making base more resilient and secure — recently confirmed by Project Zero to have stopped some real attacks, by the way — and I *really* don't want for Chromium to regress.

Toward that end, keep in mind that we always need to build Abseil with its hardening mode turned on in Chromium. That's a minimum requirement.

In my dream world, we'd 'productize' as much of base as we could, re-designing it as we go and as/if necessary for ergonomic safety, and make it as easy as possible for any callers (inside or outside of Chromium) to call it. The world needs a safety-oriented C++ library that remains as efficient as possible, and base is low-key succeeding at that, IMO. AFAICT nobody else is attempting to do this, so there's an unmet need in the world.

I'd like to second this point: for absl things we do start using in //base, I'd want to make sure we have some smoke tests to ensure that the appropriate CHECKs are enabled. To me, this is especially important because my understanding is absl transparently maps through to the standard library version if it's recent enough (e.g. support for optional, string_view, variant). I want to make sure we don't end up in a situation where we had security hardening and then lost it by upgrading a new C++ version.

This is a good point.  @palmer: Do we currently have death tests that verify our hardening of different things?  If not, it seems like we should add them.

Regarding mapping through, you can toggle between "force std/force absl/auto-select"; see https://abseil.io/blog/201901115-options#read-more .  Thus I don't think this concern should gate, but I do think we need the tests so that we don't lose security absent-mindedly.

Yes, this is super important. I have created https://bugs.chromium.org/p/chromium/issues/detail?id=1096623 to address this concern.

 

Proposal: Treat this similarly to a new C++ version, in that we start with most things default-banned, allow on a by-request basis, and aim to default-allow within two years.  Track in the Chromium style guide and chromium-cpp.appspot.com.

How often does absl change/add new things? I'm a bit hesitant to say that we should just default-allow in two years, depending on how often new concepts/functionality is introduced. It seems to me the most value comes from the container types, especially for interop with other third-party libraries that are using absl at library boundaries.

Looking at this more, I share your core concern.  It's a big concerning new functionality upstream adds gains significant usage in Chromium before we've had a chance to review-in-principle.  But since Abseil is less "versioned" than C++ itself, it's harder to guarantee that from both a linguistic perspective ("Abseil v3 is allowed" is not very meaningful) or a technical one (we can live on LTS branches instead of HEAD, but those are just basically cut-every-six-months).

For a steady-state goal, we have several choices I can think of:

1. "Laissez faire": Since Abseil is guided by principles and values closer to Chromium's than C++ is, assume that reactively disallowing problematic functionality as needed will be sufficient
2. "Proactive consideration": We monitor what goes into Abseil, discuss new additions periodically, and rule on them reasonably quickly 
3. "Constant vigilance": Only explicitly allowed functionality may ever be used (enforced by e.g. PRESUBMIT); using more requires an explicit request

Personally I'm OK with either 1 or 2; I don't think 3 is warranted.  If we do 2, we'll need to figure out who/what is responsible for said monitoring.

We (me and danilchap@) roll Abseil in Chromium every week and we have just scheduled a recurring monthly meeting with the Abseil team so for the steady state we should be able to do 2 (talk about new features earlier and notify cxx@ when they roll).

[Jan Wilken Dörrie]

base::span (and to some extent base::StringPiece) are in an interesting position where they are closer to the std:: equivalents than the absl:: ones (and also happen to have security checks). Thus aliasing base::span to absl::Span and base::StringPiece to absl::string_view would require a lot of work or is downright impossible. In these cases I therefore vote to keep the base versions.

However, we can still add implicit conversions between the base and absl versions to alleviate pain at 3rd party API boundaries. Considering that both span and StringPiece are cheap view types this wouldn't cause expensive copies.

We can of course also revisit this once abseil is closer to the C++ standard.

 

Hash containers are one where I'd like to see the simplest, clearest guidance possible, and we already have enough containers that I'd want us to understand precisely how they should slot into that guidance before we allow them.  So they wouldn't be on my initial list either, but certainly worth the time to look at in more detail.

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/CAAHOzFCY3BEOtBa2CO%2Brtm7WpxxHJiQtxfF1d_HAQehuH2ijxw%40mail.gmail.com.

[Karl Wiberg]

On Mon, Jun 22, 2020 at 3:59 PM Jan Wilken Dörrie <jdoe...@chromium.org> wrote:

On Thu, Jun 18, 2020 at 2:59 AM Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Jun 17, 2020 at 3:30 PM Daniel Cheng <dch...@chromium.org> wrote:

On Tue, Jun 16, 2020 at 9:14 PM Chris Palmer <pal...@chromium.org> wrote:

To me the key thing is that we not lose safety and correctness by preferring absl::foo to base::foo. We've put a lot of work into making base more resilient and secure — recently confirmed by Project Zero to have stopped some real attacks, by the way — and I *really* don't want for Chromium to regress.

Toward that end, keep in mind that we always need to build Abseil with its hardening mode turned on in Chromium. That's a minimum requirement.

In my dream world, we'd 'productize' as much of base as we could, re-designing it as we go and as/if necessary for ergonomic safety, and make it as easy as possible for any callers (inside or outside of Chromium) to call it. The world needs a safety-oriented C++ library that remains as efficient as possible, and base is low-key succeeding at that, IMO. AFAICT nobody else is attempting to do this, so there's an unmet need in the world.

I'd like to second this point: for absl things we do start using in //base, I'd want to make sure we have some smoke tests to ensure that the appropriate CHECKs are enabled. To me, this is especially important because my understanding is absl transparently maps through to the standard library version if it's recent enough (e.g. support for optional, string_view, variant). I want to make sure we don't end up in a situation where we had security hardening and then lost it by upgrading a new C++ version.

This is a good point.  @palmer: Do we currently have death tests that verify our hardening of different things?  If not, it seems like we should add them.

Regarding mapping through, you can toggle between "force std/force absl/auto-select"; see https://abseil.io/blog/201901115-options#read-more .  Thus I don't think this concern should gate, but I do think we need the tests so that we don't lose security absent-mindedly.

Proposal: Treat this similarly to a new C++ version, in that we start with most things default-banned, allow on a by-request basis, and aim to default-allow within two years.  Track in the Chromium style guide and chromium-cpp.appspot.com.

How often does absl change/add new things? I'm a bit hesitant to say that we should just default-allow in two years, depending on how often new concepts/functionality is introduced. It seems to me the most value comes from the container types, especially for interop with other third-party libraries that are using absl at library boundaries.

Looking at this more, I share your core concern.  It's a big concerning new functionality upstream adds gains significant usage in Chromium before we've had a chance to review-in-principle.  But since Abseil is less "versioned" than C++ itself, it's harder to guarantee that from both a linguistic perspective ("Abseil v3 is allowed" is not very meaningful) or a technical one (we can live on LTS branches instead of HEAD, but those are just basically cut-every-six-months).

For a steady-state goal, we have several choices I can think of:

1. "Laissez faire": Since Abseil is guided by principles and values closer to Chromium's than C++ is, assume that reactively disallowing problematic functionality as needed will be sufficient
2. "Proactive consideration": We monitor what goes into Abseil, discuss new additions periodically, and rule on them reasonably quickly 
3. "Constant vigilance": Only explicitly allowed functionality may ever be used (enforced by e.g. PRESUBMIT); using more requires an explicit request

Personally I'm OK with either 1 or 2; I don't think 3 is warranted.  If we do 2, we'll need to figure out who/what is responsible for said monitoring.

Proposal: Allow absl::any and absl::variant to start with, as these polyfill C++17 types, have no base:: implementation, and have consumer interest .  Allow other types and functions to begin with only as necessary to compile against third-party code using Abseil and convert to non-absl:: types.  Create a list of overlaps between base:: and absl:: (e.g. base::Optional vs. absl::optional, base::StringPiece vs. absl::string_view, base::Time and related classes vs. absl::Time and related classes, etc.) and decide a plan for deduplication going forward.  Bias towards std:: types, then absl:: types, then base:: types.

I can't say that I personally care for any, but maybe I don't understand the use cases. I know there's a CL that wants to use absl::any, but it's hard to tell exactly what the intended usage is (since it's a support library for third-party code).

In Chromium itself, usage would be rare, basically for type-erased stuff and "arbitrary property" sorts of cases (View class properties, WebContents user data, etc.).  It's convenient but not critical for these and I'm not wedded to having it early or at all, but any objections we'd make to absl::any will also apply to std::any in C++17, so it's probably less about discussing whether to take absl's version as whether we think the feature is risky enough that we should permanently ban it.

 

Finally, I'd like to add that span and absl's hash containers should probably be on this list: in particular, I'd really like to see absl's hash containers be the default recommendation (though we'll likely need to benchmark ARM).

I didn't put ::span on the initial list since base::span exists and I didn't want to start off with a list that overlaps at all (since any overlapping thing should have a long-term plan about where we're converging to), but yes, with things like ::span and ::optional where base, absl, and std all aim to implement the identical API, I think there's a solid case for using first absl and then std, modulo security check support.

base::span (and to some extent base::StringPiece) are in an interesting position where they are closer to the std:: equivalents than the absl:: ones (and also happen to have security checks). Thus aliasing base::span to absl::Span and base::StringPiece to absl::string_view would require a lot of work or is downright impossible. In these cases I therefore vote to keep the base versions.

In WebRTC, we decided to not replace our span type rtc::ArrayView with absl::Span for precisely this reason. We didn't want to have to do migrations with behavior changes twice, first to absl::Span and then to C++20 std::span, so our plan was (and still is) to just wait and see what Abseil decides to do w.r.t. the absl::Span—std::span incompatibility. https://webrtc.googlesource.com/src/+/HEAD/abseil-in-webrtc.md

However, we can still add implicit conversions between the base and absl versions to alleviate pain at 3rd party API boundaries. Considering that both span and StringPiece are cheap view types this wouldn't cause expensive copies.

We can of course also revisit this once abseil is closer to the C++ standard.

 

Hash containers are one where I'd like to see the simplest, clearest guidance possible, and we already have enough containers that I'd want us to understand precisely how they should slot into that guidance before we allow them.  So they wouldn't be on my initial list either, but certainly worth the time to look at in more detail.

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/CAAHOzFCY3BEOtBa2CO%2Brtm7WpxxHJiQtxfF1d_HAQehuH2ijxw%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/CALB5StYbk_vVJE%2BXaN5-eBw_7t3tjHUB7uCtsdU1wgC2g73eoQ%40mail.gmail.com.

--

Karl Wiberg

Software Engineer

kwi...@google.com

+46 70 696 1024

[Peter Kasting]

Summary of where I believe things stand; please correct misimpressions/omissions:

1. Agreement in principle to do this
• No unresolved objections; resolved in favor pending details below.  Object now if you feel otherwise, as there's a separate thread on this list now requesting approval for a specific first-party use.
2. A high-level idea of how to roll out
• General concurrence for starting with most/all things banned and allowing gradually.
• Long-term policy cannot be "default-allow" as absl adds new capabilities and there's insufficient versioning to say "absl17 is allowed" as we do with C++.
• Proposal: Use same policy as with C++ ("force decision by two years"), but applied to individual pieces of absl.  As new functionality is rolled in, add it to our canonical list, default-banned, with date added; that's the start date for the two year timeframe.  The start date for all existing functionality is whenever we create this list.  Enforce all bans with PRESUBMIT.  mbonadei@/danilchap@ to establish procedures for ensuring absl is rolled in a timely fashion, new functionality is added to the default-banned list, PRESUBMIT checks are added, and cxx@ is notified if necessary.
• No significant discussion on using chromium-cpp.appspot.com to hold the canonical list.  I assume this is OK.
3. Specific details
• Agreement that absl usage must not regress security checks, and we should have death tests for these.  Bug on file, WIP.
• Some discussion of base:: and absl:: overlaps.
• Proposal: If base:: and absl:: functionality overlap, do not allow the absl functionality without a plan to migrate usage.  The standard plan is to migrate the base:: functionality to match absl, type-alias to verify, allow absl usage, and remove the base:: type.  Ideally this should not move us farther away from std:: compliance.  //base OWNERS to establish procedures for determining and documenting overlaps.
• Insufficient discussion of usage at third-party code boundaries.
• Proposal: All banned functionality is allowed at third-party code boundaries, but only as minimally necessary to compile and link; data must be converted to allowed types/containers for any further processing.
• Open question: Do people just ignore PRESUBMIT warnings for that then?
• Open question: Might there be things so unsafe we cannot even allow this level of usage?
• Significant discussion of specific pieces.
• Known problems:
• absl::string_view cannot replace base::StringPiece16
• absl::any will not work correctly in component builds
• absl::span is less std::-compliant than base::span
• Explicit interest expressed:
• absl::variant
• absl::optional
• hash containers
• Proposal: Once the above general procedures are in place, propose allowing absl::variant.  Some //base OWNER (danakj@?) to propose allowing absl::optional, with migration plan.  dcheng@ to propose allowing hash containers, with sufficient details about how these fit with our existing containers, usage guidance, performance metrics, etc.  Propose formally banning all three "known problems" cases above until those problems are addressed.

I will wait at least a week before considering any of the proposals above to be accepted.

PK

[dan...@chromium.org]

On Fri, Jun 26, 2020 at 11:26 AM Peter Kasting <pkas...@chromium.org> wrote:

Summary of where I believe things stand; please correct misimpressions/omissions:

1. Agreement in principle to do this
• No unresolved objections; resolved in favor pending details below.  Object now if you feel otherwise, as there's a separate thread on this list now requesting approval for a specific first-party use.
2. A high-level idea of how to roll out
• General concurrence for starting with most/all things banned and allowing gradually.
• Long-term policy cannot be "default-allow" as absl adds new capabilities and there's insufficient versioning to say "absl17 is allowed" as we do with C++.
• Proposal: Use same policy as with C++ ("force decision by two years"), but applied to individual pieces of absl.  As new functionality is rolled in, add it to our canonical list, default-banned, with date added; that's the start date for the two year timeframe.  The start date for all existing functionality is whenever we create this list.  Enforce all bans with PRESUBMIT.  mbonadei@/danilchap@ to establish procedures for ensuring absl is rolled in a timely fashion, new functionality is added to the default-banned list, PRESUBMIT checks are added, and cxx@ is notified if necessary.
• No significant discussion on using chromium-cpp.appspot.com to hold the canonical list.  I assume this is OK.
3. Specific details
• Agreement that absl usage must not regress security checks, and we should have death tests for these.  Bug on file, WIP.
• Some discussion of base:: and absl:: overlaps.
• Proposal: If base:: and absl:: functionality overlap, do not allow the absl functionality without a plan to migrate usage.  The standard plan is to migrate the base:: functionality to match absl, type-alias to verify, allow absl usage, and remove the base:: type.  Ideally this should not move us farther away from std:: compliance.  //base OWNERS to establish procedures for determining and documenting overlaps.
• Insufficient discussion of usage at third-party code boundaries.
• Proposal: All banned functionality is allowed at third-party code boundaries, but only as minimally necessary to compile and link; data must be converted to allowed types/containers for any further processing.
• Open question: Do people just ignore PRESUBMIT warnings for that then?

I don't think we should ever ignore warnings. We should have an allowlist to files that do conversions out of third party. 

• Open question: Might there be things so unsafe we cannot even allow this level of usage?

If they are then they are unsafe to bring into third_party as well, correct? I don't think we have any real intention to monitor absl usage inside third_party deps in that way, do we? 

• Significant discussion of specific pieces.
• Known problems:
• absl::string_view cannot replace base::StringPiece16
• absl::any will not work correctly in component builds
• absl::span is less std::-compliant than base::span
• Explicit interest expressed:
• absl::variant
• absl::optional
• hash containers
• Proposal: Once the above general procedures are in place, propose allowing absl::variant.  Some //base OWNER (danakj@?) to propose allowing absl::optional, with migration plan.  dcheng@ to propose allowing hash containers, with sufficient details about how these fit with our existing containers, usage guidance, performance metrics, etc.  Propose formally banning all three "known problems" cases above until those problems are addressed.

I will wait at least a week before considering any of the proposals above to be accepted.

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/CAAHOzFD30xvsYi2ot0s43LwEee%2BrOFXWo%3DrsUcuekp%3DHgiirFA%40mail.gmail.com.

[Peter Kasting]

On Fri, Jun 26, 2020 at 9:51 AM <dan...@chromium.org> wrote:

On Fri, Jun 26, 2020 at 11:26 AM Peter Kasting <pkas...@chromium.org> wrote:

Summary of where I believe things stand; please correct misimpressions/omissions:

1. Specific details
   

• Insufficient discussion of usage at third-party code boundaries.
  
• Proposal: All banned functionality is allowed at third-party code boundaries, but only as minimally necessary to compile and link; data must be converted to allowed types/containers for any further processing.
• Open question: Do people just ignore PRESUBMIT warnings for that then?

I don't think we should ever ignore warnings. We should have an allowlist to files that do conversions out of third party.

So for example, in https://chromium-review.googlesource.com/c/chromium/src/+/2228543 ,

count_down_latch*, scheduled_executor*, system_clock.cc depend on absl::Time/Duration/etc.

crypto.cc depends on absl::string_view

Would this directory have a PRESUBMIT.py that basically allows these specific dependencies in these specific files?  Would we instead record them somewhere in a metadata file a la DEPS?  Would we just whitelist this directory?

• Open question: Might there be things so unsafe we cannot even allow this level of usage?

If they are then they are unsafe to bring into third_party as well, correct? I don't think we have any real intention to monitor absl usage inside third_party deps in that way, do we?

Yeah, it's worrisome.  One of my concerns is e.g. if absl::any were to not get fixed to work correctly in the component build, and third_party code used it, we could see arbitrary, hard-to-detect bustage in component builds.  That seems bad, but I'm not sure what to do here?

PK

[dan...@chromium.org]

On Fri, Jun 26, 2020 at 1:28 PM Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Jun 26, 2020 at 9:51 AM <dan...@chromium.org> wrote:

On Fri, Jun 26, 2020 at 11:26 AM Peter Kasting <pkas...@chromium.org> wrote:

Summary of where I believe things stand; please correct misimpressions/omissions:

1. Specific details
   
• Insufficient discussion of usage at third-party code boundaries.
  
• Proposal: All banned functionality is allowed at third-party code boundaries, but only as minimally necessary to compile and link; data must be converted to allowed types/containers for any further processing.
• Open question: Do people just ignore PRESUBMIT warnings for that then?

I don't think we should ever ignore warnings. We should have an allowlist to files that do conversions out of third party.

So for example, in https://chromium-review.googlesource.com/c/chromium/src/+/2228543 ,

count_down_latch*, scheduled_executor*, system_clock.cc depend on absl::Time/Duration/etc.

crypto.cc depends on absl::string_view

Would this directory have a PRESUBMIT.py that basically allows these specific dependencies in these specific files?  Would we instead record them somewhere in a metadata file a la DEPS?  Would we just whitelist this directory?

I'd suggest listing each file, and the allowlist means we don't check absl:: usage inside it at all (or IOW treat it like third_party/, whatever we decide to do there, which I currently assume is nothing.. )

 

• Open question: Might there be things so unsafe we cannot even allow this level of usage?

If they are then they are unsafe to bring into third_party as well, correct? I don't think we have any real intention to monitor absl usage inside third_party deps in that way, do we?

Yeah, it's worrisome.  One of my concerns is e.g. if absl::any were to not get fixed to work correctly in the component build, and third_party code used it, we could see arbitrary, hard-to-detect bustage in component builds.  That seems bad, but I'm not sure what to do here?

I am not sure either. We can ban some things in there, and prevent rolls if they are present, if PRESUBMIT checks things on a roll - I am not sure it would since those files aren't part of the commit?

 

PK

[Mirko Bonadei]

Thanks Peter! Sorry for the late reply but I wanted to address some issues with the absl component build support before focusing on this. Reply below.

On Friday, June 26, 2020 at 5:26:25 PM UTC+2 Peter Kasting wrote:

Summary of where I believe things stand; please correct misimpressions/omissions:

1. Agreement in principle to do this
   • No unresolved objections; resolved in favor pending details below.  Object now if you feel otherwise, as there's a separate thread on this list now requesting approval for a specific first-party use.
2. A high-level idea of how to roll out
   • General concurrence for starting with most/all things banned and allowing gradually.
   • Long-term policy cannot be "default-allow" as absl adds new capabilities and there's insufficient versioning to say "absl17 is allowed" as we do with C++.
     • Proposal: Use same policy as with C++ ("force decision by two years"), but applied to individual pieces of absl.  As new functionality is rolled in, add it to our canonical list, default-banned, with date added; that's the start date for the two year timeframe.  The start date for all existing functionality is whenever we create this list.  Enforce all bans with PRESUBMIT.  mbonadei@/danilchap@ to establish procedures for ensuring absl is rolled in a timely fashion, new functionality is added to the default-banned list, PRESUBMIT checks are added, and cxx@ is notified if necessary.

This sounds good to me. We are already rolling //third_party/abseil-cpp weekly and holding monthly meeting with Abseil. The only thing to do is create a list and have a procedure to keep it up to date and notify cxx@ in case we roll a new functionality. For the ban, I propose to use https://source.chromium.org/chromium/chromium/src/+/master:buildtools/checkdeps/ and keep "-absl" in the main DEPS file. When the functionality will be allowed just add a "+absl/..." to explicitly list allowed headers (we do something similar in WebRTC).

• No significant discussion on using chromium-cpp.appspot.com to hold the canonical list.  I assume this is OK.

Sounds good to me.

1. Specific details
   • Agreement that absl usage must not regress security checks, and we should have death tests for these.  Bug on file, WIP.

This is now done (crbug.com/1096623). 

• Some discussion of base:: and absl:: overlaps.
  • Proposal: If base:: and absl:: functionality overlap, do not allow the absl functionality without a plan to migrate usage.  The standard plan is to migrate the base:: functionality to match absl, type-alias to verify, allow absl usage, and remove the base:: type.  Ideally this should not move us farther away from std:: compliance.  //base OWNERS to establish procedures for determining and documenting overlaps.
• Insufficient discussion of usage at third-party code boundaries.
  • Proposal: All banned functionality is allowed at third-party code boundaries, but only as minimally necessary to compile and link; data must be converted to allowed types/containers for any further processing.
    • Open question: Do people just ignore PRESUBMIT warnings for that then?

I propose they add a DEPS file in project subfolder where they need to #include the banned header and explicitly list it with a comment about using the absl feature only to use the third_party library.

If I understand correctly, third_party libraries don't have any ban on absl usage. Is this correct?  (see next point, related).

• Open question: Might there be things so unsafe we cannot even allow this level of usage?

I think absl::any (and probably std::any) needs to be banned also in third_party code because they don't work in component builds without RTTI. Should we go one step further with absl::any and ban it also from third_party?

[Mirko Bonadei]

On Friday, June 26, 2020 at 6:51:14 PM UTC+2 danakj wrote:

On Fri, Jun 26, 2020 at 11:26 AM Peter Kasting <pkas...@chromium.org> wrote:

Summary of where I believe things stand; please correct misimpressions/omissions:

1. Agreement in principle to do this
   • No unresolved objections; resolved in favor pending details below.  Object now if you feel otherwise, as there's a separate thread on this list now requesting approval for a specific first-party use.
2. A high-level idea of how to roll out
   • General concurrence for starting with most/all things banned and allowing gradually.
   • Long-term policy cannot be "default-allow" as absl adds new capabilities and there's insufficient versioning to say "absl17 is allowed" as we do with C++.
     • Proposal: Use same policy as with C++ ("force decision by two years"), but applied to individual pieces of absl.  As new functionality is rolled in, add it to our canonical list, default-banned, with date added; that's the start date for the two year timeframe.  The start date for all existing functionality is whenever we create this list.  Enforce all bans with PRESUBMIT.  mbonadei@/danilchap@ to establish procedures for ensuring absl is rolled in a timely fashion, new functionality is added to the default-banned list, PRESUBMIT checks are added, and cxx@ is notified if necessary.
   • No significant discussion on using chromium-cpp.appspot.com to hold the canonical list.  I assume this is OK.
3. Specific details
   • Agreement that absl usage must not regress security checks, and we should have death tests for these.  Bug on file, WIP.
   • Some discussion of base:: and absl:: overlaps.
     • Proposal: If base:: and absl:: functionality overlap, do not allow the absl functionality without a plan to migrate usage.  The standard plan is to migrate the base:: functionality to match absl, type-alias to verify, allow absl usage, and remove the base:: type.  Ideally this should not move us farther away from std:: compliance.  //base OWNERS to establish procedures for determining and documenting overlaps.
   • Insufficient discussion of usage at third-party code boundaries.
     • Proposal: All banned functionality is allowed at third-party code boundaries, but only as minimally necessary to compile and link; data must be converted to allowed types/containers for any further processing.
       • Open question: Do people just ignore PRESUBMIT warnings for that then?

I don't think we should ever ignore warnings. We should have an allowlist to files that do conversions out of third party. 

• Open question: Might there be things so unsafe we cannot even allow this level of usage?

If they are then they are unsafe to bring into third_party as well, correct? I don't think we have any real intention to monitor absl usage inside third_party deps in that way, do we? 

Correct. I think we should for some functionalities and probably this should be done beyond absl itself. For example, if a third_party library has a type that breaks component builds, I think we should avoid using it or we propose a change. 

[Mirko Bonadei]

On Friday, June 26, 2020 at 7:28:19 PM UTC+2 Peter Kasting wrote:

On Fri, Jun 26, 2020 at 9:51 AM <dan...@chromium.org> wrote:

On Fri, Jun 26, 2020 at 11:26 AM Peter Kasting <pkas...@chromium.org> wrote:

Summary of where I believe things stand; please correct misimpressions/omissions:

1. Specific details
   

• Insufficient discussion of usage at third-party code boundaries.
  
  • Proposal: All banned functionality is allowed at third-party code boundaries, but only as minimally necessary to compile and link; data must be converted to allowed types/containers for any further processing.
    • Open question: Do people just ignore PRESUBMIT warnings for that then?

I don't think we should ever ignore warnings. We should have an allowlist to files that do conversions out of third party.

So for example, in https://chromium-review.googlesource.com/c/chromium/src/+/2228543 ,

count_down_latch*, scheduled_executor*, system_clock.cc depend on absl::Time/Duration/etc.

crypto.cc depends on absl::string_view

Would this directory have a PRESUBMIT.py that basically allows these specific dependencies in these specific files?  Would we instead record them somewhere in a metadata file a la DEPS?  Would we just whitelist this directory?

DEPS's include_rules should work well here, see https://chromium-review.googlesource.com/c/chromium/src/+/2228543/46/chrome/services/sharing/nearby/DEPS, I am going to comment to list the headers explicitly instead of doing "+absl". 

[Mirko Bonadei]

Ah, rolls. I forgot about them, in that case checkdeps will not detect it. But the BUILD.gn file will still have to depend on //third_party/abseil-cpp:absl (visibility doesn't allow to depend directly on source_sets) and if the banned functionality is not in the absl component the library owner will try to add it and we should be able to detect the dependency on a banned functionality. I would prefer something more strict though.

 

 

PK

[Peter Kasting]

On Fri, Jul 10, 2020 at 12:43 PM Mirko Bonadei <mbon...@chromium.org> wrote:

• Open question: Might there be things so unsafe we cannot even allow this level of usage?

I think absl::any (and probably std::any) needs to be banned also in third_party code because they don't work in component builds without RTTI. Should we go one step further with absl::any and ban it also from third_party?

So it works in component builds if you enable RTTI?  Is that because they try to use RTTI first and only fall back to (whatever the thing that doesn't work in component builds is) when that's unavailable?

I worry about whether there are other constructs that might quietly break without RTTI that we don't know about.  Should we try to build third_party (non-Blink) code with RTTI enabled (maybe just for component builds)?  Can we somehow compile-fail in places where someone is trying to do a fallback like this?

PK

[Mirko Bonadei]

On Friday, July 10, 2020 at 9:56:51 PM UTC+2 Peter Kasting wrote:

On Fri, Jul 10, 2020 at 12:43 PM Mirko Bonadei <mbon...@chromium.org> wrote:

• Open question: Might there be things so unsafe we cannot even allow this level of usage?

I think absl::any (and probably std::any) needs to be banned also in third_party code because they don't work in component builds without RTTI. Should we go one step further with absl::any and ban it also from third_party?

So it works in component builds if you enable RTTI?  Is that because they try to use RTTI first and only fall back to (whatever the thing that doesn't work in component builds is) when that's unavailable?

Yes, compiling with -frtti solves the absl::any issue in component build. Basically without RTTI there is no typeid() so it falls back on the comparison of a template class data member address but in different shared libraries the same type will be detected as a different type because it will have a different address. Here the absl code: https://source.chromium.org/chromium/chromium/src/+/master:third_party/abseil-cpp/absl/base/internal/fast_type_id.h;l=26-42;drc=43864667272f140c8d3a9dcce1dc1fbb260e3c74, used here: https://source.chromium.org/chromium/chromium/src/+/master:third_party/abseil-cpp/absl/types/any.h;l=407;drc=2a208ffb896a0a0559fec57573f1c2a6ae7ffd6f.

But also std::any has something like that when RTTI is not enabled: https://source.chromium.org/chromium/chromium/src/+/master:buildtools/third_party/libc++/trunk/include/any;l=153-174;drc=eae6ef923a8f3bc8948abdfa8f6b6ae06544b8a2.

 

I worry about whether there are other constructs that might quietly break without RTTI that we don't know about.  Should we try to build third_party (non-Blink) code with RTTI enabled (maybe just for component builds)?  Can we somehow compile-fail in places where someone is trying to do a fallback like this?

As far as I know this is the only one that can fail in a silent mode. I have asked Abseil and there are no current plans for further usage but they might in the future. As of today they use it for absl::any, absl/flags (not used by Chromium) and absl/random. So these should be transitively banned unless there is a workaround (I have opened a bug to see if there is another way to simulate RTTI). 

> Should we try to build third_party (non-Blink) code with RTTI enabled (maybe just for component builds)? 

I think we should not do this because enabling RTTI only on a part of the build graph might lead to other types of issues.

> Can we somehow compile-fail in places where someone is trying to do a fallback like this?

Yes, we can probably carry a patch in the Chromium absl and compile fail when FastTypeId is used (only in Chromium, because WebRTC uses absl/flags for some standalone binaries).

 

PK

[Peter Kasting]

Updated detailed proposal below.

1. Agreement in principle: Resolved; first-party use of Abseil is directionally approved
2. Roll-out idea:
• chromium-cpp.appspot.com will list the banned/allowed/TBD subsets of absl, as it does for C++ versions.  [AI: Add a starting list, all banned-as-TBD, of everything currently in absl.  Proposed owner: volunteers needed]
• third_party/abseil-cpp/ will be rolled weekly.  Team that performs rolls will add any new functionality to the TBD list, with the current date, or appropriately notify cxx@ to do so.  [Owners: mbonadei, danilchap]
• cxx@ will be the point of first contact for requesting bans/allowances for various items, and will involve other knowledgeable owners where relevant.  Decisions will be by consensus, as with C++ standards features.  [Owner: cxx]
• If an item is still TBD by two years after introduction, cxx@ will discuss it with the goal of explicitly banning or allowing it.  In the absence of any significant reason to ban, the feature will be allowed.  (Again, matches C++ standards features.)  [Owner: pkasting]
• First-party code will only be allowed to use explicitly-allowed parts of absl.  Enforcement will be via the root DEPS file, which will continue to contain the existing "-absl" rule, but will gain specific "+absl/..." allowances as approved.  [Owner: cxx]
• Third-party code will be allowed to use anything in absl except explicitly-blocked pieces due to incompatibility with Chromium (e.g. absl::any).  Enforcement will be via ensuring third_party/abseil_cpp/BUILD.gn does not have the blocked functionality in the absl target or its dependencies, so attempts to use this will not link.  [AI: Remove absl/types:any from that file, audit remaining, fix relevant resulting bustage.  Proposed owner: mbonadei]
• For FastTypeId specifically, we can carry a local patch that causes a compile failure in the broken case, as a backstop.  [AI: Add this patch.  Proposed owner: mbonadei]
• Interface code between first- and third-party will be allowed to use anything the third-party library uses, only as minimally necessary to convert to the subset of absl that first-party code can generally use.  Enforcement will be via code review.  (It would be nice to claim that such code would need to add explicit DEPS rules for all not-generally-allowed types, but due to both lacking IWYU enforcement and lacking "gn check" enforcement, it's possible for CLs to land without doing this -- as https://chromium-review.googlesource.com/c/chromium/src/+/2228543 did.  When possible, though, it would be good to add a relevant DEPS file to list all necessary headers.)  [Proposed owner: third_party/OWNERS?]
3. Guidelines for banning/allowing items in absl:
• Items which overlap existing functionality will only be allowed in the context of a transition plan that ensures we do not carry multiple solutions indefinitely.  By default, this plan is "make the existing functionality match the absl behavior; use a type alias to change the implementation to be based on absl; remove old implementation and convert the alias to direct usage of the absl type".
  
• Transitions to absl concepts must not cause regressions on safety/security checks, or introduce the risk of silently doing so in the future.
• For reimplementations of STL concepts, migrations to absl types with lower standards compliance than the existing Chromium solutions will generally not be considered.  (It's the requester's responsibility to work with the absl team to bring absl up-to-par first.)
4. Specific items mentioned so far:
• absl::any should be banned for first- and third-party code due to silent incorrect functionality in component builds.  [Proposed owner: pkasting]
• absl::Span should be banned for first-party code due to being less std::-compliant than base::Span.  [Proposed owner: pkasting]
• absl::string_view should be banned for first-party code due to only working with 8-bit characters.  [Proposed owner: pkasting]
• absl::flat_hash_map/flat_hash_set should be discussed for allowing, including details/proposed guidance relative to existing containers.  [Proposed owner: dcheng]
• absl::optional should be discussed for allowing, including transition plan details from base::Optional.  [Proposed owner: //base/OWNERS volunteers needed]
• absl::variant should be discussed for allowing.  [Proposed owner: pkasting]

Let me know if I overlooked or misstated anything.  (If you read through and approve, I encourage explicitly saying so -- this is a large enough change that vocal affirmation is preferable to me over silent assent.)

Assuming we want to move forward, the immediate blocking items will be:

• Getting the full list of absl items together for chromium-cpp.appspot.com (maybe https://docs.ros.org/api/abseil_cpp/html/namespaceabsl.html helps here)
• Adding a FastTypeId compile break for component builds
• Maybe a quick sanity-check that the enforcement guardrails above work as we expect, and someone can't easily add non-compliant code, but compliant code works? [Proposed owner: volunteers needed]
• Announcement to chromium-dev@ [Proposed owner: pkasting]

PK

[Jan Wilken Dörrie]

Thanks a lot for driving this, Peter!

On Thu, Jul 16, 2020 at 3:04 AM Peter Kasting <pkas...@chromium.org> wrote:

Updated detailed proposal below.

1. Agreement in principle: Resolved; first-party use of Abseil is directionally approved
2. Roll-out idea:
• chromium-cpp.appspot.com will list the banned/allowed/TBD subsets of absl, as it does for C++ versions.  [AI: Add a starting list, all banned-as-TBD, of everything currently in absl.  Proposed owner: volunteers needed]
• third_party/abseil-cpp/ will be rolled weekly.  Team that performs rolls will add any new functionality to the TBD list, with the current date, or appropriately notify cxx@ to do so.  [Owners: mbonadei, danilchap]
• cxx@ will be the point of first contact for requesting bans/allowances for various items, and will involve other knowledgeable owners where relevant.  Decisions will be by consensus, as with C++ standards features.  [Owner: cxx]
• If an item is still TBD by two years after introduction, cxx@ will discuss it with the goal of explicitly banning or allowing it.  In the absence of any significant reason to ban, the feature will be allowed.  (Again, matches C++ standards features.)  [Owner: pkasting]
• First-party code will only be allowed to use explicitly-allowed parts of absl.  Enforcement will be via the root DEPS file, which will continue to contain the existing "-absl" rule, but will gain specific "+absl/..." allowances as approved.  [Owner: cxx]
• Third-party code will be allowed to use anything in absl except explicitly-blocked pieces due to incompatibility with Chromium (e.g. absl::any).  Enforcement will be via ensuring third_party/abseil_cpp/BUILD.gn does not have the blocked functionality in the absl target or its dependencies, so attempts to use this will not link.  [AI: Remove absl/types:any from that file, audit remaining, fix relevant resulting bustage.  Proposed owner: mbonadei]
• For FastTypeId specifically, we can carry a local patch that causes a compile failure in the broken case, as a backstop.  [AI: Add this patch.  Proposed owner: mbonadei]
• Interface code between first- and third-party will be allowed to use anything the third-party library uses, only as minimally necessary to convert to the subset of absl that first-party code can generally use.  Enforcement will be via code review.  (It would be nice to claim that such code would need to add explicit DEPS rules for all not-generally-allowed types, but due to both lacking IWYU enforcement and lacking "gn check" enforcement, it's possible for CLs to land without doing this -- as https://chromium-review.googlesource.com/c/chromium/src/+/2228543 did.  When possible, though, it would be good to add a relevant DEPS file to list all necessary headers.)  [Proposed owner: third_party/OWNERS?]
3. Guidelines for banning/allowing items in absl:
• Items which overlap existing functionality will only be allowed in the context of a transition plan that ensures we do not carry multiple solutions indefinitely.  By default, this plan is "make the existing functionality match the absl behavior; use a type alias to change the implementation to be based on absl; remove old implementation and convert the alias to direct usage of the absl type".
  
• Transitions to absl concepts must not cause regressions on safety/security checks, or introduce the risk of silently doing so in the future.
• For reimplementations of STL concepts, migrations to absl types with lower standards compliance than the existing Chromium solutions will generally not be considered.  (It's the requester's responsibility to work with the absl team to bring absl up-to-par first.)
4. Specific items mentioned so far:
• absl::any should be banned for first- and third-party code due to silent incorrect functionality in component builds.  [Proposed owner: pkasting]
• absl::Span should be banned for first-party code due to being less std::-compliant than base::Span.  [Proposed owner: pkasting]
• absl::string_view should be banned for first-party code due to only working with 8-bit characters.  [Proposed owner: pkasting]
• absl::flat_hash_map/flat_hash_set should be discussed for allowing, including details/proposed guidance relative to existing containers.  [Proposed owner: dcheng]
• absl::optional should be discussed for allowing, including transition plan details from base::Optional.  [Proposed owner: //base/OWNERS volunteers needed]
• absl::variant should be discussed for allowing.  [Proposed owner: pkasting]

Let me know if I overlooked or misstated anything.  (If you read through and approve, I encourage explicitly saying so -- this is a large enough change that vocal affirmation is preferable to me over silent assent.)

I approve of the outlined proposal.

 

Assuming we want to move forward, the immediate blocking items will be:

• Getting the full list of absl items together for chromium-cpp.appspot.com (maybe https://docs.ros.org/api/abseil_cpp/html/namespaceabsl.html helps here)

Alternatively, would it make sense to have an allow-list and block-list for headers of the public interface? A simple find returns this, which is likely a bit more easy to manage than a list of every symbol in the absl namespace.

Best,

Jan

 

• Adding a FastTypeId compile break for component builds
• Maybe a quick sanity-check that the enforcement guardrails above work as we expect, and someone can't easily add non-compliant code, but compliant code works? [Proposed owner: volunteers needed]
• Announcement to chromium-dev@ [Proposed owner: pkasting]

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/CAAHOzFC4OXwSPnEdo0V7FpHU5qj8wALSQMtwMeYfby5BAY2kPQ%40mail.gmail.com.

[Peter Kasting]

On Wed, Jul 15, 2020 at 11:12 PM Jan Wilken Dörrie <jdoe...@chromium.org> wrote:

Thanks a lot for driving this, Peter!

<3

Assuming we want to move forward, the immediate blocking items will be:

• Getting the full list of absl items together for chromium-cpp.appspot.com (maybe https://docs.ros.org/api/abseil_cpp/html/namespaceabsl.html helps here)

Alternatively, would it make sense to have an allow-list and block-list for headers of the public interface? A simple find returns this, which is likely a bit more easy to manage than a list of every symbol in the absl namespace.

Maybe.  I don't remember who put together the C++11 and C++14 lists (I think Nico?), and those did a good job of conceptually grouping things -- sometimes symbol-by-symbol, sometimes a bunch of related symbols, and sometimes whole headers or large logical pieces.  I'd love something like that if it makes sense, as it was useful to browser through and ask myself if something would be a boon to a problem I was solving, and thus I wanted to propose we allow it.

PK

[Jeremy Roman]

Makes sense to me.

--

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/CAAHOzFD7s8HVUng17Q6k9%2B%3D6HURJ6oHzmRokzxWX4cMU2HZ8Nw%40mail.gmail.com.

[dani...@google.com]

On Thursday, July 16, 2020 at 3:04:28 AM UTC+2, Peter Kasting wrote:

Updated detailed proposal below.

1. Agreement in principle: Resolved; first-party use of Abseil is directionally approved
2. Roll-out idea:
• chromium-cpp.appspot.com will list the banned/allowed/TBD subsets of absl, as it does for C++ versions.  [AI: Add a starting list, all banned-as-TBD, of everything currently in absl.  Proposed owner: volunteers needed]

I can volunteer:

https://chromium-review.googlesource.com/c/chromium/src/+/2302173 

(but can also gladly pass to another volunteer)

[Mirko Bonadei]

On Thursday, July 16, 2020 at 3:04:28 AM UTC+2 Peter Kasting wrote:

Updated detailed proposal below.

1. Agreement in principle: Resolved; first-party use of Abseil is directionally approved
2. Roll-out idea:
   • chromium-cpp.appspot.com will list the banned/allowed/TBD subsets of absl, as it does for C++ versions.  [AI: Add a starting list, all banned-as-TBD, of everything currently in absl.  Proposed owner: volunteers needed]
   • third_party/abseil-cpp/ will be rolled weekly.  Team that performs rolls will add any new functionality to the TBD list, with the current date, or appropriately notify cxx@ to do so.  [Owners: mbonadei, danilchap]
   • cxx@ will be the point of first contact for requesting bans/allowances for various items, and will involve other knowledgeable owners where relevant.  Decisions will be by consensus, as with C++ standards features.  [Owner: cxx]
   • If an item is still TBD by two years after introduction, cxx@ will discuss it with the goal of explicitly banning or allowing it.  In the absence of any significant reason to ban, the feature will be allowed.  (Again, matches C++ standards features.)  [Owner: pkasting]
   • First-party code will only be allowed to use explicitly-allowed parts of absl.  Enforcement will be via the root DEPS file, which will continue to contain the existing "-absl" rule, but will gain specific "+absl/..." allowances as approved.  [Owner: cxx]
   • Third-party code will be allowed to use anything in absl except explicitly-blocked pieces due to incompatibility with Chromium (e.g. absl::any).  Enforcement will be via ensuring third_party/abseil_cpp/BUILD.gn does not have the blocked functionality in the absl target or its dependencies, so attempts to use this will not link.  [AI: Remove absl/types:any from that file, audit remaining, fix relevant resulting bustage.  Proposed owner: mbonadei]

Done, I think the current list of absl source_sets in the //third_party/abseil-cpp:absl component is fine. With regard to the "not linking" part, it is a bit tricky because some things are header only libraries and they will link fine. For example, absl::any will link even if not among the deps of //third_party/abseil-cpp:absl. Here gn check helps:

> gn check out/Debug/

ERROR at //third_party/abseil-cpp/foo.cc:1:11: Include not allowed.

#include "absl/types/any.h"

          ^---------------

It is not in any dependency of

  //third_party/abseil-cpp:absl

The include file is in the target(s):

  //third_party/abseil-cpp/absl/types:any

which should somehow be reachable.

And //third_party/abseil-cpp/absl/types:any cannot be depended on directly. Maybe this is enough but if we want to be sure nothing depends on these headers I think we need a presubmit check. What do you think?

• For FastTypeId specifically, we can carry a local patch that causes a compile failure in the broken case, as a backstop.  [AI: Add this patch.  Proposed owner: mbonadei]

Done: https://chromium-review.googlesource.com/c/chromium/src/+/2307215.

../../third_party/abseil-cpp/absl/base/internal/fast_type_id.h:42:2: error: "This feature requires RTTI to work in component builds."

#error "This feature requires RTTI to work in component builds."

• Interface code between first- and third-party will be allowed to use anything the third-party library uses, only as minimally necessary to convert to the subset of absl that first-party code can generally use.  Enforcement will be via code review.  (It would be nice to claim that such code would need to add explicit DEPS rules for all not-generally-allowed types, but due to both lacking IWYU enforcement and lacking "gn check" enforcement, it's possible for CLs to land without doing this -- as https://chromium-review.googlesource.com/c/chromium/src/+/2228543 did.  When possible, though, it would be good to add a relevant DEPS file to list all necessary headers.)  [Proposed owner: third_party/OWNERS?]

I think we should follow up and turn on GN check, let me file a bug.

1. Guidelines for banning/allowing items in absl:
   • Items which overlap existing functionality will only be allowed in the context of a transition plan that ensures we do not carry multiple solutions indefinitely.  By default, this plan is "make the existing functionality match the absl behavior; use a type alias to change the implementation to be based on absl; remove old implementation and convert the alias to direct usage of the absl type".
     
   • Transitions to absl concepts must not cause regressions on safety/security checks, or introduce the risk of silently doing so in the future.
   • For reimplementations of STL concepts, migrations to absl types with lower standards compliance than the existing Chromium solutions will generally not be considered.  (It's the requester's responsibility to work with the absl team to bring absl up-to-par first.)
2. Specific items mentioned so far:
   • absl::any should be banned for first- and third-party code due to silent incorrect functionality in component builds.  [Proposed owner: pkasting]
   • absl::Span should be banned for first-party code due to being less std::-compliant than base::Span.  [Proposed owner: pkasting]
   • absl::string_view should be banned for first-party code due to only working with 8-bit characters.  [Proposed owner: pkasting]
   • absl::flat_hash_map/flat_hash_set should be discussed for allowing, including details/proposed guidance relative to existing containers.  [Proposed owner: dcheng]
   • absl::optional should be discussed for allowing, including transition plan details from base::Optional.  [Proposed owner: //base/OWNERS volunteers needed]
   • absl::variant should be discussed for allowing.  [Proposed owner: pkasting]

Let me know if I overlooked or misstated anything.  (If you read through and approve, I encourage explicitly saying so -- this is a large enough change that vocal affirmation is preferable to me over silent assent.)

Look good to me, thanks for driving this!

 

Assuming we want to move forward, the immediate blocking items will be:

• Getting the full list of absl items together for chromium-cpp.appspot.com (maybe https://docs.ros.org/api/abseil_cpp/html/namespaceabsl.html helps here)
• Adding a FastTypeId compile break for component builds

https://chromium-review.googlesource.com/c/chromium/src/+/2307215 

• Maybe a quick sanity-check that the enforcement guardrails above work as we expect, and someone can't easily add non-compliant code, but compliant code works? [Proposed owner: volunteers needed]

I can do this but since I have added most of these guardrails it is better if someone else does this to have a peer review.

[Peter Kasting]

Updated status: no significant disagreement, so my previously-spelled-out plan is the plan of record.

Details for a few items in replies below.

On Wed, Jul 15, 2020 at 6:04 PM Peter Kasting <pkas...@chromium.org> wrote:

Updated detailed proposal below.

1. Roll-out idea:
   
• chromium-cpp.appspot.com will list the banned/allowed/TBD subsets of absl, as it does for C++ versions.  [AI: Add a starting list, all banned-as-TBD, of everything currently in absl.  Proposed owner: volunteers needed]

WIP: https://chromium-review.googlesource.com/c/chromium/src/+/2302173 

• For FastTypeId specifically, we can carry a local patch that causes a compile failure in the broken case, as a backstop.  [AI: Add this patch.  Proposed owner: mbonadei]

Per mbonadei@, "there is an upstream CL with good possibilities to land in absl directly"; fallback is https://chromium-review.googlesource.com/c/chromium/src/+/2307215 .

• It would be nice to claim that such code would need to add explicit DEPS rules for all not-generally-allowed types, but due to both lacking IWYU enforcement and lacking "gn check" enforcement, it's possible for CLs to land without doing this -- as https://chromium-review.googlesource.com/c/chromium/src/+/2228543 did.

FWIW, this was fixed in https://chromium-review.googlesource.com/c/chromium/src/+/2309702 . 

• Maybe a quick sanity-check that the enforcement guardrails above work as we expect, and someone can't easily add non-compliant code, but compliant code works? [Proposed owner: volunteers needed]
  

mbonadei@ is fallback here, but this could benefit from a volunteer.  No particular special knowledge needed (in fact it's likely better that you know no more than the average Chrome engineer would).

PK

[Peter Kasting]

Probably final update.

Further followups on a few things below.  As a result of these, I have https://chromium-review.googlesource.com/c/chromium/src/+/2327638 out for review to do (what I think are) the final code tweaks to allow Abseil usage in first party code; once that lands, I will send an announcement to chromium-dev@.

On Fri, Jul 24, 2020 at 3:32 PM Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Jul 15, 2020 at 6:04 PM Peter Kasting <pkas...@chromium.org> wrote:

Updated detailed proposal below.

1. Roll-out idea:
   
• chromium-cpp.appspot.com will list the banned/allowed/TBD subsets of absl, as it does for C++ versions.  [AI: Add a starting list, all banned-as-TBD, of everything currently in absl.  Proposed owner: volunteers needed]

WIP: https://chromium-review.googlesource.com/c/chromium/src/+/2302173 

This has now landed and is live on https://chromium-cpp.appspot.com/ .  Thanks danilchap@! 

• Maybe a quick sanity-check that the enforcement guardrails above work as we expect, and someone can't easily add non-compliant code, but compliant code works? [Proposed owner: volunteers needed]
  

mbonadei@ is fallback here, but this could benefit from a volunteer.  No particular special knowledge needed (in fact it's likely better that you know no more than the average Chrome engineer would).

I spun up a CL to try adding absl::variant (separate request-for-approval thread to come): https://chromium-review.googlesource.com/c/chromium/src/+/2327456

As part of this I verified that

• Failing to add the right dependency to the .gn public_deps resulted in (catastrophic) build failures
• Failing to specifically allow variant.h in the root DEPS file resulted in PRESUBMIT errors

I think this is a good-enough test of the above.

PK

[Jan Wilken Dörrie]

On Thu, Jul 16, 2020 at 3:04 AM Peter Kasting <pkas...@chromium.org> wrote:

Specific items mentioned so far:

• absl::optional should be discussed for allowing, including transition plan details from base::Optional.  [Proposed owner: //base/OWNERS volunteers needed]

While I'm not technically a //base OWNER I wouldn't mind taking this on, unless folks have objections.

--

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/CAAHOzFBCG6aWDak-7zfCss2wD4w-R-JKBfZjxrqAkFiEVG2Ecg%40mail.gmail.com.

[Peter Kasting]

On Mon, Aug 10, 2020 at 4:49 AM Jan Wilken Dörrie <jdoe...@chromium.org> wrote:

On Thu, Jul 16, 2020 at 3:04 AM Peter Kasting <pkas...@chromium.org> wrote:

Specific items mentioned so far:

• absl::optional should be discussed for allowing, including transition plan details from base::Optional.  [Proposed owner: //base/OWNERS volunteers needed]

While I'm not technically a //base OWNER I wouldn't mind taking this on, unless folks have objections.

+1, anyone can propose the thread and you're definitely qualified to propose/direct the plan.

PK 

[Chris Palmer]

Yes, we have death tests for some of base/containers, as well as for base/optional.h. As part of a "Do I feel this base/ hardening project is complete?" audit that's on my list, I'll see if there are any more death tests to add.

On Wed, Jun 17, 2020 at 5:59 PM Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Jun 17, 2020 at 3:30 PM Daniel Cheng <dch...@chromium.org> wrote:

On Tue, Jun 16, 2020 at 9:14 PM Chris Palmer <pal...@chromium.org> wrote:

To me the key thing is that we not lose safety and correctness by preferring absl::foo to base::foo. We've put a lot of work into making base more resilient and secure — recently confirmed by Project Zero to have stopped some real attacks, by the way — and I *really* don't want for Chromium to regress.

Toward that end, keep in mind that we always need to build Abseil with its hardening mode turned on in Chromium. That's a minimum requirement.

In my dream world, we'd 'productize' as much of base as we could, re-designing it as we go and as/if necessary for ergonomic safety, and make it as easy as possible for any callers (inside or outside of Chromium) to call it. The world needs a safety-oriented C++ library that remains as efficient as possible, and base is low-key succeeding at that, IMO. AFAICT nobody else is attempting to do this, so there's an unmet need in the world.

I'd like to second this point: for absl things we do start using in //base, I'd want to make sure we have some smoke tests to ensure that the appropriate CHECKs are enabled. To me, this is especially important because my understanding is absl transparently maps through to the standard library version if it's recent enough (e.g. support for optional, string_view, variant). I want to make sure we don't end up in a situation where we had security hardening and then lost it by upgrading a new C++ version.

This is a good point.  @palmer: Do we currently have death tests that verify our hardening of different things?  If not, it seems like we should add them.

Regarding mapping through, you can toggle between "force std/force absl/auto-select"; see https://abseil.io/blog/201901115-options#read-more .  Thus I don't think this concern should gate, but I do think we need the tests so that we don't lose security absent-mindedly.

Proposal: Treat this similarly to a new C++ version, in that we start with most things default-banned, allow on a by-request basis, and aim to default-allow within two years.  Track in the Chromium style guide and chromium-cpp.appspot.com.

How often does absl change/add new things? I'm a bit hesitant to say that we should just default-allow in two years, depending on how often new concepts/functionality is introduced. It seems to me the most value comes from the container types, especially for interop with other third-party libraries that are using absl at library boundaries.

Looking at this more, I share your core concern.  It's a big concerning new functionality upstream adds gains significant usage in Chromium before we've had a chance to review-in-principle.  But since Abseil is less "versioned" than C++ itself, it's harder to guarantee that from both a linguistic perspective ("Abseil v3 is allowed" is not very meaningful) or a technical one (we can live on LTS branches instead of HEAD, but those are just basically cut-every-six-months).

For a steady-state goal, we have several choices I can think of:

1. "Laissez faire": Since Abseil is guided by principles and values closer to Chromium's than C++ is, assume that reactively disallowing problematic functionality as needed will be sufficient
2. "Proactive consideration": We monitor what goes into Abseil, discuss new additions periodically, and rule on them reasonably quickly 
3. "Constant vigilance": Only explicitly allowed functionality may ever be used (enforced by e.g. PRESUBMIT); using more requires an explicit request

Personally I'm OK with either 1 or 2; I don't think 3 is warranted.  If we do 2, we'll need to figure out who/what is responsible for said monitoring.

Proposal: Allow absl::any and absl::variant to start with, as these polyfill C++17 types, have no base:: implementation, and have consumer interest .  Allow other types and functions to begin with only as necessary to compile against third-party code using Abseil and convert to non-absl:: types.  Create a list of overlaps between base:: and absl:: (e.g. base::Optional vs. absl::optional, base::StringPiece vs. absl::string_view, base::Time and related classes vs. absl::Time and related classes, etc.) and decide a plan for deduplication going forward.  Bias towards std:: types, then absl:: types, then base:: types.

I can't say that I personally care for any, but maybe I don't understand the use cases. I know there's a CL that wants to use absl::any, but it's hard to tell exactly what the intended usage is (since it's a support library for third-party code).

In Chromium itself, usage would be rare, basically for type-erased stuff and "arbitrary property" sorts of cases (View class properties, WebContents user data, etc.).  It's convenient but not critical for these and I'm not wedded to having it early or at all, but any objections we'd make to absl::any will also apply to std::any in C++17, so it's probably less about discussing whether to take absl's version as whether we think the feature is risky enough that we should permanently ban it.

 

Finally, I'd like to add that span and absl's hash containers should probably be on this list: in particular, I'd really like to see absl's hash containers be the default recommendation (though we'll likely need to benchmark ARM).

I didn't put ::span on the initial list since base::span exists and I didn't want to start off with a list that overlaps at all (since any overlapping thing should have a long-term plan about where we're converging to), but yes, with things like ::span and ::optional where base, absl, and std all aim to implement the identical API, I think there's a solid case for using first absl and then std, modulo security check support.

[Jan Wilken Dörrie]

Thanks both.

Regarding hardening checks for optional: All accesses to the underlying data should be checked (i.e. .value(), operator* and operator->).  In base::Optional we have CHECKs, in absl::optional there are hardening asserts or exceptions, which abort() when exceptions are disabled.  So these checks should be robust enough, but we should make sure to add missing tests to AbslHardeningTest. 

Wrt migration plan: It looks like aliasing base::Optional to absl::optional already compiles today, so it'll "just" be a matter of updating the references. I likely won't have the time to seriously start this effort before next month, and thus wouldn't object if someone wants to start this adoption earlier.

Best,

Jan

--

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/CAOuvq20_g7699iwhK4Y3V3JefeXuUzc7mH7B0duDEBd8oDqBGQ%40mail.gmail.com.