[Expression parser] Look up module symbols before hunting globally

[spyffe]

Summary

When it resolved symbol-only variables, the expression parser currently looks only in the global module list. It should prefer the current module.

I've fixed that behavior by making it search the current module first, and only search globally if it finds nothing. I've also added a test case.

[clayborg]

There is a SymbolContext function sorts types based on the context contained in a SymbolContext. Seems like we should do the same thing here? See SymbolContext::SortTypeList().

[jingham]

Yes, this seems obviously right.

[jingham]

Actually, I take that back. Why do you have to call FindGlobalDataSymbol twice? Shouldn't FindGlobalDataSymbol do that work for you. After all you passed in the module. It should itself prefer symbols in the module...

It also seems wrong that we're just picking the first one in the case where we find two symbols at the same level distant from the current module. Shouldn't that be an error?

[clayborg]

In D33083#751836, @jingham wrote:

Actually, I take that back. Why do you have to call FindGlobalDataSymbol twice? Shouldn't FindGlobalDataSymbol do that work for you. After all you passed in the module. It should itself prefer symbols in the module...

It also seems wrong that we're just picking the first one in the case where we find two symbols at the same level distant from the current module. Shouldn't that be an error?

Yes, that is why I suggested always doing a full search and then with a SymbolContextList that results by sorting things from the currently module (maybe more than 1), and then from other modules (maybe more than 1). Then the picking would start:
1- prefer exported symbols from the current module, if more than 1 match this criteria, then error
2- prefer exported symbols from other modules, if more than 1 match this criteria, then error
3 - prefer private symbols from current module, if more than 1 match this criteria, then error
4 - prefer private symbols from other modules, if more than 1 match this criteria, then error

Maybe 2 and 3 should be switched? I don't think so, but others might.

[clayborg]

Bonus point for implementing a global symbol search that is aware of the above rules and can stop after step 1 if there are matches, and after step 2 if there are matches, etc.

[jingham]

If you just have symbols, you can't tell whether private symbols were visible to the current frame or not, but the likelihood is not 0 (I guess it's something like 1/number of CU's). OTOH, your expression could never have seen a private symbol from another module, and 1/nCU > 0. So I would weakly argue for flipping 2 & 3.

[jingham]

But note that the real solution to that problem is implementing some syntax for "symbol from CU", "symbol from Function" etc, as we've discussed in the past. Then what we do by default will be less important, since you have a way to override it.

[spyffe]

I also would flip 2 and 3. I'll give this a shot.

[spyffe]

Improved symbol lookup per Greg and Jim's suggestion, with flipped steps 2 and 3.
Also added testing for the error case.

[clayborg]

The searching code is indeed better and what we now want, but I question of this code should live in ClangExpressionDeclMap::FindGlobalDataSymbol. A better home for this might be in SymbolContext::FindGlobalDataSymbol? Then others can use this functionality. Other clients must want this kind of functionality (other expression parsers). Seems a shame to have it live somewhere where someone would have to copy the code. Thoughts anyone?

[jingham]

I think Greg's right. I can't see anything expression parser specific to how you do this search. I was a little worried that a lot of the other searches will return multiple names (maybe sorting to indicate closeness.) So this one might be confusing. But the API name says you are looking for ONE data symbol, so the behavior when you can't tell amongst the matches actually makes sense.

[clayborg]

Yeah, we need to document exactly what this is doing: looking for the best symbol as an expression parser would want it, preferring symbols from the current module in the SymbolContext, and then looking everywhere. Errors if multiple exist in current module, or in any other modules if not in current, bla bla.

[labath]

Is this feature really darwin specific? Isn't the __private_extern__ thingy equivalent to __attribute__(visibility("hidden"))), which is supported by gcc and clang alike?

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile

1

For a portable way to write a Makefile with multiple shared libraries, please look at testcases/lang/cpp/namespace_definitions. The solution is not very elegant, but at least it avoid hardcoding .dylib everywhere.

[spyffe]

Two changes after Greg and Pavel's suggestions:

• Moved the symbol lookup function into the global context; and
• Rewrote the test case's build system to be more generic.

[labath]

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk

1 ↗

(On Diff #99084)

Thanks for the effort. It almost works for me :), except for the part where you clear out the CFLAGS. We cannot do that, as CFLAGS sometimes contains important flags that we cannot compile without. The "canonical" way to compile without debug info is to use the CFLAGS_NO_DEBUG flag and write the compile rule yourself.

This makefile worked for me, and I hope it will work for you as well as it was pieced together from existing tests:

LEVEL := ../../../make

DYLIB_NAME := Two
DYLIB_C_SOURCES := Two/Two.c Two/TwoConstant.c
DYLIB_ONLY := YES

include $(LEVEL)/Makefile.rules

CFLAGS += -fPIC

Two/TwoConstant.o: Two/TwoConstant.c
	$(CC) $(CFLAGS_NO_DEBUG) -c $< -o $@

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py

17

Since the test does not depend on debug info, you may want to consider adding
NO_DEBUG_INFO_TESTCASE = True here to avoid running it multiple times.

[spyffe]

Updated to reflect Pavel Labath's suggestions:

• Used CFLAGS_NO_DEBUG where appropriate
• Marked the testcase as having no debug info variants

[clayborg]

Just a few makefile changes where CFLAGS is being modified and this will be good to go. Much better location for this. Sean, please keep an eye out for this kind of thing in the future, I believe there is a lot of type searching code and function searching code that could have the same thing done in the future.

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile

7

CFLAGS_EXTRAS is the way to add CFLAGS:

CFLAGS_EXTRAS += "-g -O0"

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk

9 ↗

(On Diff #99186)

avoid modifying CFLAGS, modify CFLAGS_EXTRAS and then use it if needed

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two.mk

9 ↗

(On Diff #99186)

avoid modifying CFLAGS, modify CFLAGS_EXTRAS

[spyffe]

Used CFLAGS_EXTRAS instead of CFLAGS at Greg Clayton's suggestion.

[clayborg]

Very nice.

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile

+LEVEL = ../../../make
+
+DEBUG_CFLAGS = -g -O0
+NDEBUG_CFLAGS = -O0
+LDFLAGS = $(CFLAGS) -lobjc -framework Foundation
+
+all: a.out libOne.dylib libTwo.dylib
+
+libOne.dylib: One/One.c One/OneConstant.c
+  $(CC) $(DEBUG_CFLAGS) -I. -c -o One.o One/One.c
+  $(CC) $(NDEBUG_CFLAGS) -I. -c -o OneConstant.o One/OneConstant.c
+  $(CC) $(LDFLAGS) -shared -o libOne.dylib One.o OneConstant.o
+  dsymutil libOne.dylib
+
+libTwo.dylib: Two/Two.c Two/TwoConstant.c
+  $(CC) $(DEBUG_CFLAGS) -I. -c -o Two.o Two/Two.c
+  $(CC) $(NDEBUG_CFLAGS) -I. -c -o TwoConstant.o Two/TwoConstant.c
+  $(CC) $(LDFLAGS) -shared -o libTwo.dylib Two.o TwoConstant.o
+  dsymutil libTwo.dylib
+
+a.out: main.c libOne.dylib libTwo.dylib
+  $(CC) $(DEBUG_CFLAGS) $(LDFLAGS) -I. -L. -lOne -lTwo -o a.out main.c 
+
+.PHONY: clean
+
+clean:
+  rm -rf *.dylib *.o *.dSYM a.out

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h

+#ifndef ONE_H
+#define ONE_H
+void one();
+#endif

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c

+#include "One.h"
+#include <stdio.h>
+
+void one() {
+  printf("One\n"); // break here
+}

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c

+__private_extern__ int conflicting_symbol = 11111;

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py

+"""Test that conflicting symbols in different shared libraries work correctly"""
+
+from __future__ import print_function
+
+
+import os
+import time
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestRealDefinition(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipUnlessDarwin
+    def test_frame_var_after_stop_at_implementation(self):
+        if self.getArchitecture() == 'i386':
+            self.skipTest("requires modern objc runtime")
+        self.build()
+        self.common_setup()
+
+        One_line = line_number('One/One.c', '// break here')
+        Two_line = line_number('Two/Two.c', '// break here')
+        main_line = line_number('main.c', '// break here')
+        lldbutil.run_break_set_by_file_and_line(
+            self, 'One.c', One_line, num_expected_locations=1, loc_exact=True)
+        lldbutil.run_break_set_by_file_and_line(
+            self, 'Two.c', Two_line, num_expected_locations=1, loc_exact=True)
+        lldbutil.run_break_set_by_file_and_line(
+            self, 'main.c', main_line, num_expected_locations=1, loc_exact=True)
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        # The stop reason of the thread should be breakpoint.
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs=['stopped',
+                             'stop reason = breakpoint'])
+
+        self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,
+                    substrs=[' resolved, hit count = 1'])
+
+        # This should display correctly.
+        self.expect(
+            "expr (unsigned long long)conflicting_symbol",
+            "Symbol from One should be found",
+            substrs=[
+                "11111"])
+
+        self.runCmd("continue", RUN_SUCCEEDED)
+
+        # The stop reason of the thread should be breakpoint.
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs=['stopped',
+                             'stop reason = breakpoint'])
+
+        self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,
+                    substrs=[' resolved, hit count = 1'])
+
+        self.expect(
+            "expr (unsigned long long)conflicting_symbol",
+            "Symbol from Two should be found",
+            substrs=[
+                "22222"])
+
+        self.runCmd("continue", RUN_SUCCEEDED)
+
+        # The stop reason of the thread should be breakpoint.
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+                    substrs=['stopped',
+                             'stop reason = breakpoint'])
+
+        self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,
+                    substrs=[' resolved, hit count = 1'])
+
+        self.expect(
+            "expr (unsigned long long)conflicting_symbol",
+            "An error should be printed when symbols can't be ordered",
+            error=True,
+            substrs=[
+                "Multiple internal symbols"])
+
+    def common_setup(self):
+        exe = os.path.join(os.getcwd(), "a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h

+#ifndef TWO_H
+#define TWO_H
+void two();
+#endif

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c

+#include "Two.h"
+#include <stdio.h>
+
+void two() {
+  printf("Two\n"); // break here
+}

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c

+__private_extern__ int conflicting_symbol = 22222;

packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c

+#include "One/One.h"
+#include "Two/Two.h"
+
+#include <stdio.h>
+
+int main() {
+  one();
+  two();
+  printf("main\n"); // break here
+  return(0); 
+}

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h

private:
   /// Get this parser's ID for use in extracting parser- and JIT-specific
   /// data from persistent variables.
   //----------------------------------------------------------------------
   uint64_t GetParserID() { return (uint64_t) this; }
 
   //------------------------------------------------------------------
   /// Given a target, find a data symbol that has the given name.
   ///
-  /// @param[in] target
-  ///     The target to use as the basis for the search.
+  /// @param[in] lookup_context
+  ///     The symbol context to use to order the search.
   ///
   /// @param[in] name
   ///     The name as a plain C string.
   ///
-  /// @param[in] module
-  ///     The module to limit the search to. This can be NULL
+  /// @param[in] error
+  ///     An error to populate with information if there is a lookup failure
   ///
   /// @return
   ///     The LLDB Symbol found, or NULL if none was found.
   //------------------------------------------------------------------
-  const Symbol *FindGlobalDataSymbol(Target &target, const ConstString &name,
-                                     Module *module = NULL);
+  const Symbol *FindGlobalDataSymbol(SymbolContext &lookup_context,
+                                     const ConstString &name,
+                                     Error &error);
 
   //------------------------------------------------------------------
   /// Given a target, find a variable that matches the given name and
   /// type.
   ///
   /// @param[in] target
   ///     The target to use as a basis for finding the variable.
   ///

source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

if (!m_parser_vars->m_exe_ctx.GetTargetPtr())
     return false;
 
   return GetSymbolAddress(m_parser_vars->m_exe_ctx.GetTargetRef(),
                           m_parser_vars->m_exe_ctx.GetProcessPtr(), name,
                           symbol_type);
 }
 
 const Symbol *ClangExpressionDeclMap::FindGlobalDataSymbol(
-    Target &target, const ConstString &name, lldb_private::Module *module) {
-  SymbolContextList sc_list;
+    SymbolContext &lookup_context, const ConstString &name, Error &error) {
+  error.Clear();
   
-  if (module)
-    module->FindSymbolsWithNameAndType(name, eSymbolTypeAny, sc_list);
-  else
-    target.GetImages().FindSymbolsWithNameAndType(name, eSymbolTypeAny,
-                                                  sc_list);
+  if (!lookup_context.target_sp) {
+    return nullptr;
+  }
+  
+  Target &target = *lookup_context.target_sp;
+  Module *module = lookup_context.module_sp.get();
   
+  auto ProcessMatches = [this, &name, &target, module]
+      (SymbolContextList &sc_list, Error &error) -> const Symbol* {
+    llvm::SmallVector<const Symbol *, 1> external_symbols;
+    llvm::SmallVector<const Symbol *, 1> internal_symbols;
     const uint32_t matches = sc_list.GetSize();
     for (uint32_t i = 0; i < matches; ++i) {
       SymbolContext sym_ctx;
       sc_list.GetContextAtIndex(i, sym_ctx);
       if (sym_ctx.symbol) {
         const Symbol *symbol = sym_ctx.symbol;
         const Address sym_address = symbol->GetAddress();
         
         if (sym_address.IsValid()) {
           switch (symbol->GetType()) {
           case eSymbolTypeData:
           case eSymbolTypeRuntime:
           case eSymbolTypeAbsolute:
           case eSymbolTypeObjCClass:
           case eSymbolTypeObjCMetaClass:
           case eSymbolTypeObjCIVar:
             if (symbol->GetDemangledNameIsSynthesized()) {
               // If the demangled name was synthesized, then don't use it
               // for expressions. Only let the symbol match if the mangled
               // named matches for these symbols.
               if (symbol->GetMangled().GetMangledName() != name)
                 break;
             }
-          return symbol;
-
+            if (symbol->IsExternal()) {
+              external_symbols.push_back(symbol);
+            } else {
+              internal_symbols.push_back(symbol);
+            }
+            break;
           case eSymbolTypeReExported: {
             ConstString reexport_name = symbol->GetReExportedSymbolName();
             if (reexport_name) {
               ModuleSP reexport_module_sp;
               ModuleSpec reexport_module_spec;
               reexport_module_spec.GetPlatformFileSpec() =
-                symbol->GetReExportedSymbolSharedLibrary();
               if (reexport_module_spec.GetPlatformFileSpec()) {
                 reexport_module_sp =
-                  target.GetImages().FindFirstModule(reexport_module_spec);
                 if (!reexport_module_sp) {
                   reexport_module_spec.GetPlatformFileSpec()
-                    .GetDirectory()
-                    .Clear();
                   reexport_module_sp =
-                    target.GetImages().FindFirstModule(reexport_module_spec);
                 }
               }
               // Don't allow us to try and resolve a re-exported symbol if it is
-            // the same
-            // as the current symbol
+              // the same as the current symbol
               if (name == symbol->GetReExportedSymbolName() &&
                   module == reexport_module_sp.get())
-              return NULL;
+                return nullptr;
               
-            return FindGlobalDataSymbol(target,
+              return FindGlobalDataSymbol(sym_ctx,
                                           symbol->GetReExportedSymbolName(),
-                                        reexport_module_sp.get());
+                                          error);
             }
           } break;
               
           case eSymbolTypeCode: // We already lookup functions elsewhere
           case eSymbolTypeVariable:
           case eSymbolTypeLocal:
           case eSymbolTypeParam:
           case eSymbolTypeTrampoline:
           case eSymbolTypeInvalid:
           case eSymbolTypeException:
           case eSymbolTypeSourceFile:
           case eSymbolTypeHeaderFile:
           case eSymbolTypeObjectFile:
           case eSymbolTypeCommonBlock:
           case eSymbolTypeBlock:
           case eSymbolTypeVariableType:
           case eSymbolTypeLineEntry:
           case eSymbolTypeLineHeader:
           case eSymbolTypeScopeBegin:
           case eSymbolTypeScopeEnd:
           case eSymbolTypeAdditional:
           case eSymbolTypeCompiler:
           case eSymbolTypeInstrumentation:
           case eSymbolTypeUndefined:
           case eSymbolTypeResolver:
             break;
           }
         }
       }
     }
         
-  return NULL;
+    if (external_symbols.size() > 1) {
+      StreamString ss;
+      ss.Printf("Multiple external symbols found for '%s'\n", name.AsCString());
+      for (const Symbol *symbol : external_symbols) {
+        symbol->GetDescription(&ss, eDescriptionLevelFull, &target);
+      }
+      ss.PutChar('\n');
+      error.SetErrorString(ss.GetData());
+      return nullptr;
+    } else if (external_symbols.size()) {
+      return external_symbols[0];
+    } else if (internal_symbols.size() > 1) {
+      StreamString ss;
+      ss.Printf("Multiple internal symbols found for '%s'\n", name.AsCString());
+      for (const Symbol *symbol : internal_symbols) {
+        symbol->GetDescription(&ss, eDescriptionLevelVerbose, &target);
+        ss.PutChar('\n');
+      }
+      error.SetErrorString(ss.GetData());
+      return nullptr;
+    } else if (internal_symbols.size()) {
+      return internal_symbols[0];
+    } else {
+      return nullptr; // don't set error, because we might find something later
+    }
+  };
+  
+  if (module) {
+    SymbolContextList sc_list;
+    module->FindSymbolsWithNameAndType(name, eSymbolTypeAny, sc_list);
+    const Symbol *const module_symbol = ProcessMatches(sc_list, error);
+  
+    if (!error.Success()) {
+      return nullptr;
+    } else if (module_symbol) {
+      return module_symbol;
+    }
+  }
+  
+  {
+    SymbolContextList sc_list;
+    target.GetImages().FindSymbolsWithNameAndType(name, eSymbolTypeAny,
+                                                  sc_list);
+    const Symbol *const target_symbol = ProcessMatches(sc_list, error);
+    
+    if (!error.Success()) {
+      return nullptr;
+    } else if (target_symbol) {
+      return target_symbol;
+    }
+  }
+  
+  return nullptr; // no error; we just didn't find anything
 }
 
 lldb::VariableSP ClangExpressionDeclMap::FindGlobalVariable(
     Target &target, ModuleSP &module, const ConstString &name,
     CompilerDeclContext *namespace_decl, TypeFromUser *type) {
   VariableList vars;
 
   if (module && namespace_decl)
if (!context.m_found.function_with_type_info) {
         }
       } while (0);
     }
 
     if (target && !context.m_found.variable && !namespace_decl) {
       // We couldn't find a non-symbol variable for this.  Now we'll hunt for
       // a generic
       // data symbol, and -- if it is found -- treat it as a variable.
+      Error error;
+      
+      const Symbol *data_symbol = FindGlobalDataSymbol(m_parser_vars->m_sym_ctx,
+                                                       name, error);
       
-      const Symbol *data_symbol = FindGlobalDataSymbol(*target, name);
+      if (!error.Success()) {
+        const unsigned diag_id =
+            m_ast_context->getDiagnostics().getCustomDiagID(
+                clang::DiagnosticsEngine::Level::Error, "%0");
+        m_ast_context->getDiagnostics().Report(diag_id) << error.AsCString();
+      }
                                           
       if (data_symbol) {
         std::string warning("got name from symbols: ");
         warning.append(name.AsCString());
         const unsigned diag_id =
             m_ast_context->getDiagnostics().getCustomDiagID(
                 clang::DiagnosticsEngine::Level::Warning, "%0");
         m_ast_context->getDiagnostics().Report(diag_id) << warning.c_str();
         AddOneGenericVariable(context, *data_symbol, current_id);