Disallow implicit conversion from pointers to bool in llvm::toStringRef

[teemperor]

Summary

We use toStringRef in LLDB in a few places to convert const char* variables to llvm::StringRef.
toStringRef however has no const char* overload (as this is already implemented in the constructor),
so calling toStringRef with a const char will implicitly convert the pointer to true/false, which means
that toStringRef("Hello") will produce the string "true" without any compiler warning.

This patch makes the toStringRef only accept values that are actually bool.

[teemperor]

See also https://bugs.llvm.org/show_bug.cgi?id=42895#c11 where we have some of the produced logs that contain 'true' instead of the actual content.

[labath]

+@dblaikie. I know who would the llvm owner here be, but he's probably close enough.

I think this is definitely a good idea, but I'm wondering if we shouldn't take this one step further, and disallow any implicit conversions to bool for this function. Or at least conversions from pointer types...

[teemperor]

• Now disallowing all implicit conversions.
• Fixed typo in comment.

[teemperor]

I'm fine with extending this to all pointers as it doesn't seem to be used in that way anywhere.

[davide]

I think this is definitely a good idea but Chandler or DBlaikie might want to take a look at this one too.

[dblaikie]

Yeah, might be worth changing toStringRef to only accept bools:

template<typename T>
typename std::enable_if<std::is_same<T, bool>::value, StringRef>::type toStringRef(T B) { ... }

Or something like that?

That'd avoid accidental "toStringRef(5)" or the like...

[teemperor]

• Now using std::enable_if
• Fixed another implicit conversion that seems to be on purpose.

[labath]

It seems everybody agrees that this is the way forward.

llvm/include/llvm/ADT/StringExtras.h

51–58

This enable_if trick might be worth a short comment...

[dblaikie]

lldb/source/Symbol/TypeSystem.cpp

349

Perhaps Language::GetNameForLanguageType should return StringRef to start with?

(& guessing there should be an lldb test case demonstrating this fix?)

llvm/include/llvm/ADT/StringExtras.h

51–58

Sure, something like "ensure only bools are accepted here to avoid surprising implicit conversion to bool (for ints, pointers, etc)" if you like.

[teemperor]

• Add GetNameForLanguageTypeAsRef to save some typing.
• Add comment for why we use enable_if.

[teemperor]

lldb/source/Symbol/TypeSystem.cpp

349

The problem is that this is used in LLDB's SB API which returns a const char* (and we can't change that). So we can't return a StringRef as that doesn't convert back to const char * (and we can't go over std::string or anything like that as the API doesn't transfer ownership back to the caller). I added an alternative method to save some typing (and maybe we can go fully StringRef in the future if we ever change the SB API strings).

And we don't test log message content in LLDB. Also the log message only happens on systems with unsupported type system (like MIPS or something like that). And we don't have any such bot that would even run the test.

[dblaikie]

lldb/source/Symbol/TypeSystem.cpp

349

Those both seem a bit problematic...

Is there no API migration strategy in the SB API? Introducing new versions of functions and deprecating old ones?

And testing - clearly this code was buggy and worth fixing, so it ought to be worth testing. Any chance of unit testing/API-level testing any of this?

[labath]

lldb/source/Symbol/TypeSystem.cpp

349

Is there no API migration strategy in the SB API? Introducing new versions of functions and deprecating old ones?

Right now, there isn't. However, I am not very happy with SB layer restrictions constraining the internal APIs. One of the ways around that (which is already used in a some places) is to pass the value through a ConstString at the SB layer. This will guarantee that the string is null terminated and persistent (at the cost of some cpu and memory). Though I am not very fond of that solution either, it does not seem like too bad of an option for this case, as the set of language names is limited.

As for testing of logging, there definitely are ways to test that, but we (usually) don't do that. I think the best analogy here would be the LLVM_DEBUG output in llvm. This is also mainly a debugging aid, and we usually don't have dedicated tests for that (though it would certainly be possible to do that). I think both mechanisms would benefit from some testing, but the question is how much testing, and whether that time wouldn't be better spent improving test coverage elsewhere...

[dblaikie]

lldb/source/Symbol/TypeSystem.cpp

349

Fair enough. Pity, but ah well.

Might be possible to add a unit test that demonstrates the SFINAE at work (I don't know for sure, though - you'd need to introduce an overload set that would've preferred toStringRef(bool) before the change, but now prefers some overload in the unit test with different behavior you could use to differentiate the two & show the SFINAE at work)

lldb/include/lldb/Target/Language.h

public:
   // These are accessors for general information about the Languages lldb knows
   // about:
 
   static lldb::LanguageType
   GetLanguageTypeFromString(const char *string) = delete;
   static lldb::LanguageType GetLanguageTypeFromString(llvm::StringRef string);
 
   static const char *GetNameForLanguageType(lldb::LanguageType language);
+  static llvm::StringRef GetNameForLanguageTypeAsRef(lldb::LanguageType lang) {
+      return GetNameForLanguageType(lang);
+  }
 
   static void PrintAllLanguages(Stream &s, const char *prefix,
                                 const char *suffix);
 
   // return false from callback to stop iterating
   static void ForAllLanguages(std::function<bool(lldb::LanguageType)> callback);
 
   static bool LanguageIsCPlusPlus(lldb::LanguageType language);

lldb/source/Symbol/TypeSystem.cpp

if (m_clear_in_progress) {
     if (pos != m_map.end()) {
       auto *type_system = pos->second.get();
       if (type_system) {
         llvm::consumeError(std::move(error));
         return *type_system;
       }
       error = llvm::make_error<llvm::StringError>(
           "TypeSystem for language " +
-              llvm::toStringRef(Language::GetNameForLanguageType(language)) +
+              Language::GetNameForLanguageTypeAsRef(language) +
               " doesn't exist",
           llvm::inconvertibleErrorCode());
       return std::move(error);
     }
 
     for (const auto &pair : m_map) {
       if (pair.second && pair.second->SupportsLanguage(language)) {
         // Add a new mapping for "language" to point to an already existing
         // TypeSystem that supports this language
         m_map[language] = pair.second;
         if (pair.second.get()) {
           llvm::consumeError(std::move(error));
           return *pair.second.get();
         }
         error = llvm::make_error<llvm::StringError>(
             "TypeSystem for language " +
-                llvm::toStringRef(Language::GetNameForLanguageType(language)) +
+                Language::GetNameForLanguageTypeAsRef(language) +
                 " doesn't exist",
             llvm::inconvertibleErrorCode());
         return std::move(error);
       }
     }
 
     if (!can_create) {
       error = llvm::make_error<llvm::StringError>(
           "Unable to find type system for language " +
-              llvm::toStringRef(Language::GetNameForLanguageType(language)),
+              Language::GetNameForLanguageTypeAsRef(language),
           llvm::inconvertibleErrorCode());
     } else {
       // Cache even if we get a shared pointer that contains a null type system
       // back
       auto type_system_sp = TypeSystem::CreateInstance(language, module);
       m_map[language] = type_system_sp;
       if (type_system_sp.get()) {
         llvm::consumeError(std::move(error));
         return *type_system_sp.get();
       }
       error = llvm::make_error<llvm::StringError>(
           "TypeSystem for language " +
-              llvm::toStringRef(Language::GetNameForLanguageType(language)) +
+              Language::GetNameForLanguageTypeAsRef(language) +
               " doesn't exist",
           llvm::inconvertibleErrorCode());
     }
   }
 
   return std::move(error);
 }
 
if (m_clear_in_progress) {
     if (pos != m_map.end()) {
       auto *type_system = pos->second.get();
       if (type_system) {
         llvm::consumeError(std::move(error));
         return *type_system;
       }
       error = llvm::make_error<llvm::StringError>(
           "TypeSystem for language " +
-              llvm::toStringRef(Language::GetNameForLanguageType(language)) +
+              Language::GetNameForLanguageTypeAsRef(language) +
               " doesn't exist",
           llvm::inconvertibleErrorCode());
       return std::move(error);
     }
 
     for (const auto &pair : m_map) {
       if (pair.second && pair.second->SupportsLanguage(language)) {
         // Add a new mapping for "language" to point to an already existing
         // TypeSystem that supports this language
         m_map[language] = pair.second;
         if (pair.second.get()) {
           llvm::consumeError(std::move(error));
           return *pair.second.get();
         }
         error = llvm::make_error<llvm::StringError>(
             "TypeSystem for language " +
-                llvm::toStringRef(Language::GetNameForLanguageType(language)) +
+                Language::GetNameForLanguageTypeAsRef(language) +
                 " doesn't exist",
             llvm::inconvertibleErrorCode());
         return std::move(error);
       }
     }
 
     if (!can_create) {
       error = llvm::make_error<llvm::StringError>(
           "Unable to find type system for language " +
-              llvm::toStringRef(Language::GetNameForLanguageType(language)),
+              Language::GetNameForLanguageTypeAsRef(language),
           llvm::inconvertibleErrorCode());
     } else {
       // Cache even if we get a shared pointer that contains a null type system
       // back
       auto type_system_sp = TypeSystem::CreateInstance(language, target);
       m_map[language] = type_system_sp;
       if (type_system_sp.get()) {
         llvm::consumeError(std::move(error));
         return *type_system_sp.get();
       }
       error = llvm::make_error<llvm::StringError>(
           "TypeSystem for language " +
-              llvm::toStringRef(Language::GetNameForLanguageType(language)) +
+              Language::GetNameForLanguageTypeAsRef(language) +
               " doesn't exist",
           llvm::inconvertibleErrorCode());
     }
   }
 
   return std::move(error);
 }

llvm/include/llvm/ADT/StringExtras.h

 /// null string.
 inline std::vector<StringRef> toStringRefArray(const char *const *Strings) {
   std::vector<StringRef> Result;
   while (*Strings)
     Result.push_back(*Strings++);
   return Result;
 }
 
-/// Construct a string ref from a boolean.
-inline StringRef toStringRef(bool B) { return StringRef(B ? "true" : "false"); }
+/// Construct a StringRef from a bool.
+// Uses enable_if to prevent implicit conversion to bool from pointers, ints, etc.
+template <typename T>
+typename std::enable_if<std::is_same<T, bool>::value, StringRef>::type
+toStringRef(T B) {
+  return StringRef(B ? "true" : "false");
+}
 
 /// Construct a string ref from an array ref of unsigned chars.
 inline StringRef toStringRef(ArrayRef<uint8_t> Input) {
   return StringRef(reinterpret_cast<const char *>(Input.begin()), Input.size());
 }
 
 /// Construct a string ref from an array ref of unsigned chars.
 inline ArrayRef<uint8_t> arrayRefFromStringRef(StringRef Input) {
   return {Input.bytes_begin(), Input.bytes_end()};

llvm/include/llvm/CodeGen/CommandFlags.inc

if (FramePointerUsage.getNumOccurrences() > 0) {
         NewAttrs.addAttribute("frame-pointer", "all");
       else if (FramePointerUsage == llvm::FramePointer::NonLeaf)
         NewAttrs.addAttribute("frame-pointer", "non-leaf");
       else if (FramePointerUsage == llvm::FramePointer::None)
         NewAttrs.addAttribute("frame-pointer", "none");
     }
     if (DisableTailCalls.getNumOccurrences() > 0)
       NewAttrs.addAttribute("disable-tail-calls",
-                            toStringRef(DisableTailCalls));
+                            toStringRef((bool)DisableTailCalls));
     if (StackRealign)
       NewAttrs.addAttribute("stackrealign");
 
     if (TrapFuncName.getNumOccurrences() > 0)
       for (auto &B : F)
         for (auto &I : B)
           if (auto *Call = dyn_cast<CallInst>(&I))
             if (const auto *F = Call->getCalledFunction())