context::ContextValue cannot be compiled with Visual Studio 2015

[maxgolov]

ContextValue has been recently introduced in SDK. Unfortunately the two nostd::shared_ptr<...> variant members are failing to compile with Visual Studio 2015, due to issue in nostd::variant backport.

This code fails to compile:

using ContextValue = nostd::variant<bool,
                                    int64_t,
                                    uint64_t,
                                    double,
                                    nostd::shared_ptr<trace::Span>,
                                    nostd::shared_ptr<trace::SpanContext>>;

with the following error:

1>c:\work\opentelemetry-cpp\api\include\opentelemetry\nostd\variant.h(168): error C2672: 'opentelemetry::v0::nostd::detail::visitation::base::make_farray': no matching overloaded function found
1>c:\work\opentelemetry-cpp\api\include\opentelemetry\nostd\variant.h(173): note: see reference to class template instantiation 'opentelemetry::v0::nostd::detail::visitation::base::make_fmatrix_impl<F,opentelemetry::v0::nostd::detail::base<opentelemetry::v0::nostd::detail::Trait::Available,bool,int64_t,uint64_t,double,opentelemetry::v0::nostd::shared_ptr<opentelemetry::v0::trace::Span>,opentelemetry::v0::nostd::shared_ptr<opentelemetry::v0::trace::SpanContext>> &>::impl<opentelemetry::v0::nostd::integer_sequence<std::size_t>,opentelemetry::v0::nostd::integer_sequence<std::size_t>>' being compiled

whereas a simpler definition of the variant that does not use nostd::shared_ptr - compiles successfully:

using ContextValue = nostd::variant < bool, int64_t, uint64_t, double>;

The issue is in a way how nostd::variant backport handles complex objects. Primitive types are OK.

There could be a few alternative solutions to this issue:

• declare that we cannot support Visual Studio 2015
• declare that RuntimeContext API does not work with Visual Studio 2015
• simplify the implementation to avoid positioning complex objects on variant, possibly using raw pointers instead
• fix an issue in implementation of nostd::detail::visitation::base
• explore ability to substitute the nostd::variant with absl::variant ( somewhat related to [WIP] Ability to use Standard Library as a substitute for nostd:: classes #275 - ability to substitute nostd with alternate implementation )

[maxgolov]

I explored the last option. Abseil absl::variant compiles and works fine with Visual Studio 2015. The best option now seems to be for Visual Studio 2015 to compile with Abseil C++ library ( https://github.com/abseil/abseil-cpp/blob/master/absl/types/variant.h ) instead of nostd::variant. Note that Abseil is friendly-licensed already (Apache License 2.0).

Verified that this patch allows to replace the implementation in nostd/variant.h by Abseil's working implementation:

...
#elif defined(HAVE_ABSEIL)
#include "absl/types/variant.h"
OPENTELEMETRY_BEGIN_NAMESPACE
namespace nostd
{
using absl::variant;
using absl::get;
}  // namespace nostd
OPENTELEMETRY_END_NAMESPACE
namespace absl
{
namespace variant_internal
{
    void __cdecl ThrowBadVariantAccess(){};
}
}  // namespace absl
#else
...

[maxgolov]

@pyohannes @g-easy @reyang @ThomsonTan @lalitb --- I'd like to discuss this during our next meeting, that possibly on Windows (and maybe other OS) we should use Abseil's port of absl::variant. The issue is that the nostd::variant that we have - does not work on Visual Studio 2015, while Abseil's works fine. I already did changes in my fork to verify this here:
https://github.com/maxgolov/opentelemetry-cpp/blob/master/api/include/opentelemetry/nostd/variant.h

This code allows to compile with nostd classes, but uses Abseil for variant. One challenge with Abseil : it does require statically linked Abseil library.. Unless we add our own implementation of variant throw, as shown here:
https://github.com/maxgolov/opentelemetry-cpp/blob/4004c2d1ebc6bd9b1e55eaf9310726fc939f9de5/sdk/src/common/core.cc#L10

I'm thinking of approach where we use Abseil's header, but for the throw routine we provide our own stub. That way we do not have to link against Abseil.

At this time I do need Visual Studio 2015 support. One of my prominent customers is still on that compiler.

[g-easy]

Replace the implementation of nostd::variant with Abseil's variant.h (but not add a build time dependency on Abseil)?

[maxgolov]

Replace the implementation of nostd::variant with Abseil's variant.h (but not add a build time dependency on Abseil)?

@g-easy - we can use Abseil's variant.h with a stub for exception handler like this.. Maybe under compile time flag that controls whether we provide it, or if Abseil provides it:

#if defined(HAVE_ABSEIL)
namespace absl
{
namespace variant_internal
{
void __cdecl ThrowBadVariantAccess(){};
}
}  // namespace absl
#endif

At least that way we do not need to use Abseil's implementation of exception handler for header-only implementation of SDK API. I am not entirely sure if we need to use Abseil for Windows only or elsewhere too. nostd::variant works fine with pre-C++17 compilers on other OS (Linux and Mac are fine), just not with vs2015.

[g-easy]

not entirely sure if we need to use Abseil for Windows only

I would prefer to have a single implementation of nostd::variant. Especially if we're trying to maintain ABI compat.

[lalitb]

One challenge with Abseil : it does require statically linked Abseil library

Is exception handling only dependency we have as a static library ? Also, redefining ThrowBadVariantAccess mayn't be enough, as there are other symbols defined in the static library which need to be defined ( e.g, absl::bad_variant_access ) :

$ objdump -t /usr/local/lib/libabsl_bad_variant_access.a  | grep ' F '
0000000000000000  w    F .text._ZNSt9exceptionC2Ev      0000000000000019 _ZNSt9exceptionC2Ev
0000000000000000  w    F .text._ZNSt9exceptionC2Ev      0000000000000019 _ZNSt9exceptionC1Ev
0000000000000000 g     F .text  0000000000000029 _ZN4absl18bad_variant_accessD2Ev
0000000000000000 g     F .text  0000000000000029 _ZN4absl18bad_variant_accessD1Ev
000000000000002a g     F .text  000000000000002b _ZN4absl18bad_variant_accessD0Ev
0000000000000056 g     F .text  0000000000000011 _ZNK4absl18bad_variant_access4whatEv
0000000000000000  w    F .text._ZN4absl18bad_variant_accessC2Ev 0000000000000029 _ZN4absl18bad_variant_accessC2Ev
0000000000000000  w    F .text._ZN4absl18bad_variant_accessC2Ev 0000000000000029 _ZN4absl18bad_variant_accessC1Ev
0000000000000067 g     F .text  000000000000003b _ZN4absl16variant_internal21ThrowBadVariantAccessEv
00000000000000a2 g     F .text  0000000000000009 _ZN4absl16variant_internal7RethrowEv

Also are there any issues in using abseil variant implementation for all the platforms. Or other way, fixing existing nostd::variant to work for VS2015 ? That will ensure we are maintaining single variant .

[g-easy]

Do we have to link in Abseil or can we just borrow their variant.h and bring the exception handling in line with our requirements (which are to not require EH)

[lalitb]

Do we have to link in Abseil or can we just borrow their variant.h and bring the exception handling in line with our requirements (which are to not require EH)

From @maxgolov explanation, it seems to be latter - i.e, bringing abseil variant.h, and possibly other headers used within it, and define statically linked symbols within header. So this will avoid linking with abseil library.

[maxgolov]

@lalitb - that's right, it's only the exception handling part (it seems). So, in theory, we should be able to consume the Abseil's header. One issue with that though - need to check if this is internal detail, and what happens if our customer also links the Abseil's real implementation of the exception handler? Then we may end up with doubly defined symbol. Something that possibly can be addressed by yet another compiler option: whether to expect the Abseil's exception handler or statically link our own within.

@g-easy - we maintain ABI compatibility for particular OS. You'd expect Windows .exe to load .dll, and that .dll may use Abseil on Windows. Whereas you'd expect .bin to load .so (on Mac or Linux), and POSIX implementation may continue to use nostd::variant. In that sense the implementation of tracer remains ABI-stable within the OS class.

[maxgolov]

@lalitb - I am not against fixing our nostd::variant backport. I just confirmed that Abseil did it right, and this is certainly possible if we carefully review how Abseil is doing it. This is something to deal with non-primitive types where nostd::variant does not want to compile on vs2015.... IN terms of delivery: IF I need to ship it to somebody today and I'm compiling from source, I would trust (more) either STL (C++17 and above) or Abseil's (anything C++11 and above) implementation of the class.