[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
This revision was automatically updated to reflect the committed changes.
Closed by commit rL303223: [Expression parser] Look up module symbols before
hunting globally (authored by spyffe).
Changed prior to commit:
https://reviews.llvm.org/D33083?vs=99211&id=99227#toc
Repository:
rL LLVM
https://reviews.llvm.org/D33083
Files:
lldb/trunk/include/lldb/Symbol/SymbolContext.h
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two.mk
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
lldb/trunk/source/Symbol/SymbolContext.cpp
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
@@ -0,0 +1,4 @@
+#ifndef TWO_H
+#define TWO_H
+void two();
+#endif
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
@@ -0,0 +1,6 @@
+#include "Two.h"
+#include
+
+void two() {
+ printf("Two\n"); // break here
+}
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
@@ -0,0 +1 @@
+int __attribute__ ((visibility("hidden"))) conflicting_symbol = 2;
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
@@ -0,0 +1,12 @@
+LEVEL := ../../../make
+
+DYLIB_NAME := One
+DYLIB_C_SOURCES := One/One.c One/OneConstant.c
+DYLIB_ONLY := YES
+
+include $(LEVEL)/Makefile.rules
+
+CFLAGS_EXTRAS += -fPIC
+
+One/OneConstant.o: One/OneConstant.c
+ $(CC) $(CFLAGS_NO_DEBUG) -c $< -o $@
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
@@ -0,0 +1,11 @@
+#include "One/One.h"
+#include "Two/Two.h"
+
+#include
+
+int main() {
+ one();
+ two();
+ printf("main\n"); // break here
+ return(0);
+}
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
@@ -0,0 +1,4 @@
+#ifndef ONE_H
+#define ONE_H
+void one();
+#endif
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
@@ -0,0 +1,6 @@
+#include "One.h"
+#include
+
+void one() {
+ printf("One\n"); // break here
+}
Index: lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
===
--- lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
+++ lldb/trunk/packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
@@ -0,0 +1 @@
+int __attribute__ ((visibility("hidden"))) conflicting_symbol = 1;
Index: ll
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Very nice. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
spyffe updated this revision to Diff 99211.
spyffe added a comment.
Used `CFLAGS_EXTRAS` instead of `CFLAGS` at Greg Clayton's suggestion.
https://reviews.llvm.org/D33083
Files:
include/lldb/Symbol/SymbolContext.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two.mk
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
source/Symbol/SymbolContext.cpp
Index: source/Symbol/SymbolContext.cpp
===
--- source/Symbol/SymbolContext.cpp
+++ source/Symbol/SymbolContext.cpp
@@ -799,6 +799,163 @@
return true;
}
+const Symbol *
+SymbolContext::FindBestGlobalDataSymbol(const ConstString &name, Status &error) {
+ error.Clear();
+
+ if (!target_sp) {
+return nullptr;
+ }
+
+ Target &target = *target_sp;
+ Module *module = module_sp.get();
+
+ auto ProcessMatches = [this, &name, &target, module]
+ (SymbolContextList &sc_list, Status &error) -> const Symbol* {
+llvm::SmallVector external_symbols;
+llvm::SmallVector 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;
+ }
+ 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
+if (name == symbol->GetReExportedSymbolName() &&
+module == reexport_module_sp.get())
+ return nullptr;
+
+return FindBestGlobalDataSymbol(
+symbol->GetReExportedSymbolName(), 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:
+
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. 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. Comment at: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile:6 + +main.o : CFLAGS += -g -O0 + CFLAGS_EXTRAS is the way to add CFLAGS: ``` CFLAGS_EXTRAS += "-g -O0" ``` Comment at: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk:9 + +CFLAGS += -fPIC + avoid modifying CFLAGS, modify CFLAGS_EXTRAS and then use it if needed Comment at: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two.mk:9 + +CFLAGS += -fPIC + avoid modifying CFLAGS, modify CFLAGS_EXTRAS https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
spyffe updated this revision to Diff 99186.
spyffe added a comment.
Updated to reflect Pavel Labath's suggestions:
- Used `CFLAGS_NO_DEBUG` where appropriate
- Marked the testcase as having no debug info variants
https://reviews.llvm.org/D33083
Files:
include/lldb/Symbol/SymbolContext.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two.mk
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
source/Symbol/SymbolContext.cpp
Index: source/Symbol/SymbolContext.cpp
===
--- source/Symbol/SymbolContext.cpp
+++ source/Symbol/SymbolContext.cpp
@@ -799,6 +799,163 @@
return true;
}
+const Symbol *
+SymbolContext::FindBestGlobalDataSymbol(const ConstString &name, Status &error) {
+ error.Clear();
+
+ if (!target_sp) {
+return nullptr;
+ }
+
+ Target &target = *target_sp;
+ Module *module = module_sp.get();
+
+ auto ProcessMatches = [this, &name, &target, module]
+ (SymbolContextList &sc_list, Status &error) -> const Symbol* {
+llvm::SmallVector external_symbols;
+llvm::SmallVector 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;
+ }
+ 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
+if (name == symbol->GetReExportedSymbolName() &&
+module == reexport_module_sp.get())
+ return nullptr;
+
+return FindBestGlobalDataSymbol(
+symbol->GetReExportedSymbolName(), 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:
+
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
labath added inline comments. Comment at: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk:1 +LEVEL := ../../../make + 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 $@ ``` Comment at: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py:16 + +mydir = TestBase.compute_mydir(__file__) + 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. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
spyffe updated this revision to Diff 99084.
spyffe added a comment.
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.
https://reviews.llvm.org/D33083
Files:
include/lldb/Symbol/SymbolContext.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One.mk
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two.mk
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
source/Symbol/SymbolContext.cpp
Index: source/Symbol/SymbolContext.cpp
===
--- source/Symbol/SymbolContext.cpp
+++ source/Symbol/SymbolContext.cpp
@@ -799,6 +799,163 @@
return true;
}
+const Symbol *
+SymbolContext::FindBestGlobalDataSymbol(const ConstString &name, Status &error) {
+ error.Clear();
+
+ if (!target_sp) {
+return nullptr;
+ }
+
+ Target &target = *target_sp;
+ Module *module = module_sp.get();
+
+ auto ProcessMatches = [this, &name, &target, module]
+ (SymbolContextList &sc_list, Status &error) -> const Symbol* {
+llvm::SmallVector external_symbols;
+llvm::SmallVector 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;
+ }
+ 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
+if (name == symbol->GetReExportedSymbolName() &&
+module == reexport_module_sp.get())
+ return nullptr;
+
+return FindBestGlobalDataSymbol(
+symbol->GetReExportedSymbolName(), 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 eSymbolTyp
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
labath added a comment.
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?
Comment at: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile:1
+LEVEL = ../../../make
+
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.
https://reviews.llvm.org/D33083
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
clayborg added a comment. 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. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
jingham added a comment. 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. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
clayborg added a comment. 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? https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
spyffe updated this revision to Diff 98672.
spyffe added a comment.
Improved symbol lookup per Greg and Jim's suggestion, with flipped steps 2 and
3.
Also added testing for the error case.
https://reviews.llvm.org/D33083
Files:
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
Index: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
===
--- packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
+++ packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
@@ -0,0 +1,11 @@
+#include "One/One.h"
+#include "Two/Two.h"
+
+#include
+
+int main() {
+ one();
+ two();
+ printf("main\n"); // break here
+ return(0);
+}
Index: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
===
--- packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
+++ packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
@@ -0,0 +1 @@
+__private_extern__ int conflicting_symbol = 2;
Index: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
===
--- packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
+++ packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
@@ -0,0 +1,4 @@
+#ifndef TWO_H
+#define TWO_H
+void two();
+#endif
Index: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
===
--- packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
+++ packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
@@ -0,0 +1,6 @@
+#include "Two.h"
+#include
+
+void two() {
+ printf("Two\n"); // break here
+}
Index: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
===
--- packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
+++ packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
@@ -0,0 +1,87 @@
+"""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=[
+"1"])
+
+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'])
+
+
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
spyffe added a comment. I also would flip 2 and 3. I'll give this a shot. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
jingham added a comment. 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. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
jingham added a comment. 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. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
clayborg added a comment. 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. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
clayborg added a comment. In https://reviews.llvm.org/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. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
jingham added a comment. 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? https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
jingham added a comment. Yes, this seems obviously right. https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
clayborg added a comment. 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(). https://reviews.llvm.org/D33083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33083: [Expression parser] Look up module symbols before hunting globally
spyffe created this revision.
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.
https://reviews.llvm.org/D33083
Files:
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Makefile
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/One.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/One/OneConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
Index: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
===
--- source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -1526,9 +1526,15 @@
// 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.
-
- const Symbol *data_symbol = FindGlobalDataSymbol(*target, name);
-
+
+ const Symbol *module_data_symbol = (m_parser_vars->m_sym_ctx.module_sp ?
+ FindGlobalDataSymbol(*target, name,
+ m_parser_vars->m_sym_ctx.module_sp.get()) :
+ nullptr);
+
+ const Symbol *data_symbol = module_data_symbol ?
+ module_data_symbol : FindGlobalDataSymbol(*target, name);
+
if (data_symbol) {
std::string warning("got name from symbols: ");
warning.append(name.AsCString());
Index: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
===
--- packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
+++ packages/Python/lldbsuite/test/lang/c/conflicting-symbol/main.c
@@ -0,0 +1,8 @@
+#include "One/One.h"
+#include "Two/Two.h"
+
+int main() {
+ one();
+ two();
+ return(0);
+}
Index: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
===
--- packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
+++ packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/TwoConstant.c
@@ -0,0 +1 @@
+__private_extern__ int conflicting_symbol = 2;
\ No newline at end of file
Index: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
===
--- packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
+++ packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.h
@@ -0,0 +1,4 @@
+#ifndef TWO_H
+#define TWO_H
+void two();
+#endif
Index: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
===
--- packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
+++ packages/Python/lldbsuite/test/lang/c/conflicting-symbol/Two/Two.c
@@ -0,0 +1,6 @@
+#include "Two.h"
+#include
+
+void two() {
+ printf("Two\n"); // break here
+}
Index: packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
===
--- packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
+++ packages/Python/lldbsuite/test/lang/c/conflicting-symbol/TestConflictingSymbol.py
@@ -0,0 +1,67 @@
+"""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()
+
+line = line_number('One/One.c', '// break here')
+line = line_number('Two/Two.c', '// break here')
+lldbutil.run_break_set_by_file_and_line(
+self, 'One.c', line, num_expected_locations=1, loc_exact=True)
+lldbutil.run_break_set_by_file_and_line(
+self
