This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by sgraenitz on Aug 9 2018, 2:34 PM.

Details

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.

Event Timeline

sgraenitz created this revision.Aug 9 2018, 2:34 PM
sgraenitz updated this revision to Diff 160014.Aug 9 2018, 2:40 PM

Show allowed cases in ConstStringTest.UpdateMangledCounterpart and format

friss added inline comments.Aug 10 2018, 9:25 AM
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?)

Move assert after try_emplace

sgraenitz marked an inline comment as done.Aug 10 2018, 10:53 AM
sgraenitz added inline comments.
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 accepted this revision.Aug 13 2018, 2:24 PM

This looks fine to me.

This revision is now accepted and ready to land.Aug 13 2018, 2:24 PM
This revision was automatically updated to reflect the committed changes.