Be consistent when adding names and mangled names to the index

[clayborg]

Summary

We were being very inconsistent with the way we were adding names to the indexes.

For DW_TAG_subprogram and DW_TAG_inlined_subroutine we would not add the mangled name if the we didn't have a simple name. We would only add the mangled name if started with '_' (even if it matches the simple name that started with '_') or if the name differed if the mangled name didn't start with '_'.

For DW_TAG_array_type, DW_TAG_base_type, DW_TAG_class_type, DW_TAG_constant, DW_TAG_enumeration_type, DW_TAG_string_type, DW_TAG_structure_type, DW_TAG_subroutine_type, DW_TAG_typedef, DW_TAG_union_type, and DW_TAG_unspecified_type we would add the simple name and the mangled name if either existed, even if they were the same..

For DW_TAG_variable we would only follow the same rule as the DW_TAG_subprogram and DW_TAG_inlined_subroutine (so no simple name meant we wouldn't add the mangled name).

Not sure if this was on purpose or not, but wanted to post this diff for discussion on what the right thing to do should be.

The new way adds a helper function called AddMangled that takes both the name and mangled name and:

• returns false if mangled is NULL or of name == mangled (same string in .debug_str)
• returns true if name is NULL so we add the mangled name even if we don't have a simple name
• returns true if the first character of mangled is different than the first character in simple name (not just blindly accepting any mangled name that starts with '_' like before)
• return true if name differs from mangled using strcmp as a last resort

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

[aprantl]

source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

114

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.

[labath]

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...

source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

114

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.

[clayborg]

In D62756#1526996, @labath wrote:

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?

I added asserts in the else clause if code like:

if (name) {
  ...
} else {
  assert(mangled_str == nullptr);
}

And I got the assert. So this is a real issue.

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...

Fine not doing this yet.

I will post a new diff with a simpler solution that maintains performance to address Adrian's question of using StringRef.

[clayborg]

Changed to use ConstString in AddMangled which simplifies the logic quite a bit. I verified performance didn't regress.

[clayborg]

Remove commented out code.

[labath]

In D62756#1538439, @clayborg wrote:

In D62756#1526996, @labath wrote:

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?

I added asserts in the else clause if code like:

if (name) {
  ...
} else {
  assert(mangled_str == nullptr);
}

And I got the assert. So this is a real issue.

Ok, so in that case, can you add a test for this? It may be easiest to hand-craft some DIEs with appropriate attributes. Then you could either use lldb-test symbols to dump the indexes, or lldb-test symbols -find=variable --name=foo to actually query them.

source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

void ManualDWARFIndex::IndexUnit(DWARFUnit &unit, IndexSet &set) {
 
   SymbolFileDWARFDwo *dwo_symbol_file = unit.GetDwoSymbolFile();
   if (dwo_symbol_file && dwo_symbol_file->GetCompileUnit()) {
     IndexUnitImpl(*dwo_symbol_file->GetCompileUnit(), cu_language,
                   unit.GetOffset(), set);
   }
 }
 
+static inline bool AddMangled(ConstString name, ConstString mangled) {
+  return mangled && name != mangled;
+}
+
 void ManualDWARFIndex::IndexUnitImpl(
     DWARFUnit &unit, const LanguageType cu_language,
     const dw_offset_t cu_offset, IndexSet &set) {
   for (const DWARFDebugInfoEntry &die : unit.dies()) {
     const dw_tag_t tag = die.Tag();
 
     switch (tag) {
     case DW_TAG_array_type:
for (const DWARFDebugInfoEntry &die : unit.dies()) {
     case DW_TAG_variable:
       break;
 
     default:
       continue;
     }
 
     DWARFAttributes attributes;
-    const char *name = nullptr;
+    const char *name_cstr = nullptr;
     const char *mangled_cstr = nullptr;
     bool is_declaration = false;
     // bool is_artificial = false;
     bool has_address = false;
     bool has_location_or_const_value = false;
     bool is_global_or_static_variable = false;
 
     DWARFFormValue specification_die_form;
     const size_t num_attributes = die.GetAttributes(&unit, attributes);
     if (num_attributes > 0) {
       for (uint32_t i = 0; i < num_attributes; ++i) {
         dw_attr_t attr = attributes.AttributeAtIndex(i);
         DWARFFormValue form_value;
         switch (attr) {
         case DW_AT_name:
           if (attributes.ExtractFormValueAtIndex(i, form_value))
-            name = form_value.AsCString();
+            name_cstr = form_value.AsCString();
           break;
 
         case DW_AT_declaration:
           if (attributes.ExtractFormValueAtIndex(i, form_value))
             is_declaration = form_value.Unsigned() != 0;
           break;
 
         //                case DW_AT_artificial:
if (num_attributes > 0) {
       }
     }
 
     DIERef ref(unit.GetDebugSection(), cu_offset, die.GetOffset());
     switch (tag) {
     case DW_TAG_inlined_subroutine:
     case DW_TAG_subprogram:
       if (has_address) {
+        ConstString name(name_cstr);
         if (name) {
-          ObjCLanguage::MethodName objc_method(name, true);
+          ObjCLanguage::MethodName objc_method(name.GetStringRef(), true);
           if (objc_method.IsValid(true)) {
             ConstString objc_class_name_with_category(
                 objc_method.GetClassNameWithCategory());
             ConstString objc_selector_name(objc_method.GetSelector());
             ConstString objc_fullname_no_category_name(
                 objc_method.GetFullNameWithoutCategory(true));
             ConstString objc_class_name_no_category(objc_method.GetClassName());
-            set.function_fullnames.Insert(ConstString(name), ref);
+            set.function_fullnames.Insert(name, ref);
             if (objc_class_name_with_category)
               set.objc_class_selectors.Insert(objc_class_name_with_category,
                                               ref);
             if (objc_class_name_no_category &&
                 objc_class_name_no_category != objc_class_name_with_category)
               set.objc_class_selectors.Insert(objc_class_name_no_category, ref);
             if (objc_selector_name)
               set.function_selectors.Insert(objc_selector_name, ref);
             if (objc_fullname_no_category_name)
               set.function_fullnames.Insert(objc_fullname_no_category_name,
                                             ref);
           }
           // If we have a mangled name, then the DW_AT_name attribute is
           // usually the method name without the class or any parameters
           bool is_method = DWARFDIE(&unit, &die).IsMethod();
 
           if (is_method)
-            set.function_methods.Insert(ConstString(name), ref);
+            set.function_methods.Insert(name, ref);
           else
-            set.function_basenames.Insert(ConstString(name), ref);
+            set.function_basenames.Insert(name, ref);
 
           if (!is_method && !mangled_cstr && !objc_method.IsValid(true))
-            set.function_fullnames.Insert(ConstString(name), ref);
-        }
-        if (mangled_cstr) {
-          // Make sure our mangled name isn't the same string table entry as
-          // our name. If it starts with '_', then it is ok, else compare the
-          // string to make sure it isn't the same and we don't end up with
-          // duplicate entries
-          if (name && name != mangled_cstr &&
-              ((mangled_cstr[0] == '_') ||
-               (::strcmp(name, mangled_cstr) != 0))) {
-            set.function_fullnames.Insert(ConstString(mangled_cstr), ref);
-          }
+            set.function_fullnames.Insert(name, ref);
         }
+        // Make sure our mangled name isn't the same as our name.
+        ConstString mangled(mangled_cstr);
+        if (AddMangled(name, mangled))
+          set.function_fullnames.Insert(mangled, ref);
       }
       break;
 
     case DW_TAG_array_type:
     case DW_TAG_base_type:
     case DW_TAG_class_type:
     case DW_TAG_constant:
     case DW_TAG_enumeration_type:
     case DW_TAG_string_type:
     case DW_TAG_structure_type:
     case DW_TAG_subroutine_type:
     case DW_TAG_typedef:
     case DW_TAG_union_type:
     case DW_TAG_unspecified_type:
-      if (name && !is_declaration)
-        set.types.Insert(ConstString(name), ref);
-      if (mangled_cstr && !is_declaration)
-        set.types.Insert(ConstString(mangled_cstr), ref);
+      if (!is_declaration) {
+        ConstString name(name_cstr);
+        if (name)
+          set.types.Insert(name, ref);
+        // Make sure our mangled name isn't the same as our name.
+        ConstString mangled(mangled_cstr);
+        if (AddMangled(name, mangled))
+          set.types.Insert(mangled, ref);
+      }
       break;
 
     case DW_TAG_namespace:
-      if (name)
-        set.namespaces.Insert(ConstString(name), ref);
+      if (name_cstr)
+        set.namespaces.Insert(ConstString(name_cstr), ref);
       break;
 
     case DW_TAG_variable:
-      if (name && has_location_or_const_value && is_global_or_static_variable) {
-        set.globals.Insert(ConstString(name), ref);
+      if (has_location_or_const_value && is_global_or_static_variable) {
+        ConstString name(name_cstr);
+        if (name)
+          set.globals.Insert(name, ref);
         // Be sure to include variables by their mangled and demangled names if
         // they have any since a variable can have a basename "i", a mangled
         // named "_ZN12_GLOBAL__N_11iE" and a demangled mangled name
         // "(anonymous namespace)::i"...
 
-        // Make sure our mangled name isn't the same string table entry as our
-        // name. If it starts with '_', then it is ok, else compare the string
-        // to make sure it isn't the same and we don't end up with duplicate
-        // entries
-        if (mangled_cstr && name != mangled_cstr &&
-            ((mangled_cstr[0] == '_') || (::strcmp(name, mangled_cstr) != 0))) {
-          set.globals.Insert(ConstString(mangled_cstr), ref);
-        }
+        // Make sure our mangled name isn't the same as our name.
+        ConstString mangled(mangled_cstr);
+        if (AddMangled(name, mangled))
+          set.globals.Insert(mangled, ref);
       }
       break;
 
     default:
       continue;
     }
   }
 }