[Lldb-commits] [PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:1755-1759
+  QualType FromTy = ArrayFrom->getElementType();
+  QualType ToTy = ArrayTo->getElementType();
+
+  FromRecordDecl = FromTy->getAsRecordDecl();
+  ToRecordDecl = ToTy->getAsRecordDecl();

What about 2- or n-dimensional arrays?



Comment at: 
lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp:22-27
+  union {
+struct {
+  unsigned char *_s;
+} t;
+char *tt[1];
+  } U;

What's the significance of this union?


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

https://reviews.llvm.org/D86660

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

[Lldb-commits] [PATCH] D85289: [DWARFYAML][debug_info] Rename some mapping keys. NFC.

[James Henderson via Phabricator via lldb-commits]

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

Okay, LGTM. I don't mind either way, and I suspect with the offset field 
becoming optional soon, it's unlikely to appear frequently, so the verbosity is 
a non-issue then.




Comment at: lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp:53
+DebugAbbrevOffset: 0
+AddressSize:8
 Entries:

Nit: the value here is misaligned.



Comment at: llvm/unittests/DebugInfo/GSYM/GSYMTest.cpp:2296
 
 
   StringRef yamldata = R"(

Could you fix this linter nit, whilst you're here, please?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85289

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

[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I am wondering what should the platform classes which do not implement this 
feature do (although most of them could implemented I don't think you need to 
implement all of them -- PlatformGdbRemote in particular can be a bit tricky). 
It seems to me like it would be better for them to bail out if a non-standard 
shell is requested instead of silently executing a command the user did not 
intend.




Comment at: lldb/include/lldb/Target/Platform.h:633
+  const Timeout &timeout,
+  const bool run_in_default_shell = true);
 

const on a value parameter is useless. Also, default arguments on virtual 
methods are somewhat shoot-footy: 
. The [[ 
https://en.wikipedia.org/wiki/Non-virtual_interface_pattern | Non-virtual 
interface pattern ]] would solve that, and maybe have some other benefits.



Comment at: lldb/source/API/SBPlatform.cpp:59-68
+if (command_interpreter && command_interpreter[0]) {
+  full_command += command_interpreter;
+  full_command += " -c ";
+}
+
+if (shell_command && shell_command[0]) {
+  full_command += " \"";

kastiglione wrote:
> JDevlieghere wrote:
> > Given that this pattern repeats a few times in this struct, maybe a small 
> > static helper function would be nice:
> > 
> > ```static bool is_not_empty(cont char* c) { return c && c[0]; }```
> Can these be `StringRef`, and then check with `empty()`?
Rather than duplicating this logic here (and getting it wrong for windows, for 
nested quotes, etc.) it would be better to just pass the custom shell to the 
`RunShellCommand` method. Then it use the existing quoting logic, and all that 
needs to change is that it now picks up the shell from the argument (if passed) 
instead of the baked-in default.



Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:1621-1633
+if (!FileSystem::Instance().Exists(option_arg)) {
+  error.SetErrorStringWithFormat("File \"%s\" doesn't exist.",
+ option_arg.str().c_str());
+  return error;
+}
+
+if (!FileSystem::Instance().Readable(option_arg)) {

This won't work for remote platforms. The checks should be at least done in the 
Platform class, but ideally I would really just leave it to the operating 
system to figure out if it can or cannot run a given command.



Comment at: lldb/test/API/commands/platform/basic/TestPlatformCommand.py:96
+
+@expectedFailureAll(oslist=["windows"])
+@no_debug_info_test

You could build a small executable to serve as the "shell". Then, this test 
would work everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86667

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

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

If the format is going to be json, why not use llvm::json from 
`llvm/Support/JSON.h`? That's what we been migrating most of the existing stuff 
to already, so going using that for new code makes perfect sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705

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

[Lldb-commits] [PATCH] D86690: [lldb] Fix gcc 5.4.0 compile error

[David Spickett via Phabricator via lldb-commits]

DavidSpickett created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
DavidSpickett requested review of this revision.
Herald added a subscriber: JDevlieghere.

Specify type when constructing PromotionKeys,
this fixes error:
"chosen constructor is explicit in copy-initialization"
when compiling lldb with GCC 5.4.0.

This is due to std::tuple having an explicit
default constructor, see:
http://cplusplus.github.io/LWG/lwg-defects.html#2193


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86690

Files:
  lldb/source/Utility/Scalar.cpp


Index: lldb/source/Utility/Scalar.cpp
===
--- lldb/source/Utility/Scalar.cpp
+++ lldb/source/Utility/Scalar.cpp
@@ -55,9 +55,9 @@
   Category cat = GetCategory(m_type);
   switch (cat) {
   case Category::Void:
-return {cat, 0, false};
+return PromotionKey{cat, 0, false};
   case Category::Integral:
-return {cat, m_integer.getBitWidth(), !IsSigned(m_type)};
+return PromotionKey{cat, m_integer.getBitWidth(), !IsSigned(m_type)};
   case Category::Float:
 return GetFloatPromoKey(m_float.getSemantics());
   }
@@ -70,7 +70,7 @@
   &APFloat::x87DoubleExtended()};
   for (const auto &entry : llvm::enumerate(order)) {
 if (entry.value() == &sem)
-  return {Category::Float, entry.index(), false};
+  return PromotionKey{Category::Float, entry.index(), false};
   }
   llvm_unreachable("Unsupported semantics!");
 }


Index: lldb/source/Utility/Scalar.cpp
===
--- lldb/source/Utility/Scalar.cpp
+++ lldb/source/Utility/Scalar.cpp
@@ -55,9 +55,9 @@
   Category cat = GetCategory(m_type);
   switch (cat) {
   case Category::Void:
-return {cat, 0, false};
+return PromotionKey{cat, 0, false};
   case Category::Integral:
-return {cat, m_integer.getBitWidth(), !IsSigned(m_type)};
+return PromotionKey{cat, m_integer.getBitWidth(), !IsSigned(m_type)};
   case Category::Float:
 return GetFloatPromoKey(m_float.getSemantics());
   }
@@ -70,7 +70,7 @@
   &APFloat::x87DoubleExtended()};
   for (const auto &entry : llvm::enumerate(order)) {
 if (entry.value() == &sem)
-  return {Category::Float, entry.index(), false};
+  return PromotionKey{Category::Float, entry.index(), false};
   }
   llvm_unreachable("Unsupported semantics!");
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86690: [lldb] Fix gcc 5.4.0 compile error

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a reviewer: labath.
DavidSpickett added a comment.

https://www.llvm.org/docs/GettingStarted.html gives the minimum gcc as 5.1. (I 
found this cross compiling, the apt package for AArch64 is 5.4.0)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86690

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

[Lldb-commits] [PATCH] D86690: [lldb] Fix gcc 5.4.0 compile error

[Pavel Labath via Phabricator via lldb-commits]

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

Looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86690

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

[Lldb-commits] [PATCH] D86690: [lldb] Fix gcc 5.4.0 compile error

[David Spickett via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc1e6f1a7b1a8: [lldb] Fix gcc 5.4.0 compile error (authored 
by DavidSpickett).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86690

Files:
  lldb/source/Utility/Scalar.cpp


Index: lldb/source/Utility/Scalar.cpp
===
--- lldb/source/Utility/Scalar.cpp
+++ lldb/source/Utility/Scalar.cpp
@@ -55,9 +55,9 @@
   Category cat = GetCategory(m_type);
   switch (cat) {
   case Category::Void:
-return {cat, 0, false};
+return PromotionKey{cat, 0, false};
   case Category::Integral:
-return {cat, m_integer.getBitWidth(), !IsSigned(m_type)};
+return PromotionKey{cat, m_integer.getBitWidth(), !IsSigned(m_type)};
   case Category::Float:
 return GetFloatPromoKey(m_float.getSemantics());
   }
@@ -70,7 +70,7 @@
   &APFloat::x87DoubleExtended()};
   for (const auto &entry : llvm::enumerate(order)) {
 if (entry.value() == &sem)
-  return {Category::Float, entry.index(), false};
+  return PromotionKey{Category::Float, entry.index(), false};
   }
   llvm_unreachable("Unsupported semantics!");
 }


Index: lldb/source/Utility/Scalar.cpp
===
--- lldb/source/Utility/Scalar.cpp
+++ lldb/source/Utility/Scalar.cpp
@@ -55,9 +55,9 @@
   Category cat = GetCategory(m_type);
   switch (cat) {
   case Category::Void:
-return {cat, 0, false};
+return PromotionKey{cat, 0, false};
   case Category::Integral:
-return {cat, m_integer.getBitWidth(), !IsSigned(m_type)};
+return PromotionKey{cat, m_integer.getBitWidth(), !IsSigned(m_type)};
   case Category::Float:
 return GetFloatPromoKey(m_float.getSemantics());
   }
@@ -70,7 +70,7 @@
   &APFloat::x87DoubleExtended()};
   for (const auto &entry : llvm::enumerate(order)) {
 if (entry.value() == &sem)
-  return {Category::Float, entry.index(), false};
+  return PromotionKey{Category::Float, entry.index(), false};
   }
   llvm_unreachable("Unsupported semantics!");
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] c1e6f1a - [lldb] Fix gcc 5.4.0 compile error

[David Spickett via lldb-commits]

Author: David Spickett
Date: 2020-08-27T10:23:05+01:00
New Revision: c1e6f1a7b1a8cb2bb11a76b904c6f8150bfcc3a6

URL: 
https://github.com/llvm/llvm-project/commit/c1e6f1a7b1a8cb2bb11a76b904c6f8150bfcc3a6
DIFF: 
https://github.com/llvm/llvm-project/commit/c1e6f1a7b1a8cb2bb11a76b904c6f8150bfcc3a6.diff

LOG: [lldb] Fix gcc 5.4.0 compile error

Specify type when constructing PromotionKeys,
this fixes error:
"chosen constructor is explicit in copy-initialization"
when compiling lldb with GCC 5.4.0.

This is due to std::tuple having an explicit
default constructor, see:
http://cplusplus.github.io/LWG/lwg-defects.html#2193

Reviewed By: labath

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

Added: 


Modified: 
lldb/source/Utility/Scalar.cpp

Removed: 




diff  --git a/lldb/source/Utility/Scalar.cpp b/lldb/source/Utility/Scalar.cpp
index e5a1454561f2..3249ba5a02e3 100644
--- a/lldb/source/Utility/Scalar.cpp
+++ b/lldb/source/Utility/Scalar.cpp
@@ -55,9 +55,9 @@ Scalar::PromotionKey Scalar::GetPromoKey() const {
   Category cat = GetCategory(m_type);
   switch (cat) {
   case Category::Void:
-return {cat, 0, false};
+return PromotionKey{cat, 0, false};
   case Category::Integral:
-return {cat, m_integer.getBitWidth(), !IsSigned(m_type)};
+return PromotionKey{cat, m_integer.getBitWidth(), !IsSigned(m_type)};
   case Category::Float:
 return GetFloatPromoKey(m_float.getSemantics());
   }
@@ -70,7 +70,7 @@ Scalar::PromotionKey Scalar::GetFloatPromoKey(const 
llvm::fltSemantics &sem) {
   &APFloat::x87DoubleExtended()};
   for (const auto &entry : llvm::enumerate(order)) {
 if (entry.value() == &sem)
-  return {Category::Float, entry.index(), false};
+  return PromotionKey{Category::Float, entry.index(), false};
   }
   llvm_unreachable("Unsupported semantics!");
 }



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

[Lldb-commits] [PATCH] D86615: [lldb/DWARF] Fix handling of variables with both location and const_value attributes

[Pavel Labath via Phabricator via lldb-commits]

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



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3134
   if ((tag == DW_TAG_variable) || (tag == DW_TAG_constant) ||
   (tag == DW_TAG_formal_parameter && sc.function)) {
 DWARFAttributes attributes;

aprantl wrote:
> Would be nice to early-exit here, too.
I'll do that in a separate patch.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3257
+} else if (const char *str = const_value_form.AsCString()) {
+  uint32_t string_length = strlen(str) + 1;
+  location = DWARFExpression(

aprantl wrote:
> shafik wrote:
> > aprantl wrote:
> > > If we do this a lot a StringRef DWARFFormValue::AsCStringRef() call would 
> > > make sense...
> > Why `+1`?
> The NUL-terminator?
Yeah, this is supposed to create a memory view of a C string, so it (probably) 
needs to include the nul terminator. But that actually speaks against the 
`AsCStringRef` function, as I wouldn't expect that one to include the null 
terminator.

I say "probably" because I haven't been able to actually find a producer that 
would produce this kind of attribute. The closest I got was gfortran, which 
could emit emit DW_AT_const_value for string variables. However, it used block 
forms for that.

To my great surprise, my synthetic test case in that patch actually worked and 
using a string DW_AT_const_value for char arrays in C seems somewhat 
reasonable, so I kept that code. However, I could be easily convinced to delete 
this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86615

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

[Lldb-commits] [lldb] 219ccdf - [lldb/Utility] Use APSInt in the Scalar class

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-08-27T15:05:47+02:00
New Revision: 219ccdfddecb963971ad14b5c14220b896d2c2e7

URL: 
https://github.com/llvm/llvm-project/commit/219ccdfddecb963971ad14b5c14220b896d2c2e7
DIFF: 
https://github.com/llvm/llvm-project/commit/219ccdfddecb963971ad14b5c14220b896d2c2e7.diff

LOG: [lldb/Utility] Use APSInt in the Scalar class

This enables us to further simplify some code because it no longer needs
to switch on the signedness of the type (APSInt handles that).

Added: 


Modified: 
lldb/include/lldb/Utility/Scalar.h
lldb/source/Utility/Scalar.cpp
lldb/unittests/Utility/ScalarTest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/Scalar.h 
b/lldb/include/lldb/Utility/Scalar.h
index 9d3a20c93898..332bb00489d0 100644
--- a/lldb/include/lldb/Utility/Scalar.h
+++ b/lldb/include/lldb/Utility/Scalar.h
@@ -14,7 +14,7 @@
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-private-types.h"
 #include "llvm/ADT/APFloat.h"
-#include "llvm/ADT/APInt.h"
+#include "llvm/ADT/APSInt.h"
 #include 
 #include 
 #include 
@@ -38,35 +38,35 @@ namespace lldb_private {
 // and values before performing these operations. Type promotion currently
 // follows the ANSI C type promotion rules.
 class Scalar {
+  template
+  static llvm::APSInt MakeAPSInt(T v) {
+static_assert(std::is_integral::value, "");
+static_assert(sizeof(T) <= sizeof(uint64_t), "Conversion loses 
precision!");
+return llvm::APSInt(
+llvm::APInt(sizeof(T) * 8, uint64_t(v), std::is_signed::value),
+std::is_unsigned::value);
+  }
+
 public:
   // FIXME: These are host types which seems to be an odd choice.
   enum Type {
 e_void = 0,
-e_sint,
-e_uint,
+e_int,
 e_float,
   };
 
   // Constructors and Destructors
   Scalar() : m_type(e_void), m_float(0.0f) {}
-  Scalar(int v)
-  : m_type(e_sint), m_integer(sizeof(v) * 8, uint64_t(v), true),
-m_float(0.0f) {}
+  Scalar(int v) : m_type(e_int), m_integer(MakeAPSInt(v)), m_float(0.0f) {}
   Scalar(unsigned int v)
-  : m_type(e_uint), m_integer(sizeof(v) * 8, uint64_t(v), false),
-m_float(0.0f) {}
-  Scalar(long v)
-  : m_type(e_sint), m_integer(sizeof(v) * 8, uint64_t(v), true),
-m_float(0.0f) {}
+  : m_type(e_int), m_integer(MakeAPSInt(v)), m_float(0.0f) {}
+  Scalar(long v) : m_type(e_int), m_integer(MakeAPSInt(v)), m_float(0.0f) {}
   Scalar(unsigned long v)
-  : m_type(e_uint), m_integer(sizeof(v) * 8, uint64_t(v), false),
-m_float(0.0f) {}
+  : m_type(e_int), m_integer(MakeAPSInt(v)), m_float(0.0f) {}
   Scalar(long long v)
-  : m_type(e_sint), m_integer(sizeof(v) * 8, uint64_t(v), true),
-m_float(0.0f) {}
+  : m_type(e_int), m_integer(MakeAPSInt(v)), m_float(0.0f) {}
   Scalar(unsigned long long v)
-  : m_type(e_uint), m_integer(sizeof(v) * 8, uint64_t(v), false),
-m_float(0.0f) {}
+  : m_type(e_int), m_integer(MakeAPSInt(v)), m_float(0.0f) {}
   Scalar(float v) : m_type(e_float), m_float(v) {}
   Scalar(double v) : m_type(e_float), m_float(v) {}
   Scalar(long double v) : m_type(e_float), m_float(double(v)) {
@@ -75,7 +75,7 @@ class Scalar {
 llvm::APFloat::rmNearestTiesToEven, &ignore);
   }
   Scalar(llvm::APInt v)
-  : m_type(e_sint), m_integer(std::move(v)), m_float(0.0f) {}
+  : m_type(e_int), m_integer(std::move(v), false), m_float(0.0f) {}
 
   bool SignExtend(uint32_t bit_pos);
 
@@ -108,7 +108,7 @@ class Scalar {
 
   void GetValue(Stream *s, bool show_type) const;
 
-  bool IsValid() const { return (m_type >= e_sint) && (m_type <= e_float); }
+  bool IsValid() const { return (m_type >= e_int) && (m_type <= e_float); }
 
   /// Convert to an integer with \p bits and the given signedness.
   void TruncOrExtendTo(uint16_t bits, bool sign);
@@ -116,6 +116,7 @@ class Scalar {
   bool IntegralPromote(uint16_t bits, bool sign);
   bool FloatPromote(const llvm::fltSemantics &semantics);
 
+  bool IsSigned() const;
   bool MakeSigned();
 
   bool MakeUnsigned();
@@ -234,7 +235,7 @@ class Scalar {
 
   // Classes that inherit from Scalar can see and modify these
   Scalar::Type m_type;
-  llvm::APInt m_integer;
+  llvm::APSInt m_integer;
   llvm::APFloat m_float;
 
   template  T GetAs(T fail_value) const;

diff  --git a/lldb/source/Utility/Scalar.cpp b/lldb/source/Utility/Scalar.cpp
index 3249ba5a02e3..3dd2813b5eb0 100644
--- a/lldb/source/Utility/Scalar.cpp
+++ b/lldb/source/Utility/Scalar.cpp
@@ -25,6 +25,7 @@ using namespace lldb_private;
 
 using llvm::APFloat;
 using llvm::APInt;
+using llvm::APSInt;
 
 Scalar::Category Scalar::GetCategory(Scalar::Type type) {
   switch (type) {
@@ -32,32 +33,19 @@ Scalar::Category Scalar::GetCategory(Scalar::Type type) {
 return Category::Void;
   case Scalar::e_float:
 return Category::Float;
-  case Scalar::e_sint:
-  case Scalar::e_uint:
+  case Scalar::e_int:
 retu

[Lldb-commits] [lldb] 9f5927e - [lldb/DWARF] Fix handling of variables with both location and const_value attributes

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-08-27T15:05:47+02:00
New Revision: 9f5927e42bf4a7448dc9dd3a1550d1126c595dad

URL: 
https://github.com/llvm/llvm-project/commit/9f5927e42bf4a7448dc9dd3a1550d1126c595dad
DIFF: 
https://github.com/llvm/llvm-project/commit/9f5927e42bf4a7448dc9dd3a1550d1126c595dad.diff

LOG: [lldb/DWARF] Fix handling of variables with both location and const_value 
attributes

Class-level static constexpr variables can have both DW_AT_const_value
(in the "declaration") and a DW_AT_location (in the "definition")
attributes. Our code was trying to handle this, but it was brittle and
hard to follow (and broken) because it was processing the attributes in
the order in which they were found.

Refactor the code to make the intent clearer -- DW_AT_location trumps
DW_AT_const_value, and fix the bug which meant that we were not
displaying these variables properly (the culprit was the delayed parsing
of the const_value attribute due to a need to fetch the variable type.

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

Added: 
lldb/test/Shell/SymbolFile/DWARF/DW_AT_location-DW_AT_const_value.s

Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
index b401352c693d..fe6a55520978 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
@@ -42,6 +42,7 @@ class DWARFFormValue {
   DWARFFormValue(const DWARFUnit *unit) : m_unit(unit) {}
   DWARFFormValue(const DWARFUnit *unit, dw_form_t form)
   : m_unit(unit), m_form(form) {}
+  const DWARFUnit *GetUnit() const { return m_unit; }
   void SetUnit(const DWARFUnit *unit) { m_unit = unit; }
   dw_form_t Form() const { return m_form; }
   dw_form_t& FormRef() { return m_form; }

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 500d7567536e..271821b24517 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3111,18 +3111,15 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const 
SymbolContext &sc,
   const char *name = nullptr;
   const char *mangled = nullptr;
   Declaration decl;
-  uint32_t i;
   DWARFFormValue type_die_form;
   DWARFExpression location;
   bool is_external = false;
   bool is_artificial = false;
-  bool location_is_const_value_data = false;
-  bool has_explicit_location = false;
-  DWARFFormValue const_value;
+  DWARFFormValue const_value_form, location_form;
   Variable::RangeList scope_ranges;
   // AccessType accessibility = eAccessNone;
 
-  for (i = 0; i < num_attributes; ++i) {
+  for (size_t i = 0; i < num_attributes; ++i) {
 dw_attr_t attr = attributes.AttributeAtIndex(i);
 DWARFFormValue form_value;
 
@@ -3152,65 +3149,11 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const 
SymbolContext &sc,
 is_external = form_value.Boolean();
 break;
   case DW_AT_const_value:
-// If we have already found a DW_AT_location attribute, ignore this
-// attribute.
-if (!has_explicit_location) {
-  location_is_const_value_data = true;
-  // The constant value will be either a block, a data value or a
-  // string.
-  auto debug_info_data = die.GetData();
-  if (DWARFFormValue::IsBlockForm(form_value.Form())) {
-// Retrieve the value as a block expression.
-uint32_t block_offset =
-form_value.BlockData() - debug_info_data.GetDataStart();
-uint32_t block_length = form_value.Unsigned();
-location = DWARFExpression(
-module,
-DataExtractor(debug_info_data, block_offset, block_length),
-die.GetCU());
-  } else if (DWARFFormValue::IsDataForm(form_value.Form())) {
-// Constant value size does not have to match the size of the
-// variable. We will fetch the size of the type after we create
-// it.
-const_value = form_value;
-  } else if (const char *str = form_value.AsCString()) {
-uint32_t string_length = strlen(str) + 1;
-location = DWARFExpression(
-module,
-DataExtractor(str, string_length,
-  die.GetCU()->GetByteOrder(),
-  die.GetCU()->GetAddressByteSize()),
-die.GetCU());
-  }
-}
+const_

[Lldb-commits] [PATCH] D86615: [lldb/DWARF] Fix handling of variables with both location and const_value attributes

[Pavel Labath via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9f5927e42bf4: [lldb/DWARF] Fix handling of variables with 
both location and const_value… (authored by labath).

Changed prior to commit:
  https://reviews.llvm.org/D86615?vs=287940&id=288303#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86615

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_location-DW_AT_const_value.s

Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_location-DW_AT_const_value.s
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_location-DW_AT_const_value.s
@@ -0,0 +1,144 @@
+## Test that we don't get confused by variables with both location and
+## const_value attributes. Such values are produced in C++ for class-level
+## static constexpr variables.
+
+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o %t
+# RUN: %lldb %t -o "target variable A::x A::y" -o exit | FileCheck %s
+
+# CHECK-LABEL: target variable
+# CHECK: (const int) A::x = 142
+# CHECK: (const int) A::y = 242
+
+.section.rodata,"a",@progbits
+.p2align2
+_ZN1A1xE:
+.long   142
+_ZN1A1yE:
+.long   242
+
+.section.debug_abbrev,"",@progbits
+.byte   1   # Abbreviation Code
+.byte   17  # DW_TAG_compile_unit
+.byte   1   # DW_CHILDREN_yes
+.byte   37  # DW_AT_producer
+.byte   8   # DW_FORM_string
+.byte   3   # DW_AT_name
+.byte   8   # DW_FORM_string
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   3   # Abbreviation Code
+.byte   19  # DW_TAG_structure_type
+.byte   1   # DW_CHILDREN_yes
+.byte   3   # DW_AT_name
+.byte   8   # DW_FORM_string
+.byte   11  # DW_AT_byte_size
+.byte   11  # DW_FORM_data1
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   4   # Abbreviation Code
+.byte   13  # DW_TAG_member
+.byte   0   # DW_CHILDREN_no
+.byte   3   # DW_AT_name
+.byte   8   # DW_FORM_string
+.byte   73  # DW_AT_type
+.byte   19  # DW_FORM_ref4
+.byte   60  # DW_AT_declaration
+.byte   25  # DW_FORM_flag_present
+.byte   28  # DW_AT_const_value
+.byte   13  # DW_FORM_sdata
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   5   # Abbreviation Code
+.byte   38  # DW_TAG_const_type
+.byte   0   # DW_CHILDREN_no
+.byte   73  # DW_AT_type
+.byte   19  # DW_FORM_ref4
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   6   # Abbreviation Code
+.byte   36  # DW_TAG_base_type
+.byte   0   # DW_CHILDREN_no
+.byte   3   # DW_AT_name
+.byte   8   # DW_FORM_string
+.byte   62  # DW_AT_encoding
+.byte   11  # DW_FORM_data1
+.byte   11  # DW_AT_byte_size
+.byte   11  # DW_FORM_data1
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
+.byte   7   # Abbreviation Code
+.byte   52  # DW_TAG_variable
+.byte   0   # DW_CHILDREN_no
+.byte   71  # DW_AT_specification
+   

[Lldb-commits] [PATCH] D86616: [cmake] Make gtest include directories a part of the library interface

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D86616#2238946 , @ldionne wrote:

> LGTM, but I'm not an owner for any of the projects touched by this change.

I picked you because you seemed interested in the overall direction that our 
cmake support is going :), and I believe that one of our biggest problem with 
cmake is the lack of a unified direction of where our cmake support is going. 
(Also, my previous go-to person for that (@beanz), seems to be busy with other 
stuff these days.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86616

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

[Lldb-commits] [lldb] 9cb222e - [cmake] Make gtest include directories a part of the library interface

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-08-27T15:35:57+02:00
New Revision: 9cb222e749e8392517a138cf6645a7c220d671c8

URL: 
https://github.com/llvm/llvm-project/commit/9cb222e749e8392517a138cf6645a7c220d671c8
DIFF: 
https://github.com/llvm/llvm-project/commit/9cb222e749e8392517a138cf6645a7c220d671c8.diff

LOG: [cmake] Make gtest include directories a part of the library interface

This applies the same fix that D84748 did for macro definitions.
Appropriate include path is now automatically set for all libraries
which link against gtest targets, which avoids the need to set
include_directories in various parts of the project.

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

Added: 


Modified: 
flang/CMakeLists.txt
libc/benchmarks/CMakeLists.txt
lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
llvm/cmake/modules/AddLLVM.cmake
llvm/lib/Testing/Support/CMakeLists.txt
llvm/utils/unittest/CMakeLists.txt
polly/CMakeLists.txt

Removed: 




diff  --git a/flang/CMakeLists.txt b/flang/CMakeLists.txt
index 73c2db55e8f8..03440b72ec8c 100644
--- a/flang/CMakeLists.txt
+++ b/flang/CMakeLists.txt
@@ -135,13 +135,7 @@ if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   if (FLANG_INCLUDE_TESTS)
 set(UNITTEST_DIR ${LLVM_BUILD_MAIN_SRC_DIR}/utils/unittest)
 if(EXISTS ${UNITTEST_DIR}/googletest/include/gtest/gtest.h)
-  if (TARGET gtest)
-# LLVM Doesn't export gtest's include directorys, so do that here
-set_target_properties(gtest
-  PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
-  
"${UNITTEST_DIR}/googletest/include;${UNITTEST_DIR}/googlemock/include"
-  )
-  else()
+  if (NOT TARGET gtest)
 add_library(gtest
   ${UNITTEST_DIR}/googletest/src/gtest-all.cc
   ${UNITTEST_DIR}/googlemock/src/gmock-all.cc

diff  --git a/libc/benchmarks/CMakeLists.txt b/libc/benchmarks/CMakeLists.txt
index 6f3cfdb64a5f..2275dad7f653 100644
--- a/libc/benchmarks/CMakeLists.txt
+++ b/libc/benchmarks/CMakeLists.txt
@@ -53,11 +53,6 @@ function(add_libc_benchmark_unittest target_name)
 EXCLUDE_FROM_ALL
 ${LIBC_BENCHMARKS_UNITTEST_SRCS}
   )
-  target_include_directories(${target_name}
-PRIVATE
-${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include
-${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include
-  )
   target_link_libraries(${target_name}
 PRIVATE
 gtest_main

diff  --git a/lldb/unittests/TestingSupport/Symbol/CMakeLists.txt 
b/lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
index 3faec7c8030b..38a518682853 100644
--- a/lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
+++ b/lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
@@ -2,7 +2,3 @@ set_property(DIRECTORY PROPERTY EXCLUDE_FROM_ALL ON)
 add_lldb_library(lldbSymbolHelpers
   YAMLModuleTester.cpp
   )
-
-target_include_directories(lldbSymbolHelpers PUBLIC
-  ${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include
-  ${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include)

diff  --git a/llvm/cmake/modules/AddLLVM.cmake 
b/llvm/cmake/modules/AddLLVM.cmake
index 1689d36171c3..a40cf17426fe 100644
--- a/llvm/cmake/modules/AddLLVM.cmake
+++ b/llvm/cmake/modules/AddLLVM.cmake
@@ -1401,9 +1401,6 @@ function(add_unittest test_suite test_name)
 set(EXCLUDE_FROM_ALL ON)
   endif()
 
-  include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include)
-  include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include)
-
   if (SUPPORTS_VARIADIC_MACROS_FLAG)
 list(APPEND LLVM_COMPILE_FLAGS "-Wno-variadic-macros")
   endif ()

diff  --git a/llvm/lib/Testing/Support/CMakeLists.txt 
b/llvm/lib/Testing/Support/CMakeLists.txt
index 4f5345c1dc57..ed2fd8ae43b2 100644
--- a/llvm/lib/Testing/Support/CMakeLists.txt
+++ b/llvm/lib/Testing/Support/CMakeLists.txt
@@ -12,6 +12,4 @@ add_llvm_library(LLVMTestingSupport
   Support
   )
 
-include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include)
-include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include)
 target_link_libraries(LLVMTestingSupport PRIVATE gtest)

diff  --git a/llvm/utils/unittest/CMakeLists.txt 
b/llvm/utils/unittest/CMakeLists.txt
index 14c780342b60..43c8fafdfc4c 100644
--- a/llvm/utils/unittest/CMakeLists.txt
+++ b/llvm/utils/unittest/CMakeLists.txt
@@ -11,14 +11,6 @@
 #
 # Project-wide settings
 
-# Where gtest's .h files can be found.
-include_directories(
-  googletest/include
-  googletest
-  googlemock/include
-  googlemock
-  )
-
 if(WIN32)
   add_definitions(-DGTEST_OS_WINDOWS=1)
 endif()
@@ -76,6 +68,11 @@ if (NOT LLVM_ENABLE_THREADS)
   target_compile_definitions(gtest PUBLIC GTEST_HAS_PTHREAD=0)
 endif ()
 
+target_include_directories(gtest
+  PUBLIC googletest/include googlemock/include
+  PRIVATE googletest googlemock
+  )
+
 add_subdirectory(UnitTestMain)
 
 # When LLVM_LINK_LLVM_DYLIB is enabled, libLLVM.so is added to the interface

di

[Lldb-commits] [PATCH] D86616: [cmake] Make gtest include directories a part of the library interface

[Pavel Labath via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9cb222e749e8: [cmake] Make gtest include directories a part 
of the library interface (authored by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86616

Files:
  flang/CMakeLists.txt
  libc/benchmarks/CMakeLists.txt
  lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
  llvm/cmake/modules/AddLLVM.cmake
  llvm/lib/Testing/Support/CMakeLists.txt
  llvm/utils/unittest/CMakeLists.txt
  polly/CMakeLists.txt

Index: polly/CMakeLists.txt
===
--- polly/CMakeLists.txt
+++ polly/CMakeLists.txt
@@ -30,12 +30,6 @@
 if (NOT TARGET gtest)
   add_subdirectory(${UNITTEST_DIR} utils/unittest)
 endif()
-
-# LLVM Doesn't export gtest's include directorys, so do that here
-set_target_properties(gtest
-  PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
-  "${UNITTEST_DIR}/googletest/include;${UNITTEST_DIR}/googlemock/include"
-  )
 set(POLLY_GTEST_AVAIL 1)
   endif()
 
Index: llvm/utils/unittest/CMakeLists.txt
===
--- llvm/utils/unittest/CMakeLists.txt
+++ llvm/utils/unittest/CMakeLists.txt
@@ -11,14 +11,6 @@
 #
 # Project-wide settings
 
-# Where gtest's .h files can be found.
-include_directories(
-  googletest/include
-  googletest
-  googlemock/include
-  googlemock
-  )
-
 if(WIN32)
   add_definitions(-DGTEST_OS_WINDOWS=1)
 endif()
@@ -76,6 +68,11 @@
   target_compile_definitions(gtest PUBLIC GTEST_HAS_PTHREAD=0)
 endif ()
 
+target_include_directories(gtest
+  PUBLIC googletest/include googlemock/include
+  PRIVATE googletest googlemock
+  )
+
 add_subdirectory(UnitTestMain)
 
 # When LLVM_LINK_LLVM_DYLIB is enabled, libLLVM.so is added to the interface
Index: llvm/lib/Testing/Support/CMakeLists.txt
===
--- llvm/lib/Testing/Support/CMakeLists.txt
+++ llvm/lib/Testing/Support/CMakeLists.txt
@@ -12,6 +12,4 @@
   Support
   )
 
-include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include)
-include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include)
 target_link_libraries(LLVMTestingSupport PRIVATE gtest)
Index: llvm/cmake/modules/AddLLVM.cmake
===
--- llvm/cmake/modules/AddLLVM.cmake
+++ llvm/cmake/modules/AddLLVM.cmake
@@ -1401,9 +1401,6 @@
 set(EXCLUDE_FROM_ALL ON)
   endif()
 
-  include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include)
-  include_directories(${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include)
-
   if (SUPPORTS_VARIADIC_MACROS_FLAG)
 list(APPEND LLVM_COMPILE_FLAGS "-Wno-variadic-macros")
   endif ()
Index: lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
===
--- lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
+++ lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
@@ -2,7 +2,3 @@
 add_lldb_library(lldbSymbolHelpers
   YAMLModuleTester.cpp
   )
-
-target_include_directories(lldbSymbolHelpers PUBLIC
-  ${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include
-  ${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include)
Index: libc/benchmarks/CMakeLists.txt
===
--- libc/benchmarks/CMakeLists.txt
+++ libc/benchmarks/CMakeLists.txt
@@ -53,11 +53,6 @@
 EXCLUDE_FROM_ALL
 ${LIBC_BENCHMARKS_UNITTEST_SRCS}
   )
-  target_include_directories(${target_name}
-PRIVATE
-${LLVM_MAIN_SRC_DIR}/utils/unittest/googletest/include
-${LLVM_MAIN_SRC_DIR}/utils/unittest/googlemock/include
-  )
   target_link_libraries(${target_name}
 PRIVATE
 gtest_main
Index: flang/CMakeLists.txt
===
--- flang/CMakeLists.txt
+++ flang/CMakeLists.txt
@@ -135,13 +135,7 @@
   if (FLANG_INCLUDE_TESTS)
 set(UNITTEST_DIR ${LLVM_BUILD_MAIN_SRC_DIR}/utils/unittest)
 if(EXISTS ${UNITTEST_DIR}/googletest/include/gtest/gtest.h)
-  if (TARGET gtest)
-# LLVM Doesn't export gtest's include directorys, so do that here
-set_target_properties(gtest
-  PROPERTIES INTERFACE_INCLUDE_DIRECTORIES
-  "${UNITTEST_DIR}/googletest/include;${UNITTEST_DIR}/googlemock/include"
-  )
-  else()
+  if (NOT TARGET gtest)
 add_library(gtest
   ${UNITTEST_DIR}/googletest/src/gtest-all.cc
   ${UNITTEST_DIR}/googlemock/src/gmock-all.cc
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86616: [cmake] Make gtest include directories a part of the library interface

[Louis Dionne via Phabricator via lldb-commits]

ldionne added a comment.

In D86616#2241936 , @labath wrote:

> In D86616#2238946 , @ldionne wrote:
>
>> LGTM, but I'm not an owner for any of the projects touched by this change.
>
> I picked you because you seemed interested in the overall direction that our 
> cmake support is going :), and I believe that one of our biggest problem with 
> cmake is the lack of a unified direction of where our cmake support is going. 
> (Also, my previous go-to person for that (@beanz), seems to be busy with 
> other stuff these days.)

Yup, I am interested by that indeed :). I just wanted to make it clear my LGTM 
alone shouldn't be considered enough to commit to these other projects that 
might have active owners. But it does look like you've got enough approvals for 
an obviously correct improvement, IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86616

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

[Lldb-commits] [PATCH] D86436: [lldb] Fix Type::GetByteSize for pointer types

[Pavel Labath via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0de146337391: [lldb] Fix Type::GetByteSize for pointer types 
(authored by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86436

Files:
  lldb/source/Symbol/Type.cpp
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s


Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
===
--- lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
@@ -5,10 +5,10 @@
 
 # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o %t
 # RUN: %lldb %t \
-# RUN:   -o "target variable udata data1 data2 data4 data8 string strp ref4" \
+# RUN:   -o "target variable udata data1 data2 data4 data8 string strp ref4 
udata_ptr" \
 # RUN:   -o exit | FileCheck %s
 
-# CHECK-LABEL: target variable udata data1 data2 data4 data8 string strp ref4
+# CHECK-LABEL: target variable
 ## Variable specified via DW_FORM_udata. This is typical for clang (10).
 # CHECK: (unsigned long) udata = 4742474247424742
 ## Variables specified via fixed-size forms. This is typical for gcc (9).
@@ -22,6 +22,8 @@
 # CHECK: (char [7]) strp = "strp"
 ## Bogus attribute form. Let's make sure we don't crash at least.
 # CHECK: (char [7]) ref4 = 
+## A variable of pointer type.
+# CHECK: (unsigned long *) udata_ptr = 0xdeadbeefbaadf00d
 
 .section.debug_abbrev,"",@progbits
 .byte   1   # Abbreviation Code
@@ -33,6 +35,13 @@
 .byte   8   # DW_FORM_string
 .byte   0   # EOM(1)
 .byte   0   # EOM(2)
+.byte   2   # Abbreviation Code
+.byte   15  # DW_TAG_pointer_type
+.byte   0   # DW_CHILDREN_no
+.byte   73  # DW_AT_type
+.byte   19  # DW_FORM_ref4
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
 .byte   4   # Abbreviation Code
 .byte   1   # DW_TAG_array_type
 .byte   1   # DW_CHILDREN_yes
@@ -109,6 +118,9 @@
 .asciz  "unsigned long" # DW_AT_name
 .byte   8   # DW_AT_byte_size
 .byte   7   # DW_AT_encoding
+.Lulong_ptr:
+.byte   2   # Abbrev DW_TAG_pointer_type
+.long   .Lulong-.Lcu_begin0 # DW_AT_type
 
 .byte   10  # Abbrev DW_TAG_variable
 .asciz  "udata" # DW_AT_name
@@ -150,6 +162,11 @@
 .long   .Lchar_arr-.Lcu_begin0  # DW_AT_type
 .long   .Lulong-.Lcu_begin0 # DW_AT_const_value
 
+.byte   10  # Abbrev DW_TAG_variable
+.asciz  "udata_ptr" # DW_AT_name
+.long   .Lulong_ptr-.Lcu_begin0 # DW_AT_type
+.uleb128 0xdeadbeefbaadf00d # DW_AT_const_value
+
 .byte   0   # End Of Children Mark
 .Ldebug_info_end0:
 
Index: lldb/source/Symbol/Type.cpp
===
--- lldb/source/Symbol/Type.cpp
+++ lldb/source/Symbol/Type.cpp
@@ -375,6 +375,7 @@
   if (ArchSpec arch = m_symbol_file->GetObjectFile()->GetArchitecture()) {
 m_byte_size = arch.GetAddressByteSize();
 m_byte_size_has_value = true;
+return m_byte_size;
   }
 } break;
   }


Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
===
--- lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
@@ -5,10 +5,10 @@
 
 # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o %t
 # RUN: %lldb %t \
-# RUN:   -o "target variable udata data1 data2 data4 data8 string strp ref4" \
+# RUN:   -o "target variable udata data1 data2 data4 data8 string strp ref4 udata_ptr" \
 # RUN:   -o exit | FileCheck %s
 
-# CHECK-LABEL: target variable udata data1 data2 data4 data8 string strp ref4
+# CHECK-LABEL: target variable
 ## Variable specified via DW_FORM_udata. This is typical for clang (10).
 # CHECK: (unsigned long) udata = 4742474247424742
 ## Variables specified via fixed-size forms. This is typical for gcc (9).
@@ -22,6 +22,8 @@
 # CHECK: (char [7]) strp = "strp"
 ## Bogus attribute form. Let's make sure we don't crash at least.
 # CHECK: (char [7]) ref4 = 
+## A variable of pointer type.
+# CHECK: (unsigned long *) udata_ptr = 0xdeadbeefbaadf00d
 
 .section.debug_abbrev,"",@progbits
 .byte   1   # Abbreviation Code
@@ -33,6 +35,13 @@
  

[Lldb-commits] [lldb] 0de1463 - [lldb] Fix Type::GetByteSize for pointer types

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-08-27T15:37:49+02:00
New Revision: 0de1463373918ae424cdcfeaa5b318f45c528696

URL: 
https://github.com/llvm/llvm-project/commit/0de1463373918ae424cdcfeaa5b318f45c528696
DIFF: 
https://github.com/llvm/llvm-project/commit/0de1463373918ae424cdcfeaa5b318f45c528696.diff

LOG: [lldb] Fix Type::GetByteSize for pointer types

The function was returning an incorrect (empty) value on the first
invocation. Given that this only affected the first invocation, this
bug/typo went mostly unaffected. DW_AT_const_value were particularly
badly affected by this as the GetByteSize call is
SymbolFileDWARF::ParseVariableDIE is likely to be the first call of this
function, and its effects cannot be undone by retrying.

Depends on D86348.

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

Added: 


Modified: 
lldb/source/Symbol/Type.cpp
lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s

Removed: 




diff  --git a/lldb/source/Symbol/Type.cpp b/lldb/source/Symbol/Type.cpp
index ecf0575b9a57..378523d00896 100644
--- a/lldb/source/Symbol/Type.cpp
+++ b/lldb/source/Symbol/Type.cpp
@@ -375,6 +375,7 @@ llvm::Optional 
Type::GetByteSize(ExecutionContextScope *exe_scope) {
   if (ArchSpec arch = m_symbol_file->GetObjectFile()->GetArchitecture()) {
 m_byte_size = arch.GetAddressByteSize();
 m_byte_size_has_value = true;
+return m_byte_size;
   }
 } break;
   }

diff  --git a/lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s 
b/lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
index 67c89b62339b..2275ff25ce97 100644
--- a/lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
+++ b/lldb/test/Shell/SymbolFile/DWARF/DW_AT_const_value.s
@@ -5,10 +5,10 @@
 
 # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o %t
 # RUN: %lldb %t \
-# RUN:   -o "target variable udata data1 data2 data4 data8 string strp ref4" \
+# RUN:   -o "target variable udata data1 data2 data4 data8 string strp ref4 
udata_ptr" \
 # RUN:   -o exit | FileCheck %s
 
-# CHECK-LABEL: target variable udata data1 data2 data4 data8 string strp ref4
+# CHECK-LABEL: target variable
 ## Variable specified via DW_FORM_udata. This is typical for clang (10).
 # CHECK: (unsigned long) udata = 4742474247424742
 ## Variables specified via fixed-size forms. This is typical for gcc (9).
@@ -22,6 +22,8 @@
 # CHECK: (char [7]) strp = "strp"
 ## Bogus attribute form. Let's make sure we don't crash at least.
 # CHECK: (char [7]) ref4 = 
+## A variable of pointer type.
+# CHECK: (unsigned long *) udata_ptr = 0xdeadbeefbaadf00d
 
 .section.debug_abbrev,"",@progbits
 .byte   1   # Abbreviation Code
@@ -33,6 +35,13 @@
 .byte   8   # DW_FORM_string
 .byte   0   # EOM(1)
 .byte   0   # EOM(2)
+.byte   2   # Abbreviation Code
+.byte   15  # DW_TAG_pointer_type
+.byte   0   # DW_CHILDREN_no
+.byte   73  # DW_AT_type
+.byte   19  # DW_FORM_ref4
+.byte   0   # EOM(1)
+.byte   0   # EOM(2)
 .byte   4   # Abbreviation Code
 .byte   1   # DW_TAG_array_type
 .byte   1   # DW_CHILDREN_yes
@@ -109,6 +118,9 @@
 .asciz  "unsigned long" # DW_AT_name
 .byte   8   # DW_AT_byte_size
 .byte   7   # DW_AT_encoding
+.Lulong_ptr:
+.byte   2   # Abbrev DW_TAG_pointer_type
+.long   .Lulong-.Lcu_begin0 # DW_AT_type
 
 .byte   10  # Abbrev DW_TAG_variable
 .asciz  "udata" # DW_AT_name
@@ -150,6 +162,11 @@
 .long   .Lchar_arr-.Lcu_begin0  # DW_AT_type
 .long   .Lulong-.Lcu_begin0 # DW_AT_const_value
 
+.byte   10  # Abbrev DW_TAG_variable
+.asciz  "udata_ptr" # DW_AT_name
+.long   .Lulong_ptr-.Lcu_begin0 # DW_AT_type
+.uleb128 0xdeadbeefbaadf00d # DW_AT_const_value
+
 .byte   0   # End Of Children Mark
 .Ldebug_info_end0:
 



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

[Lldb-commits] [lldb] 5b2b754 - [lldb/cmake] Fix linking of lldbUtilityHelpers for 9cb222e74

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-08-27T16:06:59+02:00
New Revision: 5b2b754565602a8b49b68967e1810f592f175d6b

URL: 
https://github.com/llvm/llvm-project/commit/5b2b754565602a8b49b68967e1810f592f175d6b
DIFF: 
https://github.com/llvm/llvm-project/commit/5b2b754565602a8b49b68967e1810f592f175d6b.diff

LOG: [lldb/cmake] Fix linking of lldbUtilityHelpers for 9cb222e74

Added: 


Modified: 
lldb/unittests/TestingSupport/CMakeLists.txt

Removed: 




diff  --git a/lldb/unittests/TestingSupport/CMakeLists.txt 
b/lldb/unittests/TestingSupport/CMakeLists.txt
index 67ebe3242629..4599ada1ec50 100644
--- a/lldb/unittests/TestingSupport/CMakeLists.txt
+++ b/lldb/unittests/TestingSupport/CMakeLists.txt
@@ -6,6 +6,7 @@ add_lldb_library(lldbUtilityHelpers
   LINK_LIBS
 lldbUtility
 lldbSymbolHelpers
+gtest
 
   LINK_COMPONENTS
 Support



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

[Lldb-commits] [lldb] dd63506 - [lldb/cmake] Fix linking of lldbSymbolHelpers for 9cb222e7

[Pavel Labath via lldb-commits]

Author: Pavel Labath
Date: 2020-08-27T16:40:17+02:00
New Revision: dd635062d867835cfe893698161277cc251b4456

URL: 
https://github.com/llvm/llvm-project/commit/dd635062d867835cfe893698161277cc251b4456
DIFF: 
https://github.com/llvm/llvm-project/commit/dd635062d867835cfe893698161277cc251b4456.diff

LOG: [lldb/cmake] Fix linking of lldbSymbolHelpers for 9cb222e7

I didn't find this locally because I have a /usr/include/gtest which is
similar enough to the bundled one to make things appear to work.

Added: 


Modified: 
lldb/unittests/TestingSupport/Symbol/CMakeLists.txt

Removed: 




diff  --git a/lldb/unittests/TestingSupport/Symbol/CMakeLists.txt 
b/lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
index 38a518682853d..cdd65ca17fed2 100644
--- a/lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
+++ b/lldb/unittests/TestingSupport/Symbol/CMakeLists.txt
@@ -1,4 +1,15 @@
 set_property(DIRECTORY PROPERTY EXCLUDE_FROM_ALL ON)
 add_lldb_library(lldbSymbolHelpers
   YAMLModuleTester.cpp
+
+  LINK_LIBS
+lldbCore
+lldbHost
+lldbPluginExpressionParserClang
+lldbPluginSymbolFileDWARF
+lldbPluginTypeSystemClang
+lldbUtilityHelpers
+
+  LINK_COMPONENTS
+ObjectYAML
   )



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

[Lldb-commits] [PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Gabor Marton via Phabricator via lldb-commits]

martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:1737
 
 // If we are in the process of ImportDefinition(...) for a RecordDecl we
 // want to make sure that we are also completing each FieldDecl. There

`ImportDefinition(...)` here refers to `ASTImporter::ImportDefinition(Decl*)` 
and not any of the `ASTNodeImporter::ImportDefinition` funcitons. This is a 
very important distinction, because the former is part of the public interface, 
while the latter are private implementation functions. Could you please fix 
this in the comment?



Comment at: clang/lib/AST/ASTImporter.cpp:1755-1759
+  QualType FromTy = ArrayFrom->getElementType();
+  QualType ToTy = ArrayTo->getElementType();
+
+  FromRecordDecl = FromTy->getAsRecordDecl();
+  ToRecordDecl = ToTy->getAsRecordDecl();

labath wrote:
> What about 2- or n-dimensional arrays?
@labath, this is a very good question! And made me realize that D71378 is 
fundamentally flawed (@shafik, please take no offense). Let me explain.

So, with D71378, we added the `if (ImportedOrErr) { ... }` block to import 
definitions specifically of fields' Record types. But we forget to handle 
arrays. Now we may forget to handle multidimensional arrays ... and we may 
forget to handle other language constructs. So, we would finally end up in 
re-implementing the logic of `ASTNodeImporter::VisitFieldDecl`.

So all this should have been handled properly by the preceding import call of 
the FieldDecl! Here
```
1735: ExpectedDecl ImportedOrErr = import(From);
```
I have a suspicion that real reason why this import call fails in case of the 
public ASTImporter::ImportDefinition() is that we fail to drive through the 
import kind (`IDK_Everything`) during the import process.
Below we set IDK_Everything and start a complete import process.
```
  8784   if (auto *ToRecord = dyn_cast(To)) {
  8785 if (!ToRecord->getDefinition()) {
  8786   return Importer.ImportDefinition(   // 
ASTNodeImporter::ImportDefinition !
  8787   cast(FromDC), ToRecord,
  8788   ASTNodeImporter::IDK_Everything);
  8789 }
  8790   }
```
However, there may be many places where we fail to channel through that we want 
to do a complete import. E.g here
```
1957   ImportDefinitionIfNeeded(Base1.getType()->getAsCXXRecordDecl()))
```
we do another definition import and IDK_Everything is not set. So we may have a 
wrong kind of import since the "minimal" flag is set.

The thing is, it is really confusing and error-prone to have both the 
`ASTImporter::Minimal` flag and the `ImportDefinitionKind`. They seem to be in 
contradiction to each other some times.
I think we should get rid of the Minimal flag. And Kind should be either a full 
completion (IDK_Everythink) or just a minimal (IDK_Basic). So, I'd scrap the 
IDK_Default too. Alternatively we could have a Kind member in 
AST**//Node//**Importer.
I think we should go into this direction to avoid similar problems during 
CodeGen in the future. WDYT?


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

https://reviews.llvm.org/D86660

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

[Lldb-commits] [PATCH] D86722: [lldb] Make lldb-argdumper a dependency of liblldb

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: kastiglione, davide.
Herald added a subscriber: mgorny.
JDevlieghere requested review of this revision.

Always make `lldb-argdumper` a dependency of `liblldb`. Currently it is only a 
dependency of the python swig target because of the relative symlink in the 
python resource directory. That means that the dependency won't be there when 
`LLDB_ENABLE_PYTHON` is disabled.


https://reviews.llvm.org/D86722

Files:
  lldb/tools/argdumper/CMakeLists.txt


Index: lldb/tools/argdumper/CMakeLists.txt
===
--- lldb/tools/argdumper/CMakeLists.txt
+++ lldb/tools/argdumper/CMakeLists.txt
@@ -4,3 +4,5 @@
   LINK_COMPONENTS
 Support
 )
+
+add_dependencies(liblldb lldb-argdumper)


Index: lldb/tools/argdumper/CMakeLists.txt
===
--- lldb/tools/argdumper/CMakeLists.txt
+++ lldb/tools/argdumper/CMakeLists.txt
@@ -4,3 +4,5 @@
   LINK_COMPONENTS
 Support
 )
+
+add_dependencies(liblldb lldb-argdumper)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D86521: Revert "Use find_library for ncurses"

[Mateusz Mikuła via Phabricator via lldb-commits]

mati865 added a comment.

FYI this doesn't fully fix MinGW issue:

  $ cat tools/llvm-config/BuildVariables.inc | grep LLVM_SYSTEM_LIBS
  #define LLVM_SYSTEM_LIBS "-lpsapi -lshell32 -lole32 -luuid -ladvapi32 -lz.dll"

It's because libraries are found by their import library which is `libz.dll.a` 
in this case.
`libz.dll.a` matches `if` in line 213 and gets stripped to `z.dll` which 
doesn't match shared library regex `lib.*\.dll`.

The easiest fix would be adding:

  if(CMAKE_IMPORT_LIBRARY_PREFIX AND CMAKE_IMPORT_LIBRARY_SUFFIX AND
  zlib_library MATCHES 
"^${CMAKE_IMPORT_LIBRARY_PREFIX}.*${CMAKE_IMPORT_LIBRARY_SUFFIX }$")
STRING(REGEX REPLACE "^${CMAKE_IMPORT_LIBRARY_PREFIX}" "" zlib_library 
${zlib_library})
STRING(REGEX REPLACE "${CMAKE_IMPORT_LIBRARY_SUFFIX }$" "" zlib_library 
${zlib_library})
  endif()

Should it fixed in separate diff given the fact this is a revert?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86521

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

[Lldb-commits] [lldb] b981924 - [lldb] Move triple construction out of getArchCFlags in DarwinBuilder (NFC)

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-08-27T09:31:01-07:00
New Revision: b981924bdda71b610c349a1d502ba83af632ae98

URL: 
https://github.com/llvm/llvm-project/commit/b981924bdda71b610c349a1d502ba83af632ae98
DIFF: 
https://github.com/llvm/llvm-project/commit/b981924bdda71b610c349a1d502ba83af632ae98.diff

LOG: [lldb] Move triple construction out of getArchCFlags in DarwinBuilder (NFC)

Move the construction of the triple out of getArchCFlags in the
DarwinBuilder.

Added: 


Modified: 
lldb/packages/Python/lldbsuite/test/builders/darwin.py

Removed: 




diff  --git a/lldb/packages/Python/lldbsuite/test/builders/darwin.py 
b/lldb/packages/Python/lldbsuite/test/builders/darwin.py
index f9005397d50e..4548217c3fab 100644
--- a/lldb/packages/Python/lldbsuite/test/builders/darwin.py
+++ b/lldb/packages/Python/lldbsuite/test/builders/darwin.py
@@ -23,7 +23,35 @@ def get_os_env_from_platform(platform):
 def get_os_from_sdk(sdk):
 return sdk[:sdk.find('.')], ""
 
-from lldbsuite.test import configuration
+
+def get_os_and_env():
+if configuration.lldb_platform_name:
+return get_os_env_from_platform(configuration.lldb_platform_name)
+if configuration.apple_sdk:
+return get_os_from_sdk(configuration.apple_sdk)
+return None, None
+
+
+def get_triple():
+# Construct the vendor component.
+vendor = "apple"
+
+# Construct the os component.
+os, env = get_os_and_env()
+if os is None or env is None:
+return None, None, None, None
+
+# Get the SDK from the os and env.
+sdk = lldbutil.get_xcode_sdk(os, env)
+if not sdk:
+return None, None, None, None
+
+# Get the version from the SDK.
+version = lldbutil.get_xcode_sdk_version(sdk)
+if not version:
+return None, None, None, None
+
+return vendor, os, version, env
 
 
 class BuilderDarwin(Builder):
@@ -37,50 +65,24 @@ def getExtraMakeArgs(self):
 if configuration.dsymutil:
 args['DSYMUTIL'] = configuration.dsymutil
 
-operating_system, _ = self.getOsAndEnv()
+operating_system, _ = get_os_and_env()
 if operating_system and operating_system != "macosx":
 builder_dir = os.path.dirname(os.path.abspath(__file__))
 test_dir = os.path.dirname(builder_dir)
 entitlements = os.path.join(test_dir, 'make', 'entitlements.plist')
-args['CODESIGN'] = 'codesign --entitlements 
{}'.format(entitlements)
+args['CODESIGN'] = 'codesign --entitlements {}'.format(
+entitlements)
 
 # Return extra args as a formatted string.
 return ' '.join(
 {'{}="{}"'.format(key, value)
  for key, value in args.items()})
-def getOsAndEnv(self):
-if configuration.lldb_platform_name:
-return get_os_env_from_platform(configuration.lldb_platform_name)
-elif configuration.apple_sdk:
-return get_os_from_sdk(configuration.apple_sdk)
-return None, None
 
 def getArchCFlags(self, architecture):
 """Returns the ARCH_CFLAGS for the make system."""
-
-# Construct the arch component.
-arch = architecture if architecture else configuration.arch
-if not arch:
-arch = subprocess.check_output(['machine'
-]).rstrip().decode('utf-8')
-if not arch:
-return ""
-
-# Construct the vendor component.
-vendor = "apple"
-
-# Construct the os component.
-os, env = self.getOsAndEnv()
-if os is None or env is None:
-return ""
-
-# Get the SDK from the os and env.
-sdk = lldbutil.get_xcode_sdk(os, env)
-if not sdk:
-return ""
-
-version = lldbutil.get_xcode_sdk_version(sdk)
-if not version:
+# Get the triple components.
+vendor, os, version, env = get_triple()
+if not vendor or not os or not version or not env:
 return ""
 
 # Construct the triple from its components.



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

[Lldb-commits] [PATCH] D86722: [lldb] Make lldb-argdumper a dependency of liblldb

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa7e4a1773535: [lldb] Make lldb-argdumper a dependency of 
liblldb (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86722

Files:
  lldb/tools/argdumper/CMakeLists.txt


Index: lldb/tools/argdumper/CMakeLists.txt
===
--- lldb/tools/argdumper/CMakeLists.txt
+++ lldb/tools/argdumper/CMakeLists.txt
@@ -4,3 +4,5 @@
   LINK_COMPONENTS
 Support
 )
+
+add_dependencies(liblldb lldb-argdumper)


Index: lldb/tools/argdumper/CMakeLists.txt
===
--- lldb/tools/argdumper/CMakeLists.txt
+++ lldb/tools/argdumper/CMakeLists.txt
@@ -4,3 +4,5 @@
   LINK_COMPONENTS
 Support
 )
+
+add_dependencies(liblldb lldb-argdumper)
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] a7e4a17 - [lldb] Make lldb-argdumper a dependency of liblldb

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-08-27T09:31:02-07:00
New Revision: a7e4a1773535c64dea5c1d72d6a0a3e24378eaa1

URL: 
https://github.com/llvm/llvm-project/commit/a7e4a1773535c64dea5c1d72d6a0a3e24378eaa1
DIFF: 
https://github.com/llvm/llvm-project/commit/a7e4a1773535c64dea5c1d72d6a0a3e24378eaa1.diff

LOG: [lldb] Make lldb-argdumper a dependency of liblldb

Always make lldb-argdumper a dependency of liblldb. Currently it is only
a dependency of the python swig target because of the relative symlink
in the python resource directory. That means that the dependency won't
be there when LLDB_ENABLE_PYTHON is disabled.

Differential revision: https://reviews.llvm.org/D86722

Added: 


Modified: 
lldb/tools/argdumper/CMakeLists.txt

Removed: 




diff  --git a/lldb/tools/argdumper/CMakeLists.txt 
b/lldb/tools/argdumper/CMakeLists.txt
index 924946325194..29a2186af3cb 100644
--- a/lldb/tools/argdumper/CMakeLists.txt
+++ b/lldb/tools/argdumper/CMakeLists.txt
@@ -4,3 +4,5 @@ add_lldb_tool(lldb-argdumper ADD_TO_FRAMEWORK
   LINK_COMPONENTS
 Support
 )
+
+add_dependencies(liblldb lldb-argdumper)



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

[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Do people really call command-line shells interpreters?  I would have thought 
--shell would be a better name.  lldb already has its own command interpreter 
which is orthogonal to the shells, so it seems confusing to use the same term 
for both.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86667

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

[Lldb-commits] [PATCH] D86388: Fix use-after-free in ThreadPlan, and add test.

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I want to separate out two things here.  One is whether lldb should internally 
ask questions of a thread once we've invalidated the thread list before 
running, the other is how we present threads to the user while the process is 
running.

I was only suggesting restricting  the former.  Once we've put the internal 
data structure that handles the thread list in an undefined state because we 
are about to resume, we shouldn't turn around and ask questions of it.  That 
just seems like a bad practice.

The other question is what does it mean to hand out a thread list when the 
process is running.  It has to be a historical artifact, right?  We don't 
actually know that the threads we are handing out still exist, so I'm not 
really sure what handing them out would mean.  For some platforms we might be 
able to track threads coming and going, but I'm pretty sure that we can't 
require that everywhere, os it can't be an essential part of lldb's design.  In 
general, when the process is "continuing" lldb would like to change its 
behavior as little as possible, so unless there's a way to keep the thread list 
accurate while running that doesn't involve stopping and starting the process, 
we would rather not track this live.  I know that both the Darwin user space 
and Darwin kernel don't provide such support.

And we can't answer any meaningful questions about the thread without pausing 
at least that thread.  In non-stop mode, the threads that are running are still 
in an uncertain state, and only the threads that are stopped are known.  So 
even when lldb supports keeping some threads running while others are stopped, 
we'll have to maintain a distinction between the stopped and the running 
threads, and be clear that to know something about a running thread we'll have 
to stop it.  In fact, because the point of non-stop debugging is that there are 
some parts of the system that you really don't want to stop, in that mode we 
need to be even more careful to be clear about what operations do and don't 
require stopping the target.

But again, I don't think we need to have that discussion w.r.t. the current 
problem.  This is all about how we treat our internal state, not what we show 
to users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86388

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

[Lldb-commits] [PATCH] D86652: [lldb] Fix ProcessEventData::DoOnRemoval to support multiple listeners

[Ilya Nozhkin via Phabricator via lldb-commits]

ilya-nozhkin added a comment.

Yeah, I understand the problem of two listeners trying to change process state 
simultaneously and I agree that informing other listeners once the primary one 
finished its job would be a great solution. But the problem is that primary 
listener just provides an event to some user code (via GetEvent) and there is 
no way to find out when this code has finished without requiring it to invoke a 
handshake explicitly.

One of the possible solutions is to send a specifically marked event to a 
primary listener first. Then, make this event to invoke a handshake in its 
destructor (which would mean that primary listener doesn't need the event 
anymore). And handshake can be implemented as just a rebroadcasting a new 
instance of the event to other listeners (or via unlocking some additional 
mutex / notifying some conditional variable). However, destructor may be 
invoked not immediately after event processing is finished. For example, if a 
primary listener's thread defines an EventSP before the listening cycle and 
doesn't reset it until GetEvent returns a new one (which may not happen until 
the next stop, for example).

Anyway, the concept of primary and secondary listeners requires to forbid 
secondary listeners to do "step" or "continue" at all. So, why can't we do the 
same without any additional code? I mean, I think that the most common case of 
multiple listeners is a Python extension adding a listener to read/write some 
memory/registers on each stop taking the RunLock. So, it does not interfere 
with a primary listener except for simultaneous DoOnRemoval invocation (which 
is fixed by this patch). Moreover, as I understand, there is no any way to 
implement such extension without adding a new listener (because if one would 
try to use Debugger's default listener then all other users such as 
lldb-vscode, for example, couldn't handle process events anymore).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86652

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

[Lldb-commits] [PATCH] D86670: [intel-pt] Add a basic implementation of the dump command

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/include/lldb/Target/Target.h:1105
 
+  void SetTrace(const lldb::TraceSP &trace_sp);
+

wallace wrote:
> wallace wrote:
> > JDevlieghere wrote:
> > > Who owns the trace? If there's a 1:1 relationship between a trace and a 
> > > target, can we make the target its owner? I'm trying to avoid adding 
> > > shared pointers if possible. 
> > Well, there's a 1 to many relationship. Many targets can own the same trace 
> > object, as a single trace can have data of several processes.  On the other 
> > hand, there should no other object that could own a trace. I haven't found 
> > a better solution :(
> What do you think about changing GetTrace() to returns a Trace & instead of 
> TraceSP &, so that no other object can share the ownership?
So this is an instance of a Trace plug-in for a given set of data described by 
the JSON. Since trace files can contain multiple processes, this will need to 
be shared between multiple Target instances if the "trace load" decides to load 
all processes, not just one. So I think this should remain a TraceSP so that an 
instance of the trace plug-in can be shared.



Comment at: lldb/include/lldb/Target/Target.h:1341
   unsigned m_next_persistent_variable_index = 0;
+  lldb::TraceSP m_trace;
   /// Stores the frame recognizers of this target.

wallace wrote:
> JDevlieghere wrote:
> > Doxygen comment?
> good catch
rename to "m_trace_sp" to indicate shared pointer here. See my previous comment 
for why we need a share pointer.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:97
 
-void TraceIntelPT::Dump(lldb_private::Stream *s) const {}
+void TraceIntelPT::Dump(lldb_private::Stream *s) const {
+  s->PutCString("Settings\n");

Since one Trace plug-in instance can contain trace data for one or more 
processes, would it make sense to have more arguments here? "bool verbose" 
which would enable dumping of settings? "lldb::pid_t pid" to dump the data for 
just one process? "lldb::tid_t tid" to dump the data for just one thread? 

Or we might want to add functions to lldb_private::Target that can dump the 
trace data? Like maybe:

```
void Target::DumpTrace(Stream &strm, lldb::tid_t tid = LLDB_INVALID_THREAD_ID) {
  if (!m_trace_sp)
strm.PutCString("error: no trace data in target");
  ThreadSP thread_sp;
  if (tid != LLDB_INVALID_THREAD_ID)
thread_sp = m_process_sp->FindThreadByID(tid);
  m_trace_sp->Dump(strm, m_process_sp.get(), thread_sp.get());
}
```

Then the Trace::Dump would look like:

```
void Trace::Dump(Stream &strm, Process *process, Thread *thread, bool verbose);
```
If there is no process, dump it all trace data for all processes and all 
threads in each process.
If there is a process and no thread, dump all threads from that process.
If there is a process and thread, dump just the data for that thread

The "trace dump" command can then compose calls using this API for any 
arguments might receive?

Dump all processes all threads:
```
(lldb) trace dump
```

Dump all threads for the specified processes:
```
(lldb) trace dump --pid 123  --pid 234
```

Dump all one thread from one process:
```
(lldb) trace dump --pid 123 --tid 345
```
If the --tid is specified, then we can have only one "--pid" argument.



Comment at: lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp:98-106
+  s->PutCString("Settings\n");
+  s->Indent();
+  m_settings.Dump(*s, /*pretty_print*/ true);
+  s->IndentLess();
+
+  s->PutCString("\nSettings directory\n");
+  s->Indent();

Settings should probably be only dumped if the "bool verbose" argument is true?



Comment at: lldb/source/Target/Target.cpp:2970
+
+lldb::TraceSP &Target::GetTrace() { return m_trace; }
+

Should we return a "Trace *" from this? If nullptr, then there is no trace 
data, else we have valid trace data? No one other than Target instances should 
be adding to the ref count. But I guess this is safer as if someone wants to 
use the returned Trace pointer, some other thread could free it and cause a 
crash. So maybe leaving as a TraceSP is a good idea...



Comment at: lldb/test/API/commands/trace/TestTraceLoad.py:70
+
+self.expect("trace dump", substrs=["the current target does not have 
any traces"], error=True)

We should probably test --pid and --thread here if we decide the APIs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86670

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

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

In D85705#2241249 , @labath wrote:

> If the format is going to be json, why not use llvm::json from 
> `llvm/Support/JSON.h`? That's what we been migrating most of the existing 
> stuff to already, so going using that for new code makes perfect sense.

I am fine with this if we are ok always using JSON. StructuredData, when it 
parses JSON, is backed by llvm/Support/JSON.h, so we are already using it. If 
we don't need the flexibility to use other structured data formats we can do 
this.




Comment at: lldb/include/lldb/Target/Trace.h:157
+  std::string m_settings_dir;
+  llvm::Optional m_global_trace_file;
+  std::map>

I know I suggested we might need a trace file for all process, one per process, 
or one per thread in the JSON schema. The question is do we need to add support 
for this right away? Might be easier to go with one trace file for now unless 
IntelPT already supports each of these?



Comment at: lldb/source/Target/Trace.cpp:54
+std::string raw_schema(R"({
+  "plugin": <<>>,
+  "triple": string, // llvm-triple

Can we inline the plug-ins schema right into this stream instead of putting the 
"<<>>" text in here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705

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

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

[Yifan Shen via Phabricator via lldb-commits]

aelitashen updated this revision to Diff 288456.
aelitashen added a comment.

Keep symbol status till new version released, remove comments in tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86662

Files:
  lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py
  lldb/tools/lldb-vscode/JSONUtils.cpp


Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -354,9 +354,7 @@
 
 static std::string ConvertDebugInfoSizeToString(uint64_t debug_info) {
   std::ostringstream oss;
-  oss << " (";
   oss << std::fixed << std::setprecision(1);
-
   if (debug_info < 1024) {
 oss << debug_info << "B";
   } else if (debug_info < 1024 * 1024) {
@@ -368,9 +366,7 @@
   } else {
 double gb = double(debug_info) / (1024.0 * 1024.0 * 1024.0);
 oss << gb << "GB";
-;
   }
-  oss << ")";
   return oss.str();
 }
 llvm::json::Value CreateModule(lldb::SBModule &module) {
@@ -386,11 +382,13 @@
   object.try_emplace("path", module_path);
   if (module.GetNumCompileUnits() > 0) {
 std::string symbol_str = "Symbols loaded.";
+std::string debug_info_size;
 uint64_t debug_info = GetDebugInfoSize(module);
 if (debug_info > 0) {
-  symbol_str += ConvertDebugInfoSizeToString(debug_info);
+  debug_info_size = ConvertDebugInfoSizeToString(debug_info);
 }
 object.try_emplace("symbolStatus", symbol_str);
+object.try_emplace("debugInfoSize", debug_info_size);
 char symbol_path_arr[PATH_MAX];
 module.GetSymbolFileSpec().GetPath(symbol_path_arr,
sizeof(symbol_path_arr));
Index: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py
===
--- lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py
+++ lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py
@@ -32,26 +32,18 @@
 self.assertIn('path', program_module, 'make sure path is in module')
 self.assertEqual(program, program_module['path'])
 self.assertTrue('symbolFilePath' not in program_module, 'Make sure 
a.out.stripped has no debug info')
-self.assertEqual('Symbols not found.', program_module['symbolStatus'])
 symbols_path = self.getBuildArtifact(symbol_basename)
 self.vscode.request_evaluate('`%s' % ('target symbols add -s "%s" 
"%s"' % (program, symbols_path)))
 
-def checkSymbolsLoaded():
-active_modules = self.vscode.get_active_modules()
-program_module = active_modules[program_basename]
-return 'Symbols loaded.' == program_module['symbolStatus']
-
 def checkSymbolsLoadedWithSize():
 active_modules = self.vscode.get_active_modules()
 program_module = active_modules[program_basename]
-symbolsStatus = program_module['symbolStatus']
-symbol_regex = re.compile(r"Symbols loaded. 
\([0-9]+(\.[0-9]*)?[KMG]?B\)")
+symbolsStatus = program_module['debugInfoSize']
+symbol_regex = re.compile(r"[0-9]+(\.[0-9]*)?[KMG]?B")
 return symbol_regex.match(program_module['symbolStatus'])
 
 if expect_debug_info_size:
 self.waitUntil(checkSymbolsLoadedWithSize)
-else:
-self.waitUntil(checkSymbolsLoaded)
 active_modules = self.vscode.get_active_modules()
 program_module = active_modules[program_basename]
 self.assertEqual(program_basename, program_module['name'])


Index: lldb/tools/lldb-vscode/JSONUtils.cpp
===
--- lldb/tools/lldb-vscode/JSONUtils.cpp
+++ lldb/tools/lldb-vscode/JSONUtils.cpp
@@ -354,9 +354,7 @@
 
 static std::string ConvertDebugInfoSizeToString(uint64_t debug_info) {
   std::ostringstream oss;
-  oss << " (";
   oss << std::fixed << std::setprecision(1);
-
   if (debug_info < 1024) {
 oss << debug_info << "B";
   } else if (debug_info < 1024 * 1024) {
@@ -368,9 +366,7 @@
   } else {
 double gb = double(debug_info) / (1024.0 * 1024.0 * 1024.0);
 oss << gb << "GB";
-;
   }
-  oss << ")";
   return oss.str();
 }
 llvm::json::Value CreateModule(lldb::SBModule &module) {
@@ -386,11 +382,13 @@
   object.try_emplace("path", module_path);
   if (module.GetNumCompileUnits() > 0) {
 std::string symbol_str = "Symbols loaded.";
+std::string debug_info_size;
 uint64_t debug_info = GetDebugInfoSize(module);
 if (debug_info > 0) {
-  symbol_str += ConvertDebugInfoSizeToString(debug_info);
+  debug_info_size = ConvertDebugInfoSizeToString(debug_info);
 }
 object.try_emplace("symbolStatus", symbol_str);
+object.try_emplace("debugInfoSize", debug_info_size);
 char symbol_path_arr[PATH_MAX];
 module.GetSymbolFileSpec().GetPath

[Lldb-commits] [PATCH] D86660: Modifying ImportDeclContext(...) to ensure that we also handle the case when the FieldDecl is an ArrayType whose ElementType is a RecordDecl

[Shafik Yaghmour via Phabricator via lldb-commits]

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



Comment at: clang/lib/AST/ASTImporter.cpp:1755-1759
+  QualType FromTy = ArrayFrom->getElementType();
+  QualType ToTy = ArrayTo->getElementType();
+
+  FromRecordDecl = FromTy->getAsRecordDecl();
+  ToRecordDecl = ToTy->getAsRecordDecl();

martong wrote:
> labath wrote:
> > What about 2- or n-dimensional arrays?
> @labath, this is a very good question! And made me realize that D71378 is 
> fundamentally flawed (@shafik, please take no offense). Let me explain.
> 
> So, with D71378, we added the `if (ImportedOrErr) { ... }` block to import 
> definitions specifically of fields' Record types. But we forget to handle 
> arrays. Now we may forget to handle multidimensional arrays ... and we may 
> forget to handle other language constructs. So, we would finally end up in 
> re-implementing the logic of `ASTNodeImporter::VisitFieldDecl`.
> 
> So all this should have been handled properly by the preceding import call of 
> the FieldDecl! Here
> ```
> 1735: ExpectedDecl ImportedOrErr = import(From);
> ```
> I have a suspicion that real reason why this import call fails in case of the 
> public ASTImporter::ImportDefinition() is that we fail to drive through the 
> import kind (`IDK_Everything`) during the import process.
> Below we set IDK_Everything and start a complete import process.
> ```
>   8784   if (auto *ToRecord = dyn_cast(To)) {
>   8785 if (!ToRecord->getDefinition()) {
>   8786   return Importer.ImportDefinition(   // 
> ASTNodeImporter::ImportDefinition !
>   8787   cast(FromDC), ToRecord,
>   8788   ASTNodeImporter::IDK_Everything);
>   8789 }
>   8790   }
> ```
> However, there may be many places where we fail to channel through that we 
> want to do a complete import. E.g here
> ```
> 1957   
> ImportDefinitionIfNeeded(Base1.getType()->getAsCXXRecordDecl()))
> ```
> we do another definition import and IDK_Everything is not set. So we may have 
> a wrong kind of import since the "minimal" flag is set.
> 
> The thing is, it is really confusing and error-prone to have both the 
> `ASTImporter::Minimal` flag and the `ImportDefinitionKind`. They seem to be 
> in contradiction to each other some times.
> I think we should get rid of the Minimal flag. And Kind should be either a 
> full completion (IDK_Everythink) or just a minimal (IDK_Basic). So, I'd scrap 
> the IDK_Default too. Alternatively we could have a Kind member in 
> AST**//Node//**Importer.
> I think we should go into this direction to avoid similar problems during 
> CodeGen in the future. WDYT?
No offense, you definitely understand the Importer better than I, so I value 
your input here. I don't believe that should have other fields where we could 
have a record that effects the layout but the concern is still valid and yes I 
did miss multi-dimensional arrays.

Your theory on `IDK_Everything` not be pushed through everywhere, I did a quick 
look and it seems pretty reasonable. 

I agree that the `ASTImporter::Minimal` flag and the `ImportDefinitionKind` 
seem to inconsistent or perhaps a work-around. That seems like a bigger change 
with a lot more impact beyond this fix but worth looking into longer term. 

If pushing through `IDK_Everything` everywhere fixes the underlying issue I am 
very happy to take that approach. If not we can discuss alternatives. 



Comment at: 
lldb/test/API/commands/expression/codegen-crash-import-def-arraytype-element/main.cpp:22-27
+  union {
+struct {
+  unsigned char *_s;
+} t;
+char *tt[1];
+  } U;

labath wrote:
> What's the significance of this union?
Not sure, I was not able to discern why this is important to reproduce the 
crash. I wanted feedback on the approach so I left to further investigation 
later on. 


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

https://reviews.llvm.org/D86660

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

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

So we are adding a custom new "debugInfoSize" field to the "module" JSON 
dictionary? What happens in the release VS code? Does this information not get 
displayed, but it will get displayed in our custom modified VS Code IDE?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86662

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

[Lldb-commits] [PATCH] D86662: Simplify Symbol Status Message to Only Debug Info Size

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

All module information is only displayed in our custom VSCode. Vanilla VSCode 
doesn't have support for displaying modules.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86662

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

[Lldb-commits] [PATCH] D86745: [lldb/test] Use @skipIfWindows for PExpectTest

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht created this revision.
rupprecht added a reviewer: teemperor.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
rupprecht requested review of this revision.
Herald added a subscriber: JDevlieghere.

Annotating `PExpectTest` with `@skipIfWindows` instead of marking it as an 
empty class will make the test runner recognize it as a test class, which 
should allow me to reland adb5c23f8c0d60eeec41dcbe21d1b26184e1c97d 
.

I don't have a windows machine to verify this works, but I did some tests using 
`@skipIfLinux` and they all worked as expected. In case the `pexpect` import is 
not at all available on windows, I moved it to within the method where it's 
used.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86745

Files:
  lldb/packages/Python/lldbsuite/test/lldbpexpect.py

Index: lldb/packages/Python/lldbsuite/test/lldbpexpect.py
===
--- lldb/packages/Python/lldbsuite/test/lldbpexpect.py
+++ lldb/packages/Python/lldbsuite/test/lldbpexpect.py
@@ -13,65 +13,60 @@
 from . import lldbutil
 from lldbsuite.test.decorators import *
 
-if sys.platform.startswith('win32'):
-# llvm.org/pr22274: need a pexpect replacement for windows
-class PExpectTest(object):
-pass
-else:
-import pexpect
+@skipIfRemote
+@skipIfWindows  # llvm.org/pr22274: need a pexpect replacement for windows
+class PExpectTest(TestBase):
 
-@skipIfRemote
-class PExpectTest(TestBase):
+NO_DEBUG_INFO_TESTCASE = True
+PROMPT = "(lldb) "
 
-NO_DEBUG_INFO_TESTCASE = True
-PROMPT = "(lldb) "
+def expect_prompt(self):
+self.child.expect_exact(self.PROMPT)
 
-def expect_prompt(self):
-self.child.expect_exact(self.PROMPT)
+def launch(self, executable=None, extra_args=None, timeout=30, dimensions=None):
+logfile = getattr(sys.stdout, 'buffer',
+sys.stdout) if self.TraceOn() else None
 
-def launch(self, executable=None, extra_args=None, timeout=30, dimensions=None):
-logfile = getattr(sys.stdout, 'buffer',
-  sys.stdout) if self.TraceOn() else None
+args = ['--no-lldbinit', '--no-use-colors']
+for cmd in self.setUpCommands():
+args += ['-O', cmd]
+if executable is not None:
+args += ['--file', executable]
+if extra_args is not None:
+args.extend(extra_args)
 
-args = ['--no-lldbinit', '--no-use-colors']
-for cmd in self.setUpCommands():
-args += ['-O', cmd]
-if executable is not None:
-args += ['--file', executable]
-if extra_args is not None:
-args.extend(extra_args)
+env = dict(os.environ)
+env["TERM"]="vt100"
 
-env = dict(os.environ)
-env["TERM"]="vt100"
-
-self.child = pexpect.spawn(
-lldbtest_config.lldbExec, args=args, logfile=logfile,
-timeout=timeout, dimensions=dimensions, env=env)
+import pexpect
+self.child = pexpect.spawn(
+lldbtest_config.lldbExec, args=args, logfile=logfile,
+timeout=timeout, dimensions=dimensions, env=env)
+self.expect_prompt()
+for cmd in self.setUpCommands():
+self.child.expect_exact(cmd)
 self.expect_prompt()
-for cmd in self.setUpCommands():
-self.child.expect_exact(cmd)
-self.expect_prompt()
-if executable is not None:
-self.child.expect_exact("target create")
-self.child.expect_exact("Current executable set to")
-self.expect_prompt()
-
-def expect(self, cmd, substrs=None):
-self.assertNotIn('\n', cmd)
-self.child.sendline(cmd)
-if substrs is not None:
-for s in substrs:
-self.child.expect_exact(s)
+if executable is not None:
+self.child.expect_exact("target create")
+self.child.expect_exact("Current executable set to")
 self.expect_prompt()
 
-def quit(self, gracefully=True):
-self.child.sendeof()
-self.child.close(force=not gracefully)
-self.child = None
+def expect(self, cmd, substrs=None):
+self.assertNotIn('\n', cmd)
+self.child.sendline(cmd)
+if substrs is not None:
+for s in substrs:
+self.child.expect_exact(s)
+self.expect_prompt()
+
+def quit(self, gracefully=True):
+self.child.sendeof()
+self.child.close(force=not gracefully)
+self.child = None
 
-def cursor_forward_escape_seq(self, chars_to_move):
-"""
-Returns the escape sequence to mo

[Lldb-commits] [PATCH] D86652: [lldb] Fix ProcessEventData::DoOnRemoval to support multiple listeners

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This still seems to me a pretty fragile way to program lldb.  For instance, if 
the controlling thread gets the stop event first, and decides it wants to 
continue, it will set the process running again and by the time the memory 
reading thread gets the event, the process will have already proceeded and the 
memory read will fail.  But it it happens the other way around, it will 
succeed.  That will make for subtle bugs in your implementation.

Having a bunch of event listeners hanging out doing ancillary stop tasks also 
makes it hard to do cancellation.  For instance, if the user presses "step" 
again while you are still doing the work to present the current stop, you want 
to discard all the work you were doing for the first stop and just step again.  
People like to drive from an interesting place to another interesting place by 
whaling on the "next" button and want that to be very responsive and don't care 
if all the data isn't updated in getting there.  As long as the UI knows what 
work it cancelled and doesn't mark data as updated if it wasn't, this does seem 
to better model how people told us they want GUI debuggers to behave.

You could also implement this sort of thing with stop hooks that get run before 
any of the other actions are taken by then controlling thread (or by 
implementing this yourself.)  There isn't currently a way to write stop hooks 
in Python but it would be simple to add that.  The stop hooks don't solve the 
cancellation problem, however.

Anyway, "I wouldn't do it that way" isn't a good reason not to make a thing 
possible.  If you feel like you can have success with this added ability, I 
can't see how it would do harm.

You still need to add some tests for this new behavior.  We have been telling 
people not to do it this way pretty much forever, and so there are no tests for 
multiple process listeners.  It would be good to write a not entirely trivial 
test that tries to do a little simultaneous work in your different Listeners, 
and deal with DoOnRemoval restarting the target, as well as one of the 
Listeners doing so, to ensure we don't fall over for other reasons when we 
actually try to use the feature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86652

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

[Lldb-commits] [PATCH] D86752: [lldb/test] Use shorter test case names in TestStandardUnwind

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
rupprecht requested review of this revision.
Herald added a subscriber: JDevlieghere.

TestStandardUnwind uses the full absolute path to a set of C/C++ files as the 
test case name, which in turn is used in the name of a log file. When the 
source file is long, and the directory where log files are stored is also long, 
this causes an OSError because the log filename is too long.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86752

Files:
  lldb/test/API/functionalities/unwind/standard/TestStandardUnwind.py


Index: lldb/test/API/functionalities/unwind/standard/TestStandardUnwind.py
===
--- lldb/test/API/functionalities/unwind/standard/TestStandardUnwind.py
+++ lldb/test/API/functionalities/unwind/standard/TestStandardUnwind.py
@@ -164,7 +164,7 @@
 self.skipTest("Inferior not supported")
 self.standard_unwind_tests()
 
-test_name = "test_unwind_" + str(f)
+test_name = "test_unwind_" + str(os.path.basename(f))
 for c in ".=()/\\":
 test_name = test_name.replace(c, '_')
 


Index: lldb/test/API/functionalities/unwind/standard/TestStandardUnwind.py
===
--- lldb/test/API/functionalities/unwind/standard/TestStandardUnwind.py
+++ lldb/test/API/functionalities/unwind/standard/TestStandardUnwind.py
@@ -164,7 +164,7 @@
 self.skipTest("Inferior not supported")
 self.standard_unwind_tests()
 
-test_name = "test_unwind_" + str(f)
+test_name = "test_unwind_" + str(os.path.basename(f))
 for c in ".=()/\\":
 test_name = test_name.replace(c, '_')
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 7f717b6 - [lldb] Fix "no matching std::pair constructor" on Ubuntu 16.04 (NFC)

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-08-27T17:23:44-07:00
New Revision: 7f717b6d1f65f8474e8633b040a16c55f0ad6b96

URL: 
https://github.com/llvm/llvm-project/commit/7f717b6d1f65f8474e8633b040a16c55f0ad6b96
DIFF: 
https://github.com/llvm/llvm-project/commit/7f717b6d1f65f8474e8633b040a16c55f0ad6b96.diff

LOG: [lldb] Fix "no matching std::pair constructor" on Ubuntu 16.04 (NFC)

Fixes error: no matching constructor for initialization of
'std::pair, std::__cxx11::basic_string >'
with older toolchain (clang/libcxx) on Ubuntu 16.04. The issue is the
StringRef-to-std::string conversion.

Added: 


Modified: 
lldb/unittests/Symbol/PostfixExpressionTest.cpp

Removed: 




diff  --git a/lldb/unittests/Symbol/PostfixExpressionTest.cpp 
b/lldb/unittests/Symbol/PostfixExpressionTest.cpp
index 7def709a6090..aee153d5ccef 100644
--- a/lldb/unittests/Symbol/PostfixExpressionTest.cpp
+++ b/lldb/unittests/Symbol/PostfixExpressionTest.cpp
@@ -111,7 +111,7 @@ ParseFPOAndStringify(llvm::StringRef prog) {
   ParseFPOProgram(prog, alloc);
   std::vector> result;
   for (const auto &p : parsed)
-result.emplace_back(p.first, ASTPrinter::Print(p.second));
+result.emplace_back(p.first.str(), ASTPrinter::Print(p.second));
   return result;
 }
 



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

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 288496.
wallace added a comment.

Addressed comments

- Now using StringRef correctly whenever possible.
- Revert back to using "traceFile" only inside the thread section. That's how 
intel-pt works at the moment and, as Greg suggested, we can change the schema 
in the future if needed.
- Now the global schema construction inlines the plug-in schema.
- Simplified the CMake logic
- Separated the parser from the plugin. Now new plugins need to derive their 
own Trace implementation along with their own Parser.
- I'm still using StructuredData for the parsing. There's a chance that once we 
release this feature users will want support for other formats besides JSON, so 
for now I prefer to ask for JSON input but parse in a format-agnostic way. If 
eventually no one needs any other format, we can switch to JSON-only parsing.
- Also, now the Targets created during parsing are preserved only if the 
parsing succeeded. Otherwise, the Debugger object is left unchanged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705

Files:
  lldb/include/lldb/Core/PluginManager.h
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceSettingsParser.h
  lldb/include/lldb/Utility/StructuredData.h
  lldb/include/lldb/lldb-forward.h
  lldb/include/lldb/lldb-private-interfaces.h
  lldb/source/Commands/CMakeLists.txt
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/CommandObjectTrace.h
  lldb/source/Commands/Options.td
  lldb/source/Core/PluginManager.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Plugins/CMakeLists.txt
  lldb/source/Plugins/Trace/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.h
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceSettingsParser.cpp
  lldb/source/Utility/StructuredData.cpp
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/TestTraceSchema.py
  lldb/test/API/commands/trace/intelpt-trace/3842849.trace
  lldb/test/API/commands/trace/intelpt-trace/a.out
  lldb/test/API/commands/trace/intelpt-trace/main.cpp
  lldb/test/API/commands/trace/intelpt-trace/trace.json
  lldb/test/API/commands/trace/intelpt-trace/trace_bad.json
  lldb/test/API/commands/trace/intelpt-trace/trace_bad2.json
  lldb/test/API/commands/trace/intelpt-trace/trace_bad3.json

Index: lldb/test/API/commands/trace/intelpt-trace/trace_bad3.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace_bad3.json
@@ -0,0 +1,32 @@
+{
+  "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel",
+  "family": 6,
+  "model": 79,
+  "stepping": 1
+}
+  },
+  "triple": "x86_64-*-linux",
+  "processes": [
+{
+  "pid": 1234,
+  "threads": [
+{
+  "tid": 5678,
+  "traceFile": "3842849.trace"
+}
+  ],
+  "modules": [
+{
+  "file": "a.out",
+  "systemPath": "a.out",
+  "loadAddress": "0x0040",
+  "uuid": "6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A"
+}
+  ]
+},
+{}
+  ]
+}
Index: lldb/test/API/commands/trace/intelpt-trace/trace_bad2.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace_bad2.json
@@ -0,0 +1,12 @@
+{
+  "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel",
+  "family": 6,
+  "model": 79,
+  "stepping": 1
+}
+  },
+  "processes": []
+}
Index: lldb/test/API/commands/trace/intelpt-trace/trace_bad.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace_bad.json
@@ -0,0 +1,15 @@
+{
+  "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel",
+  "family": 6,
+  "model": 79,
+  "stepping": 1
+}
+  },
+  "triple": "x86_64-*-linux",
+  "processes": [
+123
+  ]
+}
Index: lldb/test/API/commands/trace/intelpt-trace/trace.json
===
--- /dev/null
+++ lldb/test/API/commands/trace/intelpt-trace/trace.json
@@ -0,0 +1,31 @@
+{
+  "trace": {
+"type": "intel-pt",
+"pt_cpu": {
+  "vendor": "intel",
+  "family": 6,
+  "model": 79,
+  "stepping": 1
+}
+  },
+  "triple": "x86_64-*-linux",
+  "processes": [
+{
+  "pid": 1234,
+  "threads": [
+{
+  "tid": 5678,
+  "traceFile": "3842849.trace"
+}
+  ],
+  "modules": [
+{
+  "file": "a.

[Lldb-commits] [lldb] cdcb9ab - Revert "Use find_library for ncurses"

[Galina Kistanova via lldb-commits]

Author: Harmen Stoppels
Date: 2020-08-27T17:57:26-07:00
New Revision: cdcb9ab10e53ff08293915af3cd897c42112bcc5

URL: 
https://github.com/llvm/llvm-project/commit/cdcb9ab10e53ff08293915af3cd897c42112bcc5
DIFF: 
https://github.com/llvm/llvm-project/commit/cdcb9ab10e53ff08293915af3cd897c42112bcc5.diff

LOG: Revert "Use find_library for ncurses"

The introduction of find_library for ncurses caused more issues than it solved 
problems. The current open issue is it makes the static build of LLVM fail. It 
is better to revert for now, and get back to it later.

Revert "[CMake] Fix an issue where get_system_libname creates an empty regex 
capture on windows"
This reverts commit 1ed1e16ab83f55d85c90ae43a05cbe08a00c20e0.

Revert "Fix msan build"
This reverts commit 34fe9613dda3c7d8665b609136a8c12deb122382.

Revert "[CMake] Always mark terminfo as unavailable on Windows"
This reverts commit 76bf26236f6fd453343666c3cd91de8f74ffd89d.

Revert "[CMake] Fix OCaml build failure because of absolute path in system libs"
This reverts commit 8e4acb82f71ad4effec8895b8fc957189ce95933.

Revert "[CMake] Don't look for terminfo libs when LLVM_ENABLE_TERMINFO=OFF"
This reverts commit 495f91fd33d492941c39424a32cf24bcfe192f35.

Revert "Use find_library for ncurses"
This reverts commit a52173a3e56553d7b795bcf3cdadcf6433117107.

Differential revision: https://reviews.llvm.org/D86521

Added: 


Modified: 
compiler-rt/cmake/config-ix.cmake
compiler-rt/lib/xray/tests/CMakeLists.txt
lldb/source/Core/CMakeLists.txt
llvm/cmake/config-ix.cmake
llvm/include/llvm/Config/config.h.cmake
llvm/lib/Support/CMakeLists.txt
llvm/lib/Support/Unix/Process.inc
llvm/utils/gn/secondary/llvm/include/llvm/Config/BUILD.gn

Removed: 




diff  --git a/compiler-rt/cmake/config-ix.cmake 
b/compiler-rt/cmake/config-ix.cmake
index 1428a514b55a..5f9e868de5fd 100644
--- a/compiler-rt/cmake/config-ix.cmake
+++ b/compiler-rt/cmake/config-ix.cmake
@@ -133,18 +133,17 @@ check_library_exists(pthread pthread_create "" 
COMPILER_RT_HAS_LIBPTHREAD)
 check_library_exists(execinfo backtrace "" COMPILER_RT_HAS_LIBEXECINFO)
 
 # Look for terminfo library, used in unittests that depend on LLVMSupport.
-if(LLVM_ENABLE_TERMINFO STREQUAL FORCE_ON)
-  set(MAYBE_REQUIRED REQUIRED)
-else()
-  set(MAYBE_REQUIRED)
-endif()
 if(LLVM_ENABLE_TERMINFO)
-  find_library(COMPILER_RT_TERMINFO_LIB NAMES terminfo tinfo curses ncurses 
ncursesw ${MAYBE_REQUIRED})
-endif()
-if(COMPILER_RT_TERMINFO_LIB)
-  set(LLVM_ENABLE_TERMINFO 1)
-else()
-  set(LLVM_ENABLE_TERMINFO 0)
+  foreach(library terminfo tinfo curses ncurses ncursesw)
+string(TOUPPER ${library} library_suffix)
+check_library_exists(
+  ${library} setupterm "" COMPILER_RT_HAS_TERMINFO_${library_suffix})
+if(COMPILER_RT_HAS_TERMINFO_${library_suffix})
+  set(COMPILER_RT_HAS_TERMINFO TRUE)
+  set(COMPILER_RT_TERMINFO_LIB "${library}")
+  break()
+endif()
+  endforeach()
 endif()
 
 if (ANDROID AND COMPILER_RT_HAS_LIBDL)

diff  --git a/compiler-rt/lib/xray/tests/CMakeLists.txt 
b/compiler-rt/lib/xray/tests/CMakeLists.txt
index 96a9db1ef877..a1fbccaeb6d2 100644
--- a/compiler-rt/lib/xray/tests/CMakeLists.txt
+++ b/compiler-rt/lib/xray/tests/CMakeLists.txt
@@ -55,7 +55,7 @@ set(XRAY_UNITTEST_LINK_FLAGS
 if (NOT APPLE)
   # Needed by LLVMSupport.
   append_list_if(
-LLVM_ENABLE_TERMINFO
+COMPILER_RT_HAS_TERMINFO
 -l${COMPILER_RT_TERMINFO_LIB} XRAY_UNITTEST_LINK_FLAGS)
 
   if (COMPILER_RT_STANDALONE_BUILD)

diff  --git a/lldb/source/Core/CMakeLists.txt b/lldb/source/Core/CMakeLists.txt
index 01a25045081f..a4057d11077f 100644
--- a/lldb/source/Core/CMakeLists.txt
+++ b/lldb/source/Core/CMakeLists.txt
@@ -11,8 +11,8 @@ set(LLDB_LIBEDIT_LIBS)
 
 if (LLDB_ENABLE_CURSES)
   list(APPEND LLDB_CURSES_LIBS ${CURSES_LIBRARIES} ${PANEL_LIBRARIES})
-  if(LLVM_ENABLE_TERMINFO)
-list(APPEND LLDB_CURSES_LIBS ${TERMINFO_LIB})
+  if(LLVM_ENABLE_TERMINFO AND HAVE_TERMINFO)
+list(APPEND LLDB_CURSES_LIBS ${TERMINFO_LIBS})
   endif()
   if (LLVM_BUILD_STATIC)
 list(APPEND LLDB_CURSES_LIBS gpm)

diff  --git a/llvm/cmake/config-ix.cmake b/llvm/cmake/config-ix.cmake
index 6b92180b739e..72505190e347 100644
--- a/llvm/cmake/config-ix.cmake
+++ b/llvm/cmake/config-ix.cmake
@@ -148,18 +148,19 @@ if(NOT LLVM_USE_SANITIZER MATCHES "Memory.*")
 else()
   set(HAVE_LIBEDIT 0)
 endif()
-if(LLVM_ENABLE_TERMINFO STREQUAL FORCE_ON)
-  set(MAYBE_REQUIRED REQUIRED)
-else()
-  set(MAYBE_REQUIRED)
-endif()
 if(LLVM_ENABLE_TERMINFO)
-  find_library(TERMINFO_LIB NAMES terminfo tinfo curses ncurses ncursesw 
${MAYBE_REQUIRED})
-endif()
-if(TERMINFO_LIB)
-  set(LLVM_ENABLE_TERMINFO 1)
+  set(HAVE_TERMINFO 0)
+  foreach(library terminfo tinfo curses ncurses ncursesw)
+string(TOUPPER ${library} library_suffix)
+check_library_exists(${library} s

[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 9 inline comments as done.
mib added a comment.

In D86667#2242566 , @jingham wrote:

> Do people really call command-line shells interpreters?  I would have thought 
> --shell would be a better name.  lldb already has its own command interpreter 
> which is orthogonal to the shells, so it seems confusing to use the same term 
> for both.

I think `platform shell --shell` sounds/looks repetitive so I opted for 
`-i|--interpreter` instead, as in Command-line **Interpreter**.




Comment at: lldb/source/API/SBPlatform.cpp:59
+
+if (command_interpreter && command_interpreter[0]) {
+  full_command += command_interpreter;

labath wrote:
> kastiglione wrote:
> > JDevlieghere wrote:
> > > Given that this pattern repeats a few times in this struct, maybe a small 
> > > static helper function would be nice:
> > > 
> > > ```static bool is_not_empty(cont char* c) { return c && c[0]; }```
> > Can these be `StringRef`, and then check with `empty()`?
> Rather than duplicating this logic here (and getting it wrong for windows, 
> for nested quotes, etc.) it would be better to just pass the custom shell to 
> the `RunShellCommand` method. Then it use the existing quoting logic, and all 
> that needs to change is that it now picks up the shell from the argument (if 
> passed) instead of the baked-in default.
I think since this method will eventually interact with the Python API, they 
need to be C strings.



Comment at: lldb/test/API/commands/platform/basic/TestPlatformCommand.py:109
+self.assertTrue(err.Success())
+self.assertIn("/bin/sh", sh_cmd.GetOutput())
+

kastiglione wrote:
> 
Since it's a command output, it has a newline character at the end of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86667

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

[Lldb-commits] [PATCH] D86667: [lldb/Target] Add custom interpreter option to `platform shell`

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 288531.
mib marked an inline comment as done.
mib edited the summary of this revision.
mib added a comment.

Address most comments:

- Passed down the shell interpreter path down to `Host::RunShellCommand` to be 
"assembled" with the rest of the command.
- Wrote a custom test program to simplify testing on multiple platforms.
- Updated code by using `llvm::StringRef` instead of CString when possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86667

Files:
  lldb/bindings/interface/SBPlatform.i
  lldb/include/lldb/API/SBPlatform.h
  lldb/include/lldb/Host/Host.h
  lldb/include/lldb/Target/Platform.h
  lldb/include/lldb/Target/RemoteAwarePlatform.h
  lldb/source/API/SBPlatform.cpp
  lldb/source/Commands/CommandObjectPlatform.cpp
  lldb/source/Commands/Options.td
  lldb/source/Host/common/Host.cpp
  lldb/source/Host/macosx/objcxx/Host.mm
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Target/Platform.cpp
  lldb/source/Target/RemoteAwarePlatform.cpp
  lldb/test/API/commands/platform/basic/Makefile
  lldb/test/API/commands/platform/basic/TestPlatformCommand.py
  lldb/test/API/commands/platform/basic/TestPlatformPython.py
  lldb/test/API/commands/platform/basic/myshell.c

Index: lldb/test/API/commands/platform/basic/myshell.c
===
--- /dev/null
+++ lldb/test/API/commands/platform/basic/myshell.c
@@ -0,0 +1,24 @@
+#include 
+#include 
+#include 
+
+int main(int argc, char *argv[]) {
+  if (argc < 3) {
+fprintf(stderr, "ERROR: Too few arguments (count: %d).\n", argc - 1);
+exit(1);
+  }
+
+#ifdef WIN32
+  char *cmd_opt = "/C";
+#else
+  char *cmd_opt = "-c";
+#endif
+
+  if (strncmp(argv[1], cmd_opt, 2)) {
+fprintf(stderr, "ERROR: Missing shell command option ('%s').\n", cmd_opt);
+exit(1);
+  }
+
+  printf("SUCCESS: %s\n", argv[0]);
+  return 0;
+}
Index: lldb/test/API/commands/platform/basic/TestPlatformPython.py
===
--- lldb/test/API/commands/platform/basic/TestPlatformPython.py
+++ lldb/test/API/commands/platform/basic/TestPlatformPython.py
@@ -79,3 +79,24 @@
 self.assertEqual(
 desc_data.GetType(), lldb.eStructuredDataTypeString,
 'Platform description is a string')
+
+@add_test_categories(['pyapi'])
+@no_debug_info_test
+def test_shell_interpreter(self):
+""" Test a shell with a custom interpreter """
+platform = self.dbg.GetSelectedPlatform()
+self.assertTrue(platform.IsValid())
+
+sh_cmd = lldb.SBPlatformShellCommand('/bin/zsh', 'echo $0')
+self.assertIn('/bin/zsh', sh_cmd.GetInterpreter())
+self.assertIn('echo $0', sh_cmd.GetCommand())
+
+executable = self.getBuildArtifact('myshell')
+d = {'C_SOURCES': 'myshell.c', 'EXE': executable}
+self.build(dictionary=d)
+self.addTearDownCleanup(dictionary=d)
+
+sh_cmd.SetInterpreter(executable)
+err = platform.Run(sh_cmd)
+self.assertTrue(err.Success())
+self.assertIn("SUCCESS", sh_cmd.GetOutput())
Index: lldb/test/API/commands/platform/basic/TestPlatformCommand.py
===
--- lldb/test/API/commands/platform/basic/TestPlatformCommand.py
+++ lldb/test/API/commands/platform/basic/TestPlatformCommand.py
@@ -92,3 +92,8 @@
 "error: timed out waiting for shell command to complete"])
 self.expect("shell -t 1 --  sleep 3", error=True, substrs=[
 "error: timed out waiting for shell command to complete"])
+
+@no_debug_info_test
+def test_host_shell_interpreter(self):
+""" Test the host platform shell with a different interpreter """
+self.expect("platform shell -h -i /bin/sh -- 'echo $0'", substrs=['/bin/sh'])
Index: lldb/test/API/commands/platform/basic/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/platform/basic/Makefile
@@ -0,0 +1 @@
+include Makefile.rules
Index: lldb/source/Target/RemoteAwarePlatform.cpp
===
--- lldb/source/Target/RemoteAwarePlatform.cpp
+++ lldb/source/Target/RemoteAwarePlatform.cpp
@@ -170,16 +170,31 @@
   return error;
 }
 
-Status RemoteAwarePlatform::RunShellCommand(
-const char *command, const FileSpec &working_dir, int *status_ptr,
-int *signo_ptr, std::string *command_output,
-const Timeout &timeout) {
+Status RemoteAwarePlatform::RunShellCommand(llvm::StringRef command,
+