[Lldb-commits] [PATCH] D49368: Complete user-defined types from PDB symbol files

[Aleksandr Urakov via Phabricator via lldb-commits]

aleksandr.urakov abandoned this revision.
aleksandr.urakov added a comment.

Abandoned due to https://reviews.llvm.org/D49980


https://reviews.llvm.org/D49368



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

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In https://reviews.llvm.org/D48782#1164786, @labath wrote:

> For the purposes of integration testing I think it would be better to just 
> silently accept these so that all tests work "out of the box" with dwz. It 
> might be nice to hide these details in a wrapper script.


FYI the LLDB testsuite uses DWZ in a weird way so that the DWZ common file 
(external file in `/usr/lib/debug/.dwz/` with DIEs shared by multiple `.debug` 
files) can be tested by LLDB testsuite. It makes a copy of the `.debug` file so 
that the DWZ-tool thinks most DIEs are used by two files and makes a DWZ common 
file for them. This is why such script has no use outside of the run by LLDB 
testsuite. This is why the script would not be useful on its own. Also 
currently the DWZ is ran always only on one file (two files - the one file 
being duplicated) while in practice one runs DWZ on multiple similar files (all 
files of a rpm package being built) - which is never done by the LLDB testsuite 
(the file duplication tests the DWZ common files more thoroughly IMO).


https://reviews.llvm.org/D48782



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

[Lldb-commits] [PATCH] D40475: DWZ 05/06: DWZ test mode

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

There is no DWZ-tool wrapping script as discussed at: D48782 



https://reviews.llvm.org/D40475



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

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

[Pavel Labath via Phabricator via lldb-commits]

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

This is really great. Thank you for doing this. I have some small ideas for 
improvement, but I don't think we have to go through another review cycle for 
that.




Comment at: unittests/Utility/StreamTest.cpp:38-41
+TEST_F(StreamTest, ChangingByteOrder) {
+  s.SetByteOrder(lldb::eByteOrderPDP);
+  EXPECT_EQ(lldb::eByteOrderPDP, s.GetByteOrder());
+}

 I've been wondering for a while whether we shouldn't just remove PDP 
byte order support. Most of our code doesn't really support it, and neither 
does llvm's, so this is kind of a prerequisite for switching to llvm streams. 




Comment at: unittests/Utility/StreamTest.cpp:56
+  s.PutChar('\n');
+  EXPECT_EQ(" \n", Value());
+

How do you feel about changing `Value` to call `Clear` on the underlying 
StreamString after fetching the string (and maybe renaming it to `TakeValue` or 
something)? That way, you could easily test the string printed by a specific 
function, instead of having to accumulate the expectations.



Comment at: unittests/Utility/StreamTest.cpp:88-95
+  s.QuotedCString("foo");
+  EXPECT_EQ("\"foo\"", Value());
+
+  s.QuotedCString("bar");
+  EXPECT_EQ("\"foo\"\"bar\"", Value());
+
+  s.QuotedCString(" ");

Could you use raw string literals `R"(...)"` for the expectations? It's easier 
to see what this is doing that way.


https://reviews.llvm.org/D50027



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

[Lldb-commits] [PATCH] D50038: Introduce install-lldb-framework target

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I am glad filing the cmake bug has paid off. :)

I just have one small question about this patch.




Comment at: cmake/modules/AddLLDB.cmake:81-87
+  # install-liblldb{,-stripped} is the actual target that will install the
+  # framework, so it must rely on the framework being fully built first.
+  if (LLDB_BUILD_FRAMEWORK AND ${name} STREQUAL "liblldb")
+add_dependencies(install-${name} lldb-framework)
+add_dependencies(install-lldb-framework-stripped lldb-framework)
+  endif()
+

Shouldn't this be also guarded under the `NOT LLVM_INSTALL_TOOLCHAIN_ONLY`, 
like the code above? If someone sets `LLVM_INSTALL_TOOLCHAIN_ONLY=True`, then 
the install-liblldb target will not even be generated and these lines will fail 
(or just do something weird).


https://reviews.llvm.org/D50038



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

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:21
+@add_test_categories(["libc++"])
+@skipIf(oslist=no_match(["macosx"]), compiler="clang", 
compiler_version=['<', '5.0'])
+

Could you add another line for `gcc` here? The -std=c++17 flag seems to be 
supported starting with gcc-5.1.

Also a comment that this is due to the -std flag would be helpful to people 
looking at this in the future.



Comment at: 
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:44
+if output == "(bool) has_optional = false" :
+exit(0)
+

Replace by `self.skipTest(...)`. This way, the test will be properly marked as 
skipped.



Comment at: 
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/main.cpp:15-17
+using optional_int = std::optional ;
+using optional_int_vect = std::optional ;
+using optional_string = std::optional ;

these need to be guarded by the `#if` too (i'd just move then into the main 
function).


https://reviews.llvm.org/D49271



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

[Lldb-commits] [PATCH] D49980: [PDB] Parse UDT symbols and pointers to members (combined patch)

[Aleksandr Urakov via Phabricator via lldb-commits]

aleksandr.urakov updated this revision to Diff 158194.
aleksandr.urakov added a comment.

Added support of a MSInheritance attribute.


https://reviews.llvm.org/D49980

Files:
  include/lldb/Symbol/ClangASTContext.h
  lit/SymbolFile/PDB/Inputs/ClassLayoutTest.cpp
  lit/SymbolFile/PDB/Inputs/PointerTypeTest.cpp
  lit/SymbolFile/PDB/Inputs/UdtLayoutTest.cpp
  lit/SymbolFile/PDB/Inputs/UdtLayoutTest.script
  lit/SymbolFile/PDB/class-layout.test
  lit/SymbolFile/PDB/pointers.test
  lit/SymbolFile/PDB/udt-layout.test
  source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  source/Plugins/SymbolFile/PDB/PDBASTParser.h
  source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  source/Symbol/ClangASTContext.cpp

Index: source/Symbol/ClangASTContext.cpp
===
--- source/Symbol/ClangASTContext.cpp
+++ source/Symbol/ClangASTContext.cpp
@@ -124,6 +124,84 @@
  // Use Clang for D until there is a proper language plugin for it
  language == eLanguageTypeD;
 }
+
+// Checks whether m1 is an overload of m2 (as opposed to an override). This is
+// called by addOverridesForMethod to distinguish overrides (which share a
+// vtable entry) from overloads (which require distinct entries).
+bool isOverload(clang::CXXMethodDecl *m1, clang::CXXMethodDecl *m2) {
+  // FIXME: This should detect covariant return types, but currently doesn't.
+  lldbassert(&m1->getASTContext() == &m2->getASTContext() &&
+ "Methods should have the same AST context");
+  clang::ASTContext &context = m1->getASTContext();
+
+  const auto *m1Type = llvm::cast(
+  context.getCanonicalType(m1->getType()));
+
+  const auto *m2Type = llvm::cast(
+  context.getCanonicalType(m2->getType()));
+
+  auto compareArgTypes = [&context](const clang::QualType &m1p,
+const clang::QualType &m2p) {
+return context.hasSameType(m1p.getUnqualifiedType(),
+   m2p.getUnqualifiedType());
+  };
+
+  // FIXME: In C++14 and later, we can just pass m2Type->param_type_end()
+  //as a fourth parameter to std::equal().
+  return (m1->getNumParams() != m2->getNumParams()) ||
+ !std::equal(m1Type->param_type_begin(), m1Type->param_type_end(),
+ m2Type->param_type_begin(), compareArgTypes);
+}
+
+// If decl is a virtual method, walk the base classes looking for methods that
+// decl overrides. This table of overridden methods is used by IRGen to
+// determine the vtable layout for decl's parent class.
+void addOverridesForMethod(clang::CXXMethodDecl *decl) {
+  if (!decl->isVirtual())
+return;
+
+  clang::CXXBasePaths paths;
+
+  auto find_overridden_methods =
+  [decl](const clang::CXXBaseSpecifier *specifier,
+ clang::CXXBasePath &path) {
+if (auto *base_record = llvm::dyn_cast(
+specifier->getType()->getAs()->getDecl())) {
+
+  clang::DeclarationName name = decl->getDeclName();
+
+  // If this is a destructor, check whether the base class destructor is
+  // virtual.
+  if (name.getNameKind() == clang::DeclarationName::CXXDestructorName)
+if (auto *baseDtorDecl = base_record->getDestructor()) {
+  if (baseDtorDecl->isVirtual()) {
+path.Decls = baseDtorDecl;
+return true;
+  } else
+return false;
+}
+
+  // Otherwise, search for name in the base class.
+  for (path.Decls = base_record->lookup(name); !path.Decls.empty();
+   path.Decls = path.Decls.slice(1)) {
+if (auto *method_decl =
+llvm::dyn_cast(path.Decls.front()))
+  if (method_decl->isVirtual() && !isOverload(decl, method_decl)) {
+path.Decls = method_decl;
+return true;
+  }
+  }
+}
+
+return false;
+  };
+
+  if (decl->getParent()->lookupInBases(find_overridden_methods, paths)) {
+for (auto *overridden_decl : paths.found_decls())
+  decl->addOverriddenMethod(
+  llvm::cast(overridden_decl));
+  }
+}
 }
 
 typedef lldb_private::ThreadSafeDenseMap
@@ -6373,7 +6451,7 @@
   language_flags = 0;
 
   const bool idx_is_valid = idx < GetNumChildren(type, omit_empty_base_classes);
-  uint32_t bit_offset;
+  int32_t bit_offset;
   switch (parent_type_class) {
   case clang::Type::Builtin:
 if (idx_is_valid) {
@@ -6465,8 +6543,8 @@
 cxx_record_decl, base_class_decl);
 const lldb::addr_t base_offset_addr =
 vbtable_ptr + vbtable_index * 4;
-const uint32_t base_offset =
-process->ReadUnsignedIntegerFromMemory(
+const int32_t base_offset =
+process->ReadSigned

[Lldb-commits] [PATCH] D49909: Unit test for Symtab::InitNameIndexes

[Jonas Devlieghere via Phabricator via lldb-commits]

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

I agree with Pavel, but I also don't mind having this test in the meantime. 
LGTM if you add a FIXME with the direction we want to go.


https://reviews.llvm.org/D49909



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

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

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

sgraenitz updated this revision to Diff 158206.
sgraenitz added a comment.

Remove test code, that I added accidentally. Move RichManglingInfo and 
RichManglingSpec to their own header and cpp.


https://reviews.llvm.org/D49990

Files:
  include/lldb/Core/RichManglingInfo.h
  include/lldb/Symbol/Symtab.h
  source/Core/CMakeLists.txt
  source/Core/Mangled.cpp
  source/Core/RichManglingInfo.cpp
  source/Symbol/Symtab.cpp

Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -10,9 +10,10 @@
 #include 
 #include 
 
-#include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
 #include "Plugins/Language/ObjC/ObjCLanguage.h"
+
 #include "lldb/Core/Module.h"
+#include "lldb/Core/RichManglingInfo.h"
 #include "lldb/Core/STLUtils.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Symbol/ObjectFile.h"
@@ -22,7 +23,6 @@
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/Timer.h"
-#include "llvm/Demangle/Demangle.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -213,81 +213,6 @@
   return nullptr;
 }
 
-//--
-// RichManglingInfo
-//--
-
-void RichManglingInfo::ResetProvider() {
-  // If we want to support parsers for other languages some day, we need a
-  // switch here to delete the correct parser type.
-  if (m_legacy_parser) {
-assert(m_provider == RichManglingInfo::PluginCxxLanguage);
-delete get();
-m_legacy_parser = nullptr;
-  }
-}
-
-RichManglingInfo *RichManglingSpec::CreateItaniumInfo() {
-  m_info.ResetProvider();
-  m_info.m_provider = RichManglingInfo::ItaniumPartialDemangler;
-  m_info.m_IPD = &m_IPD;
-  return &m_info;
-}
-
-RichManglingInfo *
-RichManglingSpec::CreateLegacyCxxParserInfo(const ConstString &mangled) {
-  m_info.ResetProvider();
-  m_info.m_provider = RichManglingInfo::PluginCxxLanguage;
-  m_info.m_legacy_parser = new CPlusPlusLanguage::MethodName(mangled);
-  return &m_info;
-}
-
-RichManglingInfo::~RichManglingInfo() { 
-  ResetProvider();
-  delete m_IPD_buf;
-}
-
-bool RichManglingInfo::isCtorOrDtor() const {
-  switch (m_provider) {
-  case ItaniumPartialDemangler:
-return m_IPD->isCtorOrDtor();
-  case PluginCxxLanguage: {
-// We can only check for destructors here.
-auto base_name = get()->GetBasename();
-return base_name.front() == '~';
-  }
-  }
-}
-
-bool RichManglingInfo::isFunction() const {
-  switch (m_provider) {
-  case ItaniumPartialDemangler:
-return m_IPD->isFunction();
-  case PluginCxxLanguage:
-return get()->IsValid();
-  }
-}
-
-const char *RichManglingInfo::getFunctionBaseName() const {
-  switch (m_provider) {
-  case ItaniumPartialDemangler:
-m_IPD_buf = m_IPD->getFunctionBaseName(m_IPD_buf, &m_IPD_size);
-return m_IPD_buf;
-  case PluginCxxLanguage:
-return get()->GetBasename().data();
-  }
-}
-
-const char *RichManglingInfo::getFunctionDeclContextName() const {
-  switch (m_provider) {
-  case ItaniumPartialDemangler:
-m_IPD_buf = m_IPD->getFunctionDeclContextName(m_IPD_buf, &m_IPD_size);
-return m_IPD_buf;
-  case PluginCxxLanguage:
-return get()->GetContext().data();
-  }
-}
-
 //--
 // InitNameIndexes
 //--
Index: source/Core/RichManglingInfo.cpp
===
--- /dev/null
+++ source/Core/RichManglingInfo.cpp
@@ -0,0 +1,87 @@
+//===-- RichManglingInfo.cpp *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Core/RichManglingInfo.h"
+#include "lldb/Utility/ConstString.h"
+
+#include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+void RichManglingInfo::ResetProvider() {
+  // If we want to support parsers for other languages some day, we need a
+  // switch here to delete the correct parser type.
+  if (m_legacy_parser) {
+assert(m_provider == RichManglingInfo::PluginCxxLanguage);
+delete get();
+m_legacy_parser = nullptr;
+  }
+}
+
+RichManglingInfo *RichManglingSpec::CreateItaniumInfo() {
+  m_info.ResetProvider();
+  m_info.m_provider = RichManglingInfo::ItaniumPartialDemangler;
+  m_info.m_IPD = &m_IPD;
+  return &m_info;
+}
+
+RichManglingInfo *
+RichManglingSpec::CreateLegacyCxxParserInfo(const ConstString &mangled) {
+  m_info.ResetProvider();
+  m_info.m_provider = RichManglingInfo::PluginCxxLanguage;
+  m_info.m_legacy_parser = new CPlusPlusLanguage::Me

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

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

sgraenitz marked 3 inline comments as done.
sgraenitz added a comment.

> The part I'm not sure about is whether the RichManglingInfo vs. 
> RichManglingSpec distinction brings any value. [...] So you might as well 
> just have one class, pass that to DemangleWithRichManglingInfo, and then 
> query the same object when the call returns.

The idea here was that `DemangleWithRichManglingInfo()` acts like a gate 
keeper. If it succeeds it provides read-only access to the updated 
`RichManglingInfo` in `RichManglingSpec`, otherwise it returns null. IMHO the 
value behind it is that `RichManglingInfo` does not need to handle a `NoInfo` 
case next to `ItaniumPartialDemangler` and `PluginCxxLanguage` in every single 
getter. Instead it's just not accessible in that state. (Plus: there is no 
maintenance functions that confuse the public interface of `RichManglingInfo`.) 
I don't know how to do this with only one class.

Maybe `RichManglingSpec` is not the perfect name. What about renaming it to 
`RichManglingContext`?

> I mean, the lifetime of the first is tied to the lifetime of the second, and 
> the Spec class can only have one active Info instance at any given moment.

Yes, this was handy and avoids extra heap-allocations. `const RichManglingInfo 
*` was intended to clarify: lifetimes are handled elsewhere.

> The current interface with createItaniumInfo et al. makes it seem like one 
> could call it multiple times in sequence, stashing the results, and then 
> doing some post-processing on them.

Yes, I can see that this is implicit knowledge. Do you have an idea how to make 
this more explicit? Rename to `SetItaniumInfo()` maybe?

In the end, I definitely prefer this approach over having a `NoInfo` state in 
`RichManglingInfo`. What do you think?


https://reviews.llvm.org/D49990



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

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think there is still something wrong with the diff. I can't see any of the 
callers of e.g. `createItaniumInfo` but I can see the function on both LHS and 
RHS of the diff (which shouldn't be the case as it's a new function). It looks 
like you uploaded just an ammending patch instead of the entire work. Can you 
fix that?

In https://reviews.llvm.org/D49990#1182003, @sgraenitz wrote:

> > The part I'm not sure about is whether the RichManglingInfo vs. 
> > RichManglingSpec distinction brings any value. [...] So you might as well 
> > just have one class, pass that to DemangleWithRichManglingInfo, and then 
> > query the same object when the call returns.
>
> The idea here was that `DemangleWithRichManglingInfo()` acts like a gate 
> keeper. If it succeeds it provides read-only access to the updated 
> `RichManglingInfo` in `RichManglingSpec`, otherwise it returns null. IMHO the 
> value behind it is that `RichManglingInfo` does not need to handle a `NoInfo` 
> case next to `ItaniumPartialDemangler` and `PluginCxxLanguage` in every 
> single getter. Instead it's just not accessible in that state. (Plus: there 
> is no maintenance functions that confuse the public interface of 
> `RichManglingInfo`.) I don't know how to do this with only one class.
>
> Maybe `RichManglingSpec` is not the perfect name. What about renaming it to 
> `RichManglingContext`?
>
> > I mean, the lifetime of the first is tied to the lifetime of the second, 
> > and the Spec class can only have one active Info instance at any given 
> > moment.
>
> Yes, this was handy and avoids extra heap-allocations. `const 
> RichManglingInfo *` was intended to clarify: lifetimes are handled elsewhere.
>
> > The current interface with createItaniumInfo et al. makes it seem like one 
> > could call it multiple times in sequence, stashing the results, and then 
> > doing some post-processing on them.
>
> Yes, I can see that this is implicit knowledge. Do you have an idea how to 
> make this more explicit? Rename to `SetItaniumInfo()` maybe?
>
> In the end, I definitely prefer this approach over having a `NoInfo` state in 
> `RichManglingInfo`. What do you think?


Yes, I can see what you mean here. Neither of the solutions is particularly 
appealing. I guess if I were implementing this, I'd go with the "invalid state" 
option, though I am not sure why, as usually I am opposed to invalid states. 
Maybe we can leave this to the discretion of the implementor (you).




Comment at: include/lldb/Symbol/Symtab.h:81-83
+  // No move
+  RichManglingInfo(RichManglingInfo &&) = delete;
+  RichManglingInfo &operator=(RichManglingInfo &&) = delete;

sgraenitz wrote:
> labath wrote:
> > This is implied by the deleted copy operations.
> Which are implicitly deleted too, due to the existence of the destructor 
> right? Does LLVM/LLDB have some kind of convention for it? I like to be 
> explicit on ctors&assignment ("rule of 5"), because it aids error messages, 
> but I would be fine with following the existing convention here.
As far as I know, the presence of a destructor has no impact on the state of 
copy/move operations, so you still need to delete the copy operations 
explicitly.

I don't know if there is an official policy on explicitly deleting move 
operations, but I don't remember seeing that style anywhere. However, I don't 
care much about that either.


https://reviews.llvm.org/D49990



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

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

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

sgraenitz added inline comments.



Comment at: include/lldb/Symbol/Symtab.h:81-83
+  // No move
+  RichManglingInfo(RichManglingInfo &&) = delete;
+  RichManglingInfo &operator=(RichManglingInfo &&) = delete;

labath wrote:
> sgraenitz wrote:
> > labath wrote:
> > > This is implied by the deleted copy operations.
> > Which are implicitly deleted too, due to the existence of the destructor 
> > right? Does LLVM/LLDB have some kind of convention for it? I like to be 
> > explicit on ctors&assignment ("rule of 5"), because it aids error messages, 
> > but I would be fine with following the existing convention here.
> As far as I know, the presence of a destructor has no impact on the state of 
> copy/move operations, so you still need to delete the copy operations 
> explicitly.
> 
> I don't know if there is an official policy on explicitly deleting move 
> operations, but I don't remember seeing that style anywhere. However, I don't 
> care much about that either.
> The generation of the implicitly-defined copy constructor is deprecated if T 
> has a user-defined destructor or user-defined copy assignment operator.
https://en.cppreference.com/w/cpp/language/copy_constructor

Actually, copy has no implications here and move won't work on the const 
pointer. Thus I will just remove it :)



Comment at: source/Core/Mangled.cpp:325
 
   return spec.CreateItaniumInfo();
 } else {

> I think there is still something wrong with the diff. I can't see any of the 
> callers of e.g. createItaniumInfo
Weird. The caller is here, but not shown as a change anymore..


https://reviews.llvm.org/D49990



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

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

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

sgraenitz updated this revision to Diff 158231.
sgraenitz added a comment.

Fix potential leak of m_IPD_buf. Use llvm::Any instead of void*. 
Rename: RichManglingSpec -> RichManglingContext, 
RichManglingContext::CreateXyInfo() -> RichManglingContext::SetXyInfo()


https://reviews.llvm.org/D49990

Files:
  include/lldb/Core/Mangled.h
  include/lldb/Core/RichManglingInfo.h
  include/lldb/Symbol/Symtab.h
  include/lldb/lldb-forward.h
  source/Core/CMakeLists.txt
  source/Core/Mangled.cpp
  source/Core/RichManglingInfo.cpp
  source/Symbol/Symtab.cpp

Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -10,9 +10,10 @@
 #include 
 #include 
 
-#include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
 #include "Plugins/Language/ObjC/ObjCLanguage.h"
+
 #include "lldb/Core/Module.h"
+#include "lldb/Core/RichManglingInfo.h"
 #include "lldb/Core/STLUtils.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Symbol/ObjectFile.h"
@@ -22,7 +23,6 @@
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/Timer.h"
-#include "llvm/Demangle/Demangle.h"
 
 using namespace lldb;
 using namespace lldb_private;
@@ -213,81 +213,6 @@
   return nullptr;
 }
 
-//--
-// RichManglingInfo
-//--
-
-void RichManglingInfo::ResetProvider() {
-  // If we want to support parsers for other languages some day, we need a
-  // switch here to delete the correct parser type.
-  if (m_legacy_parser) {
-assert(m_provider == RichManglingInfo::PluginCxxLanguage);
-delete get();
-m_legacy_parser = nullptr;
-  }
-}
-
-RichManglingInfo *RichManglingSpec::CreateItaniumInfo() {
-  m_info.ResetProvider();
-  m_info.m_provider = RichManglingInfo::ItaniumPartialDemangler;
-  m_info.m_IPD = &m_IPD;
-  return &m_info;
-}
-
-RichManglingInfo *
-RichManglingSpec::CreateLegacyCxxParserInfo(const ConstString &mangled) {
-  m_info.ResetProvider();
-  m_info.m_provider = RichManglingInfo::PluginCxxLanguage;
-  m_info.m_legacy_parser = new CPlusPlusLanguage::MethodName(mangled);
-  return &m_info;
-}
-
-RichManglingInfo::~RichManglingInfo() { 
-  ResetProvider();
-  delete m_IPD_buf;
-}
-
-bool RichManglingInfo::isCtorOrDtor() const {
-  switch (m_provider) {
-  case ItaniumPartialDemangler:
-return m_IPD->isCtorOrDtor();
-  case PluginCxxLanguage: {
-// We can only check for destructors here.
-auto base_name = get()->GetBasename();
-return base_name.front() == '~';
-  }
-  }
-}
-
-bool RichManglingInfo::isFunction() const {
-  switch (m_provider) {
-  case ItaniumPartialDemangler:
-return m_IPD->isFunction();
-  case PluginCxxLanguage:
-return get()->IsValid();
-  }
-}
-
-const char *RichManglingInfo::getFunctionBaseName() const {
-  switch (m_provider) {
-  case ItaniumPartialDemangler:
-m_IPD_buf = m_IPD->getFunctionBaseName(m_IPD_buf, &m_IPD_size);
-return m_IPD_buf;
-  case PluginCxxLanguage:
-return get()->GetBasename().data();
-  }
-}
-
-const char *RichManglingInfo::getFunctionDeclContextName() const {
-  switch (m_provider) {
-  case ItaniumPartialDemangler:
-m_IPD_buf = m_IPD->getFunctionDeclContextName(m_IPD_buf, &m_IPD_size);
-return m_IPD_buf;
-  case PluginCxxLanguage:
-return get()->GetContext().data();
-  }
-}
-
 //--
 // InitNameIndexes
 //--
@@ -361,7 +286,7 @@
 
 // Instantiation of the demangler is expensive, so better use a single one
 // for all entries during batch processing.
-RichManglingSpec spec;
+RichManglingContext MC;
 NameToIndexMap::Entry entry;
 
 for (entry.value = 0; entry.value < num_symbols; ++entry.value) {
@@ -392,7 +317,7 @@
 const SymbolType type = symbol->GetType();
 if (type == eSymbolTypeCode || type == eSymbolTypeResolver) {
   if (const RichManglingInfo *info =
-  mangled.DemangleWithRichManglingInfo(spec, lldb_skip_name))
+  mangled.DemangleWithRichManglingInfo(MC, lldb_skip_name))
 RegisterMangledNameEntry(entry, class_contexts,
  mangled_name_to_index, symbol_contexts,
  *info);
@@ -465,15 +390,15 @@
 UniqueCStringMap &mangled_name_to_index,
 std::vector &symbol_contexts, const RichManglingInfo &info) {
   // Only register functions that have a base name.
-  const char *base_name_cstr = info.getFunctionBaseName();
+  const char *base_name_cstr = info.GetFunctionBaseName();
   if (base_name_cstr == nullptr)
 return;
 
   // The base name will be our entry's name.
   entry.cstring = ConstString(base_name_cstr);
 
   // Register functions with no context.
-  ConstString 

[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()

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

sgraenitz created this revision.
sgraenitz added reviewers: labath, jingham, JDevlieghere, erik.pilkington.

I set up a new review, because not all the code I touched was marked as a 
change in old one anymore.

In preparation for this review, there were two earlier ones:

- https://reviews.llvm.org/D49612 introduced the ItaniumPartialDemangler to 
LLDB demangling without conceptual changes
- https://reviews.llvm.org/D49909 added a unit test that covers all relevant 
code paths in the InitNameIndexes() function

Primary goals for this patch are:
(1) Use ItaniumPartialDemangler's rich mangling info for building LLDB's name 
index.
(2) Provide a uniform interface.
(3) Improve indexing performance.

The central implementation in this patch is our new function for explicit 
demangling:

  const RichManglingInfo *
  Mangled::DemangleWithRichManglingInfo(RichManglingContext &, 
SkipMangledNameFn *)

It takes a context object and a filter function and provides read-only access 
to the rich mangling info on success, or otherwise returns null. The two new 
classes are:

- `RichManglingInfo` offers a uniform interface to query symbol properties like 
`getFunctionDeclContextName()` or `isCtorOrDtor()` that are forwarded to the 
respective provider internally (`llvm::ItaniumPartialDemangler` or 
`lldb_private::CPlusPlusLanguage::MethodName`).
- `RichManglingContext` works a bit like `LLVMContext`, it the actual 
`RichManglingInfo` returned from `DemangleWithRichManglingInfo()` and handles 
lifetime and configuration. It is likely stack-allocated and can be reused for 
multiple queries during batch processing.

The idea here is that `DemangleWithRichManglingInfo()` acts like a gate keeper. 
It only provides access to `RichManglingInfo` on success, which in turn avoids 
the need to handle a `NoInfo` state in every single one of its getters. Having 
it stored within the context, avoids extra heap allocations and aids (3). As 
instantiations of the IPD the are considered expensive, the context is the 
ideal place to store it too. An efficient filtering function 
`SkipMangledNameFn` is another piece in the performance puzzle and it helps to 
mimic the original behavior of `InitNameIndexes`.

Future potential:

- `DemangleWithRichManglingInfo()` is thread-safe, IFF using different contexts 
in different threads. This may be exploited in the future. (It's another thing 
that it has in common with `LLVMContext`.)
- The old implementation only parsed and indexed Itanium mangled names. The new 
`RichManglingInfo` can be extended for various mangling schemes and languages.

One problem with the implementation of RichManglingInfo is the inaccessibility 
of class `CPlusPlusLanguage::MethodName` (defined in 
source/Plugins/Language/..), from within any header in the Core components of 
LLDB. The rather hacky solution is to store a type erased reference and cast it 
to the correct type on access in the cpp - see 
`RichManglingInfo::get()`. At the moment there seems to be no better 
way to do it. IMHO `CPlusPlusLanguage::MethodName` should be a top-level class 
in order to enable forward delcarations (but that is a rather big change I 
guess).

First simple profiling shows a good speedup. `target create clang` now takes 
0.64s on average. Before the change I observed runtimes between 0.76s an 1.01s. 
This is still no bulletproof data (I only ran it on one machine!), but it's a 
promising indicator I think.


https://reviews.llvm.org/D50071

Files:
  include/lldb/Core/Mangled.h
  include/lldb/Core/RichManglingInfo.h
  include/lldb/Symbol/Symtab.h
  include/lldb/Utility/ConstString.h
  include/lldb/lldb-forward.h
  source/Core/CMakeLists.txt
  source/Core/Mangled.cpp
  source/Core/RichManglingInfo.cpp
  source/Symbol/Symtab.cpp

Index: source/Symbol/Symtab.cpp
===
--- source/Symbol/Symtab.cpp
+++ source/Symbol/Symtab.cpp
@@ -10,9 +10,10 @@
 #include 
 #include 
 
-#include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
 #include "Plugins/Language/ObjC/ObjCLanguage.h"
+
 #include "lldb/Core/Module.h"
+#include "lldb/Core/RichManglingInfo.h"
 #include "lldb/Core/STLUtils.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Symbol/ObjectFile.h"
@@ -215,6 +216,40 @@
 //--
 // InitNameIndexes
 //--
+namespace {
+bool lldb_skip_name(llvm::StringRef mangled, Mangled::ManglingScheme scheme) {
+  switch (scheme) {
+  case Mangled::eManglingSchemeItanium: {
+if (mangled.size() < 3 || !mangled.startswith("_Z"))
+  return true;
+
+// Avoid the following types of symbols in the index.
+switch (mangled[2]) {
+case 'G': // guard variables
+case 'T': // virtual tables, VTT structures, typeinfo structures + names
+case 'Z': // named local entities (if we eventually handle
+  // eSymbolTypeData, we will want this back

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

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

sgraenitz abandoned this revision.
sgraenitz added a comment.

>> I think there is still something wrong with the diff. I can't see any of the 
>> callers of e.g. createItaniumInfo
> 
> Weird. The caller is here, but not shown as a change anymore..

I created a new review where all my changes are marked in green and red: 
https://reviews.llvm.org/D50071
If you have any more feedback, please let me know. I will keep the new one open 
for a few days, so Jim can review it when he is back from vacation.


https://reviews.llvm.org/D49990



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

[Lldb-commits] [PATCH] D49909: Unit test for Symtab::InitNameIndexes

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

sgraenitz added a comment.

Nice. I created a ticket for me internally to turn this into a lit test.
For now I'd merge it once https://reviews.llvm.org/D50071 is done. (But I want 
to keep that one open until Jim had a look.)


https://reviews.llvm.org/D49909



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

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Alexander Polyakov via Phabricator via lldb-commits]

apolyakov added a comment.

So, do you have any thoughts about this approach letting the debugserver choose 
a tcp port and patch overall?


https://reviews.llvm.org/D49739



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

Re: [Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Greg Clayton via lldb-commits]

> On Jul 30, 2018, at 12:17 PM, Leonard Mosescu  wrote:
> 
> FYI, Breakpad & Crashpad will start generating the Microsoft flavor of ARM 
> minidumps soon.

How do we tell the difference? I am guessing the ProcessorArchitecture will 
both be set to "PROCESSOR_ARCHITECTURE_ARM". Are plug-ins expected to look at 
the byte size of the thread context then?

Greg

> 
> On Wed, Jul 25, 2018 at 9:44 AM, Leonard Mosescu via Phabricator 
> mailto:revi...@reviews.llvm.org>> wrote:
> lemo added inline comments.
> 
> 
> 
> Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:51
> +  reg_fpscr,
> +  reg_d0,   reg_d1,  reg_d2,  reg_d3,  reg_d4,  reg_d5,  reg_d6,  reg_d7,
> +  reg_d8,   reg_d9, reg_d10, reg_d11, reg_d12, reg_d13, reg_d14, reg_d15,
> 
> Pavel's comment reminded me: what about the S registers (32bit fp) and Q 
> registers (128bit Neon)?
> 
> 
> https://reviews.llvm.org/D49750 
> 
> 
> 
> 

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

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Sorry for not responding, I've been a bit busy these days. I've wanted to make 
a proof-of-concept to show you that reading the port number from stdout is 
possible, but I haven't gotten around to it yet.

However, I am happy with the current mechanism of choosing the port too (thank 
you). This should make the test much more stable.

I don't have an opinion on the rest of the patch.


https://reviews.llvm.org/D49739



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

[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thank you for updating the patch.

If I understand things correctly, we could avoid circular deps and untyped 
pointers (or `llvm::Any`, which is essentially the same thing), by moving 
`CPlusPlusLanguage::MethodName` to a separate file. Could we do that as a 
preparatory step for this patch?




Comment at: source/Symbol/Symtab.cpp:220
+namespace {
+bool lldb_skip_name(llvm::StringRef mangled, Mangled::ManglingScheme scheme) {
+  switch (scheme) {

make this `static` and drop the surrounding namespace.


https://reviews.llvm.org/D50071



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

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Alexander Polyakov via Phabricator via lldb-commits]

apolyakov added a comment.

OK, thank you for respond. Then, I think, we should wait for review from 
@clayborg or @aprantl, and if they accept the patch I'll divide it into two 
parts: SB API part and target-select one.


https://reviews.llvm.org/D49739



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

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Seems good otherwise.




Comment at: source/API/SBTarget.cpp:1468
+  }
+  if (from[0] && to[0]) {
+Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));

apolyakov wrote:
> I didn't find nullptr check in other API functions, is it OK or it's better 
> to add it?
I would convert it to a ConstString up front and then check whether the 
ConstString is empty. This way you don't have to worry about nullptr vs empty 
string.


https://reviews.llvm.org/D49739



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

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Alexander Polyakov via Phabricator via lldb-commits]

apolyakov added inline comments.



Comment at: source/API/SBTarget.cpp:1468
+  }
+  if (from[0] && to[0]) {
+Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));

aprantl wrote:
> apolyakov wrote:
> > I didn't find nullptr check in other API functions, is it OK or it's better 
> > to add it?
> I would convert it to a ConstString up front and then check whether the 
> ConstString is empty. This way you don't have to worry about nullptr vs empty 
> string.
Maybe then change method's signature from `const char *` to `ConstString`? I 
haven't done this since in other methods strings are represented as pointers to 
char, is it some rule for SB API or not?


https://reviews.llvm.org/D49739



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

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: source/API/SBTarget.cpp:1468
+  }
+  if (from[0] && to[0]) {
+Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));

apolyakov wrote:
> aprantl wrote:
> > apolyakov wrote:
> > > I didn't find nullptr check in other API functions, is it OK or it's 
> > > better to add it?
> > I would convert it to a ConstString up front and then check whether the 
> > ConstString is empty. This way you don't have to worry about nullptr vs 
> > empty string.
> Maybe then change method's signature from `const char *` to `ConstString`? I 
> haven't done this since in other methods strings are represented as pointers 
> to char, is it some rule for SB API or not?
In order to map this to Python properly I think we need to keep the signature 
using const char*.


https://reviews.llvm.org/D49739



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

[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: include/lldb/Core/RichManglingInfo.h:83-84
+public:
+  RichManglingInfo *SetItaniumInfo();
+  RichManglingInfo *SetLegacyCxxParserInfo(const ConstString &mangled);
+

I find it odd that one of these functions is stateless 
(`SetLegacyCxxParserInfo` takes the string which initializes the object as an 
argument), while the other receives the state magically via the IPD accessor.

Would it be possible to change it so that the other option is stateless too 
(`SetItaniumInfo(ConstString mangled)`)?

We could change `RichManglingInfo` to return the full demangled name too, so 
that you have access to the demangled name via this API too (which /I think/ is 
the reason why you created a separate `GetItaniumRichDemangleInfo` function).



Comment at: source/Core/RichManglingInfo.cpp:36-40
+RichManglingContext::SetLegacyCxxParserInfo(const ConstString &mangled) {
+  m_info.ResetProvider();
+  m_info.m_provider = RichManglingInfo::PluginCxxLanguage;
+  m_info.m_legacy_parser = new CPlusPlusLanguage::MethodName(mangled);
+  return &m_info;

Is this really the **mangled** name?



Comment at: source/Core/RichManglingInfo.cpp:69-80
+const char *RichManglingInfo::GetFunctionBaseName() const {
+  switch (m_provider) {
+  case ItaniumPartialDemangler:
+if (auto buf = m_IPD->getFunctionBaseName(m_IPD_buf, &m_IPD_size)) {
+  m_IPD_buf = buf;
+  return buf;
+}

Could these return `StringRef`? Am I correct in assuming that `m_IPD_size` 
holds the size of the returned string? If thats the case then you could even do 
this with no performance impact (or a positive one).


https://reviews.llvm.org/D50071



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

[Lldb-commits] [PATCH] D50038: Introduce install-lldb-framework target

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

In https://reviews.llvm.org/D50038#1181817, @labath wrote:

> I am glad filing the cmake bug has paid off. :)


Same! :)




Comment at: cmake/modules/AddLLDB.cmake:81-87
+  # install-liblldb{,-stripped} is the actual target that will install the
+  # framework, so it must rely on the framework being fully built first.
+  if (LLDB_BUILD_FRAMEWORK AND ${name} STREQUAL "liblldb")
+add_dependencies(install-${name} lldb-framework)
+add_dependencies(install-lldb-framework-stripped lldb-framework)
+  endif()
+

labath wrote:
> Shouldn't this be also guarded under the `NOT LLVM_INSTALL_TOOLCHAIN_ONLY`, 
> like the code above? If someone sets `LLVM_INSTALL_TOOLCHAIN_ONLY=True`, then 
> the install-liblldb target will not even be generated and these lines will 
> fail (or just do something weird).
Ahh, yeah I think you're right. I think it should actually be a level deeper in 
`if (NOT CMAKE_CONFIGURATION_TYPES)`. I'll amend this.


https://reviews.llvm.org/D50038



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

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Alexander Polyakov via Phabricator via lldb-commits]

apolyakov updated this revision to Diff 158319.
apolyakov added a comment.

Added converting from `const char *` to `ConstString`, documented new API.


https://reviews.llvm.org/D49739

Files:
  include/lldb/API/SBTarget.h
  lit/lit.cfg
  lit/tools/lldb-mi/target/inputs/main.c
  lit/tools/lldb-mi/target/inputs/target-select-so-path.py
  lit/tools/lldb-mi/target/lit.local.cfg
  lit/tools/lldb-mi/target/target-select-so-path.test
  scripts/interface/SBTarget.i
  source/API/SBTarget.cpp
  tools/lldb-mi/MICmdCmdTarget.cpp

Index: tools/lldb-mi/MICmdCmdTarget.cpp
===
--- tools/lldb-mi/MICmdCmdTarget.cpp
+++ tools/lldb-mi/MICmdCmdTarget.cpp
@@ -10,9 +10,8 @@
 // Overview:CMICmdCmdTargetSelect   implementation.
 
 // Third Party Headers:
-#include "lldb/API/SBCommandInterpreter.h"
-#include "lldb/API/SBCommandReturnObject.h"
 #include "lldb/API/SBStream.h"
+#include "lldb/API/SBError.h"
 
 // In-house headers:
 #include "MICmdArgValNumber.h"
@@ -52,7 +51,7 @@
 // Return:  None.
 // Throws:  None.
 //--
-CMICmdCmdTargetSelect::~CMICmdCmdTargetSelect() {}
+CMICmdCmdTargetSelect::~CMICmdCmdTargetSelect() = default;
 
 //++
 //
@@ -93,51 +92,44 @@
 
   CMICmnLLDBDebugSessionInfo &rSessionInfo(
   CMICmnLLDBDebugSessionInfo::Instance());
+  lldb::SBTarget target = rSessionInfo.GetTarget();
 
-  // Check we have a valid target
-  // Note: target created via 'file-exec-and-symbols' command
-  if (!rSessionInfo.GetTarget().IsValid()) {
+  // Check we have a valid target.
+  // Note: target created via 'file-exec-and-symbols' command.
+  if (!target.IsValid()) {
 SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_TARGET_CURRENT),
m_cmdData.strMiCmd.c_str()));
 return MIstatus::failure;
   }
 
-  // Verify that we are executing remotely
+  // Verify that we are executing remotely.
   const CMIUtilString &rRemoteType(pArgType->GetValue());
   if (rRemoteType != "remote") {
 SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_TARGET_TYPE),
m_cmdData.strMiCmd.c_str(),
rRemoteType.c_str()));
 return MIstatus::failure;
   }
 
-  // Create a URL pointing to the remote gdb stub
+  // Create a URL pointing to the remote gdb stub.
   const CMIUtilString strUrl =
   CMIUtilString::Format("connect://%s", pArgParameters->GetValue().c_str());
 
-  // Ask LLDB to connect to the target port
-  const char *pPlugin("gdb-remote");
   lldb::SBError error;
-  lldb::SBProcess process = rSessionInfo.GetTarget().ConnectRemote(
+  // Ask LLDB to connect to the target port.
+  const char *pPlugin("gdb-remote");
+  lldb::SBProcess process = target.ConnectRemote(
   rSessionInfo.GetListener(), strUrl.c_str(), pPlugin, error);
 
-  // Verify that we have managed to connect successfully
-  lldb::SBStream errMsg;
-  error.GetDescription(errMsg);
+  // Verify that we have managed to connect successfully.
   if (!process.IsValid()) {
 SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_TARGET_PLUGIN),
m_cmdData.strMiCmd.c_str(),
-   errMsg.GetData()));
-return MIstatus::failure;
-  }
-  if (error.Fail()) {
-SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_CONNECT_TO_TARGET),
-   m_cmdData.strMiCmd.c_str(),
-   errMsg.GetData()));
+   error.GetCString()));
 return MIstatus::failure;
   }
 
-  // Set the environment path if we were given one
+  // Set the environment path if we were given one.
   CMIUtilString strWkDir;
   if (rSessionInfo.SharedDataRetrieve(
   rSessionInfo.m_constStrSharedDataKeyWkDir, strWkDir)) {
@@ -150,28 +142,13 @@
 }
   }
 
-  // Set the shared object path if we were given one
+  // Set the shared object path if we were given one.
   CMIUtilString strSolibPath;
   if (rSessionInfo.SharedDataRetrieve(
-  rSessionInfo.m_constStrSharedDataSolibPath, strSolibPath)) {
-lldb::SBDebugger &rDbgr = rSessionInfo.GetDebugger();
-lldb::SBCommandInterpreter cmdIterpreter = rDbgr.GetCommandInterpreter();
-
-CMIUtilString strCmdString = CMIUtilString::Format(
-"target modules search-paths add . %s", strSolibPath.c_str());
-
-lldb::SBCommandReturnObject retObj;
-cmdIterpreter.HandleCommand(strCmdString.c_str(), retObj, false);
+  rSessionInfo.m_constStrSharedDataSolibPath, strSolibPath))
+target.AppendImageSearchPath(".", strSolibPath.c_str(), error);
 
-if (!retObj.Succeeded()) {
-  SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_FNFAILED),
- m_cmdData.strMiCmd.c_str(),
- "target-select"));
-  return MIstatus::failur

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/API/SBTarget.cpp:1476-1477
+  } else
+error.SetErrorStringWithFormat(" can't be empty "
+   "(SBTarget::%s) failed", __FUNCTION__);
+}

check if csFrom or csTo is empty and give an error message that states which 
one is invalid?


https://reviews.llvm.org/D49739



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

[Lldb-commits] [PATCH] D49963: Preliminary patch to support prompt interpolation

[Zachary Turner via Phabricator via lldb-commits]

zturner added reviewers: labath, jasonmolenda.
zturner added inline comments.



Comment at: include/lldb/Host/Editline.h:187
+  /// Register a callback to retrieve the prompt.
+  void SetPromptCallback(PromptCallbackType callback, void *baton);
+  

I'd love to stop using the `baton` idiom if possible.  can you make this 
function take an `llvm::function_ref` instead?  Then, in 
the class, store a `std::function`.  When you call 
`SetPromptCallback`, write `SetPromptCallback([this](EditLine* L) { return  
this->PromptCallback(L); });`



Comment at: include/lldb/Interpreter/PromptInterpolation.h:27
+
+  std::string InterpolatePrompt(const std::string& prompt_format);
+private:

As it stands, the class doesn't really do much.  It's basically just one 
function, and the only purpose of the `m_debugger` member is to avoid having to 
pass the Debugger object as an argument to the function.  It seems this could 
just be turned into a free function.  And while we're at it, since that free 
function is only ever used inside of `IOHandler.cpp`, it can probably just be 
marked `static` and be a global function defined in that particular TU.



Comment at: source/Core/IOHandler.cpp:451
+
+const char* IOHandlerEditline::PromptCallback(Editline *editline, void *baton) 
{
+  IOHandlerEditline *editline_reader = (IOHandlerEditline *)baton;

This can return a `StringRef`


Repository:
  rL LLVM

https://reviews.llvm.org/D49963



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

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Greg Clayton via Phabricator via lldb-commits]

clayborg updated this revision to Diff 158321.
clayborg added a comment.
Herald added a subscriber: srhines.

- Fixed inline comments
- Moved platform setting into Target::SetArchitecture(...) instead of doing 
this manually in the core file code.
- Added ARM64 w0-w31, d0-d31, s0-s31 and h0-h31 registers
- Added ARM s0-s31 and q0-q15 registers
- Improved values in registers in arm and arm64 minidump files to ensure unique 
values in each register and updated tests to test for it


https://reviews.llvm.org/D49750

Files:
  include/lldb/Target/Target.h
  lldb.xcodeproj/project.pbxproj
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/arm-linux.dmp
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/arm-macos.dmp
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/arm64-macos.dmp
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
  source/Plugins/Process/minidump/RegisterContextMinidump_ARM.h
  source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
  source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h
  source/Plugins/Process/minidump/ThreadMinidump.cpp
  source/Target/Target.cpp

Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1426,13 +1426,33 @@
   }
 }
 
-bool Target::SetArchitecture(const ArchSpec &arch_spec) {
+bool Target::SetArchitecture(const ArchSpec &arch_spec, bool set_platform) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_TARGET));
   bool missing_local_arch = !m_arch.GetSpec().IsValid();
   bool replace_local_arch = true;
   bool compatible_local_arch = false;
   ArchSpec other(arch_spec);
 
+  // Changing the architecture might mean that the currently selected platform
+  // isn't compatible. Set the platform correctly if we are asked to do so,
+  // otherwise assume the user will set the platform manually.
+  if (set_platform) {
+if (other.IsValid()) {
+  auto platform_sp = GetPlatform();
+  if (!platform_sp ||
+  !platform_sp->IsCompatibleArchitecture(other, false, nullptr)) {
+ArchSpec platform_arch;
+auto arch_platform_sp = Platform::GetPlatformForArchitecture(
+other, &platform_arch);
+if (arch_platform_sp) {
+  SetPlatform(arch_platform_sp);
+  if (platform_arch.IsValid())
+other = platform_arch;
+}
+  }
+}
+  }
+  
   if (!missing_local_arch) {
 if (m_arch.GetSpec().IsCompatibleMatch(arch_spec)) {
   other.MergeFrom(m_arch.GetSpec());
Index: source/Plugins/Process/minidump/ThreadMinidump.cpp
===
--- source/Plugins/Process/minidump/ThreadMinidump.cpp
+++ source/Plugins/Process/minidump/ThreadMinidump.cpp
@@ -13,6 +13,8 @@
 
 #include "RegisterContextMinidump_x86_32.h"
 #include "RegisterContextMinidump_x86_64.h"
+#include "RegisterContextMinidump_ARM.h"
+#include "RegisterContextMinidump_ARM64.h"
 
 // Other libraries and framework includes
 #include "Plugins/Process/Utility/RegisterContextLinux_i386.h"
@@ -88,17 +90,24 @@
   *this, reg_interface, gpregset, {}));
   break;
 }
+case llvm::Triple::aarch64: {
+  DataExtractor data(m_gpregset_data.data(), m_gpregset_data.size(),
+ lldb::eByteOrderLittle, 8);
+  m_thread_reg_ctx_sp.reset(new RegisterContextMinidump_ARM64(*this, data));
+  break;
+}
+case llvm::Triple::arm: {
+  DataExtractor data(m_gpregset_data.data(), m_gpregset_data.size(),
+ lldb::eByteOrderLittle, 8);
+  const bool apple = arch.GetTriple().getVendor() == llvm::Triple::Apple;
+  m_thread_reg_ctx_sp.reset(new RegisterContextMinidump_ARM(*this, data,
+apple));
+  break;
+}
 default:
   break;
 }
 
-if (!reg_interface) {
-  if (log)
-log->Printf("elf-core::%s:: Architecture(%d) not supported",
-__FUNCTION__, arch.GetMachine());
-  assert(false && "Architecture not supported");
-}
-
 reg_ctx_sp = m_thread_reg_ctx_sp;
   } else if (m_unwinder_ap) {
 reg_ctx_sp = m_unwinder_ap->CreateRegisterContextForFrame(frame);
Index: source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h
===
--- source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h
+++ source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h
@@ -0,0 +1,86 @@
+//===-- RegisterContextMinidump_ARM64.h -*- C++ -*-===//
+//
+// 

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:192
+
+static size_t k_num_reg_infos = llvm::array_lengthof(g_reg_infos);
+

lemo wrote:
> constexpr?
will do



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:195
+// ARM general purpose registers.
+const uint32_t g_gpr_regnums[] = {
+  reg_r0,  reg_r1,  reg_r2,  reg_r3,  reg_r4, reg_r5, reg_r6, reg_r7,

lemo wrote:
> use std::array for these kind of static arrays? (debug bounds checks, easy 
> access to the static size, ...)
Tried it but it introduces a global constructor. We try to avoid those.



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp:56
+  k_num_regs
+};
+

I would rather define a new context and avoid mutating one register context 
into another. I didn't really like the other register contexts for minidumps. I 
like to show the actual data that is available, not a translation of one set of 
data to another.


https://reviews.llvm.org/D49750



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

[Lldb-commits] [PATCH] D50087: Add doxygen comments to the StackFrameList API (NFC)

[Vedant Kumar via Phabricator via lldb-commits]

vsk created this revision.
vsk added reviewers: labath, jasonmolenda, tberghammer.

Clarify how StackFrameList works by documenting its methods. Also,
delete some dead code and insert some TODOs.


https://reviews.llvm.org/D50087

Files:
  lldb/include/lldb/Target/StackFrameList.h
  lldb/source/Target/StackFrameList.cpp

Index: lldb/source/Target/StackFrameList.cpp
===
--- lldb/source/Target/StackFrameList.cpp
+++ lldb/source/Target/StackFrameList.cpp
@@ -436,11 +436,7 @@
   if (can_create)
 GetFramesUpTo(UINT32_MAX);
 
-  uint32_t inlined_depth = GetCurrentInlinedDepth();
-  if (inlined_depth == UINT32_MAX)
-return m_frames.size();
-  else
-return m_frames.size() - inlined_depth;
+  return GetVisibleStackFrameIndex(m_frames.size());
 }
 
 void StackFrameList::Dump(Stream *s) {
@@ -620,7 +616,6 @@
   return m_selected_frame_idx;
 }
 
-// Mark a stack frame as the current frame using the frame index
 bool StackFrameList::SetSelectedFrameByIndex(uint32_t idx) {
   std::lock_guard guard(m_mutex);
   StackFrameSP frame_sp(GetFrameAtIndex(idx));
@@ -652,19 +647,6 @@
   m_concrete_frames_fetched = 0;
 }
 
-void StackFrameList::InvalidateFrames(uint32_t start_idx) {
-  std::lock_guard guard(m_mutex);
-  if (m_show_inlined_frames) {
-Clear();
-  } else {
-const size_t num_frames = m_frames.size();
-while (start_idx < num_frames) {
-  m_frames[start_idx].reset();
-  ++start_idx;
-}
-  }
-}
-
 void StackFrameList::Merge(std::unique_ptr &curr_ap,
lldb::StackFrameListSP &prev_sp) {
   std::unique_lock current_lock, previous_lock;
Index: lldb/include/lldb/Target/StackFrameList.h
===
--- lldb/include/lldb/Target/StackFrameList.h
+++ lldb/include/lldb/Target/StackFrameList.h
@@ -10,14 +10,10 @@
 #ifndef liblldb_StackFrameList_h_
 #define liblldb_StackFrameList_h_
 
-// C Includes
-// C++ Includes
 #include 
 #include 
 #include 
 
-// Other libraries and framework includes
-// Project includes
 #include "lldb/Target/StackFrame.h"
 
 namespace lldb_private {
@@ -32,39 +28,57 @@
 
   ~StackFrameList();
 
+  /// Get the number of visible frames. Frames may be created if \p can_create
+  /// is true. Synthetic (inline) frames expanded from the concrete frame #0
+  /// (aka invisible frames) are not included in this count.
   uint32_t GetNumFrames(bool can_create = true);
 
+  /// Get the frame at index \p idx. Invisible frames cannot be indexed.
   lldb::StackFrameSP GetFrameAtIndex(uint32_t idx);
 
+  /// Get the first concrete frame with index greater than or equal to \p idx.
+  /// Unlike \ref GetFrameAtIndex, this cannot return a synthetic frame.
   lldb::StackFrameSP GetFrameWithConcreteFrameIndex(uint32_t unwind_idx);
 
+  /// Retrieve the stack frame with the given ID \p stack_id.
   lldb::StackFrameSP GetFrameWithStackID(const StackID &stack_id);
 
-  // Mark a stack frame as the current frame
+  /// Mark a stack frame as the currently selected frame and return its index.
   uint32_t SetSelectedFrame(lldb_private::StackFrame *frame);
 
+  /// Get the currently selected frame index.
   uint32_t GetSelectedFrameIndex() const;
 
-  // Mark a stack frame as the current frame using the frame index
+  /// Mark a stack frame as the currently selected frame using the frame index
+  /// \p idx. Like \ref GetFrameAtIndex, invisible frames cannot be selected.
   bool SetSelectedFrameByIndex(uint32_t idx);
 
+  /// If the current inline depth is valid, subtract it from \p idx. Otherwise
+  /// simply return \p idx.
   uint32_t GetVisibleStackFrameIndex(uint32_t idx) {
 if (m_current_inlined_depth < UINT32_MAX)
   return idx - m_current_inlined_depth;
 else
   return idx;
   }
 
+  /// Calculate and set the current inline depth. This may be used to update
+  /// the StackFrameList's set of inline frames when execution stops, e.g when
+  /// a breakpoint is hit.
   void CalculateCurrentInlinedDepth();
 
+  /// If the currently selected frame comes from the currently selected thread,
+  /// point the default file and line of the thread's target to the location
+  /// specified by the frame.
   void SetDefaultFileAndLineToSelectedFrame();
 
+  /// Clear the cache of frames.
   void Clear();
 
-  void InvalidateFrames(uint32_t start_idx);
-
   void Dump(Stream *s);
 
+  /// If \p stack_frame_ptr is contained in this StackFrameList, return its
+  /// wrapping shared pointer.
   lldb::StackFrameSP
   GetStackFrameSPForStackFramePtr(StackFrame *stack_frame_ptr);
 
@@ -101,14 +115,44 @@
   typedef collection::iterator iterator;
   typedef collection::const_iterator const_iterator;
 
+  /// The thread this frame list describes.
   Thread &m_thread;
+
+  /// The old stack frame list.
+  // TODO: The old stack frame list is used to fill in missing frame info
+  // heuristically when it's otherwise unavailable (say, because the unwinder

[Lldb-commits] [PATCH] D50038: Introduce install-lldb-framework target

[Alex Langford via Phabricator via lldb-commits]

xiaobai updated this revision to Diff 158329.
xiaobai added a comment.

Address comments


https://reviews.llvm.org/D50038

Files:
  CMakeLists.txt
  cmake/modules/AddLLDB.cmake
  cmake/modules/LLDBFramework.cmake
  source/API/CMakeLists.txt

Index: source/API/CMakeLists.txt
===
--- source/API/CMakeLists.txt
+++ source/API/CMakeLists.txt
@@ -143,6 +143,17 @@
   )
 endif()
 
+if (LLDB_BUILD_FRAMEWORK)
+  set_target_properties(liblldb
+PROPERTIES
+OUTPUT_NAME LLDB
+FRAMEWORK On
+FRAMEWORK_VERSION ${LLDB_FRAMEWORK_VERSION}
+MACOSX_FRAMEWORK_INFO_PLIST ${LLDB_SOURCE_DIR}/resources/LLDB-Info.plist
+LIBRARY_OUTPUT_DIRECTORY ${LLDB_FRAMEWORK_DIR}
+  )
+endif()
+
 if (LLDB_WRAP_PYTHON)
   add_dependencies(liblldb swig_wrapper)
 endif()
Index: cmake/modules/LLDBFramework.cmake
===
--- cmake/modules/LLDBFramework.cmake
+++ cmake/modules/LLDBFramework.cmake
@@ -31,14 +31,9 @@
   )
 endif()
 
-set_target_properties(liblldb PROPERTIES
-  OUTPUT_NAME LLDB
-  FRAMEWORK On
-  FRAMEWORK_VERSION ${LLDB_FRAMEWORK_VERSION}
-  MACOSX_FRAMEWORK_INFO_PLIST ${LLDB_SOURCE_DIR}/resources/LLDB-Info.plist
-  LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/${LLDB_FRAMEWORK_INSTALL_DIR}
-  PUBLIC_HEADER "${framework_headers}")
-
 add_dependencies(lldb-framework
   lldb-framework-headers
   lldb-suite)
+
+add_custom_target(install-lldb-framework)
+add_custom_target(install-lldb-framework-stripped)
Index: cmake/modules/AddLLDB.cmake
===
--- cmake/modules/AddLLDB.cmake
+++ cmake/modules/AddLLDB.cmake
@@ -53,6 +53,11 @@
 set(out_dir lib${LLVM_LIBDIR_SUFFIX})
 if(${name} STREQUAL "liblldb" AND LLDB_BUILD_FRAMEWORK)
   set(out_dir ${LLDB_FRAMEWORK_INSTALL_DIR})
+  # The framework that is generated will install with install-liblldb
+  # because we enable CMake's framework support. CMake will copy all the
+  # headers and resources for us.
+  add_dependencies(install-lldb-framework install-${name})
+  add_dependencies(install-lldb-framework-stripped install-${name}-stripped)
 endif()
 install(TARGETS ${name}
   COMPONENT ${name}
@@ -67,12 +72,20 @@
   endif()
   if (NOT CMAKE_CONFIGURATION_TYPES)
 add_llvm_install_targets(install-${name}
- DEPENDS ${name}
+ DEPENDS $
  COMPONENT ${name})
+
+# install-liblldb{,-stripped} is the actual target that will install the
+# framework, so it must rely on the framework being fully built first.
+if (LLDB_BUILD_FRAMEWORK AND ${name} STREQUAL "liblldb")
+  add_dependencies(install-${name} lldb-framework)
+  add_dependencies(install-lldb-framework-stripped lldb-framework)
+endif()
   endif()
 endif()
   endif()
 
+
   # Hack: only some LLDB libraries depend on the clang autogenerated headers,
   # but it is simple enough to make all of LLDB depend on some of those
   # headers without negatively impacting much of anything.
@@ -124,6 +137,10 @@
 set(out_dir "bin")
 if (LLDB_BUILD_FRAMEWORK AND ARG_INCLUDE_IN_SUITE)
   set(out_dir ${LLDB_FRAMEWORK_INSTALL_DIR}/${LLDB_FRAMEWORK_RESOURCE_DIR})
+  # While install-liblldb-stripped will handle copying the tools, it will
+  # not strip them. We depend on this target to guarantee a stripped version
+  # will get installed in the framework.
+  add_dependencies(install-lldb-framework-stripped install-${name}-stripped)
 endif()
 install(TARGETS ${name}
   COMPONENT ${name}
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -51,6 +51,7 @@
 message(FATAL_ERROR "LLDB.framework can only be generated when targeting Apple platforms")
   endif()
 
+  add_custom_target(lldb-framework)
   # These are used to fill out LLDB-Info.plist. These are relevant when building
   # the framework, and must be defined before building liblldb.
   set(PRODUCT_NAME "LLDB")
@@ -60,6 +61,7 @@
 
   set(LLDB_FRAMEWORK_DIR
 ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/${LLDB_FRAMEWORK_INSTALL_DIR})
+  include(LLDBFramework)
 endif()
 
 add_subdirectory(docs)
@@ -162,10 +164,6 @@
   add_subdirectory(utils/lldb-dotest)
 endif()
 
-if (LLDB_BUILD_FRAMEWORK)
-  add_custom_target(lldb-framework)
-  include(LLDBFramework)
-endif()
 
 if (NOT LLDB_DISABLE_PYTHON)
 # Add a Post-Build Event to copy over Python files and create the symlink
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Alexander Polyakov via Phabricator via lldb-commits]

apolyakov updated this revision to Diff 158330.
apolyakov added a comment.

Made error handling more meaningful: added different error messages for cases - 
empty  path and empty  path.


https://reviews.llvm.org/D49739

Files:
  include/lldb/API/SBTarget.h
  lit/lit.cfg
  lit/tools/lldb-mi/target/inputs/main.c
  lit/tools/lldb-mi/target/inputs/target-select-so-path.py
  lit/tools/lldb-mi/target/lit.local.cfg
  lit/tools/lldb-mi/target/target-select-so-path.test
  scripts/interface/SBTarget.i
  source/API/SBTarget.cpp
  tools/lldb-mi/MICmdCmdTarget.cpp

Index: tools/lldb-mi/MICmdCmdTarget.cpp
===
--- tools/lldb-mi/MICmdCmdTarget.cpp
+++ tools/lldb-mi/MICmdCmdTarget.cpp
@@ -10,9 +10,8 @@
 // Overview:CMICmdCmdTargetSelect   implementation.
 
 // Third Party Headers:
-#include "lldb/API/SBCommandInterpreter.h"
-#include "lldb/API/SBCommandReturnObject.h"
 #include "lldb/API/SBStream.h"
+#include "lldb/API/SBError.h"
 
 // In-house headers:
 #include "MICmdArgValNumber.h"
@@ -52,7 +51,7 @@
 // Return:  None.
 // Throws:  None.
 //--
-CMICmdCmdTargetSelect::~CMICmdCmdTargetSelect() {}
+CMICmdCmdTargetSelect::~CMICmdCmdTargetSelect() = default;
 
 //++
 //
@@ -93,51 +92,44 @@
 
   CMICmnLLDBDebugSessionInfo &rSessionInfo(
   CMICmnLLDBDebugSessionInfo::Instance());
+  lldb::SBTarget target = rSessionInfo.GetTarget();
 
-  // Check we have a valid target
-  // Note: target created via 'file-exec-and-symbols' command
-  if (!rSessionInfo.GetTarget().IsValid()) {
+  // Check we have a valid target.
+  // Note: target created via 'file-exec-and-symbols' command.
+  if (!target.IsValid()) {
 SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_TARGET_CURRENT),
m_cmdData.strMiCmd.c_str()));
 return MIstatus::failure;
   }
 
-  // Verify that we are executing remotely
+  // Verify that we are executing remotely.
   const CMIUtilString &rRemoteType(pArgType->GetValue());
   if (rRemoteType != "remote") {
 SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_TARGET_TYPE),
m_cmdData.strMiCmd.c_str(),
rRemoteType.c_str()));
 return MIstatus::failure;
   }
 
-  // Create a URL pointing to the remote gdb stub
+  // Create a URL pointing to the remote gdb stub.
   const CMIUtilString strUrl =
   CMIUtilString::Format("connect://%s", pArgParameters->GetValue().c_str());
 
-  // Ask LLDB to connect to the target port
-  const char *pPlugin("gdb-remote");
   lldb::SBError error;
-  lldb::SBProcess process = rSessionInfo.GetTarget().ConnectRemote(
+  // Ask LLDB to connect to the target port.
+  const char *pPlugin("gdb-remote");
+  lldb::SBProcess process = target.ConnectRemote(
   rSessionInfo.GetListener(), strUrl.c_str(), pPlugin, error);
 
-  // Verify that we have managed to connect successfully
-  lldb::SBStream errMsg;
-  error.GetDescription(errMsg);
+  // Verify that we have managed to connect successfully.
   if (!process.IsValid()) {
 SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_TARGET_PLUGIN),
m_cmdData.strMiCmd.c_str(),
-   errMsg.GetData()));
-return MIstatus::failure;
-  }
-  if (error.Fail()) {
-SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_CONNECT_TO_TARGET),
-   m_cmdData.strMiCmd.c_str(),
-   errMsg.GetData()));
+   error.GetCString()));
 return MIstatus::failure;
   }
 
-  // Set the environment path if we were given one
+  // Set the environment path if we were given one.
   CMIUtilString strWkDir;
   if (rSessionInfo.SharedDataRetrieve(
   rSessionInfo.m_constStrSharedDataKeyWkDir, strWkDir)) {
@@ -150,28 +142,13 @@
 }
   }
 
-  // Set the shared object path if we were given one
+  // Set the shared object path if we were given one.
   CMIUtilString strSolibPath;
   if (rSessionInfo.SharedDataRetrieve(
-  rSessionInfo.m_constStrSharedDataSolibPath, strSolibPath)) {
-lldb::SBDebugger &rDbgr = rSessionInfo.GetDebugger();
-lldb::SBCommandInterpreter cmdIterpreter = rDbgr.GetCommandInterpreter();
-
-CMIUtilString strCmdString = CMIUtilString::Format(
-"target modules search-paths add . %s", strSolibPath.c_str());
-
-lldb::SBCommandReturnObject retObj;
-cmdIterpreter.HandleCommand(strCmdString.c_str(), retObj, false);
+  rSessionInfo.m_constStrSharedDataSolibPath, strSolibPath))
+target.AppendImageSearchPath(".", strSolibPath.c_str(), error);
 
-if (!retObj.Succeeded()) {
-  SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_FNFAILED),
- m_cmdData.strMiCmd.c_str(),
- "target-select

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik updated this revision to Diff 158342.
shafik marked 2 inline comments as done.
shafik added a comment.

- Adding comments
- Changing from exit to self.skipTest
- Adding skip for gcc


https://reviews.llvm.org/D49271

Files:
  lldb.xcodeproj/project.pbxproj
  
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/Makefile
  
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
  
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/main.cpp
  source/Plugins/Language/CPlusPlus/CMakeLists.txt
  source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  source/Plugins/Language/CPlusPlus/LibCxx.cpp
  source/Plugins/Language/CPlusPlus/LibCxx.h
  source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp

Index: source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp
===
--- /dev/null
+++ source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp
@@ -0,0 +1,85 @@
+//===-- LibCxxOptional.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "LibCxx.h"
+#include "lldb/DataFormatters/FormattersHelpers.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+namespace {
+
+class OptionalFrontEnd : public SyntheticChildrenFrontEnd {
+public:
+  OptionalFrontEnd(ValueObject &valobj) : SyntheticChildrenFrontEnd(valobj) {
+Update();
+  }
+
+  size_t GetIndexOfChildWithName(const ConstString &name) override {
+return formatters::ExtractIndexFromString(name.GetCString());
+  }
+
+  bool MightHaveChildren() override { return true; }
+  bool Update() override;
+  size_t CalculateNumChildren() override { return m_size; }
+  ValueObjectSP GetChildAtIndex(size_t idx) override;
+
+private:
+  size_t m_size = 0;
+  ValueObjectSP m_base_sp;
+};
+} // namespace
+
+bool OptionalFrontEnd::Update() {
+  ValueObjectSP engaged_sp(
+  m_backend.GetChildMemberWithName(ConstString("__engaged_"), true));
+
+  if (!engaged_sp)
+return false;
+
+  // __engaged_ is a bool flag and is true if the optional contains a value.
+  // Converting it to unsigned gives us a size of 1 if it contains a value
+  // and 0 if not.
+  m_size = engaged_sp->GetValueAsUnsigned(0);
+
+  return false;
+}
+
+ValueObjectSP OptionalFrontEnd::GetChildAtIndex(size_t idx) {
+  if (idx >= m_size)
+return ValueObjectSP();
+
+  // __val_ contains the underlying value of an optional if it has one.
+  // Currently because it is part of an anonymous union GetChildMemberWithName()
+  // does not peer through and find it unless we are at the parent itself.
+  // We can obtain the parent through __engaged_.
+  ValueObjectSP val_sp(
+  m_backend.GetChildMemberWithName(ConstString("__engaged_"), true)
+  ->GetParent()
+  ->GetChildAtIndex(0, true)
+  ->GetChildMemberWithName(ConstString("__val_"), true));
+
+  if (!val_sp)
+return ValueObjectSP();
+
+  CompilerType holder_type = val_sp->GetCompilerType();
+
+  if (!holder_type)
+return ValueObjectSP();
+
+  return val_sp->Clone(ConstString(llvm::formatv("Value").str()));
+}
+
+SyntheticChildrenFrontEnd *
+formatters::LibcxxOptionalFrontEndCreator(CXXSyntheticChildren *,
+  lldb::ValueObjectSP valobj_sp) {
+  if (valobj_sp)
+return new OptionalFrontEnd(*valobj_sp);
+  return nullptr;
+}
Index: source/Plugins/Language/CPlusPlus/LibCxx.h
===
--- source/Plugins/Language/CPlusPlus/LibCxx.h
+++ source/Plugins/Language/CPlusPlus/LibCxx.h
@@ -27,6 +27,10 @@
 ValueObject &valobj, Stream &stream,
 const TypeSummaryOptions &options); // libc++ std::wstring
 
+bool LibcxxOptionalSummaryProvider(
+ValueObject &valobj, Stream &stream,
+const TypeSummaryOptions &options); // libc++ std::optional<>
+
 bool LibcxxSmartPointerSummaryProvider(
 ValueObject &valobj, Stream &stream,
 const TypeSummaryOptions
@@ -133,6 +137,10 @@
 SyntheticChildrenFrontEnd *LibcxxTupleFrontEndCreator(CXXSyntheticChildren *,
   lldb::ValueObjectSP);
 
+SyntheticChildrenFrontEnd *
+LibcxxOptionalFrontEndCreator(CXXSyntheticChildren *,
+  lldb::ValueObjectSP valobj_sp);
+
 } // namespace formatters
 } // namespace lldb_private
 
Index: source/Plugins/Language/CPlusPlus/LibCxx.cpp
===
--- source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -33,6 +33,28 @@
 using namespace lldb_private;
 using namespace l

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.



Comment at: 
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:21
+@add_test_categories(["libc++"])
+@skipIf(oslist=no_match(["macosx"]), compiler="clang", 
compiler_version=['<', '5.0'])
+

labath wrote:
> Could you add another line for `gcc` here? The -std=c++17 flag seems to be 
> supported starting with gcc-5.1.
> 
> Also a comment that this is due to the -std flag would be helpful to people 
> looking at this in the future.
Adding a comment makes sense.

So `@add_test_categories(["libc++"])` won't skip for gcc bots then?


https://reviews.llvm.org/D49271



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

[Lldb-commits] [PATCH] D49963: Preliminary patch to support prompt interpolation

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

This will be very cool. Biggest issues to watch out for is to make sure it 
doesn't return incorrect values when running for things like the thread count. 
If you switch to use the 
"m_debugger.GetCommandInterpreter().GetExecutionContext()" it should solve this 
by making sure the frame and thread are not filled in when the process is 
running.  It might also be racy. For example if you say "continue", hopefully 
the process will be resumed by the time the prompt is asked to refresh itself. 
Since we get events asynchronously this might be a problem.




Comment at: include/lldb/Core/FormatEntity.h:86
   FrameIndex,
+  FrameIndex1Based,
+  FrameCount,

Rename to FrameIndexID. It is similar to the thread index ID which is one based.



Comment at: include/lldb/Core/IOHandler.h:456
+private:
+  std::string m_prompt_string;
 };

If this will only contain a prompt that is a format string, you should store 
the compiled format so we don't have to parse it each time the prompt is 
displayed. Maybe this should be:

```
  FormatEntity::Entry m_prompt_format;
```




Comment at: include/lldb/Host/Editline.h:93
 typedef std::shared_ptr EditlineHistorySP;
-
+  
 typedef bool (*IsInputCompleteCallbackType)(Editline *editline,

Remove whitespace



Comment at: source/Core/FormatEntity.cpp:125
 ENTRY("index", FrameIndex, UInt32),
+ENTRY("index1based", FrameIndex1Based, UInt32),
+ENTRY("count", FrameCount, UInt32),

rename to "index_id" and fix enum to FrameIndexID



Comment at: source/Core/FormatEntity.cpp:357
 ENUM_TO_CSTR(FrameIndex);
+ENUM_TO_CSTR(FrameIndex1Based);
+ENUM_TO_CSTR(FrameCount);

FrameIndexID



Comment at: source/Core/FormatEntity.cpp:1443
 
+  case Entry::Type::FrameIndex1Based:
+if (exe_ctx) {

FrameIndexID



Comment at: source/Interpreter/PromptInterpolation.cpp:1
+//===-- PromptInterpolation.cpp *- C++ 
-*-===//
+//

Not sure this file is needed?



Comment at: source/Interpreter/PromptInterpolation.cpp:33-46
+  TargetSP target = m_debugger.GetSelectedTarget();
+
+  if (!target) {
+interpolated_prompt << kFallbackPrompt;
+return interpolated_prompt.str();
+  }
+

Remove all of this and just call:

```
  ExecutionContext exe_ctx = 
m_debugger.GetCommandInterpreter().GetExecutionContext();
```

It will do the right thing and leave the thread and frame out of the context if 
we are running. There is no need to fallback to using kFallbackPrompt. You can 
make your prompt hardened against values not being available. Lets say the 
thread count isn't available: you can just put extra {} around the string that 
contains the thread count like:

```
"{num threads = ${thread.count}}"
```
If any variable inside the extra scope is not available, it will be left out. 
So there should be no need to use a fallback if the prompt is made correctly. 





Comment at: source/Interpreter/PromptInterpolation.cpp:50
+  FormatEntity::Entry root;
+  Status parse_status = FormatEntity::Parse(prompt_format, root);
+  bool formatted = false;

This should be parsed once and kept around. Not parsed every time the prompt is 
displayed. 


Repository:
  rL LLVM

https://reviews.llvm.org/D49963



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

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added a comment.

Thanks Greg, looks good to me (a couple of inline comments left at your 
discretion)




Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:15
 // Other libraries and framework includes
+//#include "lldb/Core/Architecture.h"
 #include "lldb/Core/Module.h"

it this set for removal?



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:195
+// ARM general purpose registers.
+const uint32_t g_gpr_regnums[] = {
+  reg_r0,  reg_r1,  reg_r2,  reg_r3,  reg_r4, reg_r5, reg_r6, reg_r7,

clayborg wrote:
> lemo wrote:
> > use std::array for these kind of static arrays? (debug bounds checks, easy 
> > access to the static size, ...)
> Tried it but it introduces a global constructor. We try to avoid those.
We shouldn't have a dynamic initializer: that's strange, if that's the case we 
have a compiler bug on our hands. A quick experiment indicates that even with 
-O0 recent clang/llvm do the right thing: https://godbolt.org/g/NMUFLP

Is the problem only with arrays of RegisterInfo structs? If that's the case the 
cause is RegisterInfo itself and std::array should not make a difference (ie. 
we'd see the dynamic initializer even with plain C arrays)


https://reviews.llvm.org/D49750



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

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: source/API/SBTarget.cpp:1467
+  }
+  const ConstString csFrom(from), csTo(to);
+  if (csFrom && csTo) {

personally I would write this as:
```
if (!csFrom)
  return error.SetErrorString(" path is empty");
if (!csTo)
  return error.SetErrorString(" path is empty");
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
if (log)
  log->Printf("SBTarget(%p)::%s: '%s' -> '%s'",
  static_cast(target_sp.get()),  __FUNCTION__,
  from, to);
target_sp->GetImageSearchPathList().Append(csFrom, csTo, true);
```


https://reviews.llvm.org/D49739



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

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Alexander Polyakov via Phabricator via lldb-commits]

apolyakov added inline comments.



Comment at: source/API/SBTarget.cpp:1467
+  }
+  const ConstString csFrom(from), csTo(to);
+  if (csFrom && csTo) {

aprantl wrote:
> personally I would write this as:
> ```
> if (!csFrom)
>   return error.SetErrorString(" path is empty");
> if (!csTo)
>   return error.SetErrorString(" path is empty");
> Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
> if (log)
>   log->Printf("SBTarget(%p)::%s: '%s' -> '%s'",
>   static_cast(target_sp.get()),  __FUNCTION__,
>   from, to);
> target_sp->GetImageSearchPathList().Append(csFrom, csTo, true);
> ```
in my opinion, the branch `if (csFrom && csTo)` will be executed in most cases, 
so it is reasonable to make it executed first, as this will benefit the branch 
prediction of the processor. Is this a worthwhile argument?


https://reviews.llvm.org/D49739



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

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik updated this revision to Diff 158362.
shafik added a comment.

Fixing gcc skipIf to check for version as well


https://reviews.llvm.org/D49271

Files:
  lldb.xcodeproj/project.pbxproj
  
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/Makefile
  
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py
  
packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/main.cpp
  source/Plugins/Language/CPlusPlus/CMakeLists.txt
  source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  source/Plugins/Language/CPlusPlus/LibCxx.cpp
  source/Plugins/Language/CPlusPlus/LibCxx.h
  source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp

Index: source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp
===
--- /dev/null
+++ source/Plugins/Language/CPlusPlus/LibCxxOptional.cpp
@@ -0,0 +1,85 @@
+//===-- LibCxxOptional.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "LibCxx.h"
+#include "lldb/DataFormatters/FormattersHelpers.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+namespace {
+
+class OptionalFrontEnd : public SyntheticChildrenFrontEnd {
+public:
+  OptionalFrontEnd(ValueObject &valobj) : SyntheticChildrenFrontEnd(valobj) {
+Update();
+  }
+
+  size_t GetIndexOfChildWithName(const ConstString &name) override {
+return formatters::ExtractIndexFromString(name.GetCString());
+  }
+
+  bool MightHaveChildren() override { return true; }
+  bool Update() override;
+  size_t CalculateNumChildren() override { return m_size; }
+  ValueObjectSP GetChildAtIndex(size_t idx) override;
+
+private:
+  size_t m_size = 0;
+  ValueObjectSP m_base_sp;
+};
+} // namespace
+
+bool OptionalFrontEnd::Update() {
+  ValueObjectSP engaged_sp(
+  m_backend.GetChildMemberWithName(ConstString("__engaged_"), true));
+
+  if (!engaged_sp)
+return false;
+
+  // __engaged_ is a bool flag and is true if the optional contains a value.
+  // Converting it to unsigned gives us a size of 1 if it contains a value
+  // and 0 if not.
+  m_size = engaged_sp->GetValueAsUnsigned(0);
+
+  return false;
+}
+
+ValueObjectSP OptionalFrontEnd::GetChildAtIndex(size_t idx) {
+  if (idx >= m_size)
+return ValueObjectSP();
+
+  // __val_ contains the underlying value of an optional if it has one.
+  // Currently because it is part of an anonymous union GetChildMemberWithName()
+  // does not peer through and find it unless we are at the parent itself.
+  // We can obtain the parent through __engaged_.
+  ValueObjectSP val_sp(
+  m_backend.GetChildMemberWithName(ConstString("__engaged_"), true)
+  ->GetParent()
+  ->GetChildAtIndex(0, true)
+  ->GetChildMemberWithName(ConstString("__val_"), true));
+
+  if (!val_sp)
+return ValueObjectSP();
+
+  CompilerType holder_type = val_sp->GetCompilerType();
+
+  if (!holder_type)
+return ValueObjectSP();
+
+  return val_sp->Clone(ConstString(llvm::formatv("Value").str()));
+}
+
+SyntheticChildrenFrontEnd *
+formatters::LibcxxOptionalFrontEndCreator(CXXSyntheticChildren *,
+  lldb::ValueObjectSP valobj_sp) {
+  if (valobj_sp)
+return new OptionalFrontEnd(*valobj_sp);
+  return nullptr;
+}
Index: source/Plugins/Language/CPlusPlus/LibCxx.h
===
--- source/Plugins/Language/CPlusPlus/LibCxx.h
+++ source/Plugins/Language/CPlusPlus/LibCxx.h
@@ -27,6 +27,10 @@
 ValueObject &valobj, Stream &stream,
 const TypeSummaryOptions &options); // libc++ std::wstring
 
+bool LibcxxOptionalSummaryProvider(
+ValueObject &valobj, Stream &stream,
+const TypeSummaryOptions &options); // libc++ std::optional<>
+
 bool LibcxxSmartPointerSummaryProvider(
 ValueObject &valobj, Stream &stream,
 const TypeSummaryOptions
@@ -133,6 +137,10 @@
 SyntheticChildrenFrontEnd *LibcxxTupleFrontEndCreator(CXXSyntheticChildren *,
   lldb::ValueObjectSP);
 
+SyntheticChildrenFrontEnd *
+LibcxxOptionalFrontEndCreator(CXXSyntheticChildren *,
+  lldb::ValueObjectSP valobj_sp);
+
 } // namespace formatters
 } // namespace lldb_private
 
Index: source/Plugins/Language/CPlusPlus/LibCxx.cpp
===
--- source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -33,6 +33,28 @@
 using namespace lldb_private;
 using namespace lldb_private::formatters;
 
+bool lldb_private::formatters::LibcxxOptiona

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: source/API/SBTarget.cpp:1467
+  }
+  const ConstString csFrom(from), csTo(to);
+  if (csFrom && csTo) {

apolyakov wrote:
> aprantl wrote:
> > personally I would write this as:
> > ```
> > if (!csFrom)
> >   return error.SetErrorString(" path is empty");
> > if (!csTo)
> >   return error.SetErrorString(" path is empty");
> > Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
> > if (log)
> >   log->Printf("SBTarget(%p)::%s: '%s' -> '%s'",
> >   static_cast(target_sp.get()),  __FUNCTION__,
> >   from, to);
> > target_sp->GetImageSearchPathList().Append(csFrom, csTo, true);
> > ```
> in my opinion, the branch `if (csFrom && csTo)` will be executed in most 
> cases, so it is reasonable to make it executed first, as this will benefit 
> the branch prediction of the processor. Is this a worthwhile argument?
Not really. First, you don't actually control the order in which the code is 
emitted by the compiler by rearranging the source code. Second, it doesn't make 
sense to think about branch prediction outside of a tight loop as the effect 
would not even be measurable. We should optimize for readability and compact 
code here.


https://reviews.llvm.org/D49739



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

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Alexander Polyakov via Phabricator via lldb-commits]

apolyakov added inline comments.



Comment at: source/API/SBTarget.cpp:1467
+  }
+  const ConstString csFrom(from), csTo(to);
+  if (csFrom && csTo) {

aprantl wrote:
> apolyakov wrote:
> > aprantl wrote:
> > > personally I would write this as:
> > > ```
> > > if (!csFrom)
> > >   return error.SetErrorString(" path is empty");
> > > if (!csTo)
> > >   return error.SetErrorString(" path is empty");
> > > Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
> > > if (log)
> > >   log->Printf("SBTarget(%p)::%s: '%s' -> '%s'",
> > >   static_cast(target_sp.get()),  __FUNCTION__,
> > >   from, to);
> > > target_sp->GetImageSearchPathList().Append(csFrom, csTo, true);
> > > ```
> > in my opinion, the branch `if (csFrom && csTo)` will be executed in most 
> > cases, so it is reasonable to make it executed first, as this will benefit 
> > the branch prediction of the processor. Is this a worthwhile argument?
> Not really. First, you don't actually control the order in which the code is 
> emitted by the compiler by rearranging the source code. Second, it doesn't 
> make sense to think about branch prediction outside of a tight loop as the 
> effect would not even be measurable. We should optimize for readability and 
> compact code here.
Yes, I should agree that thinking about branch prediction isn't so effective in 
our case. But the readability of our approaches is almost equal. At the same 
time, taking into consideration that in most cases we will have not empty 
string arguments, your approach will check 2 conditions and mine approach will 
check 1 condition.


https://reviews.llvm.org/D49739



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

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Greg Clayton via Phabricator via lldb-commits]

clayborg updated this revision to Diff 158371.
clayborg added a comment.

Removed unnecessary Xcode project changes and removed #include that wasn't 
needed.


https://reviews.llvm.org/D49750

Files:
  include/lldb/Target/Target.h
  lldb.xcodeproj/project.pbxproj
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/arm-linux.dmp
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/arm-macos.dmp
  
packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/arm64-macos.dmp
  source/Plugins/Process/minidump/MinidumpParser.cpp
  source/Plugins/Process/minidump/MinidumpParser.h
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
  source/Plugins/Process/minidump/RegisterContextMinidump_ARM.h
  source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
  source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h
  source/Plugins/Process/minidump/ThreadMinidump.cpp
  source/Target/Target.cpp

Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1426,13 +1426,33 @@
   }
 }
 
-bool Target::SetArchitecture(const ArchSpec &arch_spec) {
+bool Target::SetArchitecture(const ArchSpec &arch_spec, bool set_platform) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_TARGET));
   bool missing_local_arch = !m_arch.GetSpec().IsValid();
   bool replace_local_arch = true;
   bool compatible_local_arch = false;
   ArchSpec other(arch_spec);
 
+  // Changing the architecture might mean that the currently selected platform
+  // isn't compatible. Set the platform correctly if we are asked to do so,
+  // otherwise assume the user will set the platform manually.
+  if (set_platform) {
+if (other.IsValid()) {
+  auto platform_sp = GetPlatform();
+  if (!platform_sp ||
+  !platform_sp->IsCompatibleArchitecture(other, false, nullptr)) {
+ArchSpec platform_arch;
+auto arch_platform_sp = Platform::GetPlatformForArchitecture(
+other, &platform_arch);
+if (arch_platform_sp) {
+  SetPlatform(arch_platform_sp);
+  if (platform_arch.IsValid())
+other = platform_arch;
+}
+  }
+}
+  }
+  
   if (!missing_local_arch) {
 if (m_arch.GetSpec().IsCompatibleMatch(arch_spec)) {
   other.MergeFrom(m_arch.GetSpec());
Index: source/Plugins/Process/minidump/ThreadMinidump.cpp
===
--- source/Plugins/Process/minidump/ThreadMinidump.cpp
+++ source/Plugins/Process/minidump/ThreadMinidump.cpp
@@ -13,6 +13,8 @@
 
 #include "RegisterContextMinidump_x86_32.h"
 #include "RegisterContextMinidump_x86_64.h"
+#include "RegisterContextMinidump_ARM.h"
+#include "RegisterContextMinidump_ARM64.h"
 
 // Other libraries and framework includes
 #include "Plugins/Process/Utility/RegisterContextLinux_i386.h"
@@ -88,17 +90,24 @@
   *this, reg_interface, gpregset, {}));
   break;
 }
+case llvm::Triple::aarch64: {
+  DataExtractor data(m_gpregset_data.data(), m_gpregset_data.size(),
+ lldb::eByteOrderLittle, 8);
+  m_thread_reg_ctx_sp.reset(new RegisterContextMinidump_ARM64(*this, data));
+  break;
+}
+case llvm::Triple::arm: {
+  DataExtractor data(m_gpregset_data.data(), m_gpregset_data.size(),
+ lldb::eByteOrderLittle, 8);
+  const bool apple = arch.GetTriple().getVendor() == llvm::Triple::Apple;
+  m_thread_reg_ctx_sp.reset(new RegisterContextMinidump_ARM(*this, data,
+apple));
+  break;
+}
 default:
   break;
 }
 
-if (!reg_interface) {
-  if (log)
-log->Printf("elf-core::%s:: Architecture(%d) not supported",
-__FUNCTION__, arch.GetMachine());
-  assert(false && "Architecture not supported");
-}
-
 reg_ctx_sp = m_thread_reg_ctx_sp;
   } else if (m_unwinder_ap) {
 reg_ctx_sp = m_unwinder_ap->CreateRegisterContextForFrame(frame);
Index: source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h
===
--- source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h
+++ source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h
@@ -0,0 +1,86 @@
+//===-- RegisterContextMinidump_ARM64.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef liblldb_RegisterContextMinidump_ARM64_h_
+#define liblldb_Regist

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: source/API/SBTarget.cpp:1467
+  }
+  const ConstString csFrom(from), csTo(to);
+  if (csFrom && csTo) {

apolyakov wrote:
> aprantl wrote:
> > apolyakov wrote:
> > > aprantl wrote:
> > > > personally I would write this as:
> > > > ```
> > > > if (!csFrom)
> > > >   return error.SetErrorString(" path is empty");
> > > > if (!csTo)
> > > >   return error.SetErrorString(" path is empty");
> > > > Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
> > > > if (log)
> > > >   log->Printf("SBTarget(%p)::%s: '%s' -> '%s'",
> > > >   static_cast(target_sp.get()),  __FUNCTION__,
> > > >   from, to);
> > > > target_sp->GetImageSearchPathList().Append(csFrom, csTo, true);
> > > > ```
> > > in my opinion, the branch `if (csFrom && csTo)` will be executed in most 
> > > cases, so it is reasonable to make it executed first, as this will 
> > > benefit the branch prediction of the processor. Is this a worthwhile 
> > > argument?
> > Not really. First, you don't actually control the order in which the code 
> > is emitted by the compiler by rearranging the source code. Second, it 
> > doesn't make sense to think about branch prediction outside of a tight loop 
> > as the effect would not even be measurable. We should optimize for 
> > readability and compact code here.
> Yes, I should agree that thinking about branch prediction isn't so effective 
> in our case. But the readability of our approaches is almost equal. At the 
> same time, taking into consideration that in most cases we will have not 
> empty string arguments, your approach will check 2 conditions and mine 
> approach will check 1 condition.
It's the LLVM style to prefer early exits:

http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code


https://reviews.llvm.org/D49739



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

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:15
 // Other libraries and framework includes
+//#include "lldb/Core/Architecture.h"
 #include "lldb/Core/Module.h"

lemo wrote:
> it this set for removal?
Ah yes!



Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:196
+
+size_t RegisterContextMinidump_ARM::GetRegisterSetCount()  {
+  return k_num_reg_sets;

Not sure why. I was using the latest Xcode and got the warning. RegisterInfo is 
just a plain struct so it shouldn't cause any issues.


https://reviews.llvm.org/D49750



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

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Alexander Polyakov via Phabricator via lldb-commits]

apolyakov updated this revision to Diff 158377.
apolyakov added a comment.

Changed the order of `if` statements to follow llvm coding standards.


https://reviews.llvm.org/D49739

Files:
  include/lldb/API/SBTarget.h
  lit/lit.cfg
  lit/tools/lldb-mi/target/inputs/main.c
  lit/tools/lldb-mi/target/inputs/target-select-so-path.py
  lit/tools/lldb-mi/target/lit.local.cfg
  lit/tools/lldb-mi/target/target-select-so-path.test
  scripts/interface/SBTarget.i
  source/API/SBTarget.cpp
  tools/lldb-mi/MICmdCmdTarget.cpp

Index: tools/lldb-mi/MICmdCmdTarget.cpp
===
--- tools/lldb-mi/MICmdCmdTarget.cpp
+++ tools/lldb-mi/MICmdCmdTarget.cpp
@@ -10,9 +10,8 @@
 // Overview:CMICmdCmdTargetSelect   implementation.
 
 // Third Party Headers:
-#include "lldb/API/SBCommandInterpreter.h"
-#include "lldb/API/SBCommandReturnObject.h"
 #include "lldb/API/SBStream.h"
+#include "lldb/API/SBError.h"
 
 // In-house headers:
 #include "MICmdArgValNumber.h"
@@ -52,7 +51,7 @@
 // Return:  None.
 // Throws:  None.
 //--
-CMICmdCmdTargetSelect::~CMICmdCmdTargetSelect() {}
+CMICmdCmdTargetSelect::~CMICmdCmdTargetSelect() = default;
 
 //++
 //
@@ -93,51 +92,44 @@
 
   CMICmnLLDBDebugSessionInfo &rSessionInfo(
   CMICmnLLDBDebugSessionInfo::Instance());
+  lldb::SBTarget target = rSessionInfo.GetTarget();
 
-  // Check we have a valid target
-  // Note: target created via 'file-exec-and-symbols' command
-  if (!rSessionInfo.GetTarget().IsValid()) {
+  // Check we have a valid target.
+  // Note: target created via 'file-exec-and-symbols' command.
+  if (!target.IsValid()) {
 SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_TARGET_CURRENT),
m_cmdData.strMiCmd.c_str()));
 return MIstatus::failure;
   }
 
-  // Verify that we are executing remotely
+  // Verify that we are executing remotely.
   const CMIUtilString &rRemoteType(pArgType->GetValue());
   if (rRemoteType != "remote") {
 SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_TARGET_TYPE),
m_cmdData.strMiCmd.c_str(),
rRemoteType.c_str()));
 return MIstatus::failure;
   }
 
-  // Create a URL pointing to the remote gdb stub
+  // Create a URL pointing to the remote gdb stub.
   const CMIUtilString strUrl =
   CMIUtilString::Format("connect://%s", pArgParameters->GetValue().c_str());
 
-  // Ask LLDB to connect to the target port
-  const char *pPlugin("gdb-remote");
   lldb::SBError error;
-  lldb::SBProcess process = rSessionInfo.GetTarget().ConnectRemote(
+  // Ask LLDB to connect to the target port.
+  const char *pPlugin("gdb-remote");
+  lldb::SBProcess process = target.ConnectRemote(
   rSessionInfo.GetListener(), strUrl.c_str(), pPlugin, error);
 
-  // Verify that we have managed to connect successfully
-  lldb::SBStream errMsg;
-  error.GetDescription(errMsg);
+  // Verify that we have managed to connect successfully.
   if (!process.IsValid()) {
 SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_INVALID_TARGET_PLUGIN),
m_cmdData.strMiCmd.c_str(),
-   errMsg.GetData()));
-return MIstatus::failure;
-  }
-  if (error.Fail()) {
-SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_CONNECT_TO_TARGET),
-   m_cmdData.strMiCmd.c_str(),
-   errMsg.GetData()));
+   error.GetCString()));
 return MIstatus::failure;
   }
 
-  // Set the environment path if we were given one
+  // Set the environment path if we were given one.
   CMIUtilString strWkDir;
   if (rSessionInfo.SharedDataRetrieve(
   rSessionInfo.m_constStrSharedDataKeyWkDir, strWkDir)) {
@@ -150,28 +142,13 @@
 }
   }
 
-  // Set the shared object path if we were given one
+  // Set the shared object path if we were given one.
   CMIUtilString strSolibPath;
   if (rSessionInfo.SharedDataRetrieve(
-  rSessionInfo.m_constStrSharedDataSolibPath, strSolibPath)) {
-lldb::SBDebugger &rDbgr = rSessionInfo.GetDebugger();
-lldb::SBCommandInterpreter cmdIterpreter = rDbgr.GetCommandInterpreter();
-
-CMIUtilString strCmdString = CMIUtilString::Format(
-"target modules search-paths add . %s", strSolibPath.c_str());
-
-lldb::SBCommandReturnObject retObj;
-cmdIterpreter.HandleCommand(strCmdString.c_str(), retObj, false);
+  rSessionInfo.m_constStrSharedDataSolibPath, strSolibPath))
+target.AppendImageSearchPath(".", strSolibPath.c_str(), error);
 
-if (!retObj.Succeeded()) {
-  SetError(CMIUtilString::Format(MIRSRC(IDS_CMD_ERR_FNFAILED),
- m_cmdData.strMiCmd.c_str(),
- "target-select"));
-  return MIstatus::failure;
- 

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

[Adrian Prantl via Phabricator via lldb-commits]

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

sgtm.


https://reviews.llvm.org/D49739



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

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 158411.
teemperor marked an inline comment as done.
teemperor added a comment.

- Addressed Pavel's comments.

Will merge once I have time to watch the build bots afterwards.


https://reviews.llvm.org/D50027

Files:
  unittests/Utility/CMakeLists.txt
  unittests/Utility/StreamTest.cpp

Index: unittests/Utility/StreamTest.cpp
===
--- /dev/null
+++ unittests/Utility/StreamTest.cpp
@@ -0,0 +1,480 @@
+//===-- StreamTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Utility/StreamString.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+namespace {
+struct StreamTest : ::testing::Test {
+  // Note: Stream is an abstract class, so we use StreamString to test it. To
+  // make it easier to change this later, only methods in this class explicitly
+  // refer to the StringStream class.
+  StreamString s;
+  // We return here a std::string because that way gtest can print better
+  // assertion messages.
+  std::string TakeValue() {
+std::string result = s.GetString().str();
+s.Clear();
+return result;
+  }
+};
+}
+
+namespace {
+// A StreamTest where we expect the Stream output to be binary.
+struct BinaryStreamTest : StreamTest {
+  void SetUp() override {
+s.GetFlags().Set(Stream::eBinary);
+  }
+};
+}
+
+TEST_F(StreamTest, ChangingByteOrder) {
+  s.SetByteOrder(lldb::eByteOrderPDP);
+  EXPECT_EQ(lldb::eByteOrderPDP, s.GetByteOrder());
+}
+
+TEST_F(StreamTest, PutChar) {
+  s.PutChar('a');
+  EXPECT_EQ("a", TakeValue());
+
+  s.PutChar('1');
+  EXPECT_EQ("1", TakeValue());
+}
+
+TEST_F(StreamTest, PutCharWhitespace) {
+  s.PutChar(' ');
+  EXPECT_EQ(" ", TakeValue());
+
+  s.PutChar('\n');
+  EXPECT_EQ("\n", TakeValue());
+
+  s.PutChar('\r');
+  EXPECT_EQ("\r", TakeValue());
+
+  s.PutChar('\t');
+  EXPECT_EQ("\t", TakeValue());
+}
+
+TEST_F(StreamTest, PutCString) {
+  s.PutCString("");
+  EXPECT_EQ("", TakeValue());
+
+  s.PutCString("foobar");
+  EXPECT_EQ("foobar", TakeValue());
+
+  s.PutCString(" ");
+  EXPECT_EQ(" ", TakeValue());
+}
+
+TEST_F(StreamTest, PutCStringWithStringRef) {
+  s.PutCString(llvm::StringRef(""));
+  EXPECT_EQ("", TakeValue());
+
+  s.PutCString(llvm::StringRef("foobar"));
+  EXPECT_EQ("foobar", TakeValue());
+
+  s.PutCString(llvm::StringRef(" "));
+  EXPECT_EQ(" ", TakeValue());
+}
+
+TEST_F(StreamTest, QuotedCString) {
+  s.QuotedCString("foo");
+  EXPECT_EQ(R"("foo")", TakeValue());
+
+  s.QuotedCString("ba r");
+  EXPECT_EQ(R"("ba r")", TakeValue());
+
+  s.QuotedCString(" ");
+  EXPECT_EQ(R"(" ")", TakeValue());
+}
+
+TEST_F(StreamTest, PutCharNull) {
+  s.PutChar('\0');
+  EXPECT_EQ(std::string("\0", 1), TakeValue());
+
+  s.PutChar('a');
+  EXPECT_EQ(std::string("a", 1), TakeValue());
+}
+
+TEST_F(StreamTest, PutCStringAsRawHex8) {
+  s.PutCStringAsRawHex8("");
+  // FIXME: Check that printing 00 on an empty string is the intended behavior.
+  // It seems kind of unexpected  that we print the trailing 0 byte for empty
+  // strings, but not for non-empty strings.
+  EXPECT_EQ("00", TakeValue());
+
+  s.PutCStringAsRawHex8("foobar");
+  EXPECT_EQ("666f6f626172", TakeValue());
+
+  s.PutCStringAsRawHex8(" ");
+  EXPECT_EQ("20", TakeValue());
+}
+
+TEST_F(StreamTest, PutHex8) {
+  s.PutHex8((uint8_t)55);
+  EXPECT_EQ("37", TakeValue());
+  s.PutHex8(std::numeric_limits::max());
+  EXPECT_EQ("ff", TakeValue());
+  s.PutHex8((uint8_t)0);
+  EXPECT_EQ("00", TakeValue());
+}
+
+TEST_F(StreamTest, PutNHex8) {
+  s.PutNHex8(0, (uint8_t)55);
+  EXPECT_EQ("", TakeValue());
+  s.PutNHex8(1, (uint8_t)55);
+  EXPECT_EQ("37", TakeValue());
+  s.PutNHex8(2, (uint8_t)55);
+  EXPECT_EQ("3737", TakeValue());
+  s.PutNHex8(1, (uint8_t)56);
+  EXPECT_EQ("38", TakeValue());
+}
+
+TEST_F(StreamTest, PutHex16ByteOrderLittle) {
+  s.PutHex16(0x1234U, lldb::eByteOrderLittle);
+  EXPECT_EQ("3412", TakeValue());
+  s.PutHex16(std::numeric_limits::max(), lldb::eByteOrderLittle);
+  EXPECT_EQ("", TakeValue());
+  s.PutHex16(0U, lldb::eByteOrderLittle);
+  EXPECT_EQ("", TakeValue());
+}
+
+TEST_F(StreamTest, PutHex16ByteOrderBig) {
+  s.PutHex16(0x1234U, lldb::eByteOrderBig);
+  EXPECT_EQ("1234", TakeValue());
+  s.PutHex16(std::numeric_limits::max(), lldb::eByteOrderBig);
+  EXPECT_EQ("", TakeValue());
+  s.PutHex16(0U, lldb::eByteOrderBig);
+  EXPECT_EQ("", TakeValue());
+}
+
+TEST_F(StreamTest, PutHex32ByteOrderLittle) {
+  s.PutHex32(0x12345678U, lldb::eByteOrderLittle);
+  EXPECT_EQ("78563412", TakeValue());
+  s.PutHex32(std::numeric_limits::max(), lldb::eByteOrderLittle);
+  EXPECT_EQ("", TakeValue());
+  s.PutHex32(0U, lldb::eByteOrderLittle);
+  

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.



Comment at: unittests/Utility/StreamTest.cpp:38-41
+TEST_F(StreamTest, ChangingByteOrder) {
+  s.SetByteOrder(lldb::eByteOrderPDP);
+  EXPECT_EQ(lldb::eByteOrderPDP, s.GetByteOrder());
+}

labath wrote:
>  I've been wondering for a while whether we shouldn't just remove PDP 
> byte order support. Most of our code doesn't really support it, and neither 
> does llvm's, so this is kind of a prerequisite for switching to llvm streams. 
> 
I support this notion.  think most of LLDB's algorithms do not respect PDP 
ordering.



Comment at: unittests/Utility/StreamTest.cpp:56
+  s.PutChar('\n');
+  EXPECT_EQ(" \n", Value());
+

labath wrote:
> How do you feel about changing `Value` to call `Clear` on the underlying 
> StreamString after fetching the string (and maybe renaming it to `TakeValue` 
> or something)? That way, you could easily test the string printed by a 
> specific function, instead of having to accumulate the expectations.
Sounds good to me, makes it definitely more readable.


https://reviews.llvm.org/D50027



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

[Lldb-commits] [lldb] r338458 - Use UnknownVendor rather than UnknownArch since they're in two different enums

[Eric Christopher via lldb-commits]

Author: echristo
Date: Tue Jul 31 16:53:23 2018
New Revision: 338458

URL: http://llvm.org/viewvc/llvm-project?rev=338458&view=rev
Log:
Use UnknownVendor rather than UnknownArch since they're in two different enums
and we're switching on vendor and not arch.

Modified:
lldb/trunk/source/Plugins/Platform/Windows/PlatformWindows.cpp

Modified: lldb/trunk/source/Plugins/Platform/Windows/PlatformWindows.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Platform/Windows/PlatformWindows.cpp?rev=338458&r1=338457&r2=338458&view=diff
==
--- lldb/trunk/source/Plugins/Platform/Windows/PlatformWindows.cpp (original)
+++ lldb/trunk/source/Plugins/Platform/Windows/PlatformWindows.cpp Tue Jul 31 
16:53:23 2018
@@ -78,7 +78,7 @@ PlatformSP PlatformWindows::CreateInstan
   create = true;
   break;
 
-case llvm::Triple::UnknownArch:
+case llvm::Triple::UnknownVendor:
   create = !arch->TripleVendorWasSpecified();
   break;
 


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

[Lldb-commits] [lldb] r338460 - Android is an environment and we were comparing the android triple

[Eric Christopher via lldb-commits]

Author: echristo
Date: Tue Jul 31 16:53:24 2018
New Revision: 338460

URL: http://llvm.org/viewvc/llvm-project?rev=338460&view=rev
Log:
Android is an environment and we were comparing the android triple
against the OS rather than the environment. Also update other
uses of OS when we meant environment in the android local code.

NFC intended.

Modified:
lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp

Modified: lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp?rev=338460&r1=338459&r2=338460&view=diff
==
--- lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp (original)
+++ lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp Tue Jul 31 
16:53:24 2018
@@ -95,7 +95,7 @@ PlatformSP PlatformAndroid::CreateInstan
 }
 
 if (create) {
-  switch (triple.getOS()) {
+  switch (triple.getEnvironment()) {
   case llvm::Triple::Android:
 break;
 
@@ -103,8 +103,8 @@ PlatformSP PlatformAndroid::CreateInstan
   // Only accept "unknown" for the OS if the host is android and it
   // "unknown" wasn't specified (it was just returned because it was NOT
   // specified)
-  case llvm::Triple::OSType::UnknownOS:
-create = !arch->TripleOSWasSpecified();
+  case llvm::Triple::EnvironmentType::UnknownEnvironment:
+create = !arch->TripleEnvironmentWasSpecified();
 break;
 #endif
   default:


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

[Lldb-commits] [lldb] r338459 - Tidy up comment.

[Eric Christopher via lldb-commits]

Author: echristo
Date: Tue Jul 31 16:53:24 2018
New Revision: 338459

URL: http://llvm.org/viewvc/llvm-project?rev=338459&view=rev
Log:
Tidy up comment.

Modified:
lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp

Modified: lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp?rev=338459&r1=338458&r2=338459&view=diff
==
--- lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp (original)
+++ lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp Tue Jul 31 
16:53:24 2018
@@ -83,9 +83,9 @@ PlatformSP PlatformAndroid::CreateInstan
   break;
 
 #if defined(__ANDROID__)
-// Only accept "unknown" for the vendor if the host is android and it
+// Only accept "unknown" for the vendor if the host is android and if
 // "unknown" wasn't specified (it was just returned because it was NOT
-// specified_
+// specified).
 case llvm::Triple::VendorType::UnknownVendor:
   create = !arch->TripleVendorWasSpecified();
   break;


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

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

[Paul Robinson via Phabricator via lldb-commits]

probinson added inline comments.



Comment at: unittests/Utility/StreamTest.cpp:38-41
+TEST_F(StreamTest, ChangingByteOrder) {
+  s.SetByteOrder(lldb::eByteOrderPDP);
+  EXPECT_EQ(lldb::eByteOrderPDP, s.GetByteOrder());
+}

teemperor wrote:
> labath wrote:
> >  I've been wondering for a while whether we shouldn't just remove 
> > PDP byte order support. Most of our code doesn't really support it, and 
> > neither does llvm's, so this is kind of a prerequisite for switching to 
> > llvm streams. 
> I support this notion.  think most of LLDB's algorithms do not respect PDP 
> ordering.
PDP ordering?  As in, PDP-11?  I could imagine GDB had to support it many long 
ages ago, but AFAIK no such machine has been produced this century.


https://reviews.llvm.org/D50027



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

[Lldb-commits] [lldb] r338488 - Added initial unit test for LLDB's Stream class.

[Raphael Isemann via lldb-commits]

Author: teemperor
Date: Tue Jul 31 23:04:48 2018
New Revision: 338488

URL: http://llvm.org/viewvc/llvm-project?rev=338488&view=rev
Log:
Added initial unit test for LLDB's Stream class.

Summary:
This adds an initial small unit test for LLDB's Stream class, which should at 
least cover
most of the functions in the Stream class. StreamString is always in big endian
mode, so that's the only stream byte order path this test covers as of now. 
Also,
the binary mode still needs to be tested for all print methods.

Also adds some FIXMEs for wrong/strange result values of the Stream class that 
we hit
while testing those functions.

Reviewers: labath

Reviewed By: labath

Subscribers: probinson, labath, mgorny, lldb-commits

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

Added:
lldb/trunk/unittests/Utility/StreamTest.cpp
Modified:
lldb/trunk/unittests/Utility/CMakeLists.txt

Modified: lldb/trunk/unittests/Utility/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Utility/CMakeLists.txt?rev=338488&r1=338487&r2=338488&view=diff
==
--- lldb/trunk/unittests/Utility/CMakeLists.txt (original)
+++ lldb/trunk/unittests/Utility/CMakeLists.txt Tue Jul 31 23:04:48 2018
@@ -14,6 +14,7 @@ add_lldb_unittest(UtilityTests
   NameMatchesTest.cpp
   StatusTest.cpp
   StreamTeeTest.cpp
+  StreamTest.cpp
   StringExtractorTest.cpp
   StructuredDataTest.cpp
   TildeExpressionResolverTest.cpp

Added: lldb/trunk/unittests/Utility/StreamTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Utility/StreamTest.cpp?rev=338488&view=auto
==
--- lldb/trunk/unittests/Utility/StreamTest.cpp (added)
+++ lldb/trunk/unittests/Utility/StreamTest.cpp Tue Jul 31 23:04:48 2018
@@ -0,0 +1,480 @@
+//===-- StreamTest.cpp --*- C++ 
-*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Utility/StreamString.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+namespace {
+struct StreamTest : ::testing::Test {
+  // Note: Stream is an abstract class, so we use StreamString to test it. To
+  // make it easier to change this later, only methods in this class explicitly
+  // refer to the StringStream class.
+  StreamString s;
+  // We return here a std::string because that way gtest can print better
+  // assertion messages.
+  std::string TakeValue() {
+std::string result = s.GetString().str();
+s.Clear();
+return result;
+  }
+};
+}
+
+namespace {
+// A StreamTest where we expect the Stream output to be binary.
+struct BinaryStreamTest : StreamTest {
+  void SetUp() override {
+s.GetFlags().Set(Stream::eBinary);
+  }
+};
+}
+
+TEST_F(StreamTest, ChangingByteOrder) {
+  s.SetByteOrder(lldb::eByteOrderPDP);
+  EXPECT_EQ(lldb::eByteOrderPDP, s.GetByteOrder());
+}
+
+TEST_F(StreamTest, PutChar) {
+  s.PutChar('a');
+  EXPECT_EQ("a", TakeValue());
+
+  s.PutChar('1');
+  EXPECT_EQ("1", TakeValue());
+}
+
+TEST_F(StreamTest, PutCharWhitespace) {
+  s.PutChar(' ');
+  EXPECT_EQ(" ", TakeValue());
+
+  s.PutChar('\n');
+  EXPECT_EQ("\n", TakeValue());
+
+  s.PutChar('\r');
+  EXPECT_EQ("\r", TakeValue());
+
+  s.PutChar('\t');
+  EXPECT_EQ("\t", TakeValue());
+}
+
+TEST_F(StreamTest, PutCString) {
+  s.PutCString("");
+  EXPECT_EQ("", TakeValue());
+
+  s.PutCString("foobar");
+  EXPECT_EQ("foobar", TakeValue());
+
+  s.PutCString(" ");
+  EXPECT_EQ(" ", TakeValue());
+}
+
+TEST_F(StreamTest, PutCStringWithStringRef) {
+  s.PutCString(llvm::StringRef(""));
+  EXPECT_EQ("", TakeValue());
+
+  s.PutCString(llvm::StringRef("foobar"));
+  EXPECT_EQ("foobar", TakeValue());
+
+  s.PutCString(llvm::StringRef(" "));
+  EXPECT_EQ(" ", TakeValue());
+}
+
+TEST_F(StreamTest, QuotedCString) {
+  s.QuotedCString("foo");
+  EXPECT_EQ(R"("foo")", TakeValue());
+
+  s.QuotedCString("ba r");
+  EXPECT_EQ(R"("ba r")", TakeValue());
+
+  s.QuotedCString(" ");
+  EXPECT_EQ(R"(" ")", TakeValue());
+}
+
+TEST_F(StreamTest, PutCharNull) {
+  s.PutChar('\0');
+  EXPECT_EQ(std::string("\0", 1), TakeValue());
+
+  s.PutChar('a');
+  EXPECT_EQ(std::string("a", 1), TakeValue());
+}
+
+TEST_F(StreamTest, PutCStringAsRawHex8) {
+  s.PutCStringAsRawHex8("");
+  // FIXME: Check that printing 00 on an empty string is the intended behavior.
+  // It seems kind of unexpected  that we print the trailing 0 byte for empty
+  // strings, but not for non-empty strings.
+  EXPECT_EQ("00", TakeValue());
+
+  s.PutCStringAsRawHex8("foobar");
+  EXPECT_EQ("666f6f626172", TakeValue());
+
+  s.PutCStringAsRawHex8(" ");
+  EXPECT_EQ("20", TakeValue());
+}
+
+TEST_F(StreamTest, PutHex8) {
+  s.PutHex8

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

[Raphael Isemann via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL338488: Added initial unit test for LLDB's Stream 
class. (authored by teemperor, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50027?vs=158411&id=158467#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50027

Files:
  lldb/trunk/unittests/Utility/CMakeLists.txt
  lldb/trunk/unittests/Utility/StreamTest.cpp

Index: lldb/trunk/unittests/Utility/CMakeLists.txt
===
--- lldb/trunk/unittests/Utility/CMakeLists.txt
+++ lldb/trunk/unittests/Utility/CMakeLists.txt
@@ -14,6 +14,7 @@
   NameMatchesTest.cpp
   StatusTest.cpp
   StreamTeeTest.cpp
+  StreamTest.cpp
   StringExtractorTest.cpp
   StructuredDataTest.cpp
   TildeExpressionResolverTest.cpp
Index: lldb/trunk/unittests/Utility/StreamTest.cpp
===
--- lldb/trunk/unittests/Utility/StreamTest.cpp
+++ lldb/trunk/unittests/Utility/StreamTest.cpp
@@ -0,0 +1,480 @@
+//===-- StreamTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "lldb/Utility/StreamString.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+namespace {
+struct StreamTest : ::testing::Test {
+  // Note: Stream is an abstract class, so we use StreamString to test it. To
+  // make it easier to change this later, only methods in this class explicitly
+  // refer to the StringStream class.
+  StreamString s;
+  // We return here a std::string because that way gtest can print better
+  // assertion messages.
+  std::string TakeValue() {
+std::string result = s.GetString().str();
+s.Clear();
+return result;
+  }
+};
+}
+
+namespace {
+// A StreamTest where we expect the Stream output to be binary.
+struct BinaryStreamTest : StreamTest {
+  void SetUp() override {
+s.GetFlags().Set(Stream::eBinary);
+  }
+};
+}
+
+TEST_F(StreamTest, ChangingByteOrder) {
+  s.SetByteOrder(lldb::eByteOrderPDP);
+  EXPECT_EQ(lldb::eByteOrderPDP, s.GetByteOrder());
+}
+
+TEST_F(StreamTest, PutChar) {
+  s.PutChar('a');
+  EXPECT_EQ("a", TakeValue());
+
+  s.PutChar('1');
+  EXPECT_EQ("1", TakeValue());
+}
+
+TEST_F(StreamTest, PutCharWhitespace) {
+  s.PutChar(' ');
+  EXPECT_EQ(" ", TakeValue());
+
+  s.PutChar('\n');
+  EXPECT_EQ("\n", TakeValue());
+
+  s.PutChar('\r');
+  EXPECT_EQ("\r", TakeValue());
+
+  s.PutChar('\t');
+  EXPECT_EQ("\t", TakeValue());
+}
+
+TEST_F(StreamTest, PutCString) {
+  s.PutCString("");
+  EXPECT_EQ("", TakeValue());
+
+  s.PutCString("foobar");
+  EXPECT_EQ("foobar", TakeValue());
+
+  s.PutCString(" ");
+  EXPECT_EQ(" ", TakeValue());
+}
+
+TEST_F(StreamTest, PutCStringWithStringRef) {
+  s.PutCString(llvm::StringRef(""));
+  EXPECT_EQ("", TakeValue());
+
+  s.PutCString(llvm::StringRef("foobar"));
+  EXPECT_EQ("foobar", TakeValue());
+
+  s.PutCString(llvm::StringRef(" "));
+  EXPECT_EQ(" ", TakeValue());
+}
+
+TEST_F(StreamTest, QuotedCString) {
+  s.QuotedCString("foo");
+  EXPECT_EQ(R"("foo")", TakeValue());
+
+  s.QuotedCString("ba r");
+  EXPECT_EQ(R"("ba r")", TakeValue());
+
+  s.QuotedCString(" ");
+  EXPECT_EQ(R"(" ")", TakeValue());
+}
+
+TEST_F(StreamTest, PutCharNull) {
+  s.PutChar('\0');
+  EXPECT_EQ(std::string("\0", 1), TakeValue());
+
+  s.PutChar('a');
+  EXPECT_EQ(std::string("a", 1), TakeValue());
+}
+
+TEST_F(StreamTest, PutCStringAsRawHex8) {
+  s.PutCStringAsRawHex8("");
+  // FIXME: Check that printing 00 on an empty string is the intended behavior.
+  // It seems kind of unexpected  that we print the trailing 0 byte for empty
+  // strings, but not for non-empty strings.
+  EXPECT_EQ("00", TakeValue());
+
+  s.PutCStringAsRawHex8("foobar");
+  EXPECT_EQ("666f6f626172", TakeValue());
+
+  s.PutCStringAsRawHex8(" ");
+  EXPECT_EQ("20", TakeValue());
+}
+
+TEST_F(StreamTest, PutHex8) {
+  s.PutHex8((uint8_t)55);
+  EXPECT_EQ("37", TakeValue());
+  s.PutHex8(std::numeric_limits::max());
+  EXPECT_EQ("ff", TakeValue());
+  s.PutHex8((uint8_t)0);
+  EXPECT_EQ("00", TakeValue());
+}
+
+TEST_F(StreamTest, PutNHex8) {
+  s.PutNHex8(0, (uint8_t)55);
+  EXPECT_EQ("", TakeValue());
+  s.PutNHex8(1, (uint8_t)55);
+  EXPECT_EQ("37", TakeValue());
+  s.PutNHex8(2, (uint8_t)55);
+  EXPECT_EQ("3737", TakeValue());
+  s.PutNHex8(1, (uint8_t)56);
+  EXPECT_EQ("38", TakeValue());
+}
+
+TEST_F(StreamTest, PutHex16ByteOrderLittle) {
+  s.PutHex16(0x1234U, lldb::eByteOrderLittle);
+  EXPECT_EQ("3412", TakeValue());
+  s.PutHex16(std::numeric_limits::max(), lldb::eByteOrderLittle);
+  EXPECT_EQ("", TakeValue());
+  s.PutHex16(0U, lldb::eByteOrderLittle);
+  EXPECT_EQ("", TakeValue(

[Lldb-commits] [lldb] r338491 - Removed failing StreamTest case

[Raphael Isemann via lldb-commits]

Author: teemperor
Date: Tue Jul 31 23:35:27 2018
New Revision: 338491

URL: http://llvm.org/viewvc/llvm-project?rev=338491&view=rev
Log:
Removed failing StreamTest case

The suspicious behavior is obviously because this method reads
OOB memory, so I'll remove it for now and re-add the test alongside
the fix later.

Modified:
lldb/trunk/unittests/Utility/StreamTest.cpp

Modified: lldb/trunk/unittests/Utility/StreamTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Utility/StreamTest.cpp?rev=338491&r1=338490&r2=338491&view=diff
==
--- lldb/trunk/unittests/Utility/StreamTest.cpp (original)
+++ lldb/trunk/unittests/Utility/StreamTest.cpp Tue Jul 31 23:35:27 2018
@@ -106,12 +106,6 @@ TEST_F(StreamTest, PutCharNull) {
 }
 
 TEST_F(StreamTest, PutCStringAsRawHex8) {
-  s.PutCStringAsRawHex8("");
-  // FIXME: Check that printing 00 on an empty string is the intended behavior.
-  // It seems kind of unexpected  that we print the trailing 0 byte for empty
-  // strings, but not for non-empty strings.
-  EXPECT_EQ("00", TakeValue());
-
   s.PutCStringAsRawHex8("foobar");
   EXPECT_EQ("666f6f626172", TakeValue());
 


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