Fix: ConstString::GetConstCStringAndSetMangledCounterPart() should update the value if the key exists already

[sgraenitz]

Summary

This issue came up because it caused problems in our unit tests. The StringPool did connect counterparts only once and silently ignored the values passed in subsequent calls.
The simplest solution for the unit tests would be silent overwrite. In practice, however, it seems useful to assert that we never overwrite a different mangled counterpart.
If we ever have mangled counterparts for other languages than C++, this makes it more likely to notice collisions.

I added an assertion that allows the following cases:

• inserting a new value
• overwriting the empty string
• overwriting with an identical value

I fixed the unit tests, which used "random" strings and thus produced collisions.
It would be even better if there was a way to reset or isolate the StringPool, but that's a different story.

[sgraenitz]

Show allowed cases in ConstStringTest.UpdateMangledCounterpart and format

[friss]

source/Utility/ConstString.cpp

123–126

In an asserts build, this is going to d a bunch of additional lookups. Can we move the assert after the try_emplace and basically assert that entry.second == nullptr || !strcmp(mangled_ccstr, entry.second).

(It's unclear to me whether a pointer comparison is enough to test string equality in this case, do we know that everything passed to this function has been uniqued?)

[sgraenitz]

Move assert after try_emplace

[sgraenitz]

source/Utility/ConstString.cpp

123–126

Can we move the assert after the try_emplace

Yes, right that's better.

and basically assert that entry.second == nullptr || !strcmp(mangled_ccstr, entry.second)

I think it's worth allowing "overwrite empty string". There is code that does my_const_string.SetCString("") to indicate failure, maybe other code does it to indicate e.g. later..

It's unclear to me whether a pointer comparison is enough

I think it's safe to keep it. The ccstr postfix is used all over to indicate that the code assumes a pool string. Pool::GetConstCStringAndSetMangledCounterPart is only used once in ConstString and the Pool class is defined locally in the cpp.

[jingham]

This looks fine to me.

source/Utility/ConstString.cpp

public:
   GetConstCStringAndSetMangledCounterPart(llvm::StringRef demangled,
                                           const char *mangled_ccstr) {
     const char *demangled_ccstr = nullptr;
 
     {
       const uint8_t h = hash(demangled);
       llvm::sys::SmartScopedWriter<false> wlock(m_string_pools[h].m_mutex);
 
-      // Make string pool entry with the mangled counterpart already set
-      StringPoolEntryType &entry =
-          *m_string_pools[h]
-               .m_string_map.insert(std::make_pair(demangled, mangled_ccstr))
-               .first;
+      // Make or update string pool entry with the mangled counterpart
+      StringPool &map = m_string_pools[h].m_string_map;
+      StringPoolEntryType &entry = *map.try_emplace(demangled).first;
+
+      assert((entry.second == nullptr || entry.second == mangled_ccstr ||
+              strlen(entry.second) == 0) &&
+             "The demangled string must have a unique counterpart or otherwise "
+             "it must be empty");
+
+      entry.second = mangled_ccstr;
 
       // Extract the const version of the demangled_cstr
       demangled_ccstr = entry.getKeyData();
     }
 
     {
       // Now assign the demangled const string as the counterpart of the
       // mangled const string...

unittests/Utility/ConstStringTest.cpp

 
 using namespace lldb_private;
 
 TEST(ConstStringTest, format_provider) {
   EXPECT_EQ("foo", llvm::formatv("{0}", ConstString("foo")).str());
 }
 
 TEST(ConstStringTest, MangledCounterpart) {
-  ConstString foo("foo");
+  ConstString uvw("uvw");
   ConstString counterpart;
-  EXPECT_FALSE(foo.GetMangledCounterpart(counterpart));
+  EXPECT_FALSE(uvw.GetMangledCounterpart(counterpart));
   EXPECT_EQ("", counterpart.GetStringRef());
 
-  ConstString bar;
-  bar.SetStringWithMangledCounterpart("bar", foo);
-  EXPECT_EQ("bar", bar.GetStringRef());
+  ConstString xyz;
+  xyz.SetStringWithMangledCounterpart("xyz", uvw);
+  EXPECT_EQ("xyz", xyz.GetStringRef());
 
-  EXPECT_TRUE(bar.GetMangledCounterpart(counterpart));
-  EXPECT_EQ("foo", counterpart.GetStringRef());
+  EXPECT_TRUE(xyz.GetMangledCounterpart(counterpart));
+  EXPECT_EQ("uvw", counterpart.GetStringRef());
 
-  EXPECT_TRUE(foo.GetMangledCounterpart(counterpart));
-  EXPECT_EQ("bar", counterpart.GetStringRef());
+  EXPECT_TRUE(uvw.GetMangledCounterpart(counterpart));
+  EXPECT_EQ("xyz", counterpart.GetStringRef());
+}
+
+TEST(ConstStringTest, UpdateMangledCounterpart) {
+  { // Add counterpart
+    ConstString some1;
+    some1.SetStringWithMangledCounterpart("some", ConstString(""));
+  }
+  { // Overwrite empty string
+    ConstString some2;
+    some2.SetStringWithMangledCounterpart("some", ConstString("one"));
+  }
+  { // Overwrite with identical value
+    ConstString some2;
+    some2.SetStringWithMangledCounterpart("some", ConstString("one"));
+  }
+  { // Check counterpart is set
+    ConstString counterpart;
+    EXPECT_TRUE(ConstString("some").GetMangledCounterpart(counterpart));
+    EXPECT_EQ("one", counterpart.GetStringRef());
+  }
 }
 
 TEST(ConstStringTest, FromMidOfBufferStringRef) {
   // StringRef's into bigger buffer: no null termination
-  const char *buffer = "foobarbaz";
+  const char *buffer = "abcdefghi";
   llvm::StringRef foo_ref(buffer, 3);
   llvm::StringRef bar_ref(buffer + 3, 3);
 
   ConstString foo(foo_ref);
 
   ConstString bar;
   bar.SetStringWithMangledCounterpart(bar_ref, foo);
-  EXPECT_EQ("bar", bar.GetStringRef());
+  EXPECT_EQ("def", bar.GetStringRef());
 
   ConstString counterpart;
   EXPECT_TRUE(bar.GetMangledCounterpart(counterpart));
-  EXPECT_EQ("foo", counterpart.GetStringRef());
+  EXPECT_EQ("abc", counterpart.GetStringRef());
 
   EXPECT_TRUE(foo.GetMangledCounterpart(counterpart));
-  EXPECT_EQ("bar", counterpart.GetStringRef());
+  EXPECT_EQ("def", counterpart.GetStringRef());
 }
 
 TEST(ConstStringTest, NullAndEmptyStates) {
   ConstString foo("foo");
   EXPECT_FALSE(!foo);
   EXPECT_FALSE(foo.IsEmpty());
   EXPECT_FALSE(foo.IsNull());