Page MenuHomePhabricator

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

Authored by teemperor on Aug 8 2019, 5:31 AM.



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.

Diff Detail

Event Timeline

teemperor created this revision.Aug 8 2019, 5:31 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 8 2019, 5:31 AM

See also where we have some of the produced logs that contain 'true' instead of the actual content.

labath added a subscriber: dblaikie.

+@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 updated this revision to Diff 214141.Aug 8 2019, 6:46 AM
teemperor retitled this revision from Disallow implicit conversion from const char* to bool in llvm::toStringRef to Disallow implicit conversion from pointers to bool in llvm::toStringRef.
teemperor edited the summary of this revision. (Show Details)
  • Now disallowing all implicit conversions.
  • Fixed typo in comment.

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

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

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 updated this revision to Diff 214315.Aug 9 2019, 12:10 AM
teemperor edited the summary of this revision. (Show Details)
  • Now using std::enable_if
  • Fixed another implicit conversion that seems to be on purpose.
labath accepted this revision.Aug 9 2019, 12:59 AM

It seems everybody agrees that this is the way forward.


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

This revision is now accepted and ready to land.Aug 9 2019, 12:59 AM
dblaikie added inline comments.Aug 9 2019, 10:38 AM

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

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


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

teemperor updated this revision to Diff 220983.Sep 20 2019, 1:54 AM
teemperor marked an inline comment as done.
  • Add GetNameForLanguageTypeAsRef to save some typing.
  • Add comment for why we use enable_if.
teemperor marked 3 inline comments as done.Sep 20 2019, 1:57 AM
teemperor added inline comments.

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 added inline comments.Sep 20 2019, 1:00 PM

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 added inline comments.Sep 23 2019, 12:27 AM

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 added inline comments.Sep 23 2019, 9:27 AM

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)

teemperor abandoned this revision.Oct 24 2019, 10:19 AM
teemperor marked an inline comment as done.