[Lldb-commits] [PATCH] D60254: Add dropped ManualDWARFIndex assert()

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added a reviewer: labath.
jankratochvil added a project: LLDB.
Herald added subscribers: arphaman, aprantl.

D47253  dropped this assertion. So either add 
it back by this patch or otherwise I will remove currently unused 
`SymbolFileDWARF::GetBaseCompileUnit`.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D60254

Files:
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp


Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -92,6 +92,9 @@
 }
 
 void ManualDWARFIndex::IndexUnit(DWARFUnit &unit, IndexSet &set) {
+  assert(!unit.GetSymbolFileDWARF()->GetBaseCompileUnit() &&
+  "DWARFUnit associated with .dwo or .dwp should not be indexed directly");
+
   Log *log = LogChannelDWARF::GetLogIfAll(DWARF_LOG_LOOKUPS);
 
   if (log) {


Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -92,6 +92,9 @@
 }
 
 void ManualDWARFIndex::IndexUnit(DWARFUnit &unit, IndexSet &set) {
+  assert(!unit.GetSymbolFileDWARF()->GetBaseCompileUnit() &&
+  "DWARFUnit associated with .dwo or .dwp should not be indexed directly");
+
   Log *log = LogChannelDWARF::GetLogIfAll(DWARF_LOG_LOOKUPS);
 
   if (log) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59826: [expression] Explicitly build the lookup table of any TagDecl's context

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D59826#1452771 , @martong wrote:

> In D59826#1451536 , @clayborg wrote:
>
> > Any way to dump the AST in a test to verify we don't create multiple?
>
>
> I think I might be able to use the `log` object to dump the AST and then from 
> python it would be possible to check for duplicates. Is that a good direction?


note there's also  a new "target modules dump ast" command, which might be of 
help here. You can see it in action in the NativePDB tests 
(lit/SymbolFile/NativePDB).


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59826/new/

https://reviews.llvm.org/D59826



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60254: Add dropped ManualDWARFIndex assert()

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I don't exactly remember my thought processes there, but I guess I have dropped 
that assert during the refactor because it was obvious to me.

Now that you mention it, I don't have any particular opinion on what we should 
do. If it makes your job any easier, then go ahead and remove 
`GetBaseCompileUnit`. Otherwise, I suppose it might be good to re-add the 
assert.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60254/new/

https://reviews.llvm.org/D60254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60254: Add dropped ManualDWARFIndex assert()

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

Maybe I will have to update `GetBaseCompileUnit`, maybe not. But as I find the 
assert useful (for `-fdebug-types-section -gsplit-dwarf`) going to add back the 
assertion. Thanks for the reply.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60254/new/

https://reviews.llvm.org/D60254



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r357678 - Add dropped ManualDWARFIndex assert()

[Jan Kratochvil via lldb-commits]

Author: jankratochvil
Date: Thu Apr  4 02:24:02 2019
New Revision: 357678

URL: http://llvm.org/viewvc/llvm-project?rev=357678&view=rev
Log:
Add dropped ManualDWARFIndex assert()

D47253 dropped this assertion.

Differential Revision: https://reviews.llvm.org/D60254

Modified:
lldb/trunk/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

Modified: lldb/trunk/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp?rev=357678&r1=357677&r2=357678&view=diff
==
--- lldb/trunk/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp (original)
+++ lldb/trunk/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp Thu Apr  4 
02:24:02 2019
@@ -92,6 +92,9 @@ void ManualDWARFIndex::Index() {
 }
 
 void ManualDWARFIndex::IndexUnit(DWARFUnit &unit, IndexSet &set) {
+  assert(!unit.GetSymbolFileDWARF()->GetBaseCompileUnit() &&
+  "DWARFUnit associated with .dwo or .dwp should not be indexed directly");
+
   Log *log = LogChannelDWARF::GetLogIfAll(DWARF_LOG_LOOKUPS);
 
   if (log) {


___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60254: Add dropped ManualDWARFIndex assert()

[Jan Kratochvil via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB357678: Add dropped ManualDWARFIndex assert() (authored 
by jankratochvil, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60254?vs=193669&id=193677#toc

Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60254/new/

https://reviews.llvm.org/D60254

Files:
  source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp


Index: source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -92,6 +92,9 @@
 }
 
 void ManualDWARFIndex::IndexUnit(DWARFUnit &unit, IndexSet &set) {
+  assert(!unit.GetSymbolFileDWARF()->GetBaseCompileUnit() &&
+  "DWARFUnit associated with .dwo or .dwp should not be indexed directly");
+
   Log *log = LogChannelDWARF::GetLogIfAll(DWARF_LOG_LOOKUPS);
 
   if (log) {


Index: source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -92,6 +92,9 @@
 }
 
 void ManualDWARFIndex::IndexUnit(DWARFUnit &unit, IndexSet &set) {
+  assert(!unit.GetSymbolFileDWARF()->GetBaseCompileUnit() &&
+  "DWARFUnit associated with .dwo or .dwp should not be indexed directly");
+
   Log *log = LogChannelDWARF::GetLogIfAll(DWARF_LOG_LOOKUPS);
 
   if (log) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

To me, the greatest advantage in using Expected (ErrorOr is the old way 
of doing things; I wouldn't use that in new code) is greater clarity in what 
are the possible results of the function. If the function returns the object 
and the error in two pieces, then this leaves place for uncertainties like "but 
what is the state of the T object if the function returned an error", or "if 
the T is invalid (null), can I assume the error object will indicate failure 
too", etc. Using Expected removes all of these, because if the function 
returns an error, there is no T object to speak of and vice-versa. (Ok, if T = 
std::shared_ptr, then this still leaves the possibility of the function 
returning "success", but with an empty pointer; however, that's still a much 
smaller problem space).

Whether the caller is able to do something with that error is not that 
important here. If he doesn't want it, he can always explicitly ignore it (or 
maybe log it?). So, I would say that changing this function to use Expected 
is definitely a good thing, but it doesn't have to happen in this patch.

BTW, if you have a Status object, you can convert it to an Expected (via 
llvm::Error) by calling ToError on it. Also, you don't have to define your own 
error type if you just want to return some string. StringError 
(llvm::createStringError) should suffice for that.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60172/new/

https://reviews.llvm.org/D60172



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60180: [CMake] Don't explicitly use LLVM_LIBRARY_DIR in standalone builds

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

> From the looks of it, clang needs to set it manually because they rely on 
> llvm-config instead of using find_package.

Please note that clang *is* using `find_package`, just that instead of removing 
the old llvm-config invocation, it was deprecated first. The code I referred to 
*is not* in the deprecated llvm-config code!

> LLVM_LIBRARY_DIR is being set correctly. It appears to be getting it from the 
> LLVMConfig we get from find_package(LLVM).

Yes LLVMConfig sets the variable, but it's not going to be in the cache:

  llvm-macosx-x86_64/lib/cmake/llvm/LLVMConfig.cmake
  178:set(LLVM_LIBRARY_DIR "/path/to/llvm-macosx-x86_64/./lib")

Does `cmake -L ...` still dump it? Do in-tree builds have a cache entry for it? 
Are other subprojects relying on it? (unlikely for LLDB)
Pardon my stubbornness, but fixing such things a few weeks later is just too 
annoying.

> I think we could also get rid of LLVM_BINARY_DIR if that's the case, since it 
> appears we only set it for lit.

Yes cleanup is very important, but let's not focus only on what *can* be 
removed. IMHO a good order of priorities in build system code may be:

1. keep differences between in-tree and standalone builds small
2. keep differences between subprojects small
3. keep the code clean

If we are really sure that this code it outdated, please remove it and ideally 
also initiate a similar cleanup in clang, compiler-rt, lld, etc. (or ping 
someone to have a look).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60180/new/

https://reviews.llvm.org/D60180



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60001: Allow partial UUID matching in Minidump core file plug-in

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D60001#1453457 , @clayborg wrote:

> ok, so I think I figured out what was going on: I had the .so files still in 
> my build 
> packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new 
> directory. They didn't show up in the "svn stat" command because they are 
> ignored!!! arg! That is what was causing problems when testing on my machine.


Yeah, I can imagine this being very annoying to track down. This is very 
unfortunate. I remember looking at this problem a some time ago because it was 
causing failures on one of the bots, and not being able to conclude where this 
ignore is coming from. It looks like svn just doesn't like .so files 
altogether. Well.. I guess that's one more thing that will be solved be the 
impending switch to github.

Does this mean the direcotory-clearing part can be removed now? I've tried the 
test on a mac and it still succeeds for me. If it doesn't work for you, it 
looks like we still have some kind of nondeterminism in here, and I'd like to 
get to the bottom of that. FTR: this is the stack trace of the point where lldb 
find the module for me:

  (lldb) bt
  * thread #1, queue = 'com.apple.main-thread', stop reason = step over
* frame #0: 0x0001062ce59e 
_lldb.so`lldb_private::ModuleList::GetSharedModule(module_spec=0x7ffeefbfbef0,
 module_sp=nullptr, module_search_paths_ptr=0x000100204c80, 
old_module_sp_ptr=0x7ffeefbfc9f0, did_create_ptr=0x7ffeefbfc9ef, 
always_create=false) at ModuleList.cpp:862:19
  frame #1: 0x0001067432c9 
_lldb.so`lldb_private::Platform::GetSharedModule(this=0x000100606f38, 
spec=0x7ffeefbfcbe0)::$_0::operator()(lldb_private::ModuleSpec const&) 
const at Platform.cpp:245:15
  frame #2: 0x0001067430d4 _lldb.so`decltype(__f=0x000100606f38, 
__args=0x7ffeefbfcbe0)::$_0&>(fp)(std::__1::forward(fp0))) 
std::__1::__invoke&, 
lldb_private::FileSpecList const*, std::__1::shared_ptr*, 
bool*)::$_0&, lldb_private::ModuleSpec 
const&>(lldb_private::Platform::GetSharedModule(lldb_private::ModuleSpec 
const&, lldb_private::Process*, std::__1::shared_ptr&, 
lldb_private::FileSpecList const*, std::__1::shared_ptr*, 
bool*)::$_0&, lldb_private::ModuleSpec const&) at type_traits:4339:1
  frame #3: 0x000106743054 _lldb.so`lldb_private::Status 
std::__1::__invoke_void_return_wrapper::__call(lldb_private::Platform::GetSharedModule(lldb_private::ModuleSpec 
const&, lldb_private::Process*, std::__1::shared_ptr&, 
lldb_private::FileSpecList const*, std::__1::shared_ptr*, 
bool*)::$_0&, lldb_private::ModuleSpec const&) at __functional_base:318:16
  frame #4: 0x000106742218 
_lldb.so`std::__1::__function::__func&, 
lldb_private::FileSpecList const*, std::__1::shared_ptr*, 
bool*)::$_0, 
std::__1::allocator&, 
lldb_private::FileSpecList const*, std::__1::shared_ptr*, 
bool*)::$_0>, lldb_private::Status (lldb_private::ModuleSpec 
const&)>::operator(this=0x000100606f30, 
__arg=0x7ffeefbfcbe0)(lldb_private::ModuleSpec const&) at functional:1562:12
  frame #5: 0x000106739f37 
_lldb.so`std::__1::function::operator(this=0x7ffeefbfc570, 
__arg=0x7ffeefbfcbe0)(lldb_private::ModuleSpec const&) const at 
functional:1913:12
  frame #6: 0x00010673301f 
_lldb.so`lldb_private::Platform::GetRemoteSharedModule(this=0x00010045b210, 
module_spec=0x7ffeefbfcbe0, process=0x000101063418, module_sp=nullptr, 
module_resolver=0x7ffeefbfc570, did_create_ptr=0x7ffeefbfc9ef)> const&, 
bool*) at Platform.cpp:1593:16
  frame #7: 0x000106732c7f 
_lldb.so`lldb_private::Platform::GetSharedModule(this=0x00010045b210, 
module_spec=0x7ffeefbfcbe0, process=0x000101063418, module_sp=nullptr, 
module_search_paths_ptr=0x000100204c80, 
old_module_sp_ptr=0x7ffeefbfc9f0, did_create_ptr=0x7ffeefbfc9ef) at 
Platform.cpp:254:10
  frame #8: 0x0001067faed1 
_lldb.so`lldb_private::Target::GetSharedModule(this=0x000100806800, 
module_spec=0x7ffeefbfcbe0, error_ptr=0x7ffeefbfccf8) at 
Target.cpp:2040:34
  frame #9: 0x000107061f36 
_lldb.so`lldb_private::minidump::ProcessMinidump::ReadModuleList(this=0x000101063418)
 at ProcessMinidump.cpp:402:31
  frame #10: 0x0001070617cc 
_lldb.so`lldb_private::minidump::ProcessMinidump::DoLoadCore(this=0x000101063418)
 at ProcessMinidump.cpp:218:3
  frame #11: 0x00010675aec5 
_lldb.so`lldb_private::Process::LoadCore(this=0x000101063418) at 
Process.cpp:2629:18
  frame #12: 0x000105e045e7 
_lldb.so`lldb::SBTarget::LoadCore(this=0x00010020b590, 
core_file="linux-arm-partial-uuids-match.dmp", error=0x7ffeefbfd430) at 
SBTarget.cpp:277:34
  frame #13: 0x000105e041a2 
_lldb.so`lldb::SBTarget::LoadCore(this=0x00010020b590, 
core_file="linux-arm-partial-uuids-match.dmp") at SBTarget.cpp:262:10
  frame #14: 0x00010616a9ed 
_lldb.so`_wrap_SBTarget_LoadCore__SWIG_0((null

[Lldb-commits] [lldb] r357680 - modify-python-lldb.py: (Re)move __len__ and __iter__ support

[Pavel Labath via lldb-commits]

Author: labath
Date: Thu Apr  4 03:13:59 2019
New Revision: 357680

URL: http://llvm.org/viewvc/llvm-project?rev=357680&view=rev
Log:
modify-python-lldb.py: (Re)move __len__ and __iter__ support

Summary:
This patch moves the modify-python-lldb code for adding new functions to
the SBModule class into the SBModule interface file. As this is the last
class using this functionality, I also remove all support for this kind
of modifications from modify-python-lldb.py.

Reviewers: amccarth, clayborg, jingham

Subscribers: zturner, lldb-commits

Differential Revision: https://reviews.llvm.org/D60195

Modified:

lldb/trunk/packages/Python/lldbsuite/test/python_api/module_section/TestModuleAndSection.py
lldb/trunk/scripts/Python/modify-python-lldb.py
lldb/trunk/scripts/interface/SBModule.i
lldb/trunk/scripts/lldb.swig

Modified: 
lldb/trunk/packages/Python/lldbsuite/test/python_api/module_section/TestModuleAndSection.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/python_api/module_section/TestModuleAndSection.py?rev=357680&r1=357679&r2=357680&view=diff
==
--- 
lldb/trunk/packages/Python/lldbsuite/test/python_api/module_section/TestModuleAndSection.py
 (original)
+++ 
lldb/trunk/packages/Python/lldbsuite/test/python_api/module_section/TestModuleAndSection.py
 Thu Apr  4 03:13:59 2019
@@ -45,6 +45,7 @@ class ModuleAndSectionAPIsTestCase(TestB
 
 print("Exe module: %s" % str(exe_module))
 print("Number of sections: %d" % exe_module.GetNumSections())
+print("Number of symbols: %d" % len(exe_module))
 INDENT = ' ' * 4
 INDENT2 = INDENT * 2
 for sec in exe_module.section_iter():

Modified: lldb/trunk/scripts/Python/modify-python-lldb.py
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/scripts/Python/modify-python-lldb.py?rev=357680&r1=357679&r2=357680&view=diff
==
--- lldb/trunk/scripts/Python/modify-python-lldb.py (original)
+++ lldb/trunk/scripts/Python/modify-python-lldb.py Thu Apr  4 03:13:59 2019
@@ -75,55 +75,6 @@ one_liner_docstring_pattern = re.compile
 '^(%s|%s)""".*"""$' %
 (TWO_SPACES, EIGHT_SPACES))
 
-# This supports the iteration protocol.
-iter_def = "def __iter__(self): return lldb_iter(self, '%s', '%s')"
-section_iter = "def section_iter(self): return lldb_iter(self, '%s', '%s')"
-compile_unit_iter = "def compile_unit_iter(self): return lldb_iter(self, 
'%s', '%s')"
-
-# Called to implement the built-in function len().
-# Eligible objects are those containers with unambiguous iteration support.
-len_def = "def __len__(self): return self.%s()"
-
-# A convenience iterator for SBSymbol!
-symbol_in_section_iter_def = '''
-def symbol_in_section_iter(self, section):
-"""Given a module and its contained section, returns an iterator on the
-symbols within the section."""
-for sym in self:
-if in_range(sym, section):
-yield sym
-'''
-
-#
-# This dictionary defines a mapping from classname to (getsize, getelem) tuple.
-#
-d = {'SBModule': ('GetNumSymbols', 'GetSymbolAtIndex'),
-
- # SBModule has an additional section_iter(), see below.
- 'SBModule-section': ('GetNumSections', 'GetSectionAtIndex'),
- # And compile_unit_iter().
- 'SBModule-compile-unit': ('GetNumCompileUnits', 'GetCompileUnitAtIndex'),
- # As well as symbol_in_section_iter().
- 'SBModule-symbol-in-section': symbol_in_section_iter_def
- }
-
-def list_to_frag(list):
-"""Transform a list to equality program fragment.
-
-For example, ['GetID'] is transformed to 'self.GetID() == other.GetID()',
-and ['GetFilename', 'GetDirectory'] to 'self.GetFilename() == 
other.GetFilename()
-and self.GetDirectory() == other.GetDirectory()'.
-"""
-if not list:
-raise Exception("list should be non-empty")
-frag = StringIO.StringIO()
-for i in range(len(list)):
-if i > 0:
-frag.write(" and ")
-frag.write("self.{0}() == other.{0}()".format(list[i]))
-return frag.getvalue()
-
-
 class NewContent(StringIO.StringIO):
 """Simple facade to keep track of the previous line to be committed."""
 
@@ -159,34 +110,16 @@ new_content = NewContent()
 with open(output_name, 'r') as f_in:
 content = f_in.read()
 
-# The pattern for recognizing the beginning of an SB class definition.
-class_pattern = re.compile("^class (SB.*)\(_object\):$")
-
-# The pattern for recognizing the beginning of the __init__ method definition.
-init_pattern = re.compile("^def __init__\(self.*\):")
-
 # These define the states of our finite state machine.
 NORMAL = 1
-DEFINING_ITERATOR = 2
 CLEANUP_DOCSTRING = 8
 
-# Our FSM begins its life in the NORMAL state, and transitions to the
-# DEFINING_ITERATOR state whenever it encounters the beginning of certain class
-# d

[Lldb-commits] [PATCH] D60195: modify-python-lldb.py: (Re)move __len__ and __iter__ support

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357680: modify-python-lldb.py: (Re)move __len__ and __iter__ 
support (authored by labath, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60195/new/

https://reviews.llvm.org/D60195

Files:
  
lldb/trunk/packages/Python/lldbsuite/test/python_api/module_section/TestModuleAndSection.py
  lldb/trunk/scripts/Python/modify-python-lldb.py
  lldb/trunk/scripts/interface/SBModule.i
  lldb/trunk/scripts/lldb.swig

Index: lldb/trunk/packages/Python/lldbsuite/test/python_api/module_section/TestModuleAndSection.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/python_api/module_section/TestModuleAndSection.py
+++ lldb/trunk/packages/Python/lldbsuite/test/python_api/module_section/TestModuleAndSection.py
@@ -45,6 +45,7 @@
 
 print("Exe module: %s" % str(exe_module))
 print("Number of sections: %d" % exe_module.GetNumSections())
+print("Number of symbols: %d" % len(exe_module))
 INDENT = ' ' * 4
 INDENT2 = INDENT * 2
 for sec in exe_module.section_iter():
Index: lldb/trunk/scripts/interface/SBModule.i
===
--- lldb/trunk/scripts/interface/SBModule.i
+++ lldb/trunk/scripts/interface/SBModule.i
@@ -370,6 +370,29 @@
 operator != (const lldb::SBModule &rhs) const;
  
 %pythoncode %{
+def __len__(self):
+'''Return the number of symbols in a lldb.SBModule object.'''
+return self.GetNumSymbols()
+
+def __iter__(self):
+'''Iterate over all symbols in a lldb.SBModule object.'''
+return lldb_iter(self, 'GetNumSymbols', 'GetSymbolAtIndex')
+
+def section_iter(self):
+'''Iterate over all sections in a lldb.SBModule object.'''
+return lldb_iter(self, 'GetNumSections', 'GetSectionAtIndex')
+
+def compile_unit_iter(self):
+'''Iterate over all compile units in a lldb.SBModule object.'''
+return lldb_iter(self, 'GetNumCompileUnits', 'GetCompileUnitAtIndex')
+
+def symbol_in_section_iter(self, section):
+'''Given a module and its contained section, returns an iterator on the
+symbols within the section.'''
+for sym in self:
+if in_range(sym, section):
+yield sym
+
 class symbols_access(object):
 re_compile_type = type(re.compile('.'))
 '''A helper object that will lazily hand out lldb.SBSymbol objects for a module when supplied an index, name, or regular expression.'''
Index: lldb/trunk/scripts/lldb.swig
===
--- lldb/trunk/scripts/lldb.swig
+++ lldb/trunk/scripts/lldb.swig
@@ -91,12 +91,6 @@
 elem = getattr(obj, getelem)
 for i in range(size()):
 yield elem(i)
-
-# ==
-# The modify-python-lldb.py script is responsible for post-processing this SWIG-
-# generated lldb.py module.  It is responsible for adding support for: iteration
-# protocol: __iter__, and built-in function len(): __len__.
-# ==
 %}
 
 %include "./Python/python-typemaps.swig"
Index: lldb/trunk/scripts/Python/modify-python-lldb.py
===
--- lldb/trunk/scripts/Python/modify-python-lldb.py
+++ lldb/trunk/scripts/Python/modify-python-lldb.py
@@ -75,55 +75,6 @@
 '^(%s|%s)""".*"""$' %
 (TWO_SPACES, EIGHT_SPACES))
 
-# This supports the iteration protocol.
-iter_def = "def __iter__(self): return lldb_iter(self, '%s', '%s')"
-section_iter = "def section_iter(self): return lldb_iter(self, '%s', '%s')"
-compile_unit_iter = "def compile_unit_iter(self): return lldb_iter(self, '%s', '%s')"
-
-# Called to implement the built-in function len().
-# Eligible objects are those containers with unambiguous iteration support.
-len_def = "def __len__(self): return self.%s()"
-
-# A convenience iterator for SBSymbol!
-symbol_in_section_iter_def = '''
-def symbol_in_section_iter(self, section):
-"""Given a module and its contained section, returns an iterator on the
-symbols within the section."""
-for sym in self:
-if in_range(sym, section):
-yield sym
-'''
-
-#
-# This dictionary defines a mapping from classname to (getsize, getelem) tuple.
-#
-d = {'SBModule': ('GetNumSymbols', 'GetSymbolAtIndex'),
-
- # SBModule has an additional section_iter(), see below.
- 'SBModule-section': ('GetNumSections', 'GetSectionAtIndex'),
- # And compile_unit_iter().
- 'SBModule-compile-unit'

[Lldb-commits] [lldb] r357691 - Breakpad: Refine record classification code

[Pavel Labath via lldb-commits]

Author: labath
Date: Thu Apr  4 06:23:25 2019
New Revision: 357691

URL: http://llvm.org/viewvc/llvm-project?rev=357691&view=rev
Log:
Breakpad: Refine record classification code

Previously we would classify all STACK records into a single bucket.
This is not really helpful, because there are three distinct types of
records beginning with the token "STACK" (STACK CFI INIT, STACK CFI,
STACK WIN). To be consistent with how we're treating other records, we
should classify these as three different record types.

It also implements the logic to put "STACK CFI INIT" and "STACK CFI"
records into the same "section" of the breakpad file, as they are meant
to be read together (similar to how FUNC and LINE records are treated).

The code which performs actual parsing of these records will come in a
separate patch.

Modified:
lldb/trunk/lit/Modules/Breakpad/Inputs/discontiguous-sections.syms
lldb/trunk/lit/Modules/Breakpad/discontiguous-sections.test
lldb/trunk/lit/Modules/Breakpad/sections.test
lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
lldb/trunk/source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp
lldb/trunk/unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp

Modified: lldb/trunk/lit/Modules/Breakpad/Inputs/discontiguous-sections.syms
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/Breakpad/Inputs/discontiguous-sections.syms?rev=357691&r1=357690&r2=357691&view=diff
==
--- lldb/trunk/lit/Modules/Breakpad/Inputs/discontiguous-sections.syms 
(original)
+++ lldb/trunk/lit/Modules/Breakpad/Inputs/discontiguous-sections.syms Thu Apr  
4 06:23:25 2019
@@ -3,3 +3,5 @@ INFO CODE_ID B52499D1F0F766F
 FILE 0 /tmp/a.c
 PUBLIC 1010 0 _start
 FILE 1 /tmp/b.c
+STACK bogus
+FILE 2 /tmp/c.c

Modified: lldb/trunk/lit/Modules/Breakpad/discontiguous-sections.test
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/Breakpad/discontiguous-sections.test?rev=357691&r1=357690&r2=357691&view=diff
==
--- lldb/trunk/lit/Modules/Breakpad/discontiguous-sections.test (original)
+++ lldb/trunk/lit/Modules/Breakpad/discontiguous-sections.test Thu Apr  4 
06:23:25 2019
@@ -1,7 +1,7 @@
 # Test handling discontiguous sections.
 RUN: lldb-test object-file %p/Inputs/discontiguous-sections.syms -contents | 
FileCheck %s
 
-CHECK: Showing 5 sections
+CHECK: Showing 6 sections
 
 CHECK:ID: 0x1
 CHECK-NEXT:   Name: MODULE
@@ -25,3 +25,10 @@ CHECK:File size: 16
 CHECK-NEXT:   Data:  (
 CHECK-NEXT:   : 46494C45 2031202F 746D702F 622E630A
  |FILE 1 /tmp/b.c.|
 CHECK-NEXT:   )
+
+CHECK:ID: 0x6
+CHECK-NEXT:   Name: FILE
+CHECK:File size: 16
+CHECK-NEXT:   Data:  (
+CHECK-NEXT:   : 46494C45 2032202F 746D702F 632E630A
  |FILE 2 /tmp/c.c.|
+CHECK-NEXT:   )

Modified: lldb/trunk/lit/Modules/Breakpad/sections.test
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Modules/Breakpad/sections.test?rev=357691&r1=357690&r2=357691&view=diff
==
--- lldb/trunk/lit/Modules/Breakpad/sections.test (original)
+++ lldb/trunk/lit/Modules/Breakpad/sections.test Thu Apr  4 06:23:25 2019
@@ -73,7 +73,7 @@ CHECK-NEXT:   )
 
 CHECK:Index: 5
 CHECK-NEXT:   ID: 0x6
-CHECK-NEXT:   Name: STACK
+CHECK-NEXT:   Name: STACK CFI INIT
 CHECK-NEXT:   Type: regular
 CHECK-NEXT:   Permissions: ---
 CHECK-NEXT:   Thread specific: no

Modified: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp?rev=357691&r1=357690&r2=357691&view=diff
==
--- lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp (original)
+++ lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp Thu Apr  
4 06:23:25 2019
@@ -16,11 +16,14 @@ using namespace lldb_private;
 using namespace lldb_private::breakpad;
 
 namespace {
-enum class Token { Unknown, Module, Info, CodeID, File, Func, Public, Stack };
+enum class Token { Unknown, Module, Info, CodeID, File, Func, Public, Stack, 
CFI, Init };
 }
 
-static Token toToken(llvm::StringRef str) {
-  return llvm::StringSwitch(str)
+template
+static T stringTo(llvm::StringRef Str);
+
+template <> Token stringTo(llvm::StringRef Str) {
+  return llvm::StringSwitch(Str)
   .Case("MODULE", Token::Module)
   .Case("INFO", Token::Info)
   .Case("CODE_ID", Token::CodeID)
@@ -28,21 +31,25 @@ static Token toToken(llvm::StringRef str
   .Case("FUNC", Token::Func)
   .Case("PUBLIC", Token::Public)
   .Case("STACK", Token::Stack)
+  .Case

[Lldb-commits] [PATCH] D59775: Minidump: Add support for reading/writing strings

[James Henderson via Phabricator via lldb-commits]

jhenderson added inline comments.



Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:70
   minidump::SystemInfo Info;
+  std::string CSDVersion;
 

Don't know about lifetime here, but could this be a `StringRef`?



Comment at: unittests/Object/MinidumpTest.cpp:270
+  0,// Mis-align next string
+  2, 0, 0, 0, 'a', 0,   // String6 - "a"
+

What about a case where the size field cannot fit in the remaining data?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59775/new/

https://reviews.llvm.org/D59775



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60121: Object/Minidump: Add support for reading the ModuleList stream

[James Henderson via Phabricator via lldb-commits]

jhenderson added a comment.

Code basically looks fine, but somebody else should confirm that the format is 
correct.




Comment at: lib/Object/Minidump.cpp:55
+  if (!OptionalStream)
+return createError("No such stream", object_error::invalid_section_index);
+  auto ExpectedSize =

I wonder whether it would be worth a new class of errors for minidump files? 
After all, invalid_section_index feels a bit forced for a format without 
sections!



Comment at: unittests/Object/MinidumpTest.cpp:317
+  1, 0, 0, 0,   // NumberOfStreams,
+  0x20, 0, 0, 0,// StreamDirectoryRVA
+  0, 1, 2, 3, 4, 5, 6, 7,   // Checksum, TimeDateStamp

It's a bit jarring seeing both hex and decimal numbers in here at the same 
time. Is there a particular reason you've used hex here, but decimal for e.g. 
116 below?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60121/new/

https://reviews.llvm.org/D60121



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59775: Minidump: Add support for reading/writing strings

[Pavel Labath via Phabricator via lldb-commits]

labath marked 3 inline comments as done.
labath added inline comments.



Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:70
   minidump::SystemInfo Info;
+  std::string CSDVersion;
 

jhenderson wrote:
> Don't know about lifetime here, but could this be a `StringRef`?
A StringRef would work when constructing this object from YAML (as there the 
yaml file can provide backing storage). However, when constructing this from a 
Minidump binary, that would not work, as the data is stored there in the utf16 
form (so not the kind of "string" we usually work with).

It would probably be possible to pull of some kind of a trick like 
`yaml::BinaryRef` does, where this would be a special object, which could be 
backed either by a real (utf8) string, or by the minidump utf16 thingy. 
However, I don't think that would be worth the trouble, as these strings are 
fairly small. (Also, in order to validate the utf16 data (so I don't end up 
creating an invalid object and crash during serialization), I'd have to 
essentially decode the data into a std::string anyway and then throw the result 
away.)



Comment at: unittests/Object/MinidumpTest.cpp:270
+  0,// Mis-align next string
+  2, 0, 0, 0, 'a', 0,   // String6 - "a"
+

jhenderson wrote:
> What about a case where the size field cannot fit in the remaining data?
Ah yes, good catch. I'll add that.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59775/new/

https://reviews.llvm.org/D59775



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59775: Minidump: Add support for reading/writing strings

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 193698.
labath marked an inline comment as done.
labath added a comment.

add the size-does-not-fit test case


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59775/new/

https://reviews.llvm.org/D59775

Files:
  include/llvm/Object/Minidump.h
  include/llvm/ObjectYAML/MinidumpYAML.h
  lib/Object/Minidump.cpp
  lib/ObjectYAML/MinidumpYAML.cpp
  test/tools/obj2yaml/basic-minidump.yaml
  unittests/Object/MinidumpTest.cpp
  unittests/ObjectYAML/MinidumpYAMLTest.cpp

Index: unittests/ObjectYAML/MinidumpYAMLTest.cpp
===
--- unittests/ObjectYAML/MinidumpYAMLTest.cpp
+++ unittests/ObjectYAML/MinidumpYAMLTest.cpp
@@ -33,7 +33,6 @@
   - Type:SystemInfo
 Processor Arch:  ARM64
 Platform ID: Linux
-CSD Version RVA: 0x01020304
 CPU:
   CPUID:   0x05060708
   - Type:LinuxMaps
@@ -54,7 +53,6 @@
   const SystemInfo &SysInfo = *ExpectedSysInfo;
   EXPECT_EQ(ProcessorArchitecture::ARM64, SysInfo.ProcessorArch);
   EXPECT_EQ(OSPlatform::Linux, SysInfo.PlatformId);
-  EXPECT_EQ(0x01020304u, SysInfo.CSDVersionRVA);
   EXPECT_EQ(0x05060708u, SysInfo.CPU.Arm.CPUID);
 
   EXPECT_EQ(StreamType::LinuxMaps, File.streams()[1].Type);
Index: unittests/Object/MinidumpTest.cpp
===
--- unittests/Object/MinidumpTest.cpp
+++ unittests/Object/MinidumpTest.cpp
@@ -249,3 +249,38 @@
   EXPECT_EQ(0x08070605u, Info.CPU.X86.FeatureInfo);
   EXPECT_EQ(0x02010009u, Info.CPU.X86.AMDExtendedFeatures);
 }
+
+TEST(MinidumpFile, getString) {
+  std::vector ManyStrings{
+  // Header
+  'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+  2, 0, 0, 0,   // NumberOfStreams,
+  0x20, 0, 0, 0,// StreamDirectoryRVA
+  0, 1, 2, 3, 4, 5, 6, 7,   // Checksum, TimeDateStamp
+  8, 9, 0, 1, 2, 3, 4, 5,   // Flags
+// Stream Directory
+  0, 0, 0, 0, 0, 0, 0, 0,   // Type, DataSize,
+  0x20, 0, 0, 0,// RVA
+  1, 0, 0, 0, 0, 0, // String1 - odd length
+  0, 0, 1, 0, 0, 0, // String2 - too long
+  2, 0, 0, 0, 0, 0xd8,  // String3 - invalid utf16
+  0, 0, 0, 0, 0, 0, // String4 - ""
+  2, 0, 0, 0, 'a', 0,   // String5 - "a"
+  0,// Mis-align next string
+  2, 0, 0, 0, 'a', 0,   // String6 - "a"
+
+  };
+  auto ExpectedFile = create(ManyStrings);
+  ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+  const MinidumpFile &File = **ExpectedFile;
+  EXPECT_THAT_EXPECTED(File.getString(44), Failed());
+  EXPECT_THAT_EXPECTED(File.getString(50), Failed());
+  EXPECT_THAT_EXPECTED(File.getString(56), Failed());
+  EXPECT_THAT_EXPECTED(File.getString(62), HasValue(""));
+  EXPECT_THAT_EXPECTED(File.getString(68), HasValue("a"));
+  EXPECT_THAT_EXPECTED(File.getString(75), HasValue("a"));
+
+  // Check the case when the size field does not fit into the remaining data.
+  EXPECT_THAT_EXPECTED(File.getString(ManyStrings.size() - 2),
+   Failed());
+}
Index: test/tools/obj2yaml/basic-minidump.yaml
===
--- test/tools/obj2yaml/basic-minidump.yaml
+++ test/tools/obj2yaml/basic-minidump.yaml
@@ -5,7 +5,7 @@
   - Type:SystemInfo
 Processor Arch:  ARM64
 Platform ID: Linux
-CSD Version RVA: 0x01020304
+CSD Version: Linux 3.13.0-91-generic
 CPU: 
   CPUID:   0x05060708
   - Type:LinuxAuxv
@@ -22,7 +22,7 @@
 # CHECK-NEXT:   - Type:SystemInfo
 # CHECK-NEXT: Processor Arch:  ARM64
 # CHECK-NEXT: Platform ID: Linux
-# CHECK-NEXT: CSD Version RVA: 0x01020304
+# CHECK-NEXT: CSD Version: Linux 3.13.0-91-generic
 # CHECK-NEXT: CPU: 
 # CHECK-NEXT:   CPUID:   0x05060708
 # CHECK-NEXT:   - Type:LinuxAuxv
Index: lib/ObjectYAML/MinidumpYAML.cpp
===
--- lib/ObjectYAML/MinidumpYAML.cpp
+++ lib/ObjectYAML/MinidumpYAML.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "llvm/ObjectYAML/MinidumpYAML.h"
+#include "llvm/Support/ConvertUTF.h"
 
 using namespace llvm;
 using namespace llvm::MinidumpYAML;
@@ -39,6 +40,8 @@
 return allocateArray(makeArrayRef(Data));
   }
 
+  size_t allocateString(StringRef Str);
+
   void writeTo(raw_ostream &OS) const;
 
 private:
@@ -48,6 +51,26 @@
 };
 } // namespace
 
+size_t BlobAllocator::allocateString(StringRef Str) {
+  SmallVector WStr;
+  bool OK = convertUTF8ToUTF16String(Str, WStr);
+  assert(OK && "I

[Lldb-commits] [PATCH] D60121: Object/Minidump: Add support for reading the ModuleList stream

[Pavel Labath via Phabricator via lldb-commits]

labath marked 2 inline comments as done.
labath added a comment.

In D60121#1454817 , @jhenderson wrote:

> Code basically looks fine, but somebody else should confirm that the format 
> is correct.


BTW, the structure definitions are just copied from lldb 
(https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/Process/minidump/MinidumpTypes.h
 and other files in that directory). I'm also porting the lldb parsing code to 
use the new llvm stuff as soon as the llvm stuff is ready as a way to ensure 
things remain interoperable with the existing minidumps and related tests.




Comment at: lib/Object/Minidump.cpp:55
+  if (!OptionalStream)
+return createError("No such stream", object_error::invalid_section_index);
+  auto ExpectedSize =

jhenderson wrote:
> I wonder whether it would be worth a new class of errors for minidump files? 
> After all, invalid_section_index feels a bit forced for a format without 
> sections!
Yes, I've been wondering about that too. In practice, I don't expect that the 
consumers will wan't to differentiate the error cases too much here (it usually 
does not matter if the stream was not present in the file, or if it was 
truncated -- the end result is the same -- you cannot use it). However, for 
clarity it might be better to do have a separate type anyway. I'll whip up a 
separate patch for that.



Comment at: unittests/Object/MinidumpTest.cpp:317
+  1, 0, 0, 0,   // NumberOfStreams,
+  0x20, 0, 0, 0,// StreamDirectoryRVA
+  0, 1, 2, 3, 4, 5, 6, 7,   // Checksum, TimeDateStamp

jhenderson wrote:
> It's a bit jarring seeing both hex and decimal numbers in here at the same 
> time. Is there a particular reason you've used hex here, but decimal for e.g. 
> 116 below?
Yeah, I agree this is not ideal. I started with hex here because it looked more 
"binary", but then I realized that:
a) decimal is shorter
b) the "size" fields are present in the static_asserts as decimal, and so it 
was easier to copy the numbers from there as decimal

I'll replace all of these to use decimal, except the "version" field, as that 
is defined in hex in the header.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60121/new/

https://reviews.llvm.org/D60121



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59775: Minidump: Add support for reading/writing strings

[James Henderson via Phabricator via lldb-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59775/new/

https://reviews.llvm.org/D59775



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60121: Object/Minidump: Add support for reading the ModuleList stream

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 193713.
labath marked an inline comment as done.
labath added a comment.

- avoid using mismatched error codes
- use decimal numbers more consistently in the tests


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60121/new/

https://reviews.llvm.org/D60121

Files:
  include/llvm/BinaryFormat/Minidump.h
  include/llvm/Object/Minidump.h
  lib/Object/Minidump.cpp
  unittests/Object/MinidumpTest.cpp

Index: unittests/Object/MinidumpTest.cpp
===
--- unittests/Object/MinidumpTest.cpp
+++ unittests/Object/MinidumpTest.cpp
@@ -284,3 +284,115 @@
   EXPECT_THAT_EXPECTED(File.getString(ManyStrings.size() - 2),
Failed());
 }
+
+TEST(MinidumpFile, getModuleList) {
+  std::vector OneModule{
+  // Header
+  'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+  1, 0, 0, 0,   // NumberOfStreams,
+  32, 0, 0, 0,  // StreamDirectoryRVA
+  0, 1, 2, 3, 4, 5, 6, 7,   // Checksum, TimeDateStamp
+  0, 0, 0, 0, 0, 0, 0, 0,   // Flags
+// Stream Directory
+  4, 0, 0, 0, 112, 0, 0, 0, // Type, DataSize,
+  44, 0, 0, 0,  // RVA
+  // ModuleList
+  1, 0, 0, 0, // NumberOfModules
+  1, 2, 3, 4, 5, 6, 7, 8, // BaseOfImage
+  9, 0, 1, 2, 3, 4, 5, 6, // SizeOfImage, Checksum
+  7, 8, 9, 0, 1, 2, 3, 4, // TimeDateStamp, ModuleNameRVA
+  0, 0, 0, 0, 0, 0, 0, 0, // Signature, StructVersion
+  0, 0, 0, 0, 0, 0, 0, 0, // FileVersion
+  0, 0, 0, 0, 0, 0, 0, 0, // ProductVersion
+  0, 0, 0, 0, 0, 0, 0, 0, // FileFlagsMask, FileFlags
+  0, 0, 0, 0, // FileOS
+  0, 0, 0, 0, 0, 0, 0, 0, // FileType, FileSubType
+  0, 0, 0, 0, 0, 0, 0, 0, // FileDate
+  1, 2, 3, 4, 5, 6, 7, 8, // CvRecord
+  9, 0, 1, 2, 3, 4, 5, 6, // MiscRecord
+  7, 8, 9, 0, 1, 2, 3, 4, // Reserved0
+  5, 6, 7, 8, 9, 0, 1, 2, // Reserved1
+  };
+  // Same as before, but with a padded module list.
+  std::vector PaddedModule{
+  // Header
+  'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+  1, 0, 0, 0,   // NumberOfStreams,
+  32, 0, 0, 0,  // StreamDirectoryRVA
+  0, 1, 2, 3, 4, 5, 6, 7,   // Checksum, TimeDateStamp
+  0, 0, 0, 0, 0, 0, 0, 0,   // Flags
+// Stream Directory
+  4, 0, 0, 0, 116, 0, 0, 0, // Type, DataSize,
+  44, 0, 0, 0,  // RVA
+  // ModuleList
+  1, 0, 0, 0, // NumberOfModules
+  0, 0, 0, 0, // Padding
+  1, 2, 3, 4, 5, 6, 7, 8, // BaseOfImage
+  9, 0, 1, 2, 3, 4, 5, 6, // SizeOfImage, Checksum
+  7, 8, 9, 0, 1, 2, 3, 4, // TimeDateStamp, ModuleNameRVA
+  0, 0, 0, 0, 0, 0, 0, 0, // Signature, StructVersion
+  0, 0, 0, 0, 0, 0, 0, 0, // FileVersion
+  0, 0, 0, 0, 0, 0, 0, 0, // ProductVersion
+  0, 0, 0, 0, 0, 0, 0, 0, // FileFlagsMask, FileFlags
+  0, 0, 0, 0, // FileOS
+  0, 0, 0, 0, 0, 0, 0, 0, // FileType, FileSubType
+  0, 0, 0, 0, 0, 0, 0, 0, // FileDate
+  1, 2, 3, 4, 5, 6, 7, 8, // CvRecord
+  9, 0, 1, 2, 3, 4, 5, 6, // MiscRecord
+  7, 8, 9, 0, 1, 2, 3, 4, // Reserved0
+  5, 6, 7, 8, 9, 0, 1, 2, // Reserved1
+  };
+
+  for (const std::vector &Data : {OneModule, PaddedModule}) {
+auto ExpectedFile = create(Data);
+ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
+const MinidumpFile &File = **ExpectedFile;
+Expected> ExpectedModule = File.getModuleList();
+ASSERT_THAT_EXPECTED(ExpectedModule, Succeeded());
+ASSERT_EQ(1u, ExpectedModule->size());
+const Module &M = ExpectedModule.get()[0];
+EXPECT_EQ(0x0807060504030201u, M.BaseOfImage);
+EXPECT_EQ(0x02010009u, M.SizeOfImage);
+EXPECT_EQ(0x06050403u, M.Checksum);
+EXPECT_EQ(0x00090807u, M.TimeDateStamp);
+EXPECT_EQ(0x04030201u, M.ModuleNameRVA);
+EXPECT_EQ(0x04030201u, M.CvRecord.DataSize);
+EXPECT_EQ(0x08070605u, M.CvRecord.RVA);
+EXPECT_EQ(0x02010009u, M.MiscRecord.DataSize);
+EXPECT_EQ(0x06050403u, M.MiscRecord.RVA);
+EXPECT_EQ(0x0403020100090807u, M.Reserved0);
+EXPECT_EQ(0x0201000908070605u, M.Reserved1);
+  }
+
+  std::vector StreamTooShort{
+  // Header
+  'M', 'D', 'M', 'P', 0x93, 0xa7, 0, 0, // Signature, Version
+  1, 0, 0, 0,   // NumberOfStreams,
+  32, 0, 0, 0,  // StreamDirectoryRVA
+  0, 1, 2, 3, 4, 5, 6, 7,   // Checksum, TimeDateStamp
+  0, 0, 0, 0, 0, 0, 0, 0,   // Flags
+// Stream Directory
+  4, 0, 0, 0, 111, 0, 0, 0, // Type, DataSize,
+ 

[Lldb-commits] [PATCH] D60121: Object/Minidump: Add support for reading the ModuleList stream

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added inline comments.



Comment at: lib/Object/Minidump.cpp:55
+  if (!OptionalStream)
+return createError("No such stream", object_error::invalid_section_index);
+  auto ExpectedSize =

labath wrote:
> jhenderson wrote:
> > I wonder whether it would be worth a new class of errors for minidump 
> > files? After all, invalid_section_index feels a bit forced for a format 
> > without sections!
> Yes, I've been wondering about that too. In practice, I don't expect that the 
> consumers will wan't to differentiate the error cases too much here (it 
> usually does not matter if the stream was not present in the file, or if it 
> was truncated -- the end result is the same -- you cannot use it). However, 
> for clarity it might be better to do have a separate type anyway. I'll whip 
> up a separate patch for that.
After some soul-searching I decided to just abandon the idea of returning error 
codes here, and just return the generic parse_failed error. That's what most 
other Object formats do (presumably they don't have a good use for 
distinguishing the codes either), and having an own error type would require 
some non-obvious design choices. (Like, since I'd still inherit from 
BinaryError, which inherits from ECError, I'd still have to interoperate with 
std::error_code somehow. This means I'd still have to map things to the 
object_error enum, or I'd have to define a new error_category, which is a 
deprecated way of doing things).


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60121/new/

https://reviews.llvm.org/D60121



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60121: Object/Minidump: Add support for reading the ModuleList stream

[James Henderson via Phabricator via lldb-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

Okay, no more comments from me.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60121/new/

https://reviews.llvm.org/D60121



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60268: Breakpad: Parse Stack CFI records

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: clayborg, amccarth, markmentovai.
Herald added a subscriber: aprantl.

This patch adds support for parsing STACK CFI records from breakpad
files. For the expressions specifying the values of registers, only a
basic parse is performed -- the record is split into a bunch of key-value
pairs, where both keys and values are strings. The idea is that these
will be handed off to the postfix expression -> dwarf compiler, once
it is extracted from the internals of the NativePDB plugin


https://reviews.llvm.org/D60268

Files:
  source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
  source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
  unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp

Index: unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
===
--- unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
+++ unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp
@@ -97,3 +97,36 @@
   EXPECT_EQ(llvm::None, PublicRecord::parse("PUBLIC m"));
   EXPECT_EQ(llvm::None, PublicRecord::parse("PUBLIC"));
 }
+
+TEST(StackCFIInitRecord, parse) {
+  EXPECT_EQ(StackCFIInitRecord(0x47, 0x8,
+   {{".cfa", "$esp 4 +"}, {"$eip", ".cfa 4 - ^"}}),
+StackCFIInitRecord::parse(
+"STACK CFI INIT 47 8 .cfa: $esp 4 + $eip: .cfa 4 - ^"));
+  EXPECT_EQ(StackCFIInitRecord(0x47, 0x8, {{".cfa", "$esp 4 +"}}),
+StackCFIInitRecord::parse("STACK CFI INIT 47 8 .cfa: $esp 4 +"));
+
+  EXPECT_EQ(llvm::None, StackCFIInitRecord::parse(
+"STACK CFI INIT 47 8 .cfa: $esp 4 + $eip:"));
+  EXPECT_EQ(llvm::None,
+StackCFIInitRecord::parse("STACK CFI INIT 47 8 .cfa: $eip: .cfa"));
+  EXPECT_EQ(llvm::None,
+StackCFIInitRecord::parse("STACK CFI INIT 47 8 $esp .cfa: $esp"));
+  EXPECT_EQ(llvm::None, StackCFIInitRecord::parse("STACK CFI INIT 47 8 .cfa:"));
+  EXPECT_EQ(llvm::None, StackCFIInitRecord::parse("STACK CFI INIT 47 8"));
+  EXPECT_EQ(llvm::None, StackCFIInitRecord::parse("STACK CFI INIT 47"));
+  EXPECT_EQ(llvm::None, StackCFIInitRecord::parse("STACK CFI INIT"));
+  EXPECT_EQ(llvm::None, StackCFIInitRecord::parse("STACK CFI"));
+  EXPECT_EQ(llvm::None, StackCFIInitRecord::parse("STACK"));
+  EXPECT_EQ(llvm::None, StackCFIInitRecord::parse(
+"STACK CFI 47 .cfa: $esp 4 + $eip: .cfa 4 - ^"));
+}
+
+TEST(StackCFIRecord, parse) {
+  EXPECT_EQ(
+  StackCFIRecord(0x47, {{".cfa", "$esp 4 +"}, {"$eip", ".cfa 4 - ^"}}),
+  StackCFIRecord::parse("STACK CFI 47 .cfa: $esp 4 + $eip: .cfa 4 - ^"));
+
+  EXPECT_EQ(llvm::None,
+StackCFIRecord::parse("STACK CFI INIT 47 8 .cfa: $esp 4 +"));
+}
Index: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
===
--- source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
+++ source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h
@@ -11,6 +11,7 @@
 
 #include "lldb/Utility/UUID.h"
 #include "lldb/lldb-types.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Support/FormatProviders.h"
@@ -141,6 +142,39 @@
 bool operator==(const PublicRecord &L, const PublicRecord &R);
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const PublicRecord &R);
 
+class StackCFIInitRecord : public Record {
+public:
+  static llvm::Optional parse(llvm::StringRef Line);
+  StackCFIInitRecord(
+  lldb::addr_t Address, lldb::addr_t Size,
+  llvm::DenseMap UnwindRules)
+  : Record(StackCFIInit), Address(Address), Size(Size),
+UnwindRules(std::move(UnwindRules)) {}
+
+  lldb::addr_t Address;
+  lldb::addr_t Size;
+  llvm::DenseMap UnwindRules;
+};
+
+bool operator==(const StackCFIInitRecord &L, const StackCFIInitRecord &R);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
+  const StackCFIInitRecord &R);
+
+class StackCFIRecord : public Record {
+public:
+  static llvm::Optional parse(llvm::StringRef Line);
+  StackCFIRecord(lldb::addr_t Address,
+ llvm::DenseMap UnwindRules)
+  : Record(StackCFIInit), Address(Address),
+UnwindRules(std::move(UnwindRules)) {}
+
+  lldb::addr_t Address;
+  llvm::DenseMap UnwindRules;
+};
+
+bool operator==(const StackCFIRecord &L, const StackCFIRecord &R);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const StackCFIRecord &R);
+
 } // namespace breakpad
 } // namespace lldb_private
 
Index: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
===
--- source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
+++ source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp
@@ -359,6 +359,120 @@
  R.Name);
 }
 
+static bool
+parseStackCFI(llvm::StringRef Line, lldb::addr_t &Address, lldb::addr_t *Size,
+  llvm::DenseMap &UnwindRules) 

[Lldb-commits] [PATCH] D60271: PDBFPO: Use references instead of pointers, where possible

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: aleksandr.urakov, amccarth.

The code was passing pointers around, expecting they would be not null.
In c++ it is possible to convey this notion explicitly by using a
reference instead.

Not all uses of pointers could be converted to references (e.g. one
can't store references in a container), but this will at least make it
locally obvious that code is dealing with nonnull pointers.


https://reviews.llvm.org/D60271

Files:
  source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp

Index: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
+++ source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
@@ -53,7 +53,7 @@
 
 public:
   virtual ~FPOProgramNode() = default;
-  virtual void Accept(FPOProgramASTVisitor *visitor) = 0;
+  virtual void Accept(FPOProgramASTVisitor &visitor) = 0;
 
   Kind GetKind() const { return m_token_kind; }
 
@@ -66,7 +66,7 @@
   FPOProgramNodeSymbol(llvm::StringRef name)
   : FPOProgramNode(Symbol), m_name(name) {}
 
-  void Accept(FPOProgramASTVisitor *visitor) override;
+  void Accept(FPOProgramASTVisitor &visitor) override;
 
   llvm::StringRef GetName() const { return m_name; }
 
@@ -79,7 +79,7 @@
   FPOProgramNodeRegisterRef(uint32_t lldb_reg_num)
   : FPOProgramNode(Register), m_lldb_reg_num(lldb_reg_num) {}
 
-  void Accept(FPOProgramASTVisitor *visitor) override;
+  void Accept(FPOProgramASTVisitor &visitor) override;
 
   uint32_t GetLLDBRegNum() const { return m_lldb_reg_num; }
 
@@ -92,7 +92,7 @@
   FPOProgramNodeIntegerLiteral(uint32_t value)
   : FPOProgramNode(IntegerLiteral), m_value(value) {}
 
-  void Accept(FPOProgramASTVisitor *visitor) override;
+  void Accept(FPOProgramASTVisitor &visitor) override;
 
   uint32_t GetValue() const { return m_value; }
 
@@ -108,12 +108,12 @@
 Align,
   };
 
-  FPOProgramNodeBinaryOp(OpType op_type, FPOProgramNode *left,
- FPOProgramNode *right)
-  : FPOProgramNode(BinaryOp), m_op_type(op_type), m_left(left),
-m_right(right) {}
+  FPOProgramNodeBinaryOp(OpType op_type, FPOProgramNode &left,
+ FPOProgramNode &right)
+  : FPOProgramNode(BinaryOp), m_op_type(op_type), m_left(&left),
+m_right(&right) {}
 
-  void Accept(FPOProgramASTVisitor *visitor) override;
+  void Accept(FPOProgramASTVisitor &visitor) override;
 
   OpType GetOpType() const { return m_op_type; }
 
@@ -135,10 +135,10 @@
 Deref,
   };
 
-  FPOProgramNodeUnaryOp(OpType op_type, FPOProgramNode *operand)
-  : FPOProgramNode(UnaryOp), m_op_type(op_type), m_operand(operand) {}
+  FPOProgramNodeUnaryOp(OpType op_type, FPOProgramNode &operand)
+  : FPOProgramNode(UnaryOp), m_op_type(op_type), m_operand(&operand) {}
 
-  void Accept(FPOProgramASTVisitor *visitor) override;
+  void Accept(FPOProgramASTVisitor &visitor) override;
 
   OpType GetOpType() const { return m_op_type; }
 
@@ -154,31 +154,31 @@
 public:
   virtual ~FPOProgramASTVisitor() = default;
 
-  virtual void Visit(FPOProgramNodeSymbol *node) {}
-  virtual void Visit(FPOProgramNodeRegisterRef *node) {}
-  virtual void Visit(FPOProgramNodeIntegerLiteral *node) {}
-  virtual void Visit(FPOProgramNodeBinaryOp *node) {}
-  virtual void Visit(FPOProgramNodeUnaryOp *node) {}
+  virtual void Visit(FPOProgramNodeSymbol &node) {}
+  virtual void Visit(FPOProgramNodeRegisterRef &node) {}
+  virtual void Visit(FPOProgramNodeIntegerLiteral &node) {}
+  virtual void Visit(FPOProgramNodeBinaryOp &node) {}
+  virtual void Visit(FPOProgramNodeUnaryOp &node) {}
 };
 
-void FPOProgramNodeSymbol::Accept(FPOProgramASTVisitor *visitor) {
-  visitor->Visit(this);
+void FPOProgramNodeSymbol::Accept(FPOProgramASTVisitor &visitor) {
+  visitor.Visit(*this);
 }
 
-void FPOProgramNodeRegisterRef::Accept(FPOProgramASTVisitor *visitor) {
-  visitor->Visit(this);
+void FPOProgramNodeRegisterRef::Accept(FPOProgramASTVisitor &visitor) {
+  visitor.Visit(*this);
 }
 
-void FPOProgramNodeIntegerLiteral::Accept(FPOProgramASTVisitor *visitor) {
-  visitor->Visit(this);
+void FPOProgramNodeIntegerLiteral::Accept(FPOProgramASTVisitor &visitor) {
+  visitor.Visit(*this);
 }
 
-void FPOProgramNodeBinaryOp::Accept(FPOProgramASTVisitor *visitor) {
-  visitor->Visit(this);
+void FPOProgramNodeBinaryOp::Accept(FPOProgramASTVisitor &visitor) {
+  visitor.Visit(*this);
 }
 
-void FPOProgramNodeUnaryOp::Accept(FPOProgramASTVisitor *visitor) {
-  visitor->Visit(this);
+void FPOProgramNodeUnaryOp::Accept(FPOProgramASTVisitor &visitor) {
+  visitor.Visit(*this);
 }
 
 class FPOProgramASTVisitorMergeDependent : public FPOProgramASTVisitor {
@@ -191,10 +191,8 @@
   void Merge(FPOProgramNode *&node_ref);
 
 private:
-  void Visit(FPOProgramNodeRegisterRef *node) override {}
-  void Visit(FPOProgramNodeIntegerLiteral *nod

[Lldb-commits] [PATCH] D60268: Breakpad: Parse Stack CFI records

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

This looks good.  I have a few questions inline, but none of those are major 
concerns.




Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:173
+  llvm::DenseMap UnwindRules;
+};
+

I'm not a fan of deep class hierarchies, but given that StackCFIRecord is a 
subset of StackCFIInitRecord and given that the naming suggests 
StackCFIInitRecord is a concrete type of StackCFIRecord, should 
StackCFIInitRecord derive from StackCFIRecord?  (Or perhaps contain one?)

If not, perhaps a comment to explain why not.



Comment at: unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp:123
+"STACK CFI 47 .cfa: $esp 4 + $eip: .cfa 4 - ^"));
+}
+

All of the negative parsing tests seem to be incomplete (e.g., truncated or a 
missing `INIT`).  Should there be others, like having the wrong token type?  Or 
an invalid token?  Or one that has extra tokens at the end of an otherwise 
valid record?

I see you're following the pattern for the other types here, but is that enough?



Comment at: unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp:132
+StackCFIRecord::parse("STACK CFI INIT 47 8 .cfa: $esp 4 +"));
+}

This test relies on the parsing test for the StackCFIInitRecord having covered 
most of the cases.  That works because the parsing implementation is shared, so 
I'm OK with it.  Will others be concerned that this test isn't as independent 
as it could be?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60268/new/

https://reviews.llvm.org/D60268



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60271: PDBFPO: Use references instead of pointers, where possible

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

LGTM.

I noticed this also deleted two overloads of Visit from 
FPOProgramASTVisitorDWARFCodegen, but that appears to be harmless (the base 
class overloads were also no-ops).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60271/new/

https://reviews.llvm.org/D60271



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60152: Fix and simplify PrepareCommandsForSourcing

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

llvm/tools/lldb/tools/driver/Driver.cpp:495:12: error: comparison of integers 
of different signs: 'ssize_t' (aka 'long') and 'size_t' (aka 'unsigned long') 
[-Werror,-Wsign-compare]

  if (nrwr != commands_size) {
   ^  ~

1 error generated.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60152/new/

https://reviews.llvm.org/D60152



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60180: [CMake] Don't explicitly use LLVM_LIBRARY_DIR in standalone builds

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

In D60180#1454634 , @sgraenitz wrote:

> > LLVM_LIBRARY_DIR is being set correctly. It appears to be getting it from 
> > the LLVMConfig we get from find_package(LLVM).
>
> Yes LLVMConfig sets the variable, but it's not going to be in the cache:
>
>   llvm-macosx-x86_64/lib/cmake/llvm/LLVMConfig.cmake
>   178:set(LLVM_LIBRARY_DIR "/path/to/llvm-macosx-x86_64/./lib")
>
>
> Does `cmake -L ...` still dump it? Do in-tree builds have a cache entry for 
> it? Are other subprojects relying on it? (unlikely for LLDB)


No, invoking `cmake -L ...` does not dump this variable with this patch 
applied. In-tree builds don't dump it either, since it's not a cache variable 
when it's an in-tree build. It appears only standalone builds of subprojects do 
this.
No other subprojects are relying on LLDB having this set from what I can tell.

> Pardon my stubbornness, but fixing such things a few weeks later is just too 
> annoying.

I actually appreciate the level of scrutiny. I'd like to move more carefully 
here, since I know that my initial move from llvm-config to find_package(LLVM) 
gave you some issues with tests.
From what I remember, you added back `LLVM_MAIN_SRC_DIR`, 
`LLVM_MAIN_INCLUDE_DIR`, `LLVM_LIBRARY_DIR`, and `LLVM_BINARY_DIR` to unbreak 
the unittests.
I know `LLVM_MAIN_SRC_DIR` is used to make sure the gtest library is built and 
the headers are found correctly. I think setting this is the right thing to do 
since LLVM only exports `LLVM_BUILD_MAIN_SRC_DIR`. `LLVM_MAIN_INCLUDE_DIR` is 
for TableGen, and from what I can tell this should come from LLVMConfig now as 
well. As for the other two, `LLVM_BINARY_DIR` and `LLVM_BINARY_DIR`, these are 
used to configure lit in LLDB. These should be picked up through 
`find_package(LLVM)`. If there's a situation where any of these variables don't 
get propagated from the LLVMConfig, I'd like to know about it so I can add 
comments above these variables explaining why these variables are set since 
it's non-obvious just from grepping the lldb tree.

>> I think we could also get rid of LLVM_BINARY_DIR if that's the case, since 
>> it appears we only set it for lit.
> 
> Yes cleanup is very important, but let's not focus only on what *can* be 
> removed. IMHO a good order of priorities in build system code may be:
> 
> 1. keep differences between in-tree and standalone builds small
> 2. keep differences between subprojects small
> 3. keep the code clean
> 
>   If we are really sure that this code it outdated, please remove it and 
> ideally also initiate a similar cleanup in clang, compiler-rt, lld, etc. (or 
> ping someone to have a look).

I agree and think this is a handy methodology. A large part of my motivation 
behind this change is because I'm noticing a difference between the in-tree and 
standalone builds that is breaking a use case I want to support. In order to 
keep the differences between subprojects small, I don't mind removing it from 
other subprojects as well. That is, as soon as I'm more confident that this 
isn't going to break anything. :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60180/new/

https://reviews.llvm.org/D60180



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Thanks Pavel, I'll convert this to use Expected<> while I'm working on it.  I'm 
writing a test case right now, and looking at how DynamicPluginDarwin works 
more closely for adding & removing binaries, e.g. there's also no way to batch 
remove libraries and have a group broadcast notification be sent.  There was 
also a bug in my original patch for calling LoadScriptingResourceForModule() to 
find a python resource file in a dSYM bundle - it is currently called from 
Target::ModuleUpdated when a binary is added to the target's ModuleList, but 
I'm suppressing that call with this change, so I need to move that over to 
ModulesDidLoad.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60172/new/

https://reviews.llvm.org/D60172



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D60300: [testsuite] Split Objective-C data formatter

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: aprantl, davide, labath.
Herald added a project: LLDB.
JDevlieghere updated this revision to Diff 193822.
JDevlieghere added a comment.

Fix CF test


The testcase for objective-c data formatters is very big as it checks a bunch 
of stuff. This is annoying when using the lit test driver, because it prevents 
us from running the different cases in parallel. As a result, it's always one 
of the last few tests that complete. This patch splits the test into multiple 
files that share a common base class. This way lit can run the different tests 
in parallel.


https://reviews.llvm.org/D60300

Files:
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/ObjCDataFormatterTestCase.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCCF.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCExpr.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCKVO.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSBundle.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSContainer.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSData.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDate.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSError.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSURL.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCPlain.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjNSException.py

Index: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjNSException.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjNSException.py
@@ -0,0 +1,41 @@
+# encoding: utf-8
+"""
+Test lldb data formatter subsystem.
+"""
+
+from __future__ import print_function
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+from ObjCDataFormatterTestCase import ObjCDataFormatterTestCase
+
+
+class ObjCDataFormatterNSException(ObjCDataFormatterTestCase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+# Call super's setUp().
+ObjCDataFormatterTestCase.setUp(self)
+
+@skipUnlessDarwin
+def test_nsexception_with_run_command(self):
+"""Test formatters for NSException."""
+self.appkit_tester_impl(self.nsexception_data_formatter_commands)
+
+def nsexception_data_formatter_commands(self):
+self.expect(
+'frame variable except0 except1 except2 except3',
+substrs=[
+'(NSException *) except0 = ',
+'name: @"TheGuyWhoHasNoName" - reason: @"cuz it\'s funny"',
+'(NSException *) except1 = ',
+'name: @"TheGuyWhoHasNoName~1" - reason: @"cuz it\'s funny"',
+'(NSException *) except2 = ',
+'name: @"TheGuyWhoHasNoName`2" - reason: @"cuz it\'s funny"',
+'(NSException *) except3 = ',
+'name: @"TheGuyWhoHasNoName/3" - reason: @"cuz it\'s funny"'
+])
Index: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCPlain.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCPlain.py
@@ -0,0 +1,96 @@
+# encoding: utf-8
+"""
+Test lldb data formatter subsystem.
+"""
+
+from __future__ import print_function
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+from ObjCDataFormatterTestCase import ObjCDataFormatterTestCase
+
+
+class ObjCDataFormatterNSPlain(ObjCDataFormatterTestCase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+# Call super's setUp().
+ObjCDataFormatterTestCase.setUp(self)
+
+@skipUnlessDarwin
+def test_plain_objc_with_run_command(self):
+"""Test basic ObjC formatting behavior."""
+self.build()
+self.plain_data_formatter_commands()
+
+def plain_data_formatter_commands(self):
+

[Lldb-commits] [PATCH] D60300: [testsuite] Split Objective-C data formatter

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 193822.
JDevlieghere added a comment.

Fix CF test


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60300/new/

https://reviews.llvm.org/D60300

Files:
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/ObjCDataFormatterTestCase.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCCF.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCExpr.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCKVO.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSBundle.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSContainer.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSData.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSDate.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSError.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCNSURL.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCPlain.py
  
lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjNSException.py

Index: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjNSException.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjNSException.py
@@ -0,0 +1,41 @@
+# encoding: utf-8
+"""
+Test lldb data formatter subsystem.
+"""
+
+from __future__ import print_function
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+from ObjCDataFormatterTestCase import ObjCDataFormatterTestCase
+
+
+class ObjCDataFormatterNSException(ObjCDataFormatterTestCase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+# Call super's setUp().
+ObjCDataFormatterTestCase.setUp(self)
+
+@skipUnlessDarwin
+def test_nsexception_with_run_command(self):
+"""Test formatters for NSException."""
+self.appkit_tester_impl(self.nsexception_data_formatter_commands)
+
+def nsexception_data_formatter_commands(self):
+self.expect(
+'frame variable except0 except1 except2 except3',
+substrs=[
+'(NSException *) except0 = ',
+'name: @"TheGuyWhoHasNoName" - reason: @"cuz it\'s funny"',
+'(NSException *) except1 = ',
+'name: @"TheGuyWhoHasNoName~1" - reason: @"cuz it\'s funny"',
+'(NSException *) except2 = ',
+'name: @"TheGuyWhoHasNoName`2" - reason: @"cuz it\'s funny"',
+'(NSException *) except3 = ',
+'name: @"TheGuyWhoHasNoName/3" - reason: @"cuz it\'s funny"'
+])
Index: lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCPlain.py
===
--- /dev/null
+++ lldb/packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-objc/TestDataFormatterObjCPlain.py
@@ -0,0 +1,96 @@
+# encoding: utf-8
+"""
+Test lldb data formatter subsystem.
+"""
+
+from __future__ import print_function
+
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+from ObjCDataFormatterTestCase import ObjCDataFormatterTestCase
+
+
+class ObjCDataFormatterNSPlain(ObjCDataFormatterTestCase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def setUp(self):
+# Call super's setUp().
+ObjCDataFormatterTestCase.setUp(self)
+
+@skipUnlessDarwin
+def test_plain_objc_with_run_command(self):
+"""Test basic ObjC formatting behavior."""
+self.build()
+self.plain_data_formatter_commands()
+
+def plain_data_formatter_commands(self):
+"""Test basic ObjC formatting behavior."""
+self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+lldbutil.run_break_set_by_file_and_line(
+self, "main.m", self.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=[

[Lldb-commits] [PATCH] D60300: [testsuite] Split Objective-C data formatter

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

I think this is good regardless for readability, but I would really appreciate 
if we can collect some numbers to see how effective this change actually is.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60300/new/

https://reviews.llvm.org/D60300



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits