[Lldb-commits] [PATCH] D62756: Be consistent when adding names and mangled names to the index

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Being consistent definitely sounds like a good idea. Since this does change behavior somewhat, I'm wondering whether it would make sense to add a test here. The thing that's not clear to me is whether this change in behavior is only theoretical, or if it can also happen in a real-world scenario. We can always hand-construct an DIE which will have a linkage name, but no "normal" name, but I don't see how would this ever happen in practice. How did you initially discover this inconsistency?

> We might think about adding a feature where if there is a mangled name and no simple name, chop up the demangled name like we do in the ObjectFile plug-ins when we see mangled names so we extract the basename out and then could use this as the simple name. This might help expressions find things correctly by basename

My worry about that is two-fold:

- it adds a demangling step (a pretty expensive operation) to something that is already the biggest bottleneck in loading DWARF
- this isn't something that we would do when using accelerator tables, so this will result in inconsistent behavior in these two cases

To understand the tradeoffs here, it would be good to understand under what circumstances can this situation occur...



================
Comment at: source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp:114
+static inline bool AddMangled(const char *name, const char *mangled) {
+  if (mangled == nullptr || name == mangled)
+    return false;
----------------
aprantl wrote:
> Even in a static function I feel weird comparing two char* by pointer value. Can you use llvm::StringRefs instead? then we don't need to call the unsafe strcmp either and just trust that operator== is implemented efficiently.
Since this is *the* hottest piece of code when not using the accelerator tables, I think it makes sense to spend some time to squeeze every last bit of performace from it. The problem I see with constructing a StringRef is that it will force a linear scan of the c string regardless of the result of comparison. Since (I expect) the most comon result of this function will be "true", it means we will end up adding this string to the lookup tables, which will mean another linear scan due to the construction of ConstString.

Though, I guess that means that if the most frequent (>95%) result here is "unequal", we can just optimistically ConstString-ify both names, and rely on the ConstString operator== to do the fast comparison. That may result in less overall branches in the code and faster execution, but this is just a speculation on my part.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62756/new/

https://reviews.llvm.org/D62756