Page MenuHomePhabricator

Fix StringRef::strLen in windows with clang++ C++17
Needs RevisionPublic

Authored by isuruf on Mar 28 2020, 2:02 PM.

Details

Summary

This was found when compiling f18 using clang++ on windows.
f18 requires C++17 standard, but char_traits::length is not
a constexpr as required by the C++17 standard.

Diff Detail

Event Timeline

isuruf created this revision.Mar 28 2020, 2:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2020, 2:02 PM
aaron.ballman added a subscriber: aaron.ballman.

I looked at the definition available in MSVC 2017 (15.9) and it does have constexpr support, but it requires setting the conformance mode to properly honor __cplusplus. If you set /Zc:__cplusplus, I believe this code should work. Have you given that a shot?

isuruf added a comment.Tue, May 5, 8:05 AM

I've only used this from clang. How do I set it using clang's gnu driver?

I've only used this from clang. How do I set it using clang's gnu driver?

clang-cl.exe /std:c++17 /Zc:__cplusplus /E -Xclang -dM C:\Users\aballman\Desktop\test.cpp (test.cpp is an empty file) lists #define __cplusplus 201703L, while dropping either /Zc: or changing /std: changes the reported value.

rnk added a comment.Tue, May 5, 3:58 PM

If the compiler has the builtin, IMO we should prefer the builtin. It's always going to be better and faster than the library implementation. I would recommend reordering the ifdefs to prefer __builtin_strlen over the STL.

isuruf updated this revision to Diff 262257.Tue, May 5, 4:31 PM

Prefer compiler builtin over C++ standard library

rnk accepted this revision.Tue, May 5, 4:57 PM

lgtm

llvm/include/llvm/ADT/StringRef.h
84

Oh, I thought that would eliminate the need for this ifdef check. Do we actually support < 1916 MSVC versions? I guess we do, so this is necessary. :( Sorry that makes my original suggestion less helpful.

This revision is now accepted and ready to land.Tue, May 5, 4:57 PM
aaron.ballman added inline comments.Tue, May 5, 5:06 PM
llvm/include/llvm/ADT/StringRef.h
84

I still think the addition of !defined(_MSC_VER) is incorrect and that LLVM's CMake needs to be updated to pass /Zc:__cplusplus to MSVC.

However, I think there's a different change to make possibly. We require MSVC 2017 with the latest updates installed (https://llvm.org/docs/GettingStartedVS.html#software), but _MSC_VER == 1916 is 2017 Update 9 (Visual Studio 2017 version 15.9.11). I currently have Visual Studio 2017 version 15.9.22 installed, so it seems like the version check for the builtin branch can turn into defined(_MSC_VER) and the changes to the STL branch can be removed.

aaron.ballman requested changes to this revision.Tue, May 5, 5:07 PM
This revision now requires changes to proceed.Tue, May 5, 5:07 PM
isuruf marked an inline comment as done.Tue, May 5, 5:36 PM
isuruf added inline comments.
llvm/include/llvm/ADT/StringRef.h
84

I'm using _MSC_VER == 1911 and LLVM builds fine with that version. It doesn't have the correct constexpr value. Hence this differential.

isuruf marked an inline comment as done.Tue, May 5, 5:39 PM
isuruf added inline comments.
llvm/include/llvm/ADT/StringRef.h
84

I'm using a cross-compiling setup with clang-cl and MSVC headers and libraries. 1911 was the last version that Microsoft published as a package that I can use on Linux easily.

aaron.ballman added inline comments.Wed, May 6, 4:52 AM
llvm/include/llvm/ADT/StringRef.h
84

I'm using _MSC_VER == 1911 and LLVM builds fine with that version. It doesn't have the correct constexpr value. Hence this differential.

This does not meet our minimum system requirements, unfortunately.

I'm using a cross-compiling setup with clang-cl and MSVC headers and libraries. 1911 was the last version that Microsoft published as a package that I can use on Linux easily.

This is an interesting use case to consider, thank you for sharing it! Does 19.11 support /Zc:__cplusplus? I have access to 19.10, which does not, and 19.14, which does. If 19.11 supports a conforming __cplusplus value, then I think supporting that version isn't much of a problem. If it doesn't support that flag, then we have to replicate this sort of workaround in too many places and I'd be opposed to supporting that version. For reference, 19.11 is VS 2017 Update 4 and 19.16 is VS 2017 Update 9, so there's a pretty big delta between the versions.

isuruf marked an inline comment as done.Wed, May 6, 7:40 AM
isuruf added inline comments.
llvm/include/llvm/ADT/StringRef.h
84

clang-cl supports that flag regardless of the VS installation used. I also see some codepaths on the MSVC headers that use #if __cplusplus > 201402. If there's any other info you'd like, I'd be happy to share.

aaron.ballman added inline comments.Wed, May 6, 11:06 AM
llvm/include/llvm/ADT/StringRef.h
84

Thank you! I don't think I need any further information; I think we need to add the /Zc flag in CMake and remove the _MSC_VER changes here (but the reordering to prefer builtins is good). @rnk, does that make sense to you as well?

isuruf marked an inline comment as done.Wed, May 6, 11:24 AM
isuruf added inline comments.
llvm/include/llvm/ADT/StringRef.h
84

@aaron.ballman, that still doesn't fix my problem where std::char_traits<char>::length(Str) is not constexpr on 1911 even if __cplusplus is set to a higher value.

mstorsjo added inline comments.
llvm/include/llvm/ADT/StringRef.h
84

I'm curious - in which way was 1911 published that made it easier to use on Linux than other versions?

On https://github.com/mstorsjo/msvc-wine/ I have a python script that fetches Visual Studio installer manifests and downloads and unpacks the files without needing to actually run wine. (The other half of the repo, scripts for fixing up inconsistent header casing and wrapping MSVC in Wine, is optional if you just want it downloaded and unpacked.)

aaron.ballman added inline comments.Wed, May 6, 12:24 PM
llvm/include/llvm/ADT/StringRef.h
84

Sorry, you're right, that's still true. I guess it boils down to whether we support 19.11 or not. According to our documentation, we don't support it, which is why I came to the conclusion I did and suggested those changes, but that doesn't help you much.

However, if we do want to support it with a small change, we could figure out what _MSC_VER value to test against and test that here. What I'm opposed to with this patch it restricts us from ever using the STL with Visual Studio, which is a heavy hammer.

isuruf marked an inline comment as done.Mon, May 18, 11:43 PM
isuruf added inline comments.
llvm/include/llvm/ADT/StringRef.h
84

What I'm opposed to with this patch it restricts us from ever using the STL with Visual Studio, which is a heavy hammer.

As @rnk said, it's always better to use the compiler builtins over STL.

aaron.ballman added inline comments.Tue, May 19, 5:08 AM
llvm/include/llvm/ADT/StringRef.h
84

We seem to be going around in circles. This is what I'm looking for: if the builtin is available, use it (the builtin is available in all versions of MSVC we support). If the builtin is not available, check the STL and if it's new enough, use it (we should not get here for MSVC because we should be using the builtin). If the STL is not available, use the manual implementation (we shouldn't ever get here, but having a fallback seems sensible). I believe this is what I'm looking for:

    // Constexpr version of std::strlen.
    static constexpr size_t strLen(const char *Str) {
#if __has_builtin(__builtin_strlen) || defined(__GNUC__) || defined(_MSC_VER)
      return __builtin_strlen(Str);
#elif __cplusplus > 201402L
      return std::char_traits<char>::length(Str);
#else
      const char *Begin = Str;
      while (*Str != '\0')
        ++Str;
      return Str - Begin;
#endif
    }

We don't have to set /Zc in CMake to make this work, though I still argue we should be setting it regardless (though perhaps not as part of this patch).