[Lldb-commits] [PATCH] D32316: Change UniqueCStringMap to use ConstString as the key

[Zachary Turner via lldb-commits]

On Thu, Apr 20, 2017 at 9:00 PM Greg Clayton via Phabricator via
lldb-commits <lldb-commits at lists.llvm.org> wrote:

> clayborg requested changes to this revision.
> clayborg added a comment.
> This revision now requires changes to proceed.
>
> Very close. A few misuses of ConstString and this will be good to go.
>
>
>
> ================
> Comment at: include/lldb/Interpreter/Property.h:43
> +  ConstString GetName() const { return m_name; }
> +  ConstString GetDescription() const { return m_description; }
>
> ----------------
> This shouldn't be const-ified, Only names of things like variables,
> enumerators, typenames, paths, and other strings that are going to be
> uniqued should go into the ConstStringPool
>
>
> ================
> Comment at: include/lldb/Symbol/ObjectFile.h:569
>    /// Some object files may have an identifier string embedded in them,
> -  /// e.g. in a Mach-O core file using the LC_IDENT load command (which
> +  /// e.g. in a Mach-O core file using the LC_IDENT load command (which
>    /// is obsolete, but can still be found in some old files)
> ----------------
> Remove whitespace changes. Do as separate checkin if desired. This helps
> keep the noise down in the change in case things need to be cherry picked,.
>
>
> ================
> Comment at: include/lldb/Symbol/ObjectFile.h:576-577
>    //------------------------------------------------------------------
> -  virtual std::string GetIdentifierString () {
> -      return std::string();
> +  virtual std::string GetIdentifierString () {
> +      return std::string();
>    }
> ----------------
> Remove whitespace changes. Do as separate checkin if desired. This helps
> keep the noise down in the change in case things need to be cherry picked,.
>
>
> ================
> Comment at: include/lldb/Symbol/ObjectFile.h:583
>    /// "binary" in the core file.  lldb can iterate over the pages looking
> -  /// for a valid binary, but some core files may have metadata
> +  /// for a valid binary, but some core files may have metadata
>    /// describing where the main binary is exactly which removes ambiguity
> ----------------
> Remove whitespace changes. Do as separate checkin if desired. This helps
> keep the noise down in the change in case things need to be cherry picked,.
>
>
> ================
> Comment at: include/lldb/Symbol/ObjectFile.h:808-811
> +  virtual ConstString
> +  StripLinkerSymbolAnnotations(ConstString symbol_name) const {
> +    return symbol_name;
>    }
> ----------------
> This actually doesn't need to change. Since it is merely stripping off
> parts of the string, this should really stay as a StringRef and it should
> return a StringRef. Change to use StringRef for return and for argument, or
> revert.
>
>
>
> ================
> Comment at: include/lldb/Utility/ConstString.h:481
> +  char operator[](size_t Index) const {
> +    assert(Index < GetLength() && "Invalid index!");
> +    return m_string[Index];
> ----------------
> I really don't like the prospect of crashing the debugger if someone calls
> this with an invalid index. Many people ship with asserts on. I would
> either make it safe if it is indeed just for reporting or remove the assert.
>
Every other string implementation behaves the same way.  No different than
if you have a raw char array and you access it out of bounds.  If you just
remove the assert, then you're not going to find out about this until you
crash off in la-la land after a heap corruption and it will be
significantly harder to diagnose.

In any case, I don't see a way to make it safe either.  If someone accesses
an array out of its bounds, what can you really do?  People shouldn't be
doing that, and if they do we should fail immediately.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20170421/6c859037/attachment.html>