[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(
+
[Lldb-commits] [PATCH] D33025: [DWARF parser] Produce correct template parameter packs
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Very nice Repository: rL LLVM https://reviews.llvm.org/D33025 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33077: [TypeSystem] Fix inspection of Objective-C object types
spyffe created this revision.
`ptr_refs` exposed a problem in ClangASTContext's implementation; it uses an
accessor to convert a `QualType` into an `ObjCObjectPointerType`, but the
accessor is not fully general. `getAs()` is the safer way to go.
I've added a test case that uses `ptr_refs` in a way that would crash before
the fix.
Repository:
rL LLVM
https://reviews.llvm.org/D33077
Files:
packages/Python/lldbsuite/test/functionalities/ptr_refs-objc/Makefile
packages/Python/lldbsuite/test/functionalities/ptr_refs-objc/TestPtrRefsObjC.py
packages/Python/lldbsuite/test/functionalities/ptr_refs-objc/main.m
source/Symbol/ClangASTContext.cpp
Index: source/Symbol/ClangASTContext.cpp
===
--- source/Symbol/ClangASTContext.cpp
+++ source/Symbol/ClangASTContext.cpp
@@ -4458,7 +4458,7 @@
case clang::Type::ObjCObjectPointer: {
const clang::ObjCObjectPointerType *objc_class_type =
- qual_type->getAsObjCInterfacePointerType();
+ qual_type->getAs();
const clang::ObjCInterfaceType *objc_interface_type =
objc_class_type->getInterfaceType();
if (objc_interface_type &&
@@ -4567,7 +4567,7 @@
case clang::Type::ObjCObjectPointer: {
const clang::ObjCObjectPointerType *objc_class_type =
- qual_type->getAsObjCInterfacePointerType();
+ qual_type->getAs();
const clang::ObjCInterfaceType *objc_interface_type =
objc_class_type->getInterfaceType();
if (objc_interface_type &&
@@ -5636,7 +5636,7 @@
case clang::Type::ObjCObjectPointer: {
const clang::ObjCObjectPointerType *objc_class_type =
-qual_type->getAsObjCInterfacePointerType();
+qual_type->getAs();
const clang::ObjCInterfaceType *objc_interface_type =
objc_class_type->getInterfaceType();
if (objc_interface_type &&
@@ -5784,7 +5784,7 @@
case clang::Type::ObjCObjectPointer: {
const clang::ObjCObjectPointerType *objc_class_type =
-qual_type->getAsObjCInterfacePointerType();
+qual_type->getAs();
const clang::ObjCInterfaceType *objc_interface_type =
objc_class_type->getInterfaceType();
if (objc_interface_type &&
Index: packages/Python/lldbsuite/test/functionalities/ptr_refs-objc/main.m
===
--- packages/Python/lldbsuite/test/functionalities/ptr_refs-objc/main.m
+++ packages/Python/lldbsuite/test/functionalities/ptr_refs-objc/main.m
@@ -0,0 +1,39 @@
+//===-- main.c --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#import
+
+@interface MyClass : NSObject {
+};
+-(void)test;
+@end
+
+@implementation MyClass
+-(void)test {
+printf("%p\n", self); // break here
+}
+@end
+
+@interface MyOwner : NSObject {
+ @public id ownedThing; // should be id, to test
+};
+@end
+
+@implementation MyOwner
+@end
+
+int main (int argc, char const *argv[]) {
+@autoreleasepool {
+MyOwner *owner = [[MyOwner alloc] init];
+owner->ownedThing = [[MyClass alloc] init];
+[(MyClass*)owner->ownedThing test];
+}
+return 0;
+}
+
Index: packages/Python/lldbsuite/test/functionalities/ptr_refs-objc/TestPtrRefsObjC.py
===
--- packages/Python/lldbsuite/test/functionalities/ptr_refs-objc/TestPtrRefsObjC.py
+++ packages/Python/lldbsuite/test/functionalities/ptr_refs-objc/TestPtrRefsObjC.py
@@ -0,0 +1,50 @@
+"""
+Test the ptr_refs tool on Darwin with Objective-C
+"""
+
+from __future__ import print_function
+
+import os
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestPtrRefsObjC(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipUnlessDarwin
+def test_ptr_refs(self):
+"""Test the ptr_refs tool on Darwin with Objective-C"""
+self.build()
+exe_name = 'a.out'
+exe = os.path.join(os.getcwd(), exe_name)
+
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+
+main_file_spec = lldb.SBFileSpec('main.m')
+breakpoint = target.BreakpointCreateBySourceRegex(
+'break', main_file_spec)
+self.assertTrue(breakpoint and
+breakpoint.GetNumLocations() == 1,
+VALID_BREAKPOINT)
+
+process = target.LaunchSimple(
+None, None, self.get_process_working_directory())
+self.assertTrue(process, PROCESS_IS_VALID)
+
+# Frame #0 should be on self.line1 and the break condition should hold.
+thread =
[Lldb-commits] [PATCH] D33035: Tool for using Intel(R) Processor Trace hardware feature
krytarowski added a comment. In https://reviews.llvm.org/D33035#751280, @clayborg wrote: > In https://reviews.llvm.org/D33035#751264, @krytarowski wrote: > > > Can we please use the generic process plugin code, not the specific Linux > > one? If the generic code is insufficient - we should adjust it and alter > > instances of it (Linux and BSD). > > > There should be no need to ever #include something from a plug-in in code the > gets loaded externally. We should rely on the error strings themselves, not > be comparing them to some value. SBError has an error type (generic, posix, > mach-kernel, expression results, windows). It we are going to do anything we > can make up a new set of them, but the code in this patch is relying on the > values, when it should just be using the string value. If the string value > wasn't set correctly, fix that is the code that creates the errors. Thank you for explanation. This is even better and ideal. https://reviews.llvm.org/D33035 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33035: Tool for using Intel(R) Processor Trace hardware feature
clayborg added a comment. In https://reviews.llvm.org/D33035#751264, @krytarowski wrote: > Can we please use the generic process plugin code, not the specific Linux > one? If the generic code is insufficient - we should adjust it and alter > instances of it (Linux and BSD). There should be no need to ever #include something from a plug-in in code the gets loaded externally. We should rely on the error strings themselves, not be comparing them to some value. SBError has an error type (generic, posix, mach-kernel, expression results, windows). It we are going to do anything we can make up a new set of them, but the code in this patch is relying on the values, when it should just be using the string value. If the string value wasn't set correctly, fix that is the code that creates the errors. https://reviews.llvm.org/D33035 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33035: Tool for using Intel(R) Processor Trace hardware feature
krytarowski added a comment. Can we please use a generic process plugin code, not the specific Linux one? If the generic code is insufficient - we should adjust it and alter instances of it (Linux and BSD). https://reviews.llvm.org/D33035 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33035: Tool for using Intel(R) Processor Trace hardware feature
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
So we don't want clients of SBStructuredData to have to text scrape and write
manual decoders. See my inlined comments on the minimal modifications we need
to make to SBStructureData first. This then removes a ton of code from your
patch. The SBStructureData modifications should probably be done in separate
patch as well. internally lldb_private::StructuredData has everything (dicts,
array, strings, integers, bools, nulls) inherit from
lldb_private::StructuredData::Object. Then SBStructuredData owns a
StructuredDataImpl which has a lldb_private::StructuredData::Object shared
pointer, so a lldb::SBStructureData can be any object (dicts, array, strings,
integers, bools, nulls). So we should expose the ability to get data from
SBStructuredData. See my inlined SBStructureData additions I proposed. We
really don't want anyone doing manual JSON text scraping.
Comment at: tools/CMakeLists.txt:8
add_subdirectory(lldb-mi)
+option(LLDB_BUILD_INTEL_PT "Enable Building of Intel(R) Processor Trace Tool"
OFF)
+if (LLDB_BUILD_INTEL_PT)
Can we default this to enabled?
Comment at: tools/intel-pt/Decoder.cpp:18
+// Other libraries and framework includes
+#include "../../source/Plugins/Process/Linux/NativeProcessLinux.h"
+#include "lldb/API/SBModule.h"
This kind of reach in to internal plug-in sources shouldn't happen as it
violates the boundaries. Remove and see comment below.
Comment at: tools/intel-pt/Decoder.cpp:27
+ std::string ) {
+ uint32_t error_code = sberror.GetError();
+ switch (error_code) {
We really shouldn't be interpreting integer error codes here. The string in the
"sberror" should be what you use. Modify the code that creates these errors in
the first place to also put a string values you have here into the
lldb_private::Error to begin with and remove this function.
Comment at: tools/intel-pt/Decoder.cpp:177
+ sberror.Clear();
+ CheckDebuggerID(sbprocess, sberror);
+ if (!sberror.Success())
Why do we care which debugger this belongs to?
Comment at: tools/intel-pt/Decoder.cpp:200
+
+ const char *custom_trace_params = s.GetData();
+ std::string trace_params(custom_trace_params);
We meed to add API to SBStructuredData to expose some of the stuff from
lldb_private::StructuredData so you don't end up text scraping here. Just do
what you need for now but I would suggest at the very least:
```
class SBStructuredData {
enum Type {
eTypeArray,
eTypeDictionary,
eTypeString,
eTypeInteger,
eTypeBoolean
}
// Get the type of data in this structured data.
Type GetType();
// Get a dictionary for a key value given a key from the top level of this
structured data if type is eTypeDictionary
SBStructuredData GetValueForKey(const char *key);
// Get the value of this object as a string if type is eTypeString
bool GetStringValue(char *s, size_t max_len);
// Get an integer of this object as an integer if type is eTypeInteger
bool GetIntegerValue(uint64_t );
...
};
```
See cleanup code below if we do this.
Comment at: tools/intel-pt/Decoder.cpp:211-212
+ }
+ std::size_t pos = trace_params.find(trace_tech_key);
+ if ((pos == std::string::npos)) {
+sberror.SetErrorStringWithFormat(
```
if (!sbstructdata.GetValueForKey("trace-tech").IsValid())
```
Comment at: tools/intel-pt/Decoder.cpp:218-219
+ }
+ pos = trace_params.find(trace_tech_value);
+ if ((pos == std::string::npos)) {
+sberror.SetErrorStringWithFormat(
```
if (!sbstructdata.GetValueForKey("intel-pt").IsValid())
```
Comment at: tools/intel-pt/Decoder.cpp:538-547
+ const char *custom_trace_params = s.GetData();
+ if (!custom_trace_params || (s.GetSize() == 0)) {
+sberror.SetErrorStringWithFormat("lldb couldn't provide cpuinfo");
+return;
+ }
+
+ long int family = 0, model = 0, stepping = 0, vendor = 0;
No text scraping. Please use SBStructureData methods that we will need to add.
Comment at: tools/intel-pt/Decoder.cpp:602-603
+
+void Decoder::Parse(const std::string _params, const std::string ,
+lldb::SBError , std::string ) {
+ std::string key_value_pair_separator(",");
Remove this function, add real API to SBStructuredData
Comment at: tools/intel-pt/Decoder.cpp:1046-1047
+// SBDebugger with which this tool instance is associated.
+void Decoder::CheckDebuggerID(lldb::SBProcess ,
+ lldb::SBError ) {
+ if (!sbprocess.IsValid()) {
Remove this function?
Comment at: tools/intel-pt/Decoder.h:86
[Lldb-commits] [PATCH] D33034: Tool for using Intel(R) Processor Trace hardware feature
abhishek.aggarwal abandoned this revision. abhishek.aggarwal added a comment. Abandoning this change due to formatting problem with commit message. https://reviews.llvm.org/D33034 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
