Add Twine support for std::string_view.

[saugustine]

Summary

With Twine now ubiquitous after rG92a79dbe91413f685ab19295fc7a6297dbd6c824,
it needs support for string_view when using llvm as a library under C++ standards.

This is similar to how StringRef is handled.

[craig.topper]

llvm/lib/Support/Twine.cpp

48

Is string_view guaranteed to be null terminated?

[saugustine]

Small fix for prototype.

[saugustine]

llvm/lib/Support/Twine.cpp

48

A string_view is not guaranteed to be null terminated, so it includes the size field, which is filled in when the string_view at construction time, possibly by the constructor searching for a null in the string or array of characters, but perhaps just by passing in a length.

[craig.topper]

llvm/lib/Support/Twine.cpp

48

That's what I thought. This function is expecting to return a String that has a null terminator following it isn't it? StringRef isn't directly handled here so I wouldn't expect string_view to be.

[lattner]

LGTM, thanks!

[saugustine]

This addresses the upstream comments.

It also adds a string_view overload for raw_ostream, as now that
a string_view can be converted to both a Twine and a StringRef,
those calls would be ambiguous.

[saugustine]

llvm/lib/Support/Twine.cpp

48

I see what you are saying now. Fixed.

[tstellar]

This patch causes segmentation faults when you have an application compiled with -std=c++17 (The current gcc default) and libLLVM.so compiled with c++14 (The current LLVM project default). Is this a valid use case or do we require that all library users use the same c++ version as the library.

[jyknight]

In D103935#2879061, @tstellar wrote:

This patch causes segmentation faults when you have an application compiled with -std=c++17 (The current gcc default) and libLLVM.so compiled with c++14 (The current LLVM project default). Is this a valid use case or do we require that all library users use the same c++ version as the library.

The most immediate reason is the enum value StdStringViewKind has an (unnecessary) #ifdef around it. Making that unconditional seems entirely reasonable, and is easy enough.

But, even with that, compiling with different standards versions will result in an ODR violation in getSingleStringRef(), which may or may not cause a problem, depending on whether it's inlined, and whether the C++14-compiled version of it wins the link choice. Also, if llvm itself is compiled with c++14 while the consumer is compiled with c++17, the various print methods defined in the cpp file will not work either.

[saugustine]

I'm not sure removing the ifdef guards around the enum value is better.

That would also mean moving the #ifdef guards in all the switch statements so the enum value would be present but unhandled by doing nothing (or asserting or similar) which seems pretty wrong.

[dexonsmith]

In D103935#2879061, @tstellar wrote:

This patch causes segmentation faults when you have an application compiled with -std=c++17 (The current gcc default) and libLLVM.so compiled with c++14 (The current LLVM project default). Is this a valid use case or do we require that all library users use the same c++ version as the library.

Seems like valid/important use case to me!

I wonder if the Twine part of the patch should wait for LLVM to drop support for C++14.

llvm/include/llvm/ADT/Twine.h

299–303

Is this constructor necessary? I would have expected this to get picked up as a conversion by the StringRef constructor... but maybe there are two candidates?

[dexonsmith]

@dblaikie / @lhames , maybe one of you has ideas for how to add Twine support for std::string_view in C++17 and later without changing Twine's ABI?

I think the following won't work, because the lifetime of the StringRef wouldn't be extended to match the lifetime of the calling expression:

Twine(const std::string_view &view) : Twine(StringRef(view)) {}

[saugustine]

In D103935#2881772, @dexonsmith wrote:

Seems like valid/important use case to me!

The ODR violation issues would need to be resolved to support this case.

I wonder if the Twine part of the patch should wait for LLVM to drop support for C++14.

This patch follows on another one that enables Twine support everywhere over std::string. Before that, because strings are trivially convertable to string_views, there was no ambiguity. With that change there is ambiguity in many places, and causes similar problems to the present one for downstream C++ 17 users. Not sure which case should win.

llvm/include/llvm/ADT/Twine.h

299–303

There are indeed two candidates, so we need this one to disambiguate.

[dexonsmith]

In D103935#2881780, @saugustine wrote:

In D103935#2881772, @dexonsmith wrote:

Seems like valid/important use case to me!

The ODR violation issues would need to be resolved to support this case.

Well, there's also the option of reverting this patch.

This patch follows on another one that enables Twine support everywhere over std::string. Before that, because strings are trivially convertable to string_views, there was no ambiguity. With that change there is ambiguity in many places, and causes similar problems to the present one for downstream C++ 17 users. Not sure which case should win.

I took a quick look at the patch mentioned in the description, rG92a79dbe91413f685ab19295fc7a6297dbd6c824, which seems to be just updating MLIR. I imagine there's a way to make those APIs easier to use without breaking LLVM. IMO, a compile-time failure (problem before this patch) is much easier to reason about and fix for a library user than a miscompile (problem after this patch).

What is the ambiguity between? Twine also has a constructor directly from std::string so it's not obvious to me how that patch introduces ambiguity. If the problem is an LLVM client that's using std::string_view instead of StringRef, it seems reasonable for the client to explicitly convert to a StringRef() to use the APIs. Alternatively, specific APIs can add std::string_view wrapper overloads that defer to the Twine versions.

[saugustine]

Reverting this patch might help in the instant case, but the odds of something similar happening, where the data structure is different across different versions of the standard is very hard to guarantee. And also hard to test for.

[dexonsmith]

In D103935#2881938, @saugustine wrote:

Reverting this patch might help in the instant case, but the odds of something similar happening, where the data structure is different across different versions of the standard is very hard to guarantee. And also hard to test for.

When things are hard to test for, we usually rely on vendors to find and report problems during qualification/deployment (like @tstellar did) and then revert/refine/fix the patch that broke them.

Not sure why you see it as likely that a data structure would vary across different versions of the standard. Seems to me that'd only happen when someone has code (like this patch) that explicitly asks for it, assuming your toolchain is ABI compatible between standards versions, like clang and libc++ are.

[dblaikie]

Yeah, the current state of this is not acceptable - compatibility across different standard versions is necessary.

In D103935#2881775, @dexonsmith wrote:

@dblaikie / @lhames , maybe one of you has ideas for how to add Twine support for std::string_view in C++17 and later without changing Twine's ABI?

I think the following won't work, because the lifetime of the StringRef wouldn't be extended to match the lifetime of the calling expression:

Twine(const std::string_view &view) : Twine(StringRef(view)) {}

Yep.

Would it be so bad if we made "Child" larger by storing a StringRef (or equivalent - anything involving a pointer+length) in it (so Child would be two words rather than one)? Then we could collapse several cases into that (StringRef, string_view, std::string, SmallString, (could even do char as well)).

[MaskRay]

llvm/include has such __cplusplus dispatches in a few other places, e.g. llvm/include/llvm/ADT/StringRef.h L82 strLen. So those (-std=c++14 libLLVM.so + -std=c++17 application using llvm/include headers) users are already relying on benign ODR violation.

LLVM has -fvisibility-inlines-hidden, and an additional -Bsymbolic-functions for safeguard, after linking into libLLVM.so, the ODR violation should be benign.

Ensuring that struct/enum sizes are the same for different -std= modes is probably a sufficient fix to regular users (and a sufficiently good workaround for libLLVM.so with mixed -std= users).

---

This thread does raise the question about what degree of support we want to provide for -DLLVM_LINK_LLVM_DYLIB=on users. Before previously shared object users were explicitly not supported. Now it may change as many Linux distributions have switched to -DLLVM_LINK_LLVM_DYLIB=on for their llvm/clang packages for size. With shared objects, we have to care about all sorts of compiler options which can cause non-benign ODR issues.

[dblaikie]

In D103935#2883661, @MaskRay wrote:

llvm/include has such __cplusplus dispatches in a few other places, e.g. llvm/include/llvm/ADT/StringRef.h L82 strLen. So those (-std=c++14 libLLVM.so + -std=c++17 application using llvm/include headers) users are already relying on benign ODR violation.

LLVM has -fvisibility-inlines-hidden, and an additional -Bsymbolic-functions for safeguard, after linking into libLLVM.so, the ODR violation should be benign.

Ensuring that struct/enum sizes are the same for different -std= modes is probably a sufficient fix to regular users (and a sufficiently good workaround for libLLVM.so with mixed -std= users).

---

This thread does raise the question about what degree of support we want to provide for -DLLVM_LINK_LLVM_DYLIB=on users. Before previously shared object users were explicitly not supported. Now it may change as many Linux distributions have switched to -DLLVM_LINK_LLVM_DYLIB=on for their llvm/clang packages for size. With shared objects, we have to care about all sorts of compiler options which can cause non-benign ODR issues.

I think this will be an issue for static or dynamic linking, due to inlining. Importantly - if LLVM was built withoun string_view support but a caller was built with it and that caller tries to pass a string_view into some LLVM API then even if the layout and enum elements were consistent, we'd still hit UB/assert/etc because the Twine passed in would have that unexpected enum value which wouldn't be covered by the Twine code inside LLVM.

Having the implementation be the same (by using some form of char*+length representation which would always be supported), having an extra ctor that takes string_view but lowers to that same representation should be safe (yes, a benign ODR violation - the debug info will be a bit weird/missing that extra function/ctor - but codewise, I can't think of any way it wouldn't work).

[saugustine]

I will implement dblaikie's idea, which should unblock in the short term, but I think a larger conversation about the level of support that the project wants for this would be good to have.

[saugustine]

https://reviews.llvm.org/D106186 should fix.

[bogner]

Just ran into this issue (we were a bit behind on integrating upstream) - glad to see it's already fixed. Regarding the following:

Reverting this patch might help in the instant case, but the odds of something similar happening, where the data structure is different across different versions of the standard is very hard to guarantee. And also hard to test for.

This particular case isn't a particularly egregious case of issues being hard to detect, given that it's an ABI change based on an ifdef - LLVM has a long history of avoiding doing this kind of thing (either through being compiler agnostic with Compiler.h or using LLVM_ENABLE_ABI_BREAKING_CHECKS)

llvm/include/llvm/ADT/Twine.h

 #define LLVM_ADT_TWINE_H
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <cassert>
 #include <cstdint>
 #include <string>
+#if __cplusplus > 201402L
+#include <string_view>
+#endif
 
 namespace llvm {
 
   class formatv_object_base;
   class raw_ostream;
 
   /// Twine - A lightweight data structure for efficiently representing the
   /// concatenation of temporary values as strings.
enum NodeKind : unsigned char {
       CStringKind,
 
       /// A pointer to an std::string instance.
       StdStringKind,
 
       /// A pointer to a StringRef instance.
       StringRefKind,
 
+#if __cplusplus > 201402L
+      // A pointer to a std::string_view instance.
+      StdStringViewKind,
+#endif
+
       /// A pointer to a SmallString instance.
       SmallStringKind,
 
       /// A pointer to a formatv_object_base instance.
       FormatvObjectKind,
 
       /// A char value, to render as a character.
       CharKind,
#endif
     };
 
     union Child
     {
       const Twine *twine;
       const char *cString;
       const std::string *stdString;
       const StringRef *stringRef;
+#if __cplusplus > 201402L
+      const std::string_view *stdStringView;
+#endif
       const SmallVectorImpl<char> *smallString;
       const formatv_object_base *formatvObject;
       char character;
       unsigned int decUI;
       int decI;
       const unsigned long *decUL;
       const long *decL;
       const unsigned long long *decULL;
public:
     /*implicit*/ Twine(std::nullptr_t) = delete;
 
     /// Construct from an std::string.
     /*implicit*/ Twine(const std::string &Str) : LHSKind(StdStringKind) {
       LHS.stdString = &Str;
       assert(isValid() && "Invalid twine!");
     }
 
+#if __cplusplus > 201402L
+    /// Construct from an std::string_view.
+    /*implicit*/ Twine(const std::string_view &Str)
+        : LHSKind(StdStringViewKind) {
+      LHS.stdStringView = &Str;
+      assert(isValid() && "Invalid twine!");
+    }
+#endif
+
     /// Construct from a StringRef.
     /*implicit*/ Twine(const StringRef &Str) : LHSKind(StringRefKind) {
       LHS.stringRef = &Str;
       assert(isValid() && "Invalid twine!");
     }
 
     /// Construct from a SmallString.
     /*implicit*/ Twine(const SmallVectorImpl<char> &Str)
#endif
     /// StringRef value with getSingleStringRef().
     bool isSingleStringRef() const {
       if (getRHSKind() != EmptyKind) return false;
 
       switch (getLHSKind()) {
       case EmptyKind:
       case CStringKind:
       case StdStringKind:
+#if __cplusplus > 201402L
+      case StdStringViewKind:
+#endif
       case StringRefKind:
       case SmallStringKind:
         return true;
       default:
         return false;
       }
     }
 
#endif
     void toVector(SmallVectorImpl<char> &Out) const;
 
     /// This returns the twine as a single StringRef.  This method is only valid
     /// if isSingleStringRef() is true.
     StringRef getSingleStringRef() const {
       assert(isSingleStringRef() &&"This cannot be had as a single stringref!");
       switch (getLHSKind()) {
       default: llvm_unreachable("Out of sync with isSingleStringRef");
-      case EmptyKind:      return StringRef();
-      case CStringKind:    return StringRef(LHS.cString);
-      case StdStringKind:  return StringRef(*LHS.stdString);
-      case StringRefKind:  return *LHS.stringRef;
+      case EmptyKind:
+        return StringRef();
+      case CStringKind:
+        return StringRef(LHS.cString);
+      case StdStringKind:
+        return StringRef(*LHS.stdString);
+#if __cplusplus > 201402L
+      case StdStringViewKind:
+        return StringRef(*LHS.stdStringView);
+#endif
+      case StringRefKind:
+        return *LHS.stringRef;
       case SmallStringKind:
         return StringRef(LHS.smallString->data(), LHS.smallString->size());
       }
     }
 
     /// This returns the twine as a single StringRef if it can be
     /// represented as such. Otherwise the twine is written into the given
     /// SmallVector and a StringRef to the SmallVector's data is returned.

llvm/include/llvm/Support/raw_ostream.h

 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DataTypes.h"
 #include <cassert>
 #include <chrono>
 #include <cstddef>
 #include <cstdint>
 #include <cstring>
 #include <string>
+#if __cplusplus > 201402L
+#include <string_view>
+#endif
 #include <system_error>
 #include <type_traits>
 
 namespace llvm {
 
 class formatv_object_base;
 class format_object_base;
 class FormattedString;
raw_ostream &operator<<(const char *Str) {
     return this->operator<<(StringRef(Str));
   }
 
   raw_ostream &operator<<(const std::string &Str) {
     // Avoid the fast path, it would only increase code size for a marginal win.
     return write(Str.data(), Str.length());
   }
 
+#if __cplusplus > 201402L
+  raw_ostream &operator<<(const std::string_view &Str) {
+    return write(Str.data(), Str.length());
+  }
+#endif
+
   raw_ostream &operator<<(const SmallVectorImpl<char> &Str) {
     return write(Str.data(), Str.size());
   }
 
   raw_ostream &operator<<(unsigned long N);
   raw_ostream &operator<<(long N);
   raw_ostream &operator<<(unsigned long long N);
   raw_ostream &operator<<(long long N);

llvm/lib/Support/Twine.cpp

if (isUnary()) {
     case CStringKind:
       // Already null terminated, yay!
       return StringRef(LHS.cString);
     case StdStringKind: {
       const std::string *str = LHS.stdString;
       return StringRef(str->c_str(), str->size());
     }
     default:
       break;
     }
   }
   toVector(Out);
   Out.push_back(0);
   Out.pop_back();
   return StringRef(Out.data(), Out.size());
 }
 
 void Twine::printOneChild(raw_ostream &OS, Child Ptr,
                           NodeKind Kind) const {
   switch (Kind) {
   case Twine::NullKind: break;
   case Twine::EmptyKind: break;
   case Twine::TwineKind:
     Ptr.twine->print(OS);
     break;
   case Twine::CStringKind:
     OS << Ptr.cString;
     break;
   case Twine::StdStringKind:
     OS << *Ptr.stdString;
     break;
+#if __cplusplus > 201402L
+  case StdStringViewKind:
+    OS << StringRef(*Ptr.stdStringView);
+    break;
+#endif
   case Twine::StringRefKind:
     OS << *Ptr.stringRef;
     break;
   case Twine::SmallStringKind:
     OS << *Ptr.smallString;
     break;
   case Twine::FormatvObjectKind:
     OS << *Ptr.formatvObject;
void Twine::printOneChildRepr(raw_ostream &OS, Child Ptr,
   case Twine::CStringKind:
     OS << "cstring:\""
        << Ptr.cString << "\"";
     break;
   case Twine::StdStringKind:
     OS << "std::string:\""
        << Ptr.stdString << "\"";
     break;
+#if __cplusplus > 201402L
+  case Twine::StdStringViewKind:
+    OS << "std::string_view:\"" << StringRef(*Ptr.stdStringView) << "\"";
+    break;
+#endif
   case Twine::StringRefKind:
     OS << "stringref:\""
        << Ptr.stringRef << "\"";
     break;
   case Twine::SmallStringKind:
     OS << "smallstring:\"" << *Ptr.smallString << "\"";
     break;
   case Twine::FormatvObjectKind:

llvm/unittests/ADT/TwineTest.cpp

TEST(TwineTest, Construction) {
   EXPECT_EQ("", Twine().str());
   EXPECT_EQ("hi", Twine("hi").str());
   EXPECT_EQ("hi", Twine(std::string("hi")).str());
   EXPECT_EQ("hi", Twine(StringRef("hi")).str());
   EXPECT_EQ("hi", Twine(StringRef(std::string("hi"))).str());
   EXPECT_EQ("hi", Twine(StringRef("hithere", 2)).str());
   EXPECT_EQ("hi", Twine(SmallString<4>("hi")).str());
   EXPECT_EQ("hi", Twine(formatv("{0}", "hi")).str());
+#if __cplusplus > 201402L
+  EXPECT_EQ("hi", Twine(std::string_view("hi")).str());
+#endif
 }
 
 TEST(TwineTest, Numbers) {
   EXPECT_EQ("123", Twine(123U).str());
   EXPECT_EQ("123", Twine(123).str());
   EXPECT_EQ("-123", Twine(-123).str());
   EXPECT_EQ("123", Twine(123).str());
   EXPECT_EQ("-123", Twine(-123).str());
TEST(TwineTest, Concat) {
   EXPECT_EQ("(Twine smallstring:\"hi\" empty)", 
             repr(Twine().concat(Twine(SmallString<5>("hi")))));
   EXPECT_EQ("(Twine formatv:\"howdy\" empty)",
             repr(Twine(formatv("howdy")).concat(Twine())));
   EXPECT_EQ("(Twine formatv:\"howdy\" empty)",
             repr(Twine().concat(Twine(formatv("howdy")))));
   EXPECT_EQ("(Twine smallstring:\"hey\" cstring:\"there\")", 
             repr(Twine(SmallString<7>("hey")).concat(Twine("there"))));
+#if __cplusplus > 201402L
+  EXPECT_EQ("(Twine std::string_view:\"hey\" cstring:\"there\")",
+            repr(Twine(std::string_view("hey")).concat(Twine("there"))));
+#endif
 
   // Concatenation of unary ropes.
   EXPECT_EQ("(Twine cstring:\"a\" cstring:\"b\")", 
             repr(Twine("a").concat(Twine("b"))));
 
   // Concatenation of other ropes.
   EXPECT_EQ("(Twine rope:(Twine cstring:\"a\" cstring:\"b\") cstring:\"c\")", 
             repr(Twine("a").concat(Twine("b")).concat(Twine("c"))));