[BUG] Fix compilation issues with MSVC 2017 #196

miscco · 2023-07-11T15:26:34Z

Currently we have issues in CI when building with MSVC 2017

This PR fixes that and enables us to pass CI cleanly even with MSVC 2017 and up

It also introduces some minor cleanups

Fixes #173

griwes

One overarching thing I noticed with these changes is how inconsistent we are with the placement of constexpr, and how you're doing a bunch of changes in one of the files, but not in the other ones. We need to have a style guide conversation at some point, but since we're already heavily inconsistent, it'd be nice to avoid the noise of changing this in just some of the places, and in a change set that's not related to formatting.

(Ideally we'd just figure out a common clang format config for everything and reformat everything in one go, I guess? I think there's a way to mark commits as irrelevant in git blame, so just doing this in a single, specialized commit would be ideal.)

libcudacxx/.upstream-tests/test/std/containers/sequences/array/front_back.pass.cpp

griwes · 2023-07-14T07:07:49Z

libcudacxx/.upstream-tests/test/std/views/views.span/span.objectrep/as_bytes.pass.cpp

@@ -39,7 +42,9 @@ void testRuntimeSpan(Span sp)
        assert(spBytes.extent == sizeof(typename Span::element_type) * sp.extent);

    assert((void *) spBytes.data() == (void *) sp.data());
+#ifndef TEST_COMPILER_MSVC_2017


...why is this disabled on 2017?

There is a misscompilation with MSVC2017 in exactly that place, where it fails that assert because of some integer overflow that should not happen.

griwes · 2023-07-14T07:15:13Z

libcudacxx/include/cuda/std/detail/libcxx/include/optional

+    _LIBCUDACXX_TEMPLATE(class _Up = value_type)
+      (requires (!_LIBCUDACXX_TRAIT(is_same, __remove_cvref_t<_Up>, in_place_t)) _LIBCUDACXX_AND
+                (!_LIBCUDACXX_TRAIT(is_same, __remove_cvref_t<_Up>, optional)) _LIBCUDACXX_AND
+                ( _LIBCUDACXX_TRAIT(is_constructible, _Tp, _Up)) _LIBCUDACXX_AND
+                ( _LIBCUDACXX_TRAIT(is_convertible, _Up, _Tp)))
+    _LIBCUDACXX_INLINE_VISIBILITY constexpr
+    optional(_Up&& __v) : __base(in_place, _CUDA_VSTD::forward<_Up>(__v)) {}
+
+    _LIBCUDACXX_TEMPLATE(class _Up)
+      (requires (!_LIBCUDACXX_TRAIT(is_same, __remove_cvref_t<_Up>, in_place_t)) _LIBCUDACXX_AND
+                (!_LIBCUDACXX_TRAIT(is_same, __remove_cvref_t<_Up>, optional)) _LIBCUDACXX_AND
+                ( _LIBCUDACXX_TRAIT(is_constructible, _Tp, _Up)) _LIBCUDACXX_AND
+                (!_LIBCUDACXX_TRAIT(is_convertible, _Up, _Tp)))
+    _LIBCUDACXX_INLINE_VISIBILITY constexpr
+    explicit optional(_Up&& __v) : __base(in_place, _CUDA_VSTD::forward<_Up>(__v)) {}
+
+    _LIBCUDACXX_TEMPLATE(class _Up)
+      (requires (!_LIBCUDACXX_TRAIT(is_same, _Up, _Tp)) _LIBCUDACXX_AND
+                ( _LIBCUDACXX_TRAIT(is_constructible, _Tp, _Up const&)) _LIBCUDACXX_AND
+                ( _LIBCUDACXX_TRAIT(is_convertible, _Up const&, _Tp)) _LIBCUDACXX_AND
+                (!__check_constructible_from_opt<_Up>::value))
+    _LIBCUDACXX_INLINE_VISIBILITY constexpr
+    optional(const optional<_Up>& __v) {
        this->__construct_from(__v);
    }
-    template <class _Up, __enable_if_t<
-        _CheckOptionalLikeCtor<_Up, _Up const&>::template __enable_explicit<_Up>::__value
-    , int> = 0>
-    _LIBCUDACXX_INLINE_VISIBILITY
-    constexpr explicit optional(const optional<_Up>& __v)
-    {
+
+    _LIBCUDACXX_TEMPLATE(class _Up)
+      (requires (!_LIBCUDACXX_TRAIT(is_same, _Up, _Tp)) _LIBCUDACXX_AND
+                ( _LIBCUDACXX_TRAIT(is_constructible, _Tp, _Up const&)) _LIBCUDACXX_AND
+                (!_LIBCUDACXX_TRAIT(is_convertible, _Up const&, _Tp)) _LIBCUDACXX_AND
+                (!__check_constructible_from_opt<_Up>::value))
+    _LIBCUDACXX_INLINE_VISIBILITY constexpr
+    explicit optional(const optional<_Up>& __v) {
        this->__construct_from(__v);
    }

-    // LWG2756: conditionally explicit conversion from optional<_Up>&&
-    template <class _Up, __enable_if_t<
-        _CheckOptionalLikeCtor<_Up, _Up &&>::template __enable_implicit<_Up>::__value
-    , int> = 0>
-    _LIBCUDACXX_INLINE_VISIBILITY
-    constexpr optional(optional<_Up>&& __v)
-    {
+    _LIBCUDACXX_TEMPLATE(class _Up)
+      (requires (!_LIBCUDACXX_TRAIT(is_same, _Up, _Tp)) _LIBCUDACXX_AND
+                ( _LIBCUDACXX_TRAIT(is_constructible, _Tp, _Up)) _LIBCUDACXX_AND
+                ( _LIBCUDACXX_TRAIT(is_convertible, _Up, _Tp)) _LIBCUDACXX_AND
+                (!__check_constructible_from_opt<_Up>::value))
+    _LIBCUDACXX_INLINE_VISIBILITY constexpr
+    optional(optional<_Up>&& __v) {
        this->__construct_from(_CUDA_VSTD::move(__v));
    }
-    template <class _Up, __enable_if_t<
-        _CheckOptionalLikeCtor<_Up, _Up &&>::template __enable_explicit<_Up>::__value
-    , int> = 0>
-    _LIBCUDACXX_INLINE_VISIBILITY
-    constexpr explicit optional(optional<_Up>&& __v)
-    {
+
+    _LIBCUDACXX_TEMPLATE(class _Up)
+      (requires (!_LIBCUDACXX_TRAIT(is_same, _Up, _Tp)) _LIBCUDACXX_AND
+                ( _LIBCUDACXX_TRAIT(is_constructible, _Tp, _Up)) _LIBCUDACXX_AND
+                (!_LIBCUDACXX_TRAIT(is_convertible, _Up, _Tp)) _LIBCUDACXX_AND
+                (!__check_constructible_from_opt<_Up>::value))
+    _LIBCUDACXX_INLINE_VISIBILITY constexpr
+    explicit optional(optional<_Up>&& __v) {


I don't like this approach. It muddles the purpose of those conditions, forces me to check them in all places to verify they are the same, and generates a lot more code than was there before. If this change is absolutely necessary, I would like you to find a way to give names to these sets of conditions, instead of repeating them over and over again. Same comment on the assignment operators.

I thought I've ran this with 2017, how is it choking on this?

The issue is that MSVC 2017 is falling over its feet when confronted with

explicit optional(const optional<_Up>& __v)

Because it confuses that with the copy constructor. The same applies for any variadic constructor, where we have the same issue. But that applies more to tuple than optional

I was also not that happy with the amount of code. I will have a look whether I can pull it into static variables.

I have reworked the approach to be much more expressive. I hope that is about what you had in mind

miscco · 2023-07-14T07:41:21Z

Regarding the placement of constexpr. I personally consider constexpr more or less noise. We want to make everything constexpr if possible and we are backporting it a lot so it is not even like with other standard libraries, where we have to be careful about it. Essentially everything should either be constexpr in C++14 or if it involves memory allocations it should be constexpr with C++20

The main issue I have is that it also makes the code a lot harder to read because _LIBCUDACXX_CONSTEXPR_AFTER_CXX17 is quite a mouthful and moves relevant stuff to the right.

So my guideline is that we move necessary boilerplate to its own line, aka
_LIBCUDACXX_INLINE_VISIBILITY _LIBCUDACXX_CONSTEXPR_AFTER_CXX17 or
_LIBCUDACXX_HIDE_FROM_ABI _LIBCUDACXX_INLINE_VISIBILITY _LIBCUDACXX_CONSTEXPR_AFTER_CXX17

That leaves us with important information right at the start of the line:

the return type of member functions
whether it is a friend function
whether the constructor is explicit

griwes · 2023-07-14T07:43:08Z

My main point re: constexpr is that I'd rather we not do drive-by formatting changes like this without actually forming a policy on it, and without doing it across the entire library at once.

Also add convenient identifiers for major version

wmaxey · 2023-07-25T23:27:08Z

I remember trying to fix <span> at one point and encountering MSVC trying to instantiate a dynamic span in place of the static span and it not liking the array index. Did this come up? It might be an issue with some sub-version of MSVC.

Error was something like: Array subscript of -1 is illegal.

This has my approval if Michal has no further issues.

miscco · 2023-07-26T08:50:15Z

libcudacxx/include/cuda/std/detail/libcxx/include/span

+    template <class _OtherElementType,
+#ifdef _LIBCUDACXX_COMPILER_MSVC_2017
+              size_t _Extent2 = _Extent, enable_if_t< _Extent2 != dynamic_extent, int> = 0,
+#endif // _LIBCUDACXX_COMPILER_MSVC_2017
+              enable_if_t<!_LIBCUDACXX_TRAIT(is_same, _OtherElementType, _Tp), int> = 0,
+              enable_if_t< _LIBCUDACXX_TRAIT(is_convertible, _OtherElementType(*)[], element_type (*)[]), int> = 0>


@wmaxey this is the fix for the span with negative subscript issue

miscco added libcu++ For all items related to libcu++ bug Something isn't working right. labels Jul 11, 2023

miscco requested review from griwes and wmaxey July 11, 2023 15:26

miscco self-assigned this Jul 11, 2023

miscco changed the title ~~Fix compilation issues with MSVC 2017~~ [BUG] Fix compilation issues with MSVC 2017 Jul 11, 2023

griwes requested changes Jul 14, 2023

View reviewed changes

miscco force-pushed the fix_msvc_2017 branch 4 times, most recently from 83cdaf1 to 9e02a57 Compare July 14, 2023 09:13

miscco requested a review from griwes July 16, 2023 15:44

miscco added 12 commits July 18, 2023 08:56

Remove unneeded "-Wc++17-extensions" warning supppression

0edfc69

Use __pragma for msvc instead of _Pragma

a8b2fe8

NFC: minor cleanup for swap concept emulation

c55cad7

Replace TEST_COMPILER_C1XX with TEST_COMPILER_MSVC

0df3186

Also add convenient identifiers for major version

Make tuple compile with MSVC 2017

f0fb6f3

Make concepts tests pass with MSVC2017

8d9fc23

Make span compile with MSVC 2017

77993c1

Make expected work with MSVC2017

d630c2a

Make exchange test pass for MSVC 2017

935810b

Make optional compile with MSVC 2017

8f8cc8b

Make bind_front pass with MSVC 2017

89dd724

[skip-tests] Suppress some failing tests

c27c8a0

miscco force-pushed the fix_msvc_2017 branch from 9e02a57 to c27c8a0 Compare July 18, 2023 06:56

miscco requested review from a team as code owners July 18, 2023 06:56

miscco removed the request for review from a team July 18, 2023 06:56

miscco requested a review from ericniebler July 18, 2023 06:56

wmaxey approved these changes Jul 25, 2023

View reviewed changes

miscco commented Jul 26, 2023

View reviewed changes

griwes approved these changes Jul 26, 2023

View reviewed changes

miscco merged commit 594ca90 into NVIDIA:main Jul 26, 2023
368 checks passed

miscco deleted the fix_msvc_2017 branch July 26, 2023 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] Fix compilation issues with MSVC 2017 #196

[BUG] Fix compilation issues with MSVC 2017 #196

miscco commented Jul 11, 2023

griwes left a comment •

edited

Loading

griwes Jul 14, 2023

miscco Jul 14, 2023

griwes Jul 14, 2023

miscco Jul 14, 2023

miscco Jul 16, 2023

miscco commented Jul 14, 2023

griwes commented Jul 14, 2023

wmaxey commented Jul 25, 2023 •

edited

Loading

miscco Jul 26, 2023

[BUG] Fix compilation issues with MSVC 2017 #196

[BUG] Fix compilation issues with MSVC 2017 #196

Conversation

miscco commented Jul 11, 2023

griwes left a comment • edited Loading

Choose a reason for hiding this comment

griwes Jul 14, 2023

Choose a reason for hiding this comment

miscco Jul 14, 2023

Choose a reason for hiding this comment

griwes Jul 14, 2023

Choose a reason for hiding this comment

miscco Jul 14, 2023

Choose a reason for hiding this comment

miscco Jul 16, 2023

Choose a reason for hiding this comment

miscco commented Jul 14, 2023

griwes commented Jul 14, 2023

wmaxey commented Jul 25, 2023 • edited Loading

miscco Jul 26, 2023

Choose a reason for hiding this comment

griwes left a comment •

edited

Loading

wmaxey commented Jul 25, 2023 •

edited

Loading