[Lldb-commits] [PATCH] D50161: Add raw_ostream wrapper to the Stream class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In https://reviews.llvm.org/D50161#1187972, @teemperor wrote:

> StreamTee is copying it, which is expected to be copyable when we copy 
> CommandObjectResult around. And then I just added the copy-constructor as 
> CommandObjectResult refactoring sound time-expensive.


Sure, that's fine. I was just wondering what the obstacles are. Thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D50161



___
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.

I think we're getting there, but all these changes mean I have another round of 
comments. None of them should be major though.

I'm also wondering whether it wouldn't be good to add a couple (one for each 
mangling scheme?) unit tests for the new class, as a way to show the way the 
API is supposed to be used (without all of the Symtab stuff floating around).

In https://reviews.llvm.org/D50071#1187849, @sgraenitz wrote:

> I quickly realized, that it needs a bigger change to properly address the 
> aforementioned issues. Here's the major changes. Some documentation is todo.
>
> **StringRef to reused buffer** addressed with a split into first parse then 
> get buffer. It seems more natural that the buffer may be reused. Also we can 
> ask for `const char *` or a `StringRef`, which avoids the conversion issue.
>
>   MC.ParseFunctionBaseName();
>   llvm::StringRef base_name = MC.GetBufferRef();
>


I like this.

> The fallback case always uses `ConstString` now and it may be a bit slower 
> than before, but IMHO that's fine.

I agree it wouldn't be a disaster, but it also wasn't hard to fix things so 
this works correctly without C strings. So, I just went ahead and committed 
r338995, which should allow you to avoid messing with C strings in this class 
completely.




Comment at: include/lldb/Core/RichManglingInfo.h:59
+case ItaniumPartialDemangler:
+  assert(m_IPD_buf && "Parse before accessing buffer");
+  return llvm::StringRef(m_IPD_buf, m_IPD_str_len);

Is this the right assertion? As far as I can tell, you initialize m_IPD_buf in 
the constructor unconditionally.



Comment at: source/Core/Mangled.cpp:322-323
+  context.ParseFullName();
+  const char *demangled_cstr = context.GetBufferAsCString();
+  m_demangled.SetCStringWithMangledCounterpart(demangled_cstr, m_mangled);
+  return true;

C string no longer needed here.



Comment at: source/Core/RichManglingInfo.cpp:126
+  case ItaniumPartialDemangler: {
+auto multi_in_out = m_IPD_size;
+auto buf = m_IPD.getFunctionBaseName(m_IPD_buf, &multi_in_out);

sgraenitz wrote:
> Not sure about `multi_in_out` as a name here. It's hard to find one that fits 
> the purpose and stays below 30 chars.. IPD calls it `N`.
The name makes no difference to me. However, if you think it makes sense, you 
could try passing `getFunctionBaseName` as a member function pointer into the 
helper function, so that all of the buffer handling is completely hidden inside 
that function.



Comment at: source/Core/RichManglingInfo.cpp:47
+  ParseFullName();
+  log->Printf("demangled itanium: %s -> \"%s\"", mangled.GetCString(),
+  GetBufferAsCString());

`LLDB_LOG(log, demangled itanium: {0} -> \"{1}\"", mangled, GetBuffer());`
shorter and avoids C strings



Comment at: source/Core/RichManglingInfo.cpp:72
+get(m_cxx_method_parser)->GetBasename();
+return base_name.front() == '~';
+  }

will `base_name` always be non-empty (e.g. if the plugin failed to parse the 
string?). `base_name.startswith('~')` might be more appropriate here.



Comment at: source/Symbol/Symtab.cpp:386-387
+  MC.ParseFunctionDeclContextName();
+  const char *decl_context_cstr = MC.GetBufferAsCString();
+  if (decl_context_cstr == nullptr || decl_context_cstr[0] == '\0') {
+// This has to be a basename

`MC.GetBuffer().empty()`


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] D50327: Add ConstString::IsNull() to tell between null vs. empty strings and fix usage in Mangled::GetDemangledName()

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

sgraenitz created this revision.
sgraenitz added reviewers: labath, jingham.
Herald added a subscriber: erik.pilkington.

`IsEmpty()` and `operator bool() == false` have equal semantics. Usage in 
Mangled::GetDemangledName() was incorrect. What it actually wants is a check 
for null-string. Split this off of https://reviews.llvm.org/D50071 and added a 
test to clarify usage.


https://reviews.llvm.org/D50327

Files:
  include/lldb/Utility/ConstString.h
  source/Core/Mangled.cpp
  unittests/Utility/ConstStringTest.cpp


Index: unittests/Utility/ConstStringTest.cpp
===
--- unittests/Utility/ConstStringTest.cpp
+++ unittests/Utility/ConstStringTest.cpp
@@ -33,3 +33,20 @@
   EXPECT_TRUE(foo.GetMangledCounterpart(counterpart));
   EXPECT_EQ("bar", counterpart.GetStringRef());
 }
+
+TEST(ConstStringTest, NullAndEmptyStates) {
+  ConstString foo("foo");
+  EXPECT_FALSE(!foo);
+  EXPECT_FALSE(foo.IsEmpty());
+  EXPECT_FALSE(foo.IsNull());
+
+  ConstString empty("");
+  EXPECT_TRUE(!empty);
+  EXPECT_TRUE(empty.IsEmpty());
+  EXPECT_FALSE(empty.IsNull());
+
+  ConstString null;
+  EXPECT_TRUE(!null);
+  EXPECT_TRUE(null.IsEmpty());
+  EXPECT_TRUE(null.IsNull());
+}
Index: source/Core/Mangled.cpp
===
--- source/Core/Mangled.cpp
+++ source/Core/Mangled.cpp
@@ -242,7 +242,7 @@
 Mangled::GetDemangledName(lldb::LanguageType language) const {
   // Check to make sure we have a valid mangled name and that we haven't
   // already decoded our mangled name.
-  if (m_mangled && !m_demangled) {
+  if (m_mangled && m_demangled.IsNull()) {
 // We need to generate and cache the demangled name.
 static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
 Timer scoped_timer(func_cat, "Mangled::GetDemangledName (m_mangled = %s)",
@@ -312,7 +312,7 @@
 free(demangled_name);
   }
 }
-if (!m_demangled) {
+if (m_demangled.IsNull()) {
   // Set the demangled string to the empty string to indicate we tried to
   // parse it once and failed.
   m_demangled.SetCString("");
Index: include/lldb/Utility/ConstString.h
===
--- include/lldb/Utility/ConstString.h
+++ include/lldb/Utility/ConstString.h
@@ -345,6 +345,15 @@
   //--
   bool IsEmpty() const { return m_string == nullptr || m_string[0] == '\0'; }
 
+  //--
+  /// Test for null string.
+  ///
+  /// @return
+  /// @li \b true if there is no string associated with this instance.
+  /// @li \b false if there is a string associated with this instance.
+  //--
+  bool IsNull() const { return m_string == nullptr; }
+
   //--
   /// Set the C string value.
   ///


Index: unittests/Utility/ConstStringTest.cpp
===
--- unittests/Utility/ConstStringTest.cpp
+++ unittests/Utility/ConstStringTest.cpp
@@ -33,3 +33,20 @@
   EXPECT_TRUE(foo.GetMangledCounterpart(counterpart));
   EXPECT_EQ("bar", counterpart.GetStringRef());
 }
+
+TEST(ConstStringTest, NullAndEmptyStates) {
+  ConstString foo("foo");
+  EXPECT_FALSE(!foo);
+  EXPECT_FALSE(foo.IsEmpty());
+  EXPECT_FALSE(foo.IsNull());
+
+  ConstString empty("");
+  EXPECT_TRUE(!empty);
+  EXPECT_TRUE(empty.IsEmpty());
+  EXPECT_FALSE(empty.IsNull());
+
+  ConstString null;
+  EXPECT_TRUE(!null);
+  EXPECT_TRUE(null.IsEmpty());
+  EXPECT_TRUE(null.IsNull());
+}
Index: source/Core/Mangled.cpp
===
--- source/Core/Mangled.cpp
+++ source/Core/Mangled.cpp
@@ -242,7 +242,7 @@
 Mangled::GetDemangledName(lldb::LanguageType language) const {
   // Check to make sure we have a valid mangled name and that we haven't
   // already decoded our mangled name.
-  if (m_mangled && !m_demangled) {
+  if (m_mangled && m_demangled.IsNull()) {
 // We need to generate and cache the demangled name.
 static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
 Timer scoped_timer(func_cat, "Mangled::GetDemangledName (m_mangled = %s)",
@@ -312,7 +312,7 @@
 free(demangled_name);
   }
 }
-if (!m_demangled) {
+if (m_demangled.IsNull()) {
   // Set the demangled string to the empty string to indicate we tried to
   // parse it once and failed.
   m_demangled.SetCString("");
Index: include/lldb/Utility/ConstString.h
===
--- include/lldb/Utility/ConstString.h
+++ include/lldb/Utility/ConstString.h
@@ -345,6 +345,15 @@
   //--
   bool IsEmpty() const { return m_string == nullptr || m_string[0] == '\0'

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

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

sgraenitz added a comment.

Ok I will fix the details, rebase and update the review.

Btw: I also spilt off the `ConstString::IsNull()` addition with unit test and 
fix in `Mangled` to https://reviews.llvm.org/D50327




Comment at: source/Core/RichManglingInfo.cpp:126
+  case ItaniumPartialDemangler: {
+auto multi_in_out = m_IPD_size;
+auto buf = m_IPD.getFunctionBaseName(m_IPD_buf, &multi_in_out);

labath wrote:
> sgraenitz wrote:
> > Not sure about `multi_in_out` as a name here. It's hard to find one that 
> > fits the purpose and stays below 30 chars.. IPD calls it `N`.
> The name makes no difference to me. However, if you think it makes sense, you 
> could try passing `getFunctionBaseName` as a member function pointer into the 
> helper function, so that all of the buffer handling is completely hidden 
> inside that function.
Thought about that, but didn't want to over-engineer this. It might be 
revisited in case we add more accessors in the future.


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] D50327: Add ConstString::IsNull() to tell between null vs. empty strings and fix usage in Mangled::GetDemangledName()

[Pavel Labath via Phabricator via lldb-commits]

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

LGTM. Thanks for splitting this out.


https://reviews.llvm.org/D50327



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

[Lldb-commits] [lldb] r339015 - Fix Mangled::Compare()

[Stefan Granitz via lldb-commits]

Author: stefan.graenitz
Date: Mon Aug  6 07:15:21 2018
New Revision: 339015

URL: http://llvm.org/viewvc/llvm-project?rev=339015&view=rev
Log:
Fix Mangled::Compare()

Modified:
lldb/trunk/source/Core/Mangled.cpp

Modified: lldb/trunk/source/Core/Mangled.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Mangled.cpp?rev=339015&r1=339014&r2=339015&view=diff
==
--- lldb/trunk/source/Core/Mangled.cpp (original)
+++ lldb/trunk/source/Core/Mangled.cpp Mon Aug  6 07:15:21 2018
@@ -195,7 +195,7 @@ void Mangled::Clear() {
 int Mangled::Compare(const Mangled &a, const Mangled &b) {
   return ConstString::Compare(
   a.GetName(lldb::eLanguageTypeUnknown, ePreferMangled),
-  a.GetName(lldb::eLanguageTypeUnknown, ePreferMangled));
+  b.GetName(lldb::eLanguageTypeUnknown, ePreferMangled));
 }
 
 //--


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

[Lldb-commits] [lldb] r339014 - Add ConstString::IsNull() to tell between null vs. empty strings and fix usage in Mangled::GetDemangledName()

[Stefan Granitz via lldb-commits]

Author: stefan.graenitz
Date: Mon Aug  6 07:15:17 2018
New Revision: 339014

URL: http://llvm.org/viewvc/llvm-project?rev=339014&view=rev
Log:
Add ConstString::IsNull() to tell between null vs. empty strings and fix usage 
in Mangled::GetDemangledName()

Summary: `IsEmpty()` and `operator bool() == false` have equal semantics. Usage 
in Mangled::GetDemangledName() was incorrect. What it actually wants is a check 
for null-string. Split this off of D50071 and added a test to clarify usage.

Reviewers: labath, jingham

Subscribers: erik.pilkington, lldb-commits

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

Modified:
lldb/trunk/include/lldb/Utility/ConstString.h
lldb/trunk/source/Core/Mangled.cpp
lldb/trunk/unittests/Utility/ConstStringTest.cpp

Modified: lldb/trunk/include/lldb/Utility/ConstString.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/ConstString.h?rev=339014&r1=339013&r2=339014&view=diff
==
--- lldb/trunk/include/lldb/Utility/ConstString.h (original)
+++ lldb/trunk/include/lldb/Utility/ConstString.h Mon Aug  6 07:15:17 2018
@@ -346,6 +346,15 @@ public:
   bool IsEmpty() const { return m_string == nullptr || m_string[0] == '\0'; }
 
   //--
+  /// Test for null string.
+  ///
+  /// @return
+  /// @li \b true if there is no string associated with this instance.
+  /// @li \b false if there is a string associated with this instance.
+  //--
+  bool IsNull() const { return m_string == nullptr; }
+
+  //--
   /// Set the C string value.
   ///
   /// Set the string value in the object by uniquing the \a cstr string value

Modified: lldb/trunk/source/Core/Mangled.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/Mangled.cpp?rev=339014&r1=339013&r2=339014&view=diff
==
--- lldb/trunk/source/Core/Mangled.cpp (original)
+++ lldb/trunk/source/Core/Mangled.cpp Mon Aug  6 07:15:17 2018
@@ -242,7 +242,7 @@ const ConstString &
 Mangled::GetDemangledName(lldb::LanguageType language) const {
   // Check to make sure we have a valid mangled name and that we haven't
   // already decoded our mangled name.
-  if (m_mangled && !m_demangled) {
+  if (m_mangled && m_demangled.IsNull()) {
 // We need to generate and cache the demangled name.
 static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
 Timer scoped_timer(func_cat, "Mangled::GetDemangledName (m_mangled = %s)",
@@ -312,7 +312,7 @@ Mangled::GetDemangledName(lldb::Language
 free(demangled_name);
   }
 }
-if (!m_demangled) {
+if (m_demangled.IsNull()) {
   // Set the demangled string to the empty string to indicate we tried to
   // parse it once and failed.
   m_demangled.SetCString("");

Modified: lldb/trunk/unittests/Utility/ConstStringTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/unittests/Utility/ConstStringTest.cpp?rev=339014&r1=339013&r2=339014&view=diff
==
--- lldb/trunk/unittests/Utility/ConstStringTest.cpp (original)
+++ lldb/trunk/unittests/Utility/ConstStringTest.cpp Mon Aug  6 07:15:17 2018
@@ -33,3 +33,20 @@ TEST(ConstStringTest, MangledCounterpart
   EXPECT_TRUE(foo.GetMangledCounterpart(counterpart));
   EXPECT_EQ("bar", counterpart.GetStringRef());
 }
+
+TEST(ConstStringTest, NullAndEmptyStates) {
+  ConstString foo("foo");
+  EXPECT_FALSE(!foo);
+  EXPECT_FALSE(foo.IsEmpty());
+  EXPECT_FALSE(foo.IsNull());
+
+  ConstString empty("");
+  EXPECT_TRUE(!empty);
+  EXPECT_TRUE(empty.IsEmpty());
+  EXPECT_FALSE(empty.IsNull());
+
+  ConstString null;
+  EXPECT_TRUE(!null);
+  EXPECT_TRUE(null.IsEmpty());
+  EXPECT_TRUE(null.IsNull());
+}


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

[Lldb-commits] [PATCH] D50327: Add ConstString::IsNull() to tell between null vs. empty strings and fix usage in Mangled::GetDemangledName()

[Phabricator via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339014: Add ConstString::IsNull() to tell between null vs. 
empty strings and fix usage… (authored by stefan.graenitz, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50327?vs=159278&id=159297#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50327

Files:
  lldb/trunk/include/lldb/Utility/ConstString.h
  lldb/trunk/source/Core/Mangled.cpp
  lldb/trunk/unittests/Utility/ConstStringTest.cpp


Index: lldb/trunk/include/lldb/Utility/ConstString.h
===
--- lldb/trunk/include/lldb/Utility/ConstString.h
+++ lldb/trunk/include/lldb/Utility/ConstString.h
@@ -346,6 +346,15 @@
   bool IsEmpty() const { return m_string == nullptr || m_string[0] == '\0'; }
 
   //--
+  /// Test for null string.
+  ///
+  /// @return
+  /// @li \b true if there is no string associated with this instance.
+  /// @li \b false if there is a string associated with this instance.
+  //--
+  bool IsNull() const { return m_string == nullptr; }
+
+  //--
   /// Set the C string value.
   ///
   /// Set the string value in the object by uniquing the \a cstr string value
Index: lldb/trunk/unittests/Utility/ConstStringTest.cpp
===
--- lldb/trunk/unittests/Utility/ConstStringTest.cpp
+++ lldb/trunk/unittests/Utility/ConstStringTest.cpp
@@ -33,3 +33,20 @@
   EXPECT_TRUE(foo.GetMangledCounterpart(counterpart));
   EXPECT_EQ("bar", counterpart.GetStringRef());
 }
+
+TEST(ConstStringTest, NullAndEmptyStates) {
+  ConstString foo("foo");
+  EXPECT_FALSE(!foo);
+  EXPECT_FALSE(foo.IsEmpty());
+  EXPECT_FALSE(foo.IsNull());
+
+  ConstString empty("");
+  EXPECT_TRUE(!empty);
+  EXPECT_TRUE(empty.IsEmpty());
+  EXPECT_FALSE(empty.IsNull());
+
+  ConstString null;
+  EXPECT_TRUE(!null);
+  EXPECT_TRUE(null.IsEmpty());
+  EXPECT_TRUE(null.IsNull());
+}
Index: lldb/trunk/source/Core/Mangled.cpp
===
--- lldb/trunk/source/Core/Mangled.cpp
+++ lldb/trunk/source/Core/Mangled.cpp
@@ -242,7 +242,7 @@
 Mangled::GetDemangledName(lldb::LanguageType language) const {
   // Check to make sure we have a valid mangled name and that we haven't
   // already decoded our mangled name.
-  if (m_mangled && !m_demangled) {
+  if (m_mangled && m_demangled.IsNull()) {
 // We need to generate and cache the demangled name.
 static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
 Timer scoped_timer(func_cat, "Mangled::GetDemangledName (m_mangled = %s)",
@@ -312,7 +312,7 @@
 free(demangled_name);
   }
 }
-if (!m_demangled) {
+if (m_demangled.IsNull()) {
   // Set the demangled string to the empty string to indicate we tried to
   // parse it once and failed.
   m_demangled.SetCString("");


Index: lldb/trunk/include/lldb/Utility/ConstString.h
===
--- lldb/trunk/include/lldb/Utility/ConstString.h
+++ lldb/trunk/include/lldb/Utility/ConstString.h
@@ -346,6 +346,15 @@
   bool IsEmpty() const { return m_string == nullptr || m_string[0] == '\0'; }
 
   //--
+  /// Test for null string.
+  ///
+  /// @return
+  /// @li \b true if there is no string associated with this instance.
+  /// @li \b false if there is a string associated with this instance.
+  //--
+  bool IsNull() const { return m_string == nullptr; }
+
+  //--
   /// Set the C string value.
   ///
   /// Set the string value in the object by uniquing the \a cstr string value
Index: lldb/trunk/unittests/Utility/ConstStringTest.cpp
===
--- lldb/trunk/unittests/Utility/ConstStringTest.cpp
+++ lldb/trunk/unittests/Utility/ConstStringTest.cpp
@@ -33,3 +33,20 @@
   EXPECT_TRUE(foo.GetMangledCounterpart(counterpart));
   EXPECT_EQ("bar", counterpart.GetStringRef());
 }
+
+TEST(ConstStringTest, NullAndEmptyStates) {
+  ConstString foo("foo");
+  EXPECT_FALSE(!foo);
+  EXPECT_FALSE(foo.IsEmpty());
+  EXPECT_FALSE(foo.IsNull());
+
+  ConstString empty("");
+  EXPECT_TRUE(!empty);
+  EXPECT_TRUE(empty.IsEmpty());
+  EXPECT_FALSE(empty.IsNull());
+
+  ConstString null;
+  EXPECT_TRUE(!null);
+  EXPECT_TRUE(null.IsEmpty());
+  EXPECT_TRUE(null.IsNull());
+}
Index: lldb/trunk/source/Core/Mangled.cpp
===
--- lldb/trunk/source/Core/Mangled.cpp
+++ lldb/trunk/source/Core/M

[Lldb-commits] [PATCH] D50334: Add ConstString test FromMidOfBufferStringRef

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

sgraenitz created this revision.
sgraenitz added a reviewer: labath.

It was not immediately clear to me whether or not non-null-terminated 
StringRef's are supported in ConstString and/or the counterpart mechanism. From 
this test it seems to be fine. Maybe useful to keep?


https://reviews.llvm.org/D50334

Files:
  unittests/Utility/ConstStringTest.cpp


Index: unittests/Utility/ConstStringTest.cpp
===
--- unittests/Utility/ConstStringTest.cpp
+++ unittests/Utility/ConstStringTest.cpp
@@ -34,6 +34,26 @@
   EXPECT_EQ("bar", counterpart.GetStringRef());
 }
 
+TEST(ConstStringTest, FromMidOfBufferStringRef) {
+  // StringRef's into bigger buffer: no null termination
+  const char *buffer = "foobarbaz";
+  llvm::StringRef foo_ref(buffer, 3);
+  llvm::StringRef bar_ref(buffer + 3, 3);
+
+  ConstString foo(foo_ref);
+
+  ConstString bar;
+  bar.SetStringWithMangledCounterpart(bar_ref, foo);
+  EXPECT_EQ("bar", bar.GetStringRef());
+
+  ConstString counterpart;
+  EXPECT_TRUE(bar.GetMangledCounterpart(counterpart));
+  EXPECT_EQ("foo", counterpart.GetStringRef());
+
+  EXPECT_TRUE(foo.GetMangledCounterpart(counterpart));
+  EXPECT_EQ("bar", counterpart.GetStringRef());
+}
+
 TEST(ConstStringTest, NullAndEmptyStates) {
   ConstString foo("foo");
   EXPECT_FALSE(!foo);


Index: unittests/Utility/ConstStringTest.cpp
===
--- unittests/Utility/ConstStringTest.cpp
+++ unittests/Utility/ConstStringTest.cpp
@@ -34,6 +34,26 @@
   EXPECT_EQ("bar", counterpart.GetStringRef());
 }
 
+TEST(ConstStringTest, FromMidOfBufferStringRef) {
+  // StringRef's into bigger buffer: no null termination
+  const char *buffer = "foobarbaz";
+  llvm::StringRef foo_ref(buffer, 3);
+  llvm::StringRef bar_ref(buffer + 3, 3);
+
+  ConstString foo(foo_ref);
+
+  ConstString bar;
+  bar.SetStringWithMangledCounterpart(bar_ref, foo);
+  EXPECT_EQ("bar", bar.GetStringRef());
+
+  ConstString counterpart;
+  EXPECT_TRUE(bar.GetMangledCounterpart(counterpart));
+  EXPECT_EQ("foo", counterpart.GetStringRef());
+
+  EXPECT_TRUE(foo.GetMangledCounterpart(counterpart));
+  EXPECT_EQ("bar", counterpart.GetStringRef());
+}
+
 TEST(ConstStringTest, NullAndEmptyStates) {
   ConstString foo("foo");
   EXPECT_FALSE(!foo);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50334: Add ConstString test FromMidOfBufferStringRef

[Pavel Labath via Phabricator via lldb-commits]

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

In general, I would assume that a function taking a StringRef can take *any* 
StringRef object (including the non-terminated ones), unless the limitations 
are very clearly documented.

That said, having a test that verifies this is a legitimate thing, and since 
you've already written one, we might as well check it in.


https://reviews.llvm.org/D50334



___
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 added a comment.

Ping!

Can you review this, please?


https://reviews.llvm.org/D49980



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

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

[Zachary Turner via lldb-commits]

I am OOO this week and only have access to mobile, so hopefully someone
else can review it
On Mon, Aug 6, 2018 at 11:05 AM Aleksandr Urakov via Phabricator <
revi...@reviews.llvm.org> wrote:

> aleksandr.urakov added a comment.
>
> Ping!
>
> Can you review this, please?
>
>
> https://reviews.llvm.org/D49980
>
>
>
>
___
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)

[Zachary Turner via Phabricator via lldb-commits]

zturner added a subscriber: aleksandr.urakov.
zturner added a comment.

I am OOO this week and only have access to mobile, so hopefully someone
else can review it


https://reviews.llvm.org/D49980



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

[Lldb-commits] [PATCH] D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2).

[Greg Clayton via Phabricator via lldb-commits]

clayborg created this revision.
clayborg added reviewers: labath, zturner, markmentovai, javed.absar.
Herald added subscribers: chrib, kristof.beyls.

In this patch I add support for ARM and ARM64 break pad files. There are two 
flavors of ARM: Apple where FP is https://reviews.llvm.org/source/openmp/, and 
non Apple where FP is https://reviews.llvm.org/source/libunwind/. Added minimal 
tests that load up ARM64 and the two flavors or ARM core files with a single 
thread and known register values in each register. Each register is checked for 
the exact value.

This is a fixed version of: https://reviews.llvm.org/D49750

The changes from https://reviews.llvm.org/D49750 are:

- Don't init the m_arch in the Initialize call as a system info isn't required. 
This keeps the thread list, module list and other tests from failing
- Added -Wextended-offsetof to Xcode project so we catch use extended usages of 
offsetof before submission
- Fixed any extended offset of warnings


https://reviews.llvm.org/D50336

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
@@ -11,6 +11,8 @@
 #include "ThreadMinidump.h"
 #include "ProcessMinidump.h"
 
+#include "RegisterContextMinidump_ARM.h"
+#include "RegisterContextMinidump_ARM64.h"
 #include "RegisterContextMinidump_x86_32.h"
 #include "RegisterContextMinidump_x86_64.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

[Lldb-commits] [PATCH] D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2).

[Zachary Turner via Phabricator via lldb-commits]

zturner added a subscriber: clayborg.
zturner added a comment.

Did you see my comments on the first round about how the CMake build didn’t
work? Because I don’t see any changes to CMakeLists.txt here, which means
it still won’t work.

The easiest way to make sure you get all the fixes that may have gone in
after your initial commit is to revert the revert and build your
modifications on top of that. Then, for example, the fixes to
CMakeLists.txt would already be here.


https://reviews.llvm.org/D50336



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

Re: [Lldb-commits] [PATCH] D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2).

[Zachary Turner via lldb-commits]

Did you see my comments on the first round about how the CMake build didn’t
work? Because I don’t see any changes to CMakeLists.txt here, which means
it still won’t work.

The easiest way to make sure you get all the fixes that may have gone in
after your initial commit is to revert the revert and build your
modifications on top of that. Then, for example, the fixes to
CMakeLists.txt would already be here.
On Mon, Aug 6, 2018 at 11:14 AM Greg Clayton via Phabricator <
revi...@reviews.llvm.org> wrote:

> clayborg created this revision.
> clayborg added reviewers: labath, zturner, markmentovai, javed.absar.
> Herald added subscribers: chrib, kristof.beyls.
>
> In this patch I add support for ARM and ARM64 break pad files. There are
> two flavors of ARM: Apple where FP is
> https://reviews.llvm.org/source/openmp/, and non Apple where FP is
> https://reviews.llvm.org/source/libunwind/. Added minimal tests that load
> up ARM64 and the two flavors or ARM core files with a single thread and
> known register values in each register. Each register is checked for the
> exact value.
>
> This is a fixed version of: https://reviews.llvm.org/D49750
>
> The changes from https://reviews.llvm.org/D49750 are:
>
> - Don't init the m_arch in the Initialize call as a system info isn't
> required. This keeps the thread list, module list and other tests from
> failing
> - Added -Wextended-offsetof to Xcode project so we catch use extended
> usages of offsetof before submission
> - Fixed any extended offset of warnings
>
>
> https://reviews.llvm.org/D50336
>
> 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
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Re: [Lldb-commits] [PATCH] D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2).

[Greg Clayton via lldb-commits]

I was in the process of testing on linux. I will fix this.


> On Aug 6, 2018, at 8:19 AM, Zachary Turner  wrote:
> 
> Did you see my comments on the first round about how the CMake build didn’t 
> work? Because I don’t see any changes to CMakeLists.txt here, which means it 
> still won’t work.
> 
> The easiest way to make sure you get all the fixes that may have gone in 
> after your initial commit is to revert the revert and build your 
> modifications on top of that. Then, for example, the fixes to CMakeLists.txt 
> would already be here.
> On Mon, Aug 6, 2018 at 11:14 AM Greg Clayton via Phabricator 
> mailto:revi...@reviews.llvm.org>> wrote:
> clayborg created this revision.
> clayborg added reviewers: labath, zturner, markmentovai, javed.absar.
> Herald added subscribers: chrib, kristof.beyls.
> 
> In this patch I add support for ARM and ARM64 break pad files. There are two 
> flavors of ARM: Apple where FP is https://reviews.llvm.org/source/openmp/ 
> , and non Apple where FP is 
> https://reviews.llvm.org/source/libunwind/ 
> . Added minimal tests that load 
> up ARM64 and the two flavors or ARM core files with a single thread and known 
> register values in each register. Each register is checked for the exact 
> value.
> 
> This is a fixed version of: https://reviews.llvm.org/D49750 
> 
> 
> The changes from https://reviews.llvm.org/D49750 
>  are:
> 
> - Don't init the m_arch in the Initialize call as a system info isn't 
> required. This keeps the thread list, module list and other tests from failing
> - Added -Wextended-offsetof to Xcode project so we catch use extended usages 
> of offsetof before submission
> - Fixed any extended offset of warnings
> 
> 
> https://reviews.llvm.org/D50336 
> 
> 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
> 

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

[Lldb-commits] [PATCH] D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2).

[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 should be fine once the cmake thing is addressed.


https://reviews.llvm.org/D50336



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

[Lldb-commits] [PATCH] D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2).

[Greg Clayton via Phabricator via lldb-commits]

clayborg updated this revision to Diff 159324.
clayborg added a comment.
Herald added a subscriber: mgorny.

Added CMakeList.txt changes, tested on linux, and removed unused "log" variable.


https://reviews.llvm.org/D50336

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/CMakeLists.txt
  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
@@ -11,6 +11,8 @@
 #include "ThreadMinidump.h"
 #include "ProcessMinidump.h"
 
+#include "RegisterContextMinidump_ARM.h"
+#include "RegisterContextMinidump_ARM64.h"
 #include "RegisterContextMinidump_x86_32.h"
 #include "RegisterContextMinidump_x86_64.h"
 
@@ -54,7 +56,6 @@
 ThreadMinidump::CreateRegisterContextForFrame(StackFrame *frame) {
   RegisterContextSP reg_ctx_sp;
   uint32_t concrete_frame_idx = 0;
-  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_THREAD));
 
   if (frame)
 concrete_frame_idx = frame->GetConcreteFrameIndex();
@@ -88,17 +89,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,85 @@
+//===-- RegisterContextMinidump_ARM64.h -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This fil

[Lldb-commits] [lldb] r339032 - Add support for ARM and ARM64 breakpad generated minidump files (version 2).

[Greg Clayton via lldb-commits]

Author: gclayton
Date: Mon Aug  6 09:56:10 2018
New Revision: 339032

URL: http://llvm.org/viewvc/llvm-project?rev=339032&view=rev
Log:
Add support for ARM and ARM64 breakpad generated minidump files (version 2).

In this patch I add support for ARM and ARM64 break pad files. There are two 
flavors of ARM: Apple where FP is R7, and non Apple where FP is R11. Added 
minimal tests that load up ARM64 and the two flavors or ARM core files with a 
single thread and known register values in each register. Each register is 
checked for the exact value.

This is a fixed version of: https://reviews.llvm.org/D49750

The changes from D49750 are:

Don't init the m_arch in the Initialize call as a system info isn't required. 
This keeps the thread list, module list and other tests from failing
Added -Wextended-offsetof to Xcode project so we catch use extended usages of 
offsetof before submission
Fixed any extended offset of warnings

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


Added:

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/arm-linux.dmp
   (with props)

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/arm-macos.dmp
   (with props)

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/arm64-macos.dmp
   (with props)
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.h
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h
Modified:
lldb/trunk/include/lldb/Target/Target.h
lldb/trunk/lldb.xcodeproj/project.pbxproj

lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
lldb/trunk/source/Plugins/Process/minidump/CMakeLists.txt
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp
lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h
lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
lldb/trunk/source/Plugins/Process/minidump/ThreadMinidump.cpp
lldb/trunk/source/Target/Target.cpp

Modified: lldb/trunk/include/lldb/Target/Target.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/Target.h?rev=339032&r1=339031&r2=339032&view=diff
==
--- lldb/trunk/include/lldb/Target/Target.h (original)
+++ lldb/trunk/include/lldb/Target/Target.h Mon Aug  6 09:56:10 2018
@@ -913,28 +913,30 @@ public:
   /// Set the architecture for this target.
   ///
   /// If the current target has no Images read in, then this just sets the
-  /// architecture, which will
-  /// be used to select the architecture of the ExecutableModule when that is
-  /// set.
-  /// If the current target has an ExecutableModule, then calling
-  /// SetArchitecture with a different
+  /// architecture, which will be used to select the architecture of the
+  /// ExecutableModule when that is set. If the current target has an
+  /// ExecutableModule, then calling SetArchitecture with a different
   /// architecture from the currently selected one will reset the
-  /// ExecutableModule to that slice
-  /// of the file backing the ExecutableModule.  If the file backing the
-  /// ExecutableModule does not
-  /// contain a fork of this architecture, then this code will return false, 
and
-  /// the architecture
-  /// won't be changed.
-  /// If the input arch_spec is the same as the already set architecture, this
-  /// is a no-op.
+  /// ExecutableModule to that slice of the file backing the ExecutableModule.
+  /// If the file backing the ExecutableModule does not contain a fork of this
+  /// architecture, then this code will return false, and the architecture
+  /// won't be changed. If the input arch_spec is the same as the already set
+  /// architecture, this is a no-op.
   ///
   /// @param[in] arch_spec
   /// The new architecture.
   ///
+  /// @param[in] set_platform
+  /// If \b true, then the platform will be adjusted if the currently
+  /// selected platform is not compatible with the archicture being set.
+  /// If \b false, then just the architecture will be set even if the
+  /// currently selected platform isn't compatible (in case it might be
+  /// manually set following this function call).
+  ///
   /// @return
   /// \b true if the architecture was successfully set, \bfalse otherwise.
   //--
-  bool SetArchitecture(const ArchSpec &arch_spec);
+  bool SetArchitecture(const ArchSpec &arch_spec, bool set_platform = false);
 
   bool MergeArchitecture(const ArchSpec &arch_spec);
 

Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lldb.xcodeproj/project.pbxproj?rev=339032&r1=3

[Lldb-commits] [PATCH] D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2).

[Phabricator via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339032: Add support for ARM and ARM64 breakpad generated 
minidump files (version 2). (authored by gclayton, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50336?vs=159324&id=159326#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50336

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

Index: lldb/trunk/include/lldb/Target/Target.h
===
--- lldb/trunk/include/lldb/Target/Target.h
+++ lldb/trunk/include/lldb/Target/Target.h
@@ -913,28 +913,30 @@
   /// Set the architecture for this target.
   ///
   /// If the current target has no Images read in, then this just sets the
-  /// architecture, which will
-  /// be used to select the architecture of the ExecutableModule when that is
-  /// set.
-  /// If the current target has an ExecutableModule, then calling
-  /// SetArchitecture with a different
+  /// architecture, which will be used to select the architecture of the
+  /// ExecutableModule when that is set. If the current target has an
+  /// ExecutableModule, then calling SetArchitecture with a different
   /// architecture from the currently selected one will reset the
-  /// ExecutableModule to that slice
-  /// of the file backing the ExecutableModule.  If the file backing the
-  /// ExecutableModule does not
-  /// contain a fork of this architecture, then this code will return false, and
-  /// the architecture
-  /// won't be changed.
-  /// If the input arch_spec is the same as the already set architecture, this
-  /// is a no-op.
+  /// ExecutableModule to that slice of the file backing the ExecutableModule.
+  /// If the file backing the ExecutableModule does not contain a fork of this
+  /// architecture, then this code will return false, and the architecture
+  /// won't be changed. If the input arch_spec is the same as the already set
+  /// architecture, this is a no-op.
   ///
   /// @param[in] arch_spec
   /// The new architecture.
   ///
+  /// @param[in] set_platform
+  /// If \b true, then the platform will be adjusted if the currently
+  /// selected platform is not compatible with the archicture being set.
+  /// If \b false, then just the architecture will be set even if the
+  /// currently selected platform isn't compatible (in case it might be
+  /// manually set following this function call).
+  ///
   /// @return
   /// \b true if the architecture was successfully set, \bfalse otherwise.
   //--
-  bool SetArchitecture(const ArchSpec &arch_spec);
+  bool SetArchitecture(const ArchSpec &arch_spec, bool set_platform = false);
 
   bool MergeArchitecture(const ArchSpec &arch_spec);
 
Index: lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
@@ -189,6 +189,161 @@
 stop_description = thread.GetStopDescription(256)
 self.assertEqual(stop_description, "")
 
+def check_register_unsigned(self, set, name, expected):
+reg_value = set.GetChildMemberWithName(name)
+self.assertTrue(reg_value.IsValid(),
+'Verify we have a register named "%s"' % (name))
+self.assertEqual(reg_value.GetValueAsUnsigned(), expected,
+ 'Verify "%s" == %i' % (name, expected))
+
+def check_register_string_value(self, set, name, expected, format):
+reg_value = set.GetChildMemberWithName(name)
+  

[Lldb-commits] [PATCH] D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2).

[Phabricator via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB339032: Add support for ARM and ARM64 breakpad generated 
minidump files (version 2). (authored by gclayton, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50336?vs=159324&id=159325#toc

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D50336

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/CMakeLists.txt
  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: include/lldb/Target/Target.h
===
--- include/lldb/Target/Target.h
+++ include/lldb/Target/Target.h
@@ -913,28 +913,30 @@
   /// Set the architecture for this target.
   ///
   /// If the current target has no Images read in, then this just sets the
-  /// architecture, which will
-  /// be used to select the architecture of the ExecutableModule when that is
-  /// set.
-  /// If the current target has an ExecutableModule, then calling
-  /// SetArchitecture with a different
+  /// architecture, which will be used to select the architecture of the
+  /// ExecutableModule when that is set. If the current target has an
+  /// ExecutableModule, then calling SetArchitecture with a different
   /// architecture from the currently selected one will reset the
-  /// ExecutableModule to that slice
-  /// of the file backing the ExecutableModule.  If the file backing the
-  /// ExecutableModule does not
-  /// contain a fork of this architecture, then this code will return false, and
-  /// the architecture
-  /// won't be changed.
-  /// If the input arch_spec is the same as the already set architecture, this
-  /// is a no-op.
+  /// ExecutableModule to that slice of the file backing the ExecutableModule.
+  /// If the file backing the ExecutableModule does not contain a fork of this
+  /// architecture, then this code will return false, and the architecture
+  /// won't be changed. If the input arch_spec is the same as the already set
+  /// architecture, this is a no-op.
   ///
   /// @param[in] arch_spec
   /// The new architecture.
   ///
+  /// @param[in] set_platform
+  /// If \b true, then the platform will be adjusted if the currently
+  /// selected platform is not compatible with the archicture being set.
+  /// If \b false, then just the architecture will be set even if the
+  /// currently selected platform isn't compatible (in case it might be
+  /// manually set following this function call).
+  ///
   /// @return
   /// \b true if the architecture was successfully set, \bfalse otherwise.
   //--
-  bool SetArchitecture(const ArchSpec &arch_spec);
+  bool SetArchitecture(const ArchSpec &arch_spec, bool set_platform = false);
 
   bool MergeArchitecture(const ArchSpec &arch_spec);
 
Index: packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
===
--- packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
+++ packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py
@@ -189,6 +189,161 @@
 stop_description = thread.GetStopDescription(256)
 self.assertEqual(stop_description, "")
 
+def check_register_unsigned(self, set, name, expected):
+reg_value = set.GetChildMemberWithName(name)
+self.assertTrue(reg_value.IsValid(),
+'Verify we have a register named "%s"' % (name))
+self.assertEqual(reg_value.GetValueAsUnsigned(), expected,
+ 'Verify "%s" == %i' % (name, expected))
+
+def check_register_string_value(self, set, name, expected, format):
+reg_value = set.GetChildMemberWithName(name)
+self.assertTrue(reg_value.IsValid(),
+'Verify we have a register named "%s"' % (name))
+if format is not None:
+reg_value.SetFormat(format)
+self.assertEqual(reg_value.GetValue(), expected,
+ 'Ve

[Lldb-commits] [lldb] r339033 - Fix offsetof usage that got lost when passing patches between linux and mac.

[Greg Clayton via lldb-commits]

Author: gclayton
Date: Mon Aug  6 10:07:50 2018
New Revision: 339033

URL: http://llvm.org/viewvc/llvm-project?rev=339033&view=rev
Log:
Fix offsetof usage that got lost when passing patches between linux and mac.


Modified:
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp

Modified: 
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp?rev=339033&r1=339032&r2=339033&view=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp 
(original)
+++ lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp 
Mon Aug  6 10:07:50 2018
@@ -31,33 +31,35 @@ using namespace minidump;
 
 #define DEF_R(i)   
\
   {
\
-"r" #i, nullptr, 4, OFFSET(r[i]), eEncodingUint, eFormatHex,   
\
+"r" #i, nullptr, 4, OFFSET(r) + i * 4, eEncodingUint, eFormatHex,  
\
 {INV, dwarf_r##i, INV, INV, reg_r##i}, nullptr, nullptr, nullptr, 0
\
   }
 
 #define DEF_R_ARG(i, n)
\
   {
\
-"r" #i, "arg" #n, 4, OFFSET(r[i]), eEncodingUint, eFormatHex,  
\
+"r" #i, "arg" #n, 4, OFFSET(r) + i * 4, eEncodingUint, eFormatHex, 
\
 {INV, dwarf_r##i, LLDB_REGNUM_GENERIC_ARG1 + i, INV, reg_r##i},
\
 nullptr, nullptr, nullptr, 0   
\
   }
 
 #define DEF_D(i)   
\
   {
\
-"d" #i, nullptr, 8, OFFSET(d[i]), eEncodingVector, eFormatVectorOfUInt8,   
\
-{INV, dwarf_d##i, INV, INV, reg_d##i}, nullptr, nullptr, nullptr, 0
\
+"d" #i, nullptr, 8, OFFSET(d) + i * 8, eEncodingVector,
\
+eFormatVectorOfUInt8, {INV, dwarf_d##i, INV, INV, reg_d##i},   
\
+nullptr, nullptr, nullptr, 0\
   }
 
 #define DEF_S(i)   
\
   {
\
-"s" #i, nullptr, 4, OFFSET(s[i]), eEncodingIEEE754, eFormatFloat,  
\
+"s" #i, nullptr, 4, OFFSET(s) + i * 4, eEncodingIEEE754, eFormatFloat, 
\
 {INV, dwarf_s##i, INV, INV, reg_s##i}, nullptr, nullptr, nullptr, 0
\
   }
 
 #define DEF_Q(i)   
\
   {
\
-"q" #i, nullptr, 16, OFFSET(q[i]), eEncodingVector, eFormatVectorOfUInt8,  
\
-{INV, dwarf_q##i, INV, INV, reg_q##i}, nullptr, nullptr, nullptr, 0
\
+"q" #i, nullptr, 16, OFFSET(q) + i * 16, eEncodingVector,  
\
+eFormatVectorOfUInt8, {INV, dwarf_q##i, INV, INV, reg_q##i},   
\
+nullptr, nullptr, nullptr, 0\
   }
 
 // Zero based LLDB register numbers for this register context

Modified: 
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp?rev=339033&r1=339032&r2=339033&view=diff
==
--- 
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp 
(original)
+++ 
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp 
Mon Aug  6 10:07:50 2018
@@ -30,48 +30,48 @@ using namespace minidump;
 
 #define DEF_X(i)   
\
   {
\
-"x" #i, nullptr, 8, OFFSET(x[i]), eEncodingUint, eFormatHex,   
\
+"x" #i, nullptr, 8, OFFSET(x) + i * 8, eEncodingUint, eFormatHex,  
\
 {INV, arm64_dwarf::x##i, INV, INV, reg_x##i}, nullptr, nullptr,
\
 nullptr, 0 
\
   }
 
 #define DEF_W(i)   
\
   {
\
-"w" #i, nullptr, 4, OFFSET(x[i]), eEncodingUint, eFormatHex,   
\
+"w" #i, nullptr, 4, OFFSET(x) + i * 8, eEncodingUint, eFormatHex,  
\
 {INV, INV, INV, INV, reg_w##i}, nullptr, nullptr, nullptr, 0   
\
   }
 
 #define DEF_X_ARG(i, n) 

[Lldb-commits] [PATCH] D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2).

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Fixed offsetof issues with:

$ svn commit
Sendingsource/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
Sendingsource/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp
Transmitting file data ..done
Committing transaction...
Committed revision 339033.


Repository:
  rL LLVM

https://reviews.llvm.org/D50336



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

[Lldb-commits] [PATCH] D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2).

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Passing patches between linux and mac the offsetof fixes got lost. When binary 
files are involved, patches are trickier to pass between to machines.


Repository:
  rL LLVM

https://reviews.llvm.org/D50336



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

[Lldb-commits] [PATCH] D50304: [lldb] CommandObjectThreadUntil should set breakpoint at either on exact or the nearest subsequent line number but not on all the subsequent line numbers

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This implementation forces an extra lookup when the line number slides.  For 
instance, if there were only one line match, but it was not exact, you're going 
to look up the exact address, fail, then look it up with exact = false.

Wouldn't it be more efficient to look with exact = false first, then reset the 
line you are looking for to whatever you got back, and continue with the 
exact=true search you do for the other line numbers.


https://reviews.llvm.org/D50304



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

[Lldb-commits] [lldb] r339034 - Fix more offsetof issues.

[Greg Clayton via lldb-commits]

Author: gclayton
Date: Mon Aug  6 10:26:53 2018
New Revision: 339034

URL: http://llvm.org/viewvc/llvm-project?rev=339034&view=rev
Log:
Fix more offsetof issues.


Modified:
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp

Modified: 
lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp?rev=339034&r1=339033&r2=339034&view=diff
==
--- lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp 
(original)
+++ lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp 
Mon Aug  6 10:26:53 2018
@@ -171,7 +171,7 @@ static RegisterInfo g_reg_info_apple_fp
 "fp",
 "r7",
 4,
-OFFSET(r[7]),
+OFFSET(r) + 7 * 4,
 eEncodingUint,
 eFormatHex,
 {INV, dwarf_r7, LLDB_REGNUM_GENERIC_FP, INV, reg_r7},
@@ -184,7 +184,7 @@ static RegisterInfo g_reg_info_fp = {
 "fp",
 "r11",
 4,
-OFFSET(r[11]),
+OFFSET(r) + 11 * 4,
 eEncodingUint,
 eFormatHex,
 {INV, dwarf_r11, LLDB_REGNUM_GENERIC_FP, INV, reg_r11},
@@ -211,7 +211,7 @@ static RegisterInfo g_reg_infos[] = {
 {"sp",
  "r13",
  4,
- OFFSET(r[13]),
+ OFFSET(r) + 13 * 4,
  eEncodingUint,
  eFormatHex,
  {INV, dwarf_sp, LLDB_REGNUM_GENERIC_SP, INV, reg_sp},
@@ -222,7 +222,7 @@ static RegisterInfo g_reg_infos[] = {
 {"lr",
  "r14",
  4,
- OFFSET(r[14]),
+ OFFSET(r) + 14 * 4,
  eEncodingUint,
  eFormatHex,
  {INV, dwarf_lr, LLDB_REGNUM_GENERIC_RA, INV, reg_lr},
@@ -233,7 +233,7 @@ static RegisterInfo g_reg_infos[] = {
 {"pc",
  "r15",
  4,
- OFFSET(r[15]),
+ OFFSET(r) + 15 * 4,
  eEncodingUint,
  eFormatHex,
  {INV, dwarf_pc, LLDB_REGNUM_GENERIC_PC, INV, reg_pc},


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

[Lldb-commits] [PATCH] D50336: Add support for ARM and ARM64 breakpad generated minidump files (version 2).

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

More offsetof issues:

$ svn commit
Sendingsource/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp
Transmitting file data .done
Committing transaction...
Committed revision 339034.


Repository:
  rL LLVM

https://reviews.llvm.org/D50336



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

[Lldb-commits] [PATCH] D50271: [IRMemoryMap] Shrink Allocation make it move-only (NFC)

[Vedant Kumar via Phabricator via lldb-commits]

vsk updated this revision to Diff 159339.
vsk retitled this revision from "[IRMemoryMap] Shrink Allocation by 
sizeof(addr_t) (NFC)" to "[IRMemoryMap] Shrink Allocation make it move-only 
(NFC)".
vsk edited the summary of this revision.
vsk added a comment.

- Make Allocation move-only. This should prevent a std::vector copy in ::Malloc.


https://reviews.llvm.org/D50271

Files:
  lldb/include/lldb/Expression/IRMemoryMap.h
  lldb/source/Expression/IRMemoryMap.cpp


Index: lldb/source/Expression/IRMemoryMap.cpp
===
--- lldb/source/Expression/IRMemoryMap.cpp
+++ lldb/source/Expression/IRMemoryMap.cpp
@@ -272,8 +272,8 @@
 uint32_t permissions, uint8_t alignment,
 AllocationPolicy policy)
 : m_process_alloc(process_alloc), m_process_start(process_start),
-  m_size(size), m_permissions(permissions), m_alignment(alignment),
-  m_policy(policy), m_leak(false) {
+  m_size(size), m_policy(policy), m_leak(false), 
m_permissions(permissions),
+  m_alignment(alignment) {
   switch (policy) {
   default:
 assert(0 && "We cannot reach this!");
@@ -393,9 +393,10 @@
   lldb::addr_t mask = alignment - 1;
   aligned_address = (allocation_address + mask) & (~mask);
 
-  m_allocations[aligned_address] =
-  Allocation(allocation_address, aligned_address, allocation_size,
- permissions, alignment, policy);
+  m_allocations.emplace(
+  std::piecewise_construct, std::forward_as_tuple(aligned_address),
+  std::forward_as_tuple(allocation_address, aligned_address,
+allocation_size, permissions, alignment, policy));
 
   if (zero_memory) {
 Status write_error;
Index: lldb/include/lldb/Expression/IRMemoryMap.h
===
--- lldb/include/lldb/Expression/IRMemoryMap.h
+++ lldb/include/lldb/Expression/IRMemoryMap.h
@@ -39,7 +39,7 @@
   IRMemoryMap(lldb::TargetSP target_sp);
   ~IRMemoryMap();
 
-  enum AllocationPolicy {
+  enum AllocationPolicy : uint8_t {
 eAllocationPolicyInvalid =
 0, ///< It is an error for an allocation to have this policy.
 eAllocationPolicyHostOnly, ///< This allocation was created in the host and
@@ -91,32 +91,32 @@
 private:
   struct Allocation {
 lldb::addr_t
-m_process_alloc; ///< The (unaligned) base for the remote allocation
+m_process_alloc; ///< The (unaligned) base for the remote allocation.
 lldb::addr_t
-m_process_start; ///< The base address of the allocation in the process
-size_t m_size;   ///< The size of the requested allocation
-uint32_t m_permissions; ///< The access permissions on the memory in the
-///process.  In the host, the memory is always
-///read/write.
-uint8_t m_alignment;///< The alignment of the requested allocation
+m_process_start; ///< The base address of the allocation in the 
process.
+size_t m_size;   ///< The size of the requested allocation.
 DataBufferHeap m_data;
 
-///< Flags
+/// Flags. Keep these grouped together to avoid structure padding.
 AllocationPolicy m_policy;
 bool m_leak;
+uint8_t m_permissions; ///< The access permissions on the memory in the
+   /// process. In the host, the memory is always
+   /// read/write.
+uint8_t m_alignment;   ///< The alignment of the requested allocation.
 
   public:
 Allocation(lldb::addr_t process_alloc, lldb::addr_t process_start,
size_t size, uint32_t permissions, uint8_t alignment,
AllocationPolicy m_policy);
 
-Allocation()
-: m_process_alloc(LLDB_INVALID_ADDRESS),
-  m_process_start(LLDB_INVALID_ADDRESS), m_size(0), m_permissions(0),
-  m_alignment(0), m_data(), m_policy(eAllocationPolicyInvalid),
-  m_leak(false) {}
+DISALLOW_COPY_AND_ASSIGN(Allocation);
   };
 
+  static_assert(sizeof(Allocation) <=
+(4 * sizeof(lldb::addr_t)) + sizeof(DataBufferHeap),
+"IRMemoryMap::Allocation is larger than expected");
+
   lldb::ProcessWP m_process_wp;
   lldb::TargetWP m_target_wp;
   typedef std::map AllocationMap;


Index: lldb/source/Expression/IRMemoryMap.cpp
===
--- lldb/source/Expression/IRMemoryMap.cpp
+++ lldb/source/Expression/IRMemoryMap.cpp
@@ -272,8 +272,8 @@
 uint32_t permissions, uint8_t alignment,
 AllocationPolicy policy)
 : m_process_alloc(process_alloc), m_process_start(process_start),
-  m_size(size), m_permissions(permissions), m_alignment(alignment),
-  m_policy(policy), m_leak(false) {
+  m_size(size), m_policy(policy), m_leak(false), m_permissions(permissions),
+

[Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In https://reviews.llvm.org/D49740#1188587, @labath wrote:

> In https://reviews.llvm.org/D49740#1188055, @jingham wrote:
>
> > First off, I'm fine with Zachary's suggestion that we let the dust settle a 
> > bit before we try to organize things better.  We'll probably do a better 
> > job then.
>
>
> Thanks. I agree this can wait, but I also do have some ideas below on what 
> could done differently right now for the Scalar and State classes. Let me 
> know if you think I should try that instead. (Otherwise, I'll just go ahead 
> with this.)


It doesn't seem like you're doing anything that is going to be hard to undo, so 
I'm fine with just doing this as you have it for now.  Maybe see if SafeMachO 
can go in Host, but that's the only one that seems easy and not requiring 
deeper thought.

> 
> 
>> But just to use these cases, for instance, Scalar is the base of how we 
>> realize values in the debugger (ValueObject lives on top of this).  It would 
>> be good if its landing place was such that if I was looking at how we 
>> represent values it was logically placed for that.
> 
> I think putting Scalar next to the ValueObject stuff would make sense, if the 
> ValueObjects were the only user of this class. However,  that isn't the case 
> right now. The reason I am touching this class in the first place is that it 
> is being used from within the RegisterValue class. This means it is more than 
> just a ValueObject helper, but more like a self-contained "utility" which can 
> be reused in different contexts.
> 
> Now, I am not actually sure whether using Scalar within RegisterValue is a 
> good idea (seems a bit overkill for that), but it seemed to work, so I didn't 
> want to disturb that (and I do believe that Scalar could be useful outside of 
> the ValueObject hierarchy). However, I can take another look at what it would 
> take to stop using Scalar in the context of RegisterValues, which would free 
> us up to move RegisterValue without touching the Scalar class.

I'm not sure how much of Scalar RegisterValue needs when it isn't being used to 
back a ValueObjectRegister.  Scalar is a pretty ambitious class, since it 
supports everything you would need to back DWARF expression manipulations.  
RegisterValue does get used in a bunch of places stand alone, though a lot of 
that just seems to be setting or dumping.

But I guess I'm looking for an organization based on "groups by functionality" 
rather than "who uses what".  So even though other actors might use Scalar, 
it's reason for being is to present values.  For the purposes of understanding 
the code, grouping all the files that work together for a task seems to me like 
it would help.  But again, this is more in the service of people newer to the 
code.

> 
> 
>> State is part of how we present the Process current state, so it seems like 
>> it should be grouped with other process related  entities.
> 
> State is a bit funny. It is currently used from both liblldb and lldb-server, 
> but these use different hierarchies for "process-related entities", so that's 
> why I'm moving it to a common place. However, I can certainly see a case for 
> LLGS and liblldb having different State enums. Sharing it doesn't buy us much 
> (ATM it's just the StateAsCString function), and using a different enum in 
> LLGS would have another benefit. Right now it only uses a subset of all State 
> values, so using an different enum would allow us to capture the possible 
> states more precisely. Doing that instead of moving State.h around should be 
> easy enough.
> 
>>   And RegisterValue belongs with the other parts of lldb that take apart the 
>> machine level state of a process.
> 
> I am not sure which parts are *you* thinking about here, but I think it would 
> be nice to have this class together with all the definitions of RegisterInfo 
> structs and associated constants. These are now mostly in 
> Plugin/Process/Utility, but so is a lot of other stuff. At one point I will 
> get around to worrying about those too, so maybe then we could move all of 
> these things to a separate module.
> 
>> It will probably go away, but FastDemangle really belongs around the 
>> language handling code.  I agree that Utility is an odd place for 
>> CompletionRequest...
>> 
>> SafeMachO is weird.  It gets used in Host - both common & macosx, and we're 
>> trying not to include out of Plugins so the MachO object file plugin 
>> directory doesn't seem right.  Maybe Host is a better place, it's reason for 
>> being is so you can include both llvm/Support/MachO.h and mach/machine.h, so 
>> even though it's not directly a host functionality it look a bit like that.  
>> Not sure.
> 
> I think Host makes sense. None of the other code (except the NativeProcessXXX 
> classes, I guess) should really be including OS-specific stuff, so there 
> shouldn't be a need (in an ideal world, I don't know how far we are from it 
> right now) for this header anywhere exce

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

[Neal via Phabricator via lldb-commits]

nealsid added a comment.

In https://reviews.llvm.org/D49963#1183036, @clayborg wrote:

> 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.


Nice - TBH, I haven't used LLDB in awhile so having the prompt displayed while 
the target is running wasn't on my list of test cases, but I'll definitely add 
it.  
Perhaps there could also be an indicator like '*' in the prompt when the 
process is currently running so the user will know it's potentially out of date.


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] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 159368.
lemo added a comment.

Incorporating feedback, thanks.


https://reviews.llvm.org/D50274

Files:
  source/Core/Module.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -5842,7 +5842,7 @@
   // that loaded.
 
   // Iterate over a copy of this language runtime list in case the language
-  // runtime ModulesDidLoad somehow causes the language riuntime to be
+  // runtime ModulesDidLoad somehow causes the language runtime to be
   // unloaded.
   LanguageRuntimeCollection language_runtimes(m_language_runtimes);
   for (const auto &pair : language_runtimes) {
@@ -6095,15 +6095,15 @@
   // For each StructuredDataPlugin, if the plugin handles any of the types in
   // the supported_type_names, map that type name to that plugin. Stop when
   // we've consumed all the type names.
-  // FIXME: should we return an error if there are type names nobody 
+  // FIXME: should we return an error if there are type names nobody
   // supports?
   for (uint32_t plugin_index = 0; !const_type_names.empty(); plugin_index++) {
 auto create_instance =
PluginManager::GetStructuredDataPluginCreateCallbackAtIndex(
plugin_index);
 if (!create_instance)
   break;
-  
+
 // Create the plugin.
 StructuredDataPluginSP plugin_sp = (*create_instance)(*this);
 if (!plugin_sp) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -357,6 +357,12 @@
   // This enables most LLDB functionality involving address-to-module
   // translations (ex. identifing the module for a stack frame PC) and
   // modules/sections commands (ex. target modules list, ...)
+  if (log) {
+log->Printf("Unable to locate the matching object file, creating a "
+"placeholder module for: %s",
+name.getValue().c_str());
+  }
+
   auto placeholder_module =
   std::make_shared(module_spec);
   placeholder_module->CreateImageSection(module, GetTarget());
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1390,7 +1390,7 @@
 arch_spec.GetTriple().getOS() == llvm::Triple::OSType::UnknownOS)
   // In case of MIPSR6, the LLDB_NT_OWNER_GNU note is missing for some
   // cases (e.g. compile with -nostdlib) Hence set OS to Linux
-  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux); 
+  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux);
   }
 }
 
@@ -1494,7 +1494,7 @@
 const uint32_t sub_type = subTypeFromElfHeader(header);
 arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type,
   header.e_ident[EI_OSABI]);
-
+
 // Validate if it is ok to remove GetOsFromOSABI. Note, that now the OS is
 // determined based on EI_OSABI flag and the info extracted from ELF notes
 // (see RefineModuleDetailsFromNote). However in some cases that still
@@ -3385,8 +3385,6 @@
   if (section->GetObjectFile() != this)
 return section->GetObjectFile()->ReadSectionData(section, section_data);
 
-  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES);
-
   size_t result = ObjectFile::ReadSectionData(section, section_data);
   if (result == 0 || !section->Test(SHF_COMPRESSED))
 return result;
@@ -3397,20 +3395,25 @@
size_t(section_data.GetByteSize())},
   GetByteOrder() == eByteOrderLittle, GetAddressByteSize() == 8);
   if (!Decompressor) {
-LLDB_LOG_ERROR(log, Decompressor.takeError(),
-   "Unable to initialize decompressor for section {0}",
-   section->GetName());
-return result;
+GetModule()->ReportWarning(
+"Unable to initialize decompressor for section '%s': %s",
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;
   }
+
   auto buffer_sp =
   std::make_shared(Decompressor->getDecompressedSize(), 0);
   if (auto Error = Decompressor->decompress(
   {reinterpret_cast(buffer_sp->GetBytes()),
size_t(buffer_sp->GetByteSize())})) {
-LLDB_LOG_ERROR(log, std::move(Error), "Decompression of section {0} failed",
-   section->GetName());
-return result;
+GetModule()->ReportWarning(
+"Decompression of section '%s' failed: %s",
+section->GetName().GetCString(),
+llvm::toStrin

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked 2 inline comments as done.
lemo added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString());
+return 0;
   }

labath wrote:
> `lit/Modules/compressed-sections.yaml` test will need to be updated to 
> account for the return 0.
compressed-sections.yaml seems to be gated by REQUIRES: zlib, so it would not 
hit the 0-length path, am I wrong?


https://reviews.llvm.org/D50274



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

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString());
+return 0;
   }

lemo wrote:
> labath wrote:
> > `lit/Modules/compressed-sections.yaml` test will need to be updated to 
> > account for the return 0.
> compressed-sections.yaml seems to be gated by REQUIRES: zlib, so it would not 
> hit the 0-length path, am I wrong?
The `.bogus` section in that test deliberately contains malformed data, so you 
should still hit the first error case.

However, looking at the lldb-test implementation, it just ignores the 
`ReadSectionData` return value, and relies on the size in the DataExtractor 
instead (one day I'll declare war on functions of this type), which is probably 
why you hadn't noticed this.

I guess in this case, it would be nice to reset the data extractor before 
returning 0 and possibly teaching lldb-test to report discrepancies in the 
returned sizes.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;

This needs to be `std::move(Error)`. If you built with asserts enabled and hit 
this line, you would crash because you did not consume the `Error` object.


https://reviews.llvm.org/D50274



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

[Lldb-commits] [lldb] r339051 - [IRMemoryMap] Avoid redundant zero-init in the Allocation constructor (NFC)

[Vedant Kumar via lldb-commits]

Author: vedantk
Date: Mon Aug  6 13:13:52 2018
New Revision: 339051

URL: http://llvm.org/viewvc/llvm-project?rev=339051&view=rev
Log:
[IRMemoryMap] Avoid redundant zero-init in the Allocation constructor (NFC)

In the lldb-bench/arithmetic benchmark, 1.7% of the total running time
is spent zero-initializing a std::vector that has already been zeroed.

Modified:
lldb/trunk/include/lldb/Utility/DataBufferHeap.h
lldb/trunk/source/Expression/IRMemoryMap.cpp

Modified: lldb/trunk/include/lldb/Utility/DataBufferHeap.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/DataBufferHeap.h?rev=339051&r1=339050&r2=339051&view=diff
==
--- lldb/trunk/include/lldb/Utility/DataBufferHeap.h (original)
+++ lldb/trunk/include/lldb/Utility/DataBufferHeap.h Mon Aug  6 13:13:52 2018
@@ -90,7 +90,8 @@ public:
   /// Set the number of bytes in the data buffer.
   ///
   /// Sets the number of bytes that this object should be able to contain.
-  /// This can be used prior to copying data into the buffer.
+  /// This can be used prior to copying data into the buffer. Note that this
+  /// zero-initializes up to \p byte_size bytes.
   ///
   /// @param[in] byte_size
   /// The new size in bytes that this data buffer should attempt

Modified: lldb/trunk/source/Expression/IRMemoryMap.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Expression/IRMemoryMap.cpp?rev=339051&r1=339050&r2=339051&view=diff
==
--- lldb/trunk/source/Expression/IRMemoryMap.cpp (original)
+++ lldb/trunk/source/Expression/IRMemoryMap.cpp Mon Aug  6 13:13:52 2018
@@ -278,15 +278,11 @@ IRMemoryMap::Allocation::Allocation(lldb
   default:
 assert(0 && "We cannot reach this!");
   case eAllocationPolicyHostOnly:
+  case eAllocationPolicyMirror:
 m_data.SetByteSize(size);
-memset(m_data.GetBytes(), 0, size);
 break;
   case eAllocationPolicyProcessOnly:
 break;
-  case eAllocationPolicyMirror:
-m_data.SetByteSize(size);
-memset(m_data.GetBytes(), 0, size);
-break;
   }
 }
 


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

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;

labath wrote:
> This needs to be `std::move(Error)`. If you built with asserts enabled and 
> hit this line, you would crash because you did not consume the `Error` object.
Can you please elaborate? std::move is just a cast (and the result of 
Error::takeValue() is already an rvalue - the error object has been already 
moved into a temporary Error instance)


https://reviews.llvm.org/D50274



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

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;

lemo wrote:
> labath wrote:
> > This needs to be `std::move(Error)`. If you built with asserts enabled and 
> > hit this line, you would crash because you did not consume the `Error` 
> > object.
> Can you please elaborate? std::move is just a cast (and the result of 
> Error::takeValue() is already an rvalue - the error object has been already 
> moved into a temporary Error instance)
The llvm manual 
 says
```
All Error instances, whether success or failure, must be either checked or 
moved from (via std::move or a return) before they are destructed. Accidentally 
discarding an unchecked error will cause a program abort at the point where the 
unchecked value’s destructor is run, making it easy to identify and fix 
violations of this rule.
...
Success values are considered checked once they have been tested (by invoking 
the boolean conversion operator).
...
Failure values are considered checked once a handler for the error type has 
been activated.
```
The error object created on line 3407 (in the if-declaration) is neither moved 
from nor has it's handler invoked. You only invoke it's bool operator, which is 
not enough for it to be considered "checked" if it is in the "failure" state. 
This means it will assert once it's destructor is executed. By writing 
`llvm::toString(std::move(Error))`, you will "move" from the object, thereby 
clearing it. (It also makes sense to print out the error that you have just 
checked instead of some error from a previous step.)

Try this pattern out on a toy program to see what happens:
```
if (Error E = make_error("This is an error", 
inconvertibleErrorCode()))
  outs() << "I encountered an error but I am not handling it";
```


https://reviews.llvm.org/D50274



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

[Lldb-commits] [PATCH] D50362: Add instructions for building LLDB on Mac OS X with CMake

[Alex Langford via Phabricator via lldb-commits]

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

Reducing redundancy and changing some wording around.


https://reviews.llvm.org/D50362

Files:
  www/build.html


Index: www/build.html
===
--- www/build.html
+++ www/build.html
@@ -143,21 +143,42 @@
 
   Building LLDB on Mac OS X
   
-Building on Mac OS X is as easy as downloading the code and 
building the Xcode project or workspace:
+ There are two ways to build LLDB on Mac OS X: Using Xcode and 
using CMake
   
   
 Preliminaries
 
   XCode 4.3 or newer requires the "Command Line Tools" 
component (XCode->Preferences->Downloads->Components).
   Mac OS X Lion or newer requires installing http://swig.org";>Swig.
 
-Building LLDB
+Building LLDB with Xcode
+Building on Mac OS X with Xcode is as easy as downloading the 
code and building the Xcode project or workspace:
 
   Download the lldb sources.
   Follow the code signing instructions in 
lldb/docs/code-signing.txt
   In Xcode 3.x: lldb/lldb.xcodeproj, select the 
lldb-tool target, and build.
   In Xcode 4.x: lldb/lldb.xcworkspace, select the 
lldb-tool scheme, and build.
 
+Building LLDB with CMake
+ First download the LLVM, Clang, and LLDB sources. Refer to this page for precise instructions on this step.
+ Refer to the code signing instructions in 
lldb/docs/code-signing.txt for info on codesigning debugserver during 
the build.
+ Using CMake is documented on the http://llvm.org/docs/CMake.html";>Building LLVM with CMake page.
+Ninja is the recommended generator to use when building LLDB with 
CMake.
+
+  > cmake $PATH_TO_LLVM -G Ninja
+  > ninja lldb
+
+
+   As noted in the "Building LLVM with CMake" page mentioned 
above, you can pass
+   variables to cmake to change build behavior. If LLDB is built 
as a part of LLVM,
+   then you can pass LLVM-specific CMake variables to cmake when 
building LLDB.
+
+Here are some commonly used LLDB-specific CMake variables:
+
+  LLDB_EXPORT_ALL_SYMBOLS:BOOL : Exports 
all symbols. Useful in conjunction with CMAKE_BUILD_TYPE=Debug.
+  LLDB_BUILD_FRAMEWORK:BOOL : Builds 
LLDB.framework as Xcode would
+  LLDB_CODESIGN_IDENTITY:STRING : Skips 
building debugserver and therefore avoids codesigning anything.
+
   
   
 


Index: www/build.html
===
--- www/build.html
+++ www/build.html
@@ -143,21 +143,42 @@
 
   Building LLDB on Mac OS X
   
-Building on Mac OS X is as easy as downloading the code and building the Xcode project or workspace:
+ There are two ways to build LLDB on Mac OS X: Using Xcode and using CMake
   
   
 Preliminaries
 
   XCode 4.3 or newer requires the "Command Line Tools" component (XCode->Preferences->Downloads->Components).
   Mac OS X Lion or newer requires installing http://swig.org";>Swig.
 
-Building LLDB
+Building LLDB with Xcode
+Building on Mac OS X with Xcode is as easy as downloading the code and building the Xcode project or workspace:
 
   Download the lldb sources.
   Follow the code signing instructions in lldb/docs/code-signing.txt
   In Xcode 3.x: lldb/lldb.xcodeproj, select the lldb-tool target, and build.
   In Xcode 4.x: lldb/lldb.xcworkspace, select the lldb-tool scheme, and build.
 
+Building LLDB with CMake
+ First download the LLVM, Clang, and LLDB sources. Refer to this page for precise instructions on this step.
+ Refer to the code signing instructions in lldb/docs/code-signing.txt for info on codesigning debugserver during the build.
+ Using CMake is documented on the http://llvm.org/docs/CMake.html";>Building LLVM with CMake page.
+Ninja is the recommended generator to use when building LLDB with CMake.
+
+  > cmake $PATH_TO_LLVM -G Ninja
+  > ninja lldb
+
+
+   As noted in the "Building LLVM with CMake" page mentioned above, you can pass
+   variables to cmake to change build behavior. If LLDB is built as a part of LLVM,
+   then you can pass LLVM-specific CMake variables to cmake when building LLDB.
+
+Here are some commonly used LLDB-specific CMake variables:
+
+  

[Lldb-commits] [lldb] r339068 - Add a relocation for R_AARCH64_ABS32 in ObjectFileELF

[Stephane Sezer via lldb-commits]

Author: sas
Date: Mon Aug  6 15:04:08 2018
New Revision: 339068

URL: http://llvm.org/viewvc/llvm-project?rev=339068&view=rev
Log:
Add a relocation for R_AARCH64_ABS32 in ObjectFileELF

Summary:
.rela.debug_info relocations are being done via
ObjectFileELF::ApplyRelocations for aarch64. Currently, the switch case
that iterates over the relocation type is only implemented for a few
different types and `assert(false)`es over the rest.

Implement the relocation for R_AARCH64_ABS32 in ApplyRelocations

Reviewers: sas, xiaobai, peter.smith, clayborg, javed.absar, espindola

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

Change by Nathan Lanza 

Modified:
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

Modified: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp?rev=339068&r1=339067&r2=339068&view=diff
==
--- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (original)
+++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp Mon Aug  6 
15:04:08 2018
@@ -2697,15 +2697,20 @@ unsigned ObjectFileELF::ApplyRelocations
 break;
   }
   case R_X86_64_32:
-  case R_X86_64_32S: {
+  case R_X86_64_32S:
+  case R_AARCH64_ABS32: {
 symbol = symtab->FindSymbolByID(reloc_symbol(rel));
 if (symbol) {
   addr_t value = symbol->GetAddressRef().GetFileAddress();
   value += ELFRelocation::RelocAddend32(rel);
-  assert(
-  (reloc_type(rel) == R_X86_64_32 && (value <= UINT32_MAX)) ||
-  (reloc_type(rel) == R_X86_64_32S &&
-   ((int64_t)value <= INT32_MAX && (int64_t)value >= INT32_MIN)));
+  if (!((IsRelocABS32(rel) && value <= UINT32_MAX) ||
+(reloc_type(rel) == R_X86_64_32S &&
+ ((int64_t)value <= INT32_MAX &&
+  (int64_t)value >= INT32_MIN {
+Log *log =
+lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES);
+log->Printf("Failed to apply debug info relocations");
+  }
   uint32_t truncated_addr = (value & 0x);
   DataBufferSP &data_buffer_sp = debug_data.GetSharedDataBuffer();
   uint32_t *dst = reinterpret_cast(


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

Re: [Lldb-commits] [lldb] r339068 - Add a relocation for R_AARCH64_ABS32 in ObjectFileELF

[Davide Italiano via lldb-commits]

You should be able to add a unittest for this, I think?

--
Davide
On Mon, Aug 6, 2018 at 3:04 PM Stephane Sezer via lldb-commits
 wrote:
>
> Author: sas
> Date: Mon Aug  6 15:04:08 2018
> New Revision: 339068
>
> URL: http://llvm.org/viewvc/llvm-project?rev=339068&view=rev
> Log:
> Add a relocation for R_AARCH64_ABS32 in ObjectFileELF
>
> Summary:
> .rela.debug_info relocations are being done via
> ObjectFileELF::ApplyRelocations for aarch64. Currently, the switch case
> that iterates over the relocation type is only implemented for a few
> different types and `assert(false)`es over the rest.
>
> Implement the relocation for R_AARCH64_ABS32 in ApplyRelocations
>
> Reviewers: sas, xiaobai, peter.smith, clayborg, javed.absar, espindola
>
> Differential Revision: https://reviews.llvm.org/D49407
>
> Change by Nathan Lanza 
>
> Modified:
> lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
>
> Modified: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp?rev=339068&r1=339067&r2=339068&view=diff
> ==
> --- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (original)
> +++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp Mon Aug  6 
> 15:04:08 2018
> @@ -2697,15 +2697,20 @@ unsigned ObjectFileELF::ApplyRelocations
>  break;
>}
>case R_X86_64_32:
> -  case R_X86_64_32S: {
> +  case R_X86_64_32S:
> +  case R_AARCH64_ABS32: {
>  symbol = symtab->FindSymbolByID(reloc_symbol(rel));
>  if (symbol) {
>addr_t value = symbol->GetAddressRef().GetFileAddress();
>value += ELFRelocation::RelocAddend32(rel);
> -  assert(
> -  (reloc_type(rel) == R_X86_64_32 && (value <= UINT32_MAX)) ||
> -  (reloc_type(rel) == R_X86_64_32S &&
> -   ((int64_t)value <= INT32_MAX && (int64_t)value >= 
> INT32_MIN)));
> +  if (!((IsRelocABS32(rel) && value <= UINT32_MAX) ||
> +(reloc_type(rel) == R_X86_64_32S &&
> + ((int64_t)value <= INT32_MAX &&
> +  (int64_t)value >= INT32_MIN {
> +Log *log =
> +lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES);
> +log->Printf("Failed to apply debug info relocations");
> +  }
>uint32_t truncated_addr = (value & 0x);
>DataBufferSP &data_buffer_sp = debug_data.GetSharedDataBuffer();
>uint32_t *dst = reinterpret_cast(
>
>
> ___
> lldb-commits mailing list
> lldb-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] r339071 - Revert "Add a relocation for R_AARCH64_ABS32 in ObjectFileELF"

[Stephane Sezer via lldb-commits]

Author: sas
Date: Mon Aug  6 15:21:28 2018
New Revision: 339071

URL: http://llvm.org/viewvc/llvm-project?rev=339071&view=rev
Log:
Revert "Add a relocation for R_AARCH64_ABS32 in ObjectFileELF"

This reverts commit f055ce7eb893cd0d17ebcfd4125018f46f983aff.

Modified:
lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

Modified: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp?rev=339071&r1=339070&r2=339071&view=diff
==
--- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp (original)
+++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp Mon Aug  6 
15:21:28 2018
@@ -2697,20 +2697,15 @@ unsigned ObjectFileELF::ApplyRelocations
 break;
   }
   case R_X86_64_32:
-  case R_X86_64_32S:
-  case R_AARCH64_ABS32: {
+  case R_X86_64_32S: {
 symbol = symtab->FindSymbolByID(reloc_symbol(rel));
 if (symbol) {
   addr_t value = symbol->GetAddressRef().GetFileAddress();
   value += ELFRelocation::RelocAddend32(rel);
-  if (!((IsRelocABS32(rel) && value <= UINT32_MAX) ||
-(reloc_type(rel) == R_X86_64_32S &&
- ((int64_t)value <= INT32_MAX &&
-  (int64_t)value >= INT32_MIN {
-Log *log =
-lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES);
-log->Printf("Failed to apply debug info relocations");
-  }
+  assert(
+  (reloc_type(rel) == R_X86_64_32 && (value <= UINT32_MAX)) ||
+  (reloc_type(rel) == R_X86_64_32S &&
+   ((int64_t)value <= INT32_MAX && (int64_t)value >= INT32_MIN)));
   uint32_t truncated_addr = (value & 0x);
   DataBufferSP &data_buffer_sp = debug_data.GetSharedDataBuffer();
   uint32_t *dst = reinterpret_cast(


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

[Lldb-commits] [lldb] r339076 - [lit, python] Always add quotes around the python path in lit

[Stella Stamenova via lldb-commits]

Author: stella.stamenova
Date: Mon Aug  6 15:37:53 2018
New Revision: 339076

URL: http://llvm.org/viewvc/llvm-project?rev=339076&view=rev
Log:
[lit, python] Always add quotes around the python path in lit

Summary:
The issue with the python path is that the path to python on Windows can 
contain spaces. To make the tests always work, the path to python needs to be 
surrounded by quotes.

This is a companion change to: https://reviews.llvm.org/D50206

Reviewers: asmith, zturner

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

Modified:
lldb/trunk/lit/lit.cfg

Modified: lldb/trunk/lit/lit.cfg
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/lit.cfg?rev=339076&r1=339075&r2=339076&view=diff
==
--- lldb/trunk/lit/lit.cfg (original)
+++ lldb/trunk/lit/lit.cfg Mon Aug  6 15:37:53 2018
@@ -54,7 +54,7 @@ config.environment['LLVM_SRC_ROOT'] = ge
 config.environment['PYTHON_EXECUTABLE'] = getattr(config, 'python_executable', 
'')
 
 # Register substitutions
-config.substitutions.append(('%python', config.python_executable))
+config.substitutions.append(('%python', "'%s'" % (config.python_executable)))
 
 debugserver = lit.util.which('debugserver', lldb_tools_dir)
 lldb = "%s -S %s/lit-lldb-init" % (lit.util.which('lldb', lldb_tools_dir),


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

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

Can't speak much to the contents yet (Haven't done a thorough pass yet) but it 
looks like there's a lot of dead code you might want to remove. I commented on 
a few of them but there is probably more.




Comment at: 
packages/Python/lldbsuite/test/tools/lldb-vscode/variables/TestVSCode_variables.py:15
+
+import pprint #  TODO: REMOVE THIS PRIOR TO CHECKIN
+

Should remove this



Comment at: 
packages/Python/lldbsuite/test/tools/lldb-vscode/variables/TestVSCode_variables.py:88
+source = 'main.cpp'
+# source_path = os.path.join(os.getcwd(), source)
+breakpoint1_line = line_number(source, '// breakpoint 1')

Commented out line



Comment at: packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py:82-86
+# try:
+# return json.loads(json_str)
+# except:
+# dump_memory(0, json_str, 32, sys.stdout)
+# sys.exit(1)

Dead code



Comment at: packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py:881
+if response['success']:
+# dbg.request_setBreakpoints(source, [39])
+if options.sourceBreakpoints:

Dead code



Comment at: packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py:901-923
+# thread_infos = dbg.get_threads()
+# for thread_info in thread_infos:
+# tid = thread_info['id']
+# dbg.request_stackTrace(tid)
+# dbg.request_stackTrace(stopped_events[0]['body']['threadId'])
+# dbg.request_stackTrace(stopped_events[0]['body']['threadId'],
+#  startFrame=0)

Dead code


https://reviews.llvm.org/D50365



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

[Lldb-commits] [PATCH] D48465: Added initial code completion support for the `expr` command

[Raphael Isemann via Phabricator via lldb-commits]

teemperor updated this revision to Diff 159426.
teemperor added a comment.

- Resolved merge conflicts.


https://reviews.llvm.org/D48465

Files:
  include/lldb/Expression/ExpressionParser.h
  include/lldb/Expression/UserExpression.h
  packages/Python/lldbsuite/test/expression_command/completion/.categories
  packages/Python/lldbsuite/test/expression_command/completion/Makefile
  
packages/Python/lldbsuite/test/expression_command/completion/TestExprCompletion.py
  packages/Python/lldbsuite/test/expression_command/completion/main.cpp
  packages/Python/lldbsuite/test/expression_command/completion/other.cpp
  packages/Python/lldbsuite/test/functionalities/completion/TestCompletion.py
  packages/Python/lldbsuite/test/lldbtest.py
  source/Commands/CommandObjectExpression.cpp
  source/Commands/CommandObjectExpression.h
  source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp
  source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h
  source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
  source/Plugins/ExpressionParser/Clang/ClangUserExpression.h

Index: source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
===
--- source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
+++ source/Plugins/ExpressionParser/Clang/ClangUserExpression.h
@@ -143,6 +143,9 @@
  lldb_private::ExecutionPolicy execution_policy,
  bool keep_result_in_memory, bool generate_debug_info) override;
 
+  bool Complete(ExecutionContext &exe_ctx, StringList &matches,
+unsigned complete_pos) override;
+
   ExpressionTypeSystemHelper *GetTypeSystemHelper() override {
 return &m_type_system_helper;
   }
@@ -198,6 +201,10 @@
 lldb::TargetSP m_target_sp;
   };
 
+  /// The absolute character position in the transformed source code where the
+  /// user code (as typed by the user) starts. If the variable is empty, then we
+  /// were not able to calculate this position.
+  llvm::Optional m_user_expression_start_pos;
   ResultDelegate m_result_delegate;
 };
 
Index: source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
===
--- source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -402,6 +402,16 @@
"couldn't construct expression body");
   return llvm::Optional();
 }
+
+// Find and store the start position of the original code inside the
+// transformed code. We need this later for the code completion.
+std::size_t original_start;
+std::size_t original_end;
+bool found_bounds = source_code->GetOriginalBodyBounds(
+m_transformed_text, lang_type, original_start, original_end);
+if (found_bounds) {
+  m_user_expression_start_pos = original_start;
+}
   }
   return lang_type;
 }
@@ -591,6 +601,119 @@
   return true;
 }
 
+//--
+/// Converts an absolute position inside a given code string into
+/// a column/line pair.
+///
+/// @param[in] abs_pos
+/// A absolute position in the code string that we want to convert
+/// to a column/line pair.
+///
+/// @param[in] code
+/// A multi-line string usually representing source code.
+///
+/// @param[out] line
+/// The line in the code that contains the given absolute position.
+/// The first line in the string is indexed as 1.
+///
+/// @param[out] column
+/// The column in the line that contains the absolute position.
+/// The first character in a line is indexed as 0.
+//--
+static void AbsPosToLineColumnPos(unsigned abs_pos, llvm::StringRef code,
+  unsigned &line, unsigned &column) {
+  // Reset to code position to beginning of the file.
+  line = 0;
+  column = 0;
+
+  assert(abs_pos <= code.size() && "Absolute position outside code string?");
+
+  // We have to walk up to the position and count lines/columns.
+  for (std::size_t i = 0; i < abs_pos; ++i) {
+// If we hit a line break, we go back to column 0 and enter a new line.
+// We only handle \n because that's what we internally use to make new
+// lines for our temporary code strings.
+if (code[i] == '\n') {
+  ++line;
+  column = 0;
+  continue;
+}
+++column;
+  }
+}
+
+bool ClangUserExpression::Complete(ExecutionContext &exe_ctx,
+   StringList &matches, unsigned complete_pos) {
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+
+  // We don't want any visible feedback when completing an expression. Mostly
+  // because the results we get from an incomplete invocation are probably not
+  // correct.
+  DiagnosticManager diagnostic_manager;
+

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 159432.
lemo marked an inline comment as done.
lemo added a comment.

Updated the LIT file too


https://reviews.llvm.org/D50274

Files:
  lit/Modules/compressed-sections.yaml
  source/Core/Module.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -5842,7 +5842,7 @@
   // that loaded.
 
   // Iterate over a copy of this language runtime list in case the language
-  // runtime ModulesDidLoad somehow causes the language riuntime to be
+  // runtime ModulesDidLoad somehow causes the language runtime to be
   // unloaded.
   LanguageRuntimeCollection language_runtimes(m_language_runtimes);
   for (const auto &pair : language_runtimes) {
@@ -6095,15 +6095,15 @@
   // For each StructuredDataPlugin, if the plugin handles any of the types in
   // the supported_type_names, map that type name to that plugin. Stop when
   // we've consumed all the type names.
-  // FIXME: should we return an error if there are type names nobody 
+  // FIXME: should we return an error if there are type names nobody
   // supports?
   for (uint32_t plugin_index = 0; !const_type_names.empty(); plugin_index++) {
 auto create_instance =
PluginManager::GetStructuredDataPluginCreateCallbackAtIndex(
plugin_index);
 if (!create_instance)
   break;
-  
+
 // Create the plugin.
 StructuredDataPluginSP plugin_sp = (*create_instance)(*this);
 if (!plugin_sp) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -357,6 +357,12 @@
   // This enables most LLDB functionality involving address-to-module
   // translations (ex. identifing the module for a stack frame PC) and
   // modules/sections commands (ex. target modules list, ...)
+  if (log) {
+log->Printf("Unable to locate the matching object file, creating a "
+"placeholder module for: %s",
+name.getValue().c_str());
+  }
+
   auto placeholder_module =
   std::make_shared(module_spec);
   placeholder_module->CreateImageSection(module, GetTarget());
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1390,7 +1390,7 @@
 arch_spec.GetTriple().getOS() == llvm::Triple::OSType::UnknownOS)
   // In case of MIPSR6, the LLDB_NT_OWNER_GNU note is missing for some
   // cases (e.g. compile with -nostdlib) Hence set OS to Linux
-  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux); 
+  arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux);
   }
 }
 
@@ -1494,7 +1494,7 @@
 const uint32_t sub_type = subTypeFromElfHeader(header);
 arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type,
   header.e_ident[EI_OSABI]);
-
+
 // Validate if it is ok to remove GetOsFromOSABI. Note, that now the OS is
 // determined based on EI_OSABI flag and the info extracted from ELF notes
 // (see RefineModuleDetailsFromNote). However in some cases that still
@@ -3385,8 +3385,6 @@
   if (section->GetObjectFile() != this)
 return section->GetObjectFile()->ReadSectionData(section, section_data);
 
-  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES);
-
   size_t result = ObjectFile::ReadSectionData(section, section_data);
   if (result == 0 || !section->Test(SHF_COMPRESSED))
 return result;
@@ -3397,20 +3395,27 @@
size_t(section_data.GetByteSize())},
   GetByteOrder() == eByteOrderLittle, GetAddressByteSize() == 8);
   if (!Decompressor) {
-LLDB_LOG_ERROR(log, Decompressor.takeError(),
-   "Unable to initialize decompressor for section {0}",
-   section->GetName());
-return result;
+GetModule()->ReportWarning(
+"Unable to initialize decompressor for section '%s': %s",
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+section_data.Clear();
+return 0;
   }
+
   auto buffer_sp =
   std::make_shared(Decompressor->getDecompressedSize(), 0);
-  if (auto Error = Decompressor->decompress(
+  if (auto error = Decompressor->decompress(
   {reinterpret_cast(buffer_sp->GetBytes()),
size_t(buffer_sp->GetByteSize())})) {
-LLDB_LOG_ERROR(log, std::move(Error), "Decompression of section {0} failed",
-   section->GetName());
-return result;
+G

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;

labath wrote:
> lemo wrote:
> > labath wrote:
> > > This needs to be `std::move(Error)`. If you built with asserts enabled 
> > > and hit this line, you would crash because you did not consume the 
> > > `Error` object.
> > Can you please elaborate? std::move is just a cast (and the result of 
> > Error::takeValue() is already an rvalue - the error object has been already 
> > moved into a temporary Error instance)
> The llvm manual 
>  says
> ```
> All Error instances, whether success or failure, must be either checked or 
> moved from (via std::move or a return) before they are destructed. 
> Accidentally discarding an unchecked error will cause a program abort at the 
> point where the unchecked value’s destructor is run, making it easy to 
> identify and fix violations of this rule.
> ...
> Success values are considered checked once they have been tested (by invoking 
> the boolean conversion operator).
> ...
> Failure values are considered checked once a handler for the error type has 
> been activated.
> ```
> The error object created on line 3407 (in the if-declaration) is neither 
> moved from nor has it's handler invoked. You only invoke it's bool operator, 
> which is not enough for it to be considered "checked" if it is in the 
> "failure" state. This means it will assert once it's destructor is executed. 
> By writing `llvm::toString(std::move(Error))`, you will "move" from the 
> object, thereby clearing it. (It also makes sense to print out the error that 
> you have just checked instead of some error from a previous step.)
> 
> Try this pattern out on a toy program to see what happens:
> ```
> if (Error E = make_error("This is an error", 
> inconvertibleErrorCode()))
>   outs() << "I encountered an error but I am not handling it";
> ```
Thanks. I see, I was looking at the previous block. 

> By writing llvm::toString(std::move(Error)), you will "move" from the object, 
> thereby clearing it.

It's a nice contract, although the "move" part was not the problem nor the 
solution in itself (I took a close look at the Error class, it doesn't matter 
how much you move it, someone has to eventually call handleErrors() on it. 
Conveniently, llvm::toString() does it)


https://reviews.llvm.org/D50274



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