Make the symbol demangling loop order independent

[scott.smith]

Summary

Currently, the loop will insert entries into the class_contexts set, and then use the absence or presence to affect decisions made by later iterations of the same loop.

In order to support parallelizing the loop, this change moves those decisions to always occur after the main loop, instead of sometimes occurring after the loop.

[scott.smith]

I welcome any suggestions on how to update the comments near the code I touched. I can make the code functionally the same, but it doesn't mean I know why it's doing what it's doing :-)

source/Symbol/Symtab.cpp

382

Note the old code checked that symbol_contexts[] (aka const_context above) is "true." However, because of the code above, we would never get here unless it is true. So I removed the check.

385

Note that adding to m_method_to_index happened in both code paths in the old code, so I moved it up to the main loop.

[jingham]

Adding Greg as a reviewer. You can generally see from the CODE_OWNERS.txt file who has overall responsibility for areas in lldb, and you should at least assign them as a reviewer. For Symbol parsing stuff that's definitely Greg.

[scott.smith]

Turns out I'm planning on making more drastic changes to this loop, so there's no point in reviewing this step.

[labath]

If you're going to be making drastic changes here, could you please look at the possibility of making a more targeted test, rather than relying on the "run the whole debugger and set a breakpoint" type of tests to verify the finer details of the implementation. I was able to do a unit test for a bug in elf parsing code in D32434, which is not that far from here. That gives me hope it may be possible, but it's hard to say without knowing what kind of changes you would like to make.

source/Symbol/Symtab.cpp

for (entry.value = 0; entry.value < num_symbols; ++entry.value) {
                 // The first character of the demangled basename is '~' which
                 // means we have a class destructor. We can use this information
                 // to help us know what is a class and what isn't.
                 if (class_contexts.find(const_context) == class_contexts.end())
                   class_contexts.insert(const_context);
                 m_method_to_index.Append(entry);
               } else {
                 if (const_context && const_context[0]) {
-                  if (class_contexts.find(const_context) !=
-                      class_contexts.end()) {
-                    // The current decl context is in our "class_contexts" which
-                    // means
-                    // this is a method on a class
-                    m_method_to_index.Append(entry);
-                  } else {
-                    // We don't know if this is a function basename or a method,
-                    // so put it into a temporary collection so once we are done
-                    // we can look in class_contexts to see if each entry is a
-                    // class
-                    // or just a function and will put any remaining items into
-                    // m_method_to_index or m_basename_to_index as needed
-                    mangled_name_to_index.Append(entry);
-                    symbol_contexts[entry.value] = const_context;
-                  }
+                  // Note this is common to both paths, though
+                  m_method_to_index.Append(entry);
                 } else {
                   // No context for this function so this has to be a basename
                   m_basename_to_index.Append(entry);
                   // If there is no context (no namespaces or class scopes that
                   // come before the function name) then this also could be a
                   // fullname.
                   if (cxx_method.GetContext().empty())
                     m_name_to_index.Append(entry);
for (entry.value = 0; entry.value < num_symbols; ++entry.value) {
             objc_method.GetFullNameWithoutCategory(true));
         if (objc_method_no_category) {
           entry.cstring = objc_method_no_category.GetStringRef();
           m_name_to_index.Append(entry);
         }
       }
     }
 
-    size_t count;
-    if (!mangled_name_to_index.IsEmpty()) {
-      count = mangled_name_to_index.GetSize();
-      for (size_t i = 0; i < count; ++i) {
-        if (mangled_name_to_index.GetValueAtIndex(i, entry.value)) {
-          entry.cstring = mangled_name_to_index.GetCStringAtIndex(i);
-          if (symbol_contexts[entry.value] &&
-              class_contexts.find(symbol_contexts[entry.value]) !=
-                  class_contexts.end()) {
-            m_method_to_index.Append(entry);
-          } else {
-            // If we got here, we have something that had a context (was inside
-            // a namespace or class)
-            // yet we don't know if the entry
-            m_method_to_index.Append(entry);
-            m_basename_to_index.Append(entry);
-          }
-        }
-      }
-    }
+    size_t count = mangled_name_to_index.GetSize();
+        if (class_contexts.find(symbol_contexts[entry.value]) ==
+          // a namespace or class) yet we don't know if the entry
+
     m_name_to_index.Sort();
     m_name_to_index.SizeToFit();
     m_selector_to_index.Sort();
     m_selector_to_index.SizeToFit();
     m_basename_to_index.Sort();
     m_basename_to_index.SizeToFit();
     m_method_to_index.Sort();
     m_method_to_index.SizeToFit();