<div dir="ltr"><br><br><div class="gmail_quote">On Wed Feb 11 2015 at 2:04:38 PM Greg Clayton <<a href="mailto:gclayton@apple.com">gclayton@apple.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On Feb 11, 2015, at 11:23 AM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Wed Feb 11 2015 at 11:13:22 AM Greg Clayton <<a href="mailto:gclayton@apple.com" target="_blank">gclayton@apple.com</a>> wrote:<br>
><br>
> > On Feb 11, 2015, at 10:18 AM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
> ><br>
> > A patch is up which fixes a number of issues with strings not being null terminated in LLDB. The issues and error-proneness of using raw c strings is well documented and understood, so I would like to propose banning them in LLDB for new code [1].<br>
> ><br>
> > I realize that's a tall order, and I don't expect all the occurrences of them to go away overnight, but issues related to strings have been popping up over and over again, and it would be nice if we could deal with more interesting problems.<br>
> ><br>
> > LLVM has a very robust string manipulation library, and while it's not perfect, it would have prevented almost all of these issues from ever happening in the first place. Here is a quick summary of common C string types / operations and their corresponding LLVM equivalents:<br>
> ><br>
> > Types:<br>
> > char stack_buffer[n]; // Use llvm::SmallString<n>.<br>
><br>
> That would be fine.<br>
><br>
> > const char* foo; // Use llvm::StringRef foo.<br>
><br>
> This may be fine as long as there is absolutely _no_ string manipulation on "foo" (like "strcat(...) etc. Use std::string for anything that requires storage. And we don't want to start using any code that does "let me assign an llvm::StringRef() with a ConstString("hello").<u></u>GetCString() so I don't have to worry about ownership. Adding string to the constant string pool should be reserved for things that actually need it (any const names like namespaces, variable names, type names, paths, etc) and also for when we return "const char *" through the public API.<br>
><br>
> This also means that we can't trust that "foo" is NULL terminated (llvm::StringRef objects have a data pointer and a length and aren't required to be terminated) and it means we must call "foo.str().c_str()" any time we require NULL termination. So I actually don't like llvm::StringRef as a replacement "const char *"...<br>
<br>
> We can't assume that const char*s are null terminated either.<br>
<br>
Yes we can, that is was is assumed by const char * right now for every call we use it with.<br></blockquote><div>Right, but my point is that if we change a const char * to a StringRef(), and that const char* was assumed to be null terminated, then the StringRef is also assumed to be null terminated. You can just call StringRef::data() on it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
But that isn't what is currently meant by _any_ of the functions that take a "const char *" with no length parameter, so it means that any switch to using llvm::StringRef requires we use ".str().c_str()" which I would like to avoid.<br></blockquote><div>I don't see why this is necessary. Just because StringRef supports non-null terminated strings doesn't mean we have to use non null terminated strings with it. declaring a SmallString<> and passing by SmallVectorImpl& supports null terminating strings. You push_back(0) and then pop_back() before returning. This is no different from what we're already doing by setting foo[n] = '\0' except that it's safer since there's no chance of a buffer overrun.</div><div><br></div><div>We are already taking care to make sure all of our const char*s are properly null terminated. Therefore when they are passed to functions that accept a StringRef, they will continue to be null terminated.</div><div><br></div><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
><br>
> ><br>
> > Operations:<br>
> > strcpy, strncpy, etc // Use member functions of SmallString / StringRef<br>
> > sprintf, snprintf, etc // Use llvm::raw_ostream<br>
><br>
> We have StringStream, people should be using that. raw_ostream doesn't support all printf style formats. So all sprintf and snprintf function calls should switch to using lldb_private::StreamString.<br>
<br>
> I actually disagree here. Printf() has plenty of problems, such as non-portable and confusing arguments,<br>
<br>
I agree on this one, but we are getting around this with using PRI macros.<br>
<br>
> lack of type safety,<br>
<br>
most of this is taken care of when you have the printf format warnings on your class.<br>
<br>
> possibility of buffer overruns.<br>
<br>
There are no buffer overruns in the Stream::Printf(). Check the implementation if you need to.<br>
<br>
> I would actually prefer to use raw_ostream instead of StringStream.<br>
<br>
I don't. I have written tools using streaming style stuff before (like std::cout) and also written using printf style stuff and I find the printf style stuff more readable and maintainable. I also don't like streams that maintain state like the std::ostream does. Not sure if raw_ostream does any state management, but when they do it causes bugs later for code that doesn't always explicitly set the format, width, precision and much more:<br>
<br>
std::ostream &os;<br>
os << std::hex << hex_num;<br>
dump(os);<br>
os << hex_num2;<br>
<br>
little did we know the stream was modified by "dump()" to set the format to decimal so now you must always use:<br>
<br>
os << std::hex << hex_num2;<br>
<br>
(repeat for number width, precision and many other factors).<br></blockquote><div>I agree, that's not a pleasant experience. but as an aside, llvm::raw_ostream does not do this. </div></div></div>