Add an option to not alias C++17 types

[ceztko]

I'm indirectly using abseil-cpp in MSVC 2017 because of Native webrtc[1]. Native webrtc is currently not supporting C++17 compilation itself[2] and it's using abseil-cpp for types like absl::optional. This is an issue since since abseil types are used outside of webrtc compilation boundaries in its public API: as soon as one will use webrtc API in a C++17 compiled program it will cause stack corruption issue because caller will allocate on the stack using the aliased std::optional but within its boundaries webrtc will assume it's the custom provided absl::optional. Actually, the safest behavior would be to do aliasing only if voluntarily requested, but I guess a global macro to avoid any ABI sensitive alias attempt it's best for compatibility now. The problem with stack allocation problems in webrtc related to abseil is discussed here[3].

[1] https://webrtc.org/native-code/
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=752720
[3] https://groups.google.com/d/msg/discuss-webrtc/k57yt-0yboI/x8mFkuuCEgAJ

[ceztko]

An ugly workaround to avoid mismatches between different modules is by including absl/base/config.h, undefine alias enabling macros, and finally including the types requested. This can be put in a precompiled header. Unsure it's really safe in all circumstances, though. A supported switch, or better (I repeat) no alias at all by default, would be much more appreciated.

#include <absl/base/config.h>
#undef ABSL_HAVE_STD_ANY
#undef ABSL_HAVE_STD_OPTIONAL
#undef ABSL_HAVE_STD_VARIANT
#undef ABSL_HAVE_STD_STRING_VIEW
#include <absl/types/optional.h>
#include <absl/types/any.h>
#include <absl/types/variant.h>
#include <absl/strings/string_view.h>

[derekmauro]

https://abseil.io/about/compatibility
https://abseil.io/about/design/dropin-types

This is something we are unlikely to do. Depending on a compiled representation of code or supporting mixed compilation modes is problematic and does not scale.

The suggested compile switches just result in a combinatorial expulsion of configurations to test. Again, this doesn't scale.

I suggest building in C++14 mode until WebRTC is C++17 compliant.

[ceztko]

This is a shame. You introduce support libraries for code bases not ready to switch to c++17 and, as a result, you force other code bases to stick to c++14. Before Webrtc adoption of abseil there were no issue in using it in c++17 compiled programs and, and there shouldn't be as long as there not exist any c++17 language feature that may cause ABI issue with code compiled with C++14 configured compiler (so far I don't know any). Please note: I'm talking about language features, not library ones. Webrtc static library I'm using is compiled using exactly the same compiler version (VisualStudio 2017 15.9) and toolsets version (v141) I'm using in my final user program. Just Webrtc is compiled with a C++14 configured compiler. To me, your approach is creating more problems than the ones you are trying to solve: only explicit users of abseil (in this case Webrtc) should have responsibility when move to std types, and not this to be determined by environment creating the mismatches as I am experiencing. Is there any issue in using both std::shared_ptr and boost::shared_ptr in the same program? I am not aware of any, and so it should be for absl::optional and std::optional: it should be possible for both to coexist in a C++17 enabled compiler.

[ceztko]

Sorry if I insist, but I don't understand why you don't immediately see how big the problem is with abseil. It would be very similar as if Microsoft tomorrow comes with an update for for Visual Studio 2015 stating in the release note: "We introduced a change in toolset v140 that will make it incompatibile with v141: your VS2015 code won't be compatible with VS2017 compiled code anymore". This is roughly the effect of using abseil today in library code that is distributed in binary form, or can't be yet compiled in C++17 enabled compiler.

[ceztko]

In a nutshell: just using abseil may make code forward incompatible with c++17. This is unbelievable, especially considering the purpose of some types in the library, but it's a fact.

[tituswinters]

One of the biggest problems we've encountered is dealing with secondary support for types. You do *not* want to work in a codebase that has both std::optional and absl::optional (the two don't interconvert) and that has some template specializations or overload set support for one or the other. Abseil's design choice is very intentionally pushing away from that, more details here: https://abseil.io/about/design/dropin-types Nothing prevents you locally from adding the compiler-switches you suggest, but we are unlikely to do that. It'll hurt us too much.

…

On Sat, Feb 9, 2019 at 8:52 PM Francesco Pretto ***@***.***> wrote: In a nutshell: just using abseil may make code forward incompatible with c++17. This is unbelievable, especially considering the purpose of some types in the library, but it's a fact. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#264 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AL4o3_cp1NZdhqT15KPy5JuAoH5ZJCeBks5vL3tWgaJpZM4axalY> .

[ceztko]

Nothing prevents you locally from adding the compiler-switches you suggest,
but we are unlikely to do that. It'll hurt us too much.

Well, I hope you see the contradiction of sponsoring move to C++17 enabled compiler in a way and forcing people to stick to C++14 in another, as it is happening now in this case. While it's true that absl::optional and std::optional don't interconvert, it's not that much needed for the purpose of std::optional itself. Anyway, you can convince me it's good to favor people to move to C++17 by default aliasing std::optional and like types, (at the huge cost of unexpected ABI issues, as it's clearly happening with WebRTC users), but to not introduce yourself a flag to disable aliasing, leaving indirect users of abseil in a unsupported gray area, I think you should state why is not safe to do today and what would be the ABI issues involved in doing it anyway.

[tituswinters]

It isn't *us* forcing people to stick to C++14, it's your other dependencies. It's perfectly safe/fine to consistently use absl::optional (and other C++17 types) consistently until all of your deps have updated. In fact, given the changes between language versions, that's probably the *wise* choice. If that and the other arguments don't seem sufficiently presented in that design note I linked to you, please let me know - I certainly want to be as clear and obvious about all of the tradeoffs.

…

On Mon, Feb 11, 2019 at 9:06 AM Francesco Pretto ***@***.***> wrote: Nothing prevents you locally from adding the compiler-switches you suggest, but we are unlikely to do that. It'll hurt us too much. Well, I hope you see the contradiction of sponsoring move to C++17 enabled compiler in a way and forcing people to stick to C++14 in another, as it is happening now in this case. While it's true that absl::optional and std::optional don't interconvert, it's not that much needed for the purpose of std::optional itself. Anyway, you can convince me it's good to favor people to move to C++17 by default aliasing std::optional and like types, (at the huge cost of unexpected ABI issues, as it's clearly happening with WebRTC users), but to not introduce yourself a flag to disable aliasing, leaving indirect users of abseil in a unsupported gray area, I think you should state why is not safe to do today and what would be the ABI issues involved in doing it anyway. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#264 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AL4o3xy3Ks4BZ_UyrG_mDZzhTtgK6Useks5vMXjwgaJpZM4axalY> .

[ceztko]

It isn't us forcing people to stick to C++14, it's your other dependencies.

There are very good reasons to a library to stick the build with C++14 compiler, for example to track supported compilers and prevent developers to use C++17 features or library changes that will create ABI backward incompatible libs.

I'm sorry to say that the harm here is created by abseil creating unpredictable behavior and not offering a way to control it among code created with different language constraints, which is something largely supported by the compiler industry (microsoft, gcc, llvm...) since C++14 compiled code is by large extent forward compatible with C++17 code, something that is is not true anymore by using abseil.

It's perfectly safe/fine to consistently use absl::optional (and other C++17 types) consistently until all of your deps have updated. In fact, given the changes between language versions, that's probably the wise choice

Actually, I asked the other thing: can you state today that coexistence of std::optional and absl::optional is dangerous and may cause ABI issues? They are two different types, why there should be any problem at all? If there are no issues, why not supporting disabling of aliasing?

If that and the other arguments don't seem sufficiently presented in that design note I linked to you, please let me know - I certainly want to be as clear and obvious about all of the tradeoffs.

I'm sorry, but I think I will keep my opinion about the fact that abseil should not automatically change it's ABI depending on compiler macros. It's dangerous (the ABI issues I'm talking about are real), and against the principle of least surprise. WebRTC is not doing any mistake in enforcing C++14 compiler compiler so I cant' blame them other than the choice to use abseil at first. I did my best to inform WebRTC community about the issue and people can find this github issue easily. I hope other will comment since I'm sure the issue is more spread than just WebRTC users, which is a huge and hard to use library that has niche use cases.

My solution was eventually to confine WebRTC user code in a separate C++14 dll, something I was kind of doing already anyway, creating a boundary that will allow me to use C++17 code everywhere else. Just before WebRTC moved to absl::optional, this was not required.

[tituswinters]

At the core, Abseil is *only* prioritizing "everything in your build is built from source, with the same compiler flags." There is insufficient understanding of ABI and ODR in the broad C++ community to promise anything else without also accidentally promising things we can't/won't support. I'm sorry you have a different set of priorities - that doesn't make either of us wrong.

…

On Mon, Feb 11, 2019 at 11:04 AM Francesco Pretto ***@***.***> wrote: It isn't *us* forcing people to stick to C++14, it's your other dependencies. There are very good reasons to a library to stick the build with C++14 compiler, for example to track supported compilers and prevent developers to use C++17 features or library changes that will create ABI backward incompatible libs. I'm sorry to say that the harm here is created by abseil creating unpredictable behavior and not offering a way to control it among code created with different language constraints, which is something largely supported by the compiler industry (microsoft, gcc, llvm...) since C++14 compiled code is by large extent forward compatible with C++17 code, something that is is not true anymore by using abseil. It's perfectly safe/fine to consistently use absl::optional (and other C++17 types) consistently until all of your deps have updated. In fact, given the changes between language versions, that's probably the *wise* choice Actually, I asked the other thing: can you state today that coexistence of std::optional and absl::optional is dangerous and may cause ABI issues? They are two different types, why there should be any problem at all? If there are no issues, why not supporting disabling of aliasing? If that and the other arguments don't seem sufficiently presented in that design note I linked to you, please let me know - I certainly want to be as clear and obvious about all of the tradeoffs. I'm sorry, but I think I will keep my opinion about the fact that abseil should not automatically change it's ABI depending on compiler macros. It's dangerous (the ABI issues I'm talking about are real), and against the principle of least surprise. WebRTC is not doing any mistake in enforcing C++14 compiler compiler so I cant' blame them other than the choice to use abseil at first. I did my best to inform WebRTC community about the issue and people can find this github issue easily. I hope other will comment since I'm sure the issue is more spread than just WebRTC users, which is a huge and hard to use library that has niche use cases. My solution was eventually to confine WebRTC user code in a separate C++14 dll, something I was kind of doing already anyway, creating a boundary that will allow me to use C++17 code everywhere else. Just before WebRTC moved to absl::optional, this was not required. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#264 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AL4o339wRk4VLkF315z3ek6uf30eB847ks5vMZSUgaJpZM4axalY> .

[JonathanDCohen]

Ping on this issue. If there is nothing more to discuss for now, we can close this and reopen if it continues to cause problems. @ceztko sound okay to you?

[ceztko]

@ceztko sound okay to you?

No problem. I tried to my best to change your mind on the severity of the issue, but it's debatable topic and at some point one just live with issues and find an alternative solution.