[Lldb-commits] [PATCH] D78697: [lldb][TypeSystemClang] Desugar an elaborated type before checking if it's a typedef or getting a typedefed type

2020-04-23 Thread Aleksandr Urakov via Phabricator via lldb-commits 
aleksandr.urakov created this revision.
aleksandr.urakov added reviewers: aprantl, teemperor.
aleksandr.urakov added a project: LLDB.
Herald added a subscriber: lldb-commits.

Sometimes a result variable of some expression can be presented as an 
elaborated type. In this case the methods `IsTypedefType()` and 
`GetTypedefedType()` of `SBType` didn't work. This patch fixes that.

I didn't find the test for these API methods, so I added a basic test for this 
too.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78697

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/API/lang/cpp/typedef/Makefile
  lldb/test/API/lang/cpp/typedef/TestCppTypedef.py
  lldb/test/API/lang/cpp/typedef/main.cpp

Index: lldb/test/API/lang/cpp/typedef/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/typedef/main.cpp
@@ -0,0 +1,13 @@
+template
+struct S {
+  typedef T V;
+
+  V value;
+};
+
+typedef S SF;
+
+int main (int argc, char const *argv[]) {
+  SF s{ .5 };
+  return 0; // Set a breakpoint here
+}
Index: lldb/test/API/lang/cpp/typedef/TestCppTypedef.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/typedef/TestCppTypedef.py
@@ -0,0 +1,55 @@
+"""
+Test that we can retrieve typedefed types correctly
+"""
+
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import decorators
+
+class TestCppTypedef(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test_typedef(self):
+"""
+Test that we retrieve typedefed types correctly
+"""
+
+# Build and run until the breakpoint
+self.build()
+self.main_source_file = lldb.SBFileSpec("main.cpp")
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+self, "Set a breakpoint here", self.main_source_file)
+
+# Get the current frame
+frame = thread.GetSelectedFrame()
+
+# First of all, check that we can get a typedefed type correctly in a simple case
+
+expr_result = frame.EvaluateExpression("(SF)s")
+self.assertTrue(expr_result.IsValid(), "Can't evaluate an expression with result type `SF`")
+
+typedef_type = expr_result.GetType();
+self.assertTrue(typedef_type.IsValid(), "Can't get `SF` type of evaluated expression")
+self.assertTrue(typedef_type.IsTypedefType(), "Type `SF` should be a typedef")
+
+typedefed_type = typedef_type.GetTypedefedType()
+self.assertTrue(typedefed_type.IsValid(), "Can't get `SF` typedefed type")
+self.assertEqual(typedefed_type.GetName(), "S", "Got invalid `SF` typedefed type")
+
+# Check that we can get a typedefed type correctly in the case
+# when an elaborated type is created during the parsing
+
+expr_result = frame.EvaluateExpression("(SF::V)s.value")
+self.assertTrue(expr_result.IsValid(), "Can't evaluate an expression with result type `SF::V`")
+
+typedef_type = expr_result.GetType();
+self.assertTrue(typedef_type.IsValid(), "Can't get `SF::V` type of evaluated expression")
+self.assertTrue(typedef_type.IsTypedefType(), "Type `SF::V` should be a typedef")
+
+typedefed_type = typedef_type.GetTypedefedType()
+self.assertTrue(typedefed_type.IsValid(), "Can't get `SF::V` typedefed type")
+self.assertEqual(typedefed_type.GetName(), "float", "Got invalid `SF::V` typedefed type")
Index: lldb/test/API/lang/cpp/typedef/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/typedef/Makefile
@@ -0,0 +1,2 @@
+CXX_SOURCES := main.cpp
+include Makefile.rules
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -537,6 +537,13 @@
   Opts.ModulesLocalVisibility = 1;
 }
 
+static QualType desugarType(QualType type) {
+  while (const clang::ElaboratedType *elaboratedType =
+  llvm::dyn_cast(type))
+type = elaboratedType->desugar();
+  return type;
+}
+
 TypeSystemClang::TypeSystemClang(llvm::StringRef name,
  llvm::Triple target_triple) {
   m_display_name = name.str();
@@ -3539,7 +3546,7 @@
 bool TypeSystemClang::IsTypedefType(lldb::opaque_compiler_type_t type) {
   if (!type)
 return false;
-  return GetQualType(type)->getTypeClass() == clang::Type::Typedef;
+  return desugarType(GetQualType(type))->getTypeClass() == clang::Type::Typedef;
 }
 
 bool TypeSystemClang::IsVoidType(lldb::opaque_compiler_type_t type) {
@@ -4524,7 +4531,7 @@
 TypeSystemClang::GetTypedefedType(lldb::opaque_compiler_type_t type) {
   if (type) {
 const clang::TypedefType *typedef_type

[Lldb-commits] [PATCH] D78697: [lldb][TypeSystemClang] Desugar an elaborated type before checking if it's a typedef or getting a typedefed type

2020-04-23 Thread Pavel Labath via Phabricator via lldb-commits 
labath added inline comments.



Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:540
 
+static QualType desugarType(QualType type) {
+  while (const clang::ElaboratedType *elaboratedType =

The name of this function is fairly misleading as it only desugars elaborated 
types (but not e.g. auto, decltypes, typeof expressions, etc).

Do you want to desugar those too? If yes, then you could call 
`RemoveWrappingTypes` while passing "typedef" as the thing-to-avoid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78697



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

[Lldb-commits] [PATCH] D77043: Fix process gdb-remote usage of value_regs/invalidate_regs

2020-04-23 Thread Muhammad Omair Javaid via Phabricator via lldb-commits 
omjavaid added a comment.

In D77043#1996983 , @labath wrote:

> Where does the option which I proposed fit in? It sounds like it would be 
> something like 1a), where we don't actually modify the "invalidate" numbers 
> stored within lldb-server, but we just do the conversion at the protocol 
> level. That way the "invalidate" numbers keep their meaning as "lldb" 
> register numbers in lldb-server, just like they used to. They also keep 
> referring to "lldb" register numbers on the client side, because the client 
> puts the register "index" into the "lldb" field. The only inconsistency is 
> that the "lldb" register numbers assigned to a specific register will be 
> different on the client and server side. However:
>
> - this is not different from what happens in your patch, because you put the 
> "server lldb" number into the "process plugin" field on the client
> - it's not something we should actually rely on if we want to be able to 
> communicate with non-matching server stubs


As far as non-matching stubs are concerned lldb qRegisterInfo handler on host 
only looks for eRegisterKindEHFrame, eRegisterKindGeneric, and 
eRegisterKindDWARF. The remaining two fields of register kinds list i-e 
eRegisterKindProcessPlugin and eRegisterKindLLDB are assigned same value of 
index by host. It is interesting to note here that when LLDB goes on to access 
any register on remote side by sending a 'g/G' or 'p/P' packets it makes use of 
eRegisterKindProcessPlugin although both eRegisterKindLLDB and 
eRegisterKindProcessPlugin are assigned the same value. So this patch actually 
gives a remote stub or process plugin an option to send its own regnum if it 
has to otherwise index will be used as the default behavior. I think 
eRegisterKindProcessPlugin is there for this purpose in case a stub wants to 
provide its own register number it can do so but we are actually not allowing 
any provision of doing so in qRegisterInfo packet while this provision is 
available in target xml packet and we use it while parsing target xml.

> It seems to me like this solution has the same downsides as the current 
> patch, while having the upside of not requiring protocol changes. That means 
> it can be revisited if we find it to be a problem, while a protocol change is 
> more permanent. That's why I'd like to get a good understanding of why it 
> won't work (or be ugly) if we're not going to do it, and so far I haven't 
> gotten that.

options 1) is similar to what you have proposed whereby we update value_regs 
and invalidate_regs nos to mean indices before first qRegisterinfo. What this 
actually means is that register number of all concerned registers in register 
info list will be updated (All SVE register will have an updated register no). 
These registers numbers are also used by instruction emulation by accessing the 
register infos array which for now does pose any problem for now.

> (BTW, if other aarch64 targets (which ones?) are reporting debug registers in 
> their register lists, I don't think it would be unreasonable to do the same 
> on linux)

Linux does not expose AArch64 debug register via ptrace interface and they are 
mostly needed for manipulation of debug features like hw 
watchpoints/breakpoints. On Linux PTrace provides interface for installing hw 
watch/breakpoints but no access for debug regs. The reason i proposed moving 
debug register down is that they have the least chance of ever being used 
elsewhere in LLDB code.

Also apart from SVE registers in another patch we ll also be introducing 
AArch64 Pointer Authentication registers access support. Those register will 
also have to be moved up in register infos array because  and they may or may 
not be available for given target and we ll have to choose that by querying 
target after inferior is spawned. Future features and the features being 
optional with their registers being available or not is the reason I choose to 
write this patch rather than taking the options under discussion.


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

https://reviews.llvm.org/D77043



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

[Lldb-commits] [lldb] e327ea4 - [lldb] Fix typo in breakpoint set -r description

2020-04-23 Thread Raphael Isemann via lldb-commits 

Author: Raphael Isemann
Date: 2020-04-23T12:06:27+02:00
New Revision: e327ea4a82874f535b6ea93e39ea58643aef88bd

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

LOG: [lldb] Fix typo in breakpoint set -r description

Added: 


Modified: 
lldb/source/Commands/Options.td

Removed: 




diff  --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index b0cf97e067cc..05c7b3213e2a 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -157,7 +157,7 @@ let Command = "breakpoint set" in {
 "multiple times tomake one breakpoint for multiple methods.">;
   def breakpoint_set_func_regex : Option<"func-regex", "r">, Group<7>,
 Arg<"RegularExpression">, Required, Desc<"Set the breakpoint by function "
-"name, evaluating a regular-expression to findthe function name(s).">;
+"name, evaluating a regular-expression to find the function name(s).">;
   def breakpoint_set_basename : Option<"basename", "b">, Group<8>,
 Arg<"FunctionName">, Required, Completion<"Symbol">,
 Desc<"Set the breakpoint by function basename (C++ namespaces and 
arguments"



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

[Lldb-commits] [PATCH] D78697: [lldb][TypeSystemClang] Desugar an elaborated type before checking if it's a typedef or getting a typedefed type

2020-04-23 Thread Aleksandr Urakov via Phabricator via lldb-commits 
aleksandr.urakov updated this revision to Diff 259509.
aleksandr.urakov added reviewers: labath, leonid.mashinskiy.
aleksandr.urakov added a comment.

Hello, Pavel! Thanks for the review. Yes, this method looks like a better fit, 
I just didn't notice it before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78697

Files:
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/API/lang/cpp/typedef/Makefile
  lldb/test/API/lang/cpp/typedef/TestCppTypedef.py
  lldb/test/API/lang/cpp/typedef/main.cpp

Index: lldb/test/API/lang/cpp/typedef/main.cpp
===
--- /dev/null
+++ lldb/test/API/lang/cpp/typedef/main.cpp
@@ -0,0 +1,13 @@
+template
+struct S {
+  typedef T V;
+
+  V value;
+};
+
+typedef S SF;
+
+int main (int argc, char const *argv[]) {
+  SF s{ .5 };
+  return 0; // Set a breakpoint here
+}
Index: lldb/test/API/lang/cpp/typedef/TestCppTypedef.py
===
--- /dev/null
+++ lldb/test/API/lang/cpp/typedef/TestCppTypedef.py
@@ -0,0 +1,55 @@
+"""
+Test that we can retrieve typedefed types correctly
+"""
+
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import decorators
+
+class TestCppTypedef(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+def test_typedef(self):
+"""
+Test that we retrieve typedefed types correctly
+"""
+
+# Build and run until the breakpoint
+self.build()
+self.main_source_file = lldb.SBFileSpec("main.cpp")
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+self, "Set a breakpoint here", self.main_source_file)
+
+# Get the current frame
+frame = thread.GetSelectedFrame()
+
+# First of all, check that we can get a typedefed type correctly in a simple case
+
+expr_result = frame.EvaluateExpression("(SF)s")
+self.assertTrue(expr_result.IsValid(), "Can't evaluate an expression with result type `SF`")
+
+typedef_type = expr_result.GetType();
+self.assertTrue(typedef_type.IsValid(), "Can't get `SF` type of evaluated expression")
+self.assertTrue(typedef_type.IsTypedefType(), "Type `SF` should be a typedef")
+
+typedefed_type = typedef_type.GetTypedefedType()
+self.assertTrue(typedefed_type.IsValid(), "Can't get `SF` typedefed type")
+self.assertEqual(typedefed_type.GetName(), "S", "Got invalid `SF` typedefed type")
+
+# Check that we can get a typedefed type correctly in the case
+# when an elaborated type is created during the parsing
+
+expr_result = frame.EvaluateExpression("(SF::V)s.value")
+self.assertTrue(expr_result.IsValid(), "Can't evaluate an expression with result type `SF::V`")
+
+typedef_type = expr_result.GetType();
+self.assertTrue(typedef_type.IsValid(), "Can't get `SF::V` type of evaluated expression")
+self.assertTrue(typedef_type.IsTypedefType(), "Type `SF::V` should be a typedef")
+
+typedefed_type = typedef_type.GetTypedefedType()
+self.assertTrue(typedefed_type.IsValid(), "Can't get `SF::V` typedefed type")
+self.assertEqual(typedefed_type.GetName(), "float", "Got invalid `SF::V` typedefed type")
Index: lldb/test/API/lang/cpp/typedef/Makefile
===
--- /dev/null
+++ lldb/test/API/lang/cpp/typedef/Makefile
@@ -0,0 +1,2 @@
+CXX_SOURCES := main.cpp
+include Makefile.rules
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -3539,7 +3539,8 @@
 bool TypeSystemClang::IsTypedefType(lldb::opaque_compiler_type_t type) {
   if (!type)
 return false;
-  return GetQualType(type)->getTypeClass() == clang::Type::Typedef;
+  return RemoveWrappingTypes(GetQualType(type), {clang::Type::Typedef})
+ ->getTypeClass() == clang::Type::Typedef;
 }
 
 bool TypeSystemClang::IsVoidType(lldb::opaque_compiler_type_t type) {
@@ -4523,8 +4524,8 @@
 CompilerType
 TypeSystemClang::GetTypedefedType(lldb::opaque_compiler_type_t type) {
   if (type) {
-const clang::TypedefType *typedef_type =
-llvm::dyn_cast(GetQualType(type));
+const clang::TypedefType *typedef_type = llvm::dyn_cast(
+RemoveWrappingTypes(GetQualType(type), {clang::Type::Typedef}));
 if (typedef_type)
   return GetType(typedef_type->getDecl()->getUnderlyingType());
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] c9e6b70 - [lldb/Host] Modernize some socket functions

2020-04-23 Thread Pavel Labath via lldb-commits 

Author: Pavel Labath
Date: 2020-04-23T14:20:26+02:00
New Revision: c9e6b7010c6998b6df50ff376425d1d4e4937bea

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

LOG: [lldb/Host] Modernize some socket functions

return Expected instead of a Status object plus a Socket*&
argument.

Added: 


Modified: 
lldb/include/lldb/Host/Socket.h
lldb/include/lldb/Host/common/UDPSocket.h
lldb/source/Host/common/Socket.cpp
lldb/source/Host/common/UDPSocket.cpp
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
lldb/unittests/Host/SocketTest.cpp

Removed: 




diff  --git a/lldb/include/lldb/Host/Socket.h b/lldb/include/lldb/Host/Socket.h
index 77562276d352..36db0ec63e9d 100644
--- a/lldb/include/lldb/Host/Socket.h
+++ b/lldb/include/lldb/Host/Socket.h
@@ -36,6 +36,8 @@ typedef SOCKET NativeSocket;
 #else
 typedef int NativeSocket;
 #endif
+class TCPSocket;
+class UDPSocket;
 
 class Socket : public IOObject {
 public:
@@ -64,13 +66,16 @@ class Socket : public IOObject {
   // Initialize a Tcp Socket object in listening mode.  listen and accept are
   // implemented separately because the caller may wish to manipulate or query
   // the socket after it is initialized, but before entering a blocking accept.
-  static Status TcpListen(llvm::StringRef host_and_port,
-  bool child_processes_inherit, Socket *&socket,
-  Predicate *predicate, int backlog = 5);
-  static Status TcpConnect(llvm::StringRef host_and_port,
-   bool child_processes_inherit, Socket *&socket);
-  static Status UdpConnect(llvm::StringRef host_and_port,
-   bool child_processes_inherit, Socket *&socket);
+  static llvm::Expected>
+  TcpListen(llvm::StringRef host_and_port, bool child_processes_inherit,
+Predicate *predicate, int backlog = 5);
+
+  static llvm::Expected>
+  TcpConnect(llvm::StringRef host_and_port, bool child_processes_inherit);
+
+  static llvm::Expected>
+  UdpConnect(llvm::StringRef host_and_port, bool child_processes_inherit);
+
   static Status UnixDomainConnect(llvm::StringRef host_and_port,
   bool child_processes_inherit,
   Socket *&socket);

diff  --git a/lldb/include/lldb/Host/common/UDPSocket.h 
b/lldb/include/lldb/Host/common/UDPSocket.h
index b3db4fa29f6e..bae707e345d8 100644
--- a/lldb/include/lldb/Host/common/UDPSocket.h
+++ b/lldb/include/lldb/Host/common/UDPSocket.h
@@ -16,8 +16,8 @@ class UDPSocket : public Socket {
 public:
   UDPSocket(bool should_close, bool child_processes_inherit);
 
-  static Status Connect(llvm::StringRef name, bool child_processes_inherit,
-Socket *&socket);
+  static llvm::Expected>
+  Connect(llvm::StringRef name, bool child_processes_inherit);
 
   std::string GetRemoteConnectionURI() const override;
 

diff  --git a/lldb/source/Host/common/Socket.cpp 
b/lldb/source/Host/common/Socket.cpp
index 45c2f9eb1e41..4bcf34a6b456 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -147,71 +147,65 @@ std::unique_ptr Socket::Create(const 
SocketProtocol protocol,
   return socket_up;
 }
 
-Status Socket::TcpConnect(llvm::StringRef host_and_port,
-  bool child_processes_inherit, Socket *&socket) {
-  Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_COMMUNICATION));
-  LLDB_LOGF(log, "Socket::%s (host/port = %s)", __FUNCTION__,
-host_and_port.str().c_str());
+llvm::Expected>
+Socket::TcpConnect(llvm::StringRef host_and_port,
+   bool child_processes_inherit) {
+  Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
+  LLDB_LOG(log, "host_and_port = {0}", host_and_port);
 
   Status error;
   std::unique_ptr connect_socket(
   Create(ProtocolTcp, child_processes_inherit, error));
   if (error.Fail())
-return error;
+return error.ToError();
 
   error = connect_socket->Connect(host_and_port);
   if (error.Success())
-socket = connect_socket.release();
+return std::move(connect_socket);
 
-  return error;
+  return error.ToError();
 }
 
-Status Socket::TcpListen(llvm::StringRef host_and_port,
- bool child_processes_inherit, Socket *&socket,
- Predicate *predicate, int backlog) {
+llvm::Expected>
+Socket::TcpListen(llvm::StringRef host_and_port, bool child_processes_inherit,
+  Predicate *predicate, int backlog) {
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
-  LLDB_LOGF(log, "Socket::%s (%s)", __FUNCTION__, host_and_port.str().c_str());
+  LLDB_LOG(log, "host_and_port = {0}", host_and_port);
 
   Status error;
   std::string host_

[Lldb-commits] [lldb] 7cfa74f - [lldb/DWARF] Trust CU DW_AT_low/high_pc information when building address tables

2020-04-23 Thread Pavel Labath via lldb-commits 

Author: Pavel Labath
Date: 2020-04-23T16:12:41+02:00
New Revision: 7cfa74fc6948cc1969244a4e800de6a728951897

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

LOG: [lldb/DWARF] Trust CU DW_AT_low/high_pc information when building address 
tables

Summary:
The code in DWARFCompileUnit::BuildAddressRangeTable tries hard to avoid
relying on DW_AT_low/high_pc for compile unit range information, and
this logic is a big cause of llvm/lldb divergence in the lowest layers
of dwarf parsing code.

The implicit assumption in that code is that this information (as opposed to
DW_AT_ranges) is unreliable. However, I have not been able to verify
that assumption. It is definitely not true for all present-day
compilers (gcc, clang, icc), and it was also not the case for the
historic compilers that I have been able to get a hold of (thanks Matt
Godbolt).

All compiler included in my research either produced correct
DW_AT_ranges or .debug_aranges entries, or they produced no DW_AT_hi/lo
pc at all. The detailed findings are:
- gcc >= 4.4: produces DW_AT_ranges and .debug_aranges
- 4.1 <= gcc < 4.4: no DW_AT_ranges, no DW_AT_high_pc, .debug_aranges
  present. The upper version range here is uncertain as godbolt.org does
  not have intermediate versions.
- gcc < 4.1: no versions on godbolt.org
- clang >= 3.5: produces DW_AT_ranges, and (optionally) .debug_aranges
- 3.4 <= clang < 3.5: no DW_AT_ranges, no DW_AT_high_pc, .debug_aranges
  present.
- clang <= 3.3: no DW_AT_ranges, no DW_AT_high_pc, no .debug_aranges
- icc >= 16.0.1: produces DW_AT_ranges
- icc < 16.0.1: no functional versions on godbolt.org (some are present
  but fail to compile)

Based on this analysis, I believe it is safe to start trusting
DW_AT_low/high_pc information in dwarf as well as remove the code for
manually reconstructing range information by traversing the DIE
structure, and just keep the line table fallback. The only compilers
where this will change behavior are pre-3.4 clangs, which are almost 7
years old now. However, the functionality should remain unchanged
because we will be able to reconstruct this information from the line
table, which seems to be needed for some line-tables-only scenarios
anyway (haven't researched this too much, but at least some compilers
seem to emit DW_AT_ranges even in these situations).

In addition, benchmarks showed that for these compilers computing the
ranges via line tables is noticably faster than doing so via the DIE
tree.

Other advantages include simplifying the code base, removing some
untested code (the only test changes are recent tests with overly
reduced synthetic dwarf), and increasing llvm convergence.

Tags: #lldb

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

Added: 


Modified: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
lldb/test/Shell/SymbolFile/DWARF/DW_AT_loclists_base.s
lldb/test/Shell/SymbolFile/DWARF/debug_loc.s

Removed: 




diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
index 66fcbfdcc0ec..f54fe0662aa2 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
@@ -33,41 +33,27 @@ void DWARFCompileUnit::BuildAddressRangeTable(
 
   size_t num_debug_aranges = debug_aranges->GetNumRanges();
 
-  // First get the compile unit DIE only and check if it has a DW_AT_ranges
+  // First get the compile unit DIE only and check contains ranges information.
   const DWARFDebugInfoEntry *die = GetUnitDIEPtrOnly();
 
   const dw_offset_t cu_offset = GetOffset();
   if (die) {
 DWARFRangeList ranges;
 const size_t num_ranges =
-die->GetAttributeAddressRanges(this, ranges, false);
+die->GetAttributeAddressRanges(this, ranges, /*check_hi_lo_pc=*/true);
 if (num_ranges > 0) {
-  // This compile unit has DW_AT_ranges, assume this is correct if it is
-  // present since clang no longer makes .debug_aranges by default and it
-  // emits DW_AT_ranges for DW_TAG_compile_units. GCC also does this with
-  // recent GCC builds.
   for (size_t i = 0; i < num_ranges; ++i) {
 const DWARFRangeList::Entry &range = ranges.GetEntryRef(i);
 debug_aranges->AppendRange(cu_offset, range.GetRangeBase(),
range.GetRangeEnd());
   }
 
-  return; // We got all of our ranges from the DW_AT_ranges attribute
+  return;
 }
   }
-  // We don't have a DW_AT_ranges attribute, so we need to parse the DWARF
-
-  // If the DIEs weren't parsed, then we don't want all dies for all compi

[Lldb-commits] [lldb] f512b97 - [lldb/Utility] Improve error_code->Status conversion

2020-04-23 Thread Pavel Labath via lldb-commits 

Author: Pavel Labath
Date: 2020-04-23T16:12:41+02:00
New Revision: f512b978b0ebd09be08d879acb4feb7846370cd5

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

LOG: [lldb/Utility] Improve error_code->Status conversion

Both entities have the notion of error "namespaces". Map the errno
namespace correctly.

Added: 


Modified: 
lldb/source/Utility/Status.cpp
lldb/unittests/Utility/StatusTest.cpp

Removed: 




diff  --git a/lldb/source/Utility/Status.cpp b/lldb/source/Utility/Status.cpp
index c8ca485cdd3b..e3c4284a8e8a 100644
--- a/lldb/source/Utility/Status.cpp
+++ b/lldb/source/Utility/Status.cpp
@@ -42,8 +42,13 @@ Status::Status() : m_code(0), m_type(eErrorTypeInvalid), 
m_string() {}
 Status::Status(ValueType err, ErrorType type)
 : m_code(err), m_type(type), m_string() {}
 
+// This logic is confusing because c++ calls the traditional (posix) errno 
codes
+// "generic errors", while we use the term "generic" to mean completely
+// arbitrary (text-based) errors.
 Status::Status(std::error_code EC)
-: m_code(EC.value()), m_type(ErrorType::eErrorTypeGeneric),
+: m_code(EC.value()),
+  m_type(EC.category() == std::generic_category() ? eErrorTypePOSIX
+  : eErrorTypeGeneric),
   m_string(EC.message()) {}
 
 Status::Status(const char *format, ...)

diff  --git a/lldb/unittests/Utility/StatusTest.cpp 
b/lldb/unittests/Utility/StatusTest.cpp
index 516f10b5b439..862c063b2e06 100644
--- a/lldb/unittests/Utility/StatusTest.cpp
+++ b/lldb/unittests/Utility/StatusTest.cpp
@@ -41,6 +41,15 @@ TEST(StatusTest, ErrorConstructor) {
   EXPECT_TRUE(foo.Success());
 }
 
+TEST(StatusTest, ErrorCodeConstructor) {
+  EXPECT_TRUE(Status(std::error_code()).Success());
+
+  Status eagain = std::error_code(EAGAIN, std::generic_category());
+  EXPECT_TRUE(eagain.Fail());
+  EXPECT_EQ(eErrorTypePOSIX, eagain.GetType());
+  EXPECT_EQ(Status::ValueType(EAGAIN), eagain.GetError());
+}
+
 TEST(StatusTest, ErrorConversion) {
   EXPECT_FALSE(bool(Status().ToError()));
 



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

[Lldb-commits] [PATCH] D78712: [lldb/Host] Improve error messages on unowned read files

2020-04-23 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib marked an inline comment as done.
mib added inline comments.



Comment at: lldb/test/API/commands/target/basic/TestTargetCommand.py:343
+def test_target_create_unowned_core_file(self):
+self.expect("target create -c ~root", error=True,
+substrs=["core file '", "' is not readable"])

Currently, I try to open the root user home directory, since I sure it's not 
readable by other users on UNIX systems.

I went this route, because doing a `os.chown` requires privileges, and the test 
suites doesn't run in a privileged mode.

I'm open to suggestions on how I could test this on different platforms.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78712



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

[Lldb-commits] [PATCH] D78489: [lldb/DWARF] Trust CU DW_AT_low/high_pc information when building address tables

2020-04-23 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

Thanks for the review.

In D78489#1998060 , @clayborg wrote:

> In D78489#1996834 , @labath wrote:
>
> > I've done a similar benchmark to the last one, but measured memory usage 
> > ("RssAnon" as reported by linux). One notable difference is that I
> >
> > Currently (information retrieved from DW_AT_ranges) we use about ~330 MB of 
> > memory. If I switch to dwarf dies as the source, the memory goes all the 
> > way to 2890 MB. This number is suspiciously large -- it either means that 
> > our die freeing is not working properly, or that glibc is very bad at 
> > releasing memory back to the OS. Given the magnitude of the increase, i 
> > think it's a little bit of both. With line tables as the source the memory 
> > usage is 706 MB. It's an increase from 330, but definitely smaller than 2.8 
> > GB. (the number 330 is kind of misleading here since we're not considering 
> > removing that option -- it will always be used if it is available).
>
>
> Since we mmap in the entire DWARF, I am not surprised by taking up new memory 
> because we touch those pages and won't get those back. If you remove the DIE 
> freeing code, I will bet you see much more memory used. We definitely free 
> the memory for the DIEs and give that back, so I would be willing to bet the 
> increase you are seeing is from mmap loading pages in that we touch.


I don't think it's as simple as that. "RssAnon" should not include file-backed 
memory (which why I chose to measure it). According to `man proc`:

  * RssAnon: Size of resident anonymous memory.  (since Linux 4.5).
  * RssFile: Size of resident file mappings.  (since Linux 4.5).

I also don't think it's as simple as not freeing the DIE memory at all. There 
has to be some more complex interaction going on. If I had more time, I would 
be very interested in learning what it is, but for now I'll contend myself with 
"it's not a problem of this patch".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78489



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

[Lldb-commits] [PATCH] D78712: [lldb/Host] Improve error messages on unowned read files

2020-04-23 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib created this revision.
mib added reviewers: JDevlieghere, labath.
mib added a project: LLDB.
Herald added subscribers: lldb-commits, emaste.
mib marked an inline comment as done.
mib added inline comments.
mib edited the summary of this revision.



Comment at: lldb/test/API/commands/target/basic/TestTargetCommand.py:343
+def test_target_create_unowned_core_file(self):
+self.expect("target create -c ~root", error=True,
+substrs=["core file '", "' is not readable"])

Currently, I try to open the root user home directory, since I sure it's not 
readable by other users on UNIX systems.

I went this route, because doing a `os.chown` requires privileges, and the test 
suites doesn't run in a privileged mode.

I'm open to suggestions on how I could test this on different platforms.


When trying to read a core file that is not owned by the user running lldb
and that doesn't have read permission on the file, we can a misleading
error message

  Unable to find process plug-in for core file

This is due to the fact that currently, lldb doesn't check the file
ownership. And when trying to to open and read a core file, the syscall
fails, which prevents a process to be created.

This patch introduces the `FileSystem::ReadableByCurrentUser` method
that checks if the file has readable permissions and its ownership.
This patch also changes the error messages to be more accurate.

rdar://42630030

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78712

Files:
  lldb/include/lldb/Host/FileSystem.h
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Host/common/FileSystem.cpp
  lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
  lldb/test/API/commands/target/basic/TestTargetCommand.py

Index: lldb/test/API/commands/target/basic/TestTargetCommand.py
===
--- lldb/test/API/commands/target/basic/TestTargetCommand.py
+++ lldb/test/API/commands/target/basic/TestTargetCommand.py
@@ -337,6 +337,12 @@
 self.expect("target create -c '" + tf.name + "'", error=True,
 substrs=["core file '", "' is not readable"])
 
+@skipIfWindows
+@no_debug_info_test
+def test_target_create_unowned_core_file(self):
+self.expect("target create -c ~root", error=True,
+substrs=["core file '", "' is not readable"])
+
 @no_debug_info_test
 def test_target_create_nonexistent_sym_file(self):
 self.expect("target create -s doesntexist doesntexisteither", error=True,
Index: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
===
--- lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -91,20 +91,17 @@
 // Resolve any executable within a bundle on MacOSX
 Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
 
-if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec()))
+if (!FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec()))
+  error.SetErrorStringWithFormat(
+  "unable to find executable for '%s'",
+  resolved_module_spec.GetFileSpec().GetPath().c_str());
+else if (!FileSystem::Instance().ReadableByCurrentUser(
+ resolved_module_spec.GetFileSpec())) {
+  error.SetErrorStringWithFormat(
+  "executable '%s' is not readable by the current user",
+  resolved_module_spec.GetFileSpec().GetPath().c_str());
+} else
   error.Clear();
-else {
-  const uint32_t permissions = FileSystem::Instance().GetPermissions(
-  resolved_module_spec.GetFileSpec());
-  if (permissions && (permissions & eFilePermissionsEveryoneR) == 0)
-error.SetErrorStringWithFormat(
-"executable '%s' is not readable",
-resolved_module_spec.GetFileSpec().GetPath().c_str());
-  else
-error.SetErrorStringWithFormat(
-"unable to find executable for '%s'",
-resolved_module_spec.GetFileSpec().GetPath().c_str());
-}
   } else {
 if (m_remote_platform_sp) {
   error =
@@ -188,7 +185,7 @@
   }
 
   if (error.Fail() || !exe_module_sp) {
-if (FileSystem::Instance().Readable(
+if (FileSystem::Instance().ReadableByCurrentUser(
 resolved_module_spec.GetFileSpec())) {
   error.SetErrorStringWithFormat(
   "'%s' doesn't contain any '%s' platform architectures: %s",
@@ -196,7 +193,7 @@
   GetPluginName().GetCString(), arch_names.GetData());
 } else {
   error.SetErrorStringWithFormat(
-  "'%s' is not readable",
+  "'%s' is not readable by the current user",
   resolved_module_spec.GetFileSpec().GetPath().c_str());
 }
   }
Index: lldb/source/Host/common/FileSystem.

[Lldb-commits] [PATCH] D78489: [lldb/DWARF] Trust CU DW_AT_low/high_pc information when building address tables

2020-04-23 Thread Pavel Labath via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7cfa74fc6948: [lldb/DWARF] Trust CU DW_AT_low/high_pc 
information when building address tables (authored by labath).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78489

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/test/Shell/SymbolFile/DWARF/DW_AT_loclists_base.s
  lldb/test/Shell/SymbolFile/DWARF/debug_loc.s

Index: lldb/test/Shell/SymbolFile/DWARF/debug_loc.s
===
--- lldb/test/Shell/SymbolFile/DWARF/debug_loc.s
+++ lldb/test/Shell/SymbolFile/DWARF/debug_loc.s
@@ -153,6 +153,10 @@
 .byte   14  # DW_FORM_strp
 .byte   19  # DW_AT_language
 .byte   5   # DW_FORM_data2
+.byte   17  # DW_AT_low_pc
+.byte   1   # DW_FORM_addr
+.byte   18  # DW_AT_high_pc
+.byte   6   # DW_FORM_data4
 .byte   0   # EOM(1)
 .byte   0   # EOM(2)
 .byte   2   # Abbreviation Code
@@ -210,6 +214,8 @@
 .byte   1   # Abbrev [1] 0xb:0x50 DW_TAG_compile_unit
 .long   .Linfo_string0  # DW_AT_producer
 .short  12  # DW_AT_language
+.quad   .Lfunc_begin0   # DW_AT_low_pc
+.long   .Lfunc_end0-.Lfunc_begin0 # DW_AT_high_pc
 .byte   2   # Abbrev [2] 0x2a:0x29 DW_TAG_subprogram
 .quad   .Lfunc_begin0   # DW_AT_low_pc
 .long   .Lfunc_end0-.Lfunc_begin0 # DW_AT_high_pc
Index: lldb/test/Shell/SymbolFile/DWARF/DW_AT_loclists_base.s
===
--- lldb/test/Shell/SymbolFile/DWARF/DW_AT_loclists_base.s
+++ lldb/test/Shell/SymbolFile/DWARF/DW_AT_loclists_base.s
@@ -58,6 +58,10 @@
 .byte   5   # DW_FORM_data2
 .uleb128 0x8c   # DW_AT_loclists_base
 .byte   0x17# DW_FORM_sec_offset
+.byte   17  # DW_AT_low_pc
+.byte   1   # DW_FORM_addr
+.byte   18  # DW_AT_high_pc
+.byte   6   # DW_FORM_data4
 .byte   0   # EOM(1)
 .byte   0   # EOM(2)
 .byte   2   # Abbreviation Code
@@ -109,6 +113,8 @@
 .asciz  "Hand-written DWARF"# DW_AT_producer
 .short  12  # DW_AT_language
 .long   .Lloclists_table_base   # DW_AT_loclists_base
+.quad   loclists# DW_AT_low_pc
+.long   .Lloclists_end-loclists # DW_AT_high_pc
 .byte   2   # Abbrev [2] 0x2a:0x29 DW_TAG_subprogram
 .quad   loclists# DW_AT_low_pc
 .long   .Lloclists_end-loclists # DW_AT_high_pc
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
@@ -41,9 +41,6 @@
   bool operator==(const DWARFDebugInfoEntry &rhs) const;
   bool operator!=(const DWARFDebugInfoEntry &rhs) const;
 
-  void BuildAddressRangeTable(const DWARFUnit *cu,
-  DWARFDebugAranges *debug_aranges) const;
-
   void BuildFunctionAddressRangeTable(const DWARFUnit *cu,
   DWARFDebugAranges *debug_aranges) const;
 
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
@@ -819,34 +819,9 @@
   return name;
 }
 
-// BuildAddressRangeTable
-void DWARFDebugInfoEntry::BuildAddressRangeTable(
-const DWARFUnit *cu, DWARFDebugAranges *debug_aranges) const {
-  if (m_tag) {
-if (m_tag == DW_TAG_subprogram) {
-  dw_addr_t lo_pc = LLDB_INVALID_ADDRESS;
-  dw_addr_t hi_pc = LLDB_INVALID_ADDRESS;
-  if (GetAttributeAddressRange(cu, lo_pc, hi_pc, LLDB_INVALID_ADDRESS)) {
-/// printf("BuildAddressRangeTable() 0x%8.8x: %30s: [0x%8.8x -
-/// 0x%8.8x)\n", m_offset, DW_TAG_value_to_name(tag), lo_pc, hi_pc);
-debug_aranges->AppendRange(cu->GetOffset(), lo_pc, hi_pc);
-  }
-}
-
-const DWARFDebugInfoEntry *child = GetFirstChild();
-while (child) {
-  child->BuildAdd

[Lldb-commits] [lldb] 9321255 - [lldb/Core] Avoid more Communication::Disconnect races

2020-04-23 Thread Pavel Labath via lldb-commits 

Author: Pavel Labath
Date: 2020-04-23T16:36:21+02:00
New Revision: 9321255b8825d9165b2884204348a353af36d212

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

LOG: [lldb/Core] Avoid more Communication::Disconnect races

Calling Disconnect while the read thread is running is racy because the
thread can also call Disconnect.  This is a follow-up to b424b0bf, which
reorders other occurences of Disconnect/StopReadThread I can find, and also
adds an assertion to guard against new occurences being introduced.

Added: 


Modified: 
lldb/source/Core/Communication.cpp
lldb/source/Target/Process.cpp

Removed: 




diff  --git a/lldb/source/Core/Communication.cpp 
b/lldb/source/Core/Communication.cpp
index 2990f4157584..b358e70b1a91 100644
--- a/lldb/source/Core/Communication.cpp
+++ b/lldb/source/Core/Communication.cpp
@@ -71,8 +71,8 @@ Communication::~Communication() {
 
 void Communication::Clear() {
   SetReadThreadBytesReceivedCallback(nullptr, nullptr);
-  Disconnect(nullptr);
   StopReadThread(nullptr);
+  Disconnect(nullptr);
 }
 
 ConnectionStatus Communication::Connect(const char *url, Status *error_ptr) {
@@ -93,6 +93,8 @@ ConnectionStatus Communication::Disconnect(Status *error_ptr) 
{
   LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_COMMUNICATION),
"{0} Communication::Disconnect ()", this);
 
+  assert((!m_read_thread_enabled || m_read_thread_did_exit) &&
+ "Disconnecting while the read thread is running is racy!");
   lldb::ConnectionSP connection_sp(m_connection_sp);
   if (connection_sp) {
 ConnectionStatus status = connection_sp->Disconnect(error_ptr);

diff  --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 45dcfc489d81..989cdae8b402 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -3316,8 +3316,8 @@ Status Process::Destroy(bool force_kill) {
   DidDestroy();
   StopPrivateStateThread();
 }
-m_stdio_communication.Disconnect();
 m_stdio_communication.StopReadThread();
+m_stdio_communication.Disconnect();
 m_stdin_forward = false;
 
 if (m_process_input_reader) {



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

[Lldb-commits] [lldb] c2fec2f - [lldb] Make RNBSocketTest compile again after socket modernization

2020-04-23 Thread Raphael Isemann via lldb-commits 

Author: Raphael Isemann
Date: 2020-04-23T17:00:02+02:00
New Revision: c2fec2fb17787c14e7860a57ef4f03e085cd3a7b

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

LOG: [lldb] Make RNBSocketTest compile again after socket modernization

Commit c9e6b7010c6998b6 changed the API but didn't update this
macOS-specific test.

Added: 


Modified: 
lldb/unittests/debugserver/CMakeLists.txt
lldb/unittests/debugserver/RNBSocketTest.cpp

Removed: 




diff  --git a/lldb/unittests/debugserver/CMakeLists.txt 
b/lldb/unittests/debugserver/CMakeLists.txt
index 66c4c639fc89..c216eecd7d8a 100644
--- a/lldb/unittests/debugserver/CMakeLists.txt
+++ b/lldb/unittests/debugserver/CMakeLists.txt
@@ -15,6 +15,7 @@ add_lldb_unittest(debugserverTests
   LINK_LIBS
 lldbDebugserverCommon
 lldbHost
+LLVMTestingSupport
   LINK_COMPONENTS
 Support
   )

diff  --git a/lldb/unittests/debugserver/RNBSocketTest.cpp 
b/lldb/unittests/debugserver/RNBSocketTest.cpp
index b5a6896f3ba1..7840c48f82b4 100644
--- a/lldb/unittests/debugserver/RNBSocketTest.cpp
+++ b/lldb/unittests/debugserver/RNBSocketTest.cpp
@@ -17,6 +17,7 @@
 #include "lldb/Host/Socket.h"
 #include "lldb/Host/StringConvert.h"
 #include "lldb/Host/common/TCPSocket.h"
+#include "llvm/Testing/Support/Error.h"
 
 using namespace lldb_private;
 
@@ -26,15 +27,16 @@ std::string goodbye = "Goodbye!";
 static void ServerCallbackv4(const void *baton, in_port_t port) {
   auto child_pid = fork();
   if (child_pid == 0) {
-Socket *client_socket;
 char addr_buffer[256];
 sprintf(addr_buffer, "%s:%d", baton, port);
-Status err = Socket::TcpConnect(addr_buffer, false, client_socket);
-if (err.Fail())
-  abort();
+llvm::Expected> socket_or_err =
+Socket::TcpConnect(addr_buffer, false);
+ASSERT_THAT_EXPECTED(socket_or_err, llvm::Succeeded());
+Socket *client_socket = socket_or_err->get();
+
 char buffer[32];
 size_t read_size = 32;
-err = client_socket->Read((void *)&buffer[0], read_size);
+Status err = client_socket->Read((void *)&buffer[0], read_size);
 if (err.Fail())
   abort();
 std::string Recv(&buffer[0], read_size);
@@ -102,9 +104,10 @@ void TestSocketConnect(const char *addr) {
   Socket *server_socket;
   Predicate port_predicate;
   port_predicate.SetValue(0, eBroadcastNever);
-  Status err =
-  Socket::TcpListen(addr_wrap, false, server_socket, &port_predicate);
-  ASSERT_FALSE(err.Fail());
+  llvm::Expected> socket_or_err =
+  Socket::TcpListen(addr_wrap, false, &port_predicate);
+  ASSERT_THAT_EXPECTED(socket_or_err, llvm::Succeeded());
+  server_socket = socket_or_err->get();
 
   auto port = ((TCPSocket *)server_socket)->GetLocalPortNumber();
   auto child_pid = fork();
@@ -120,7 +123,7 @@ void TestSocketConnect(const char *addr) {
 ASSERT_EQ(bye, goodbye);
   } else {
 Socket *connected_socket;
-err = server_socket->Accept(connected_socket);
+Status err = server_socket->Accept(connected_socket);
 if (err.Fail()) {
   llvm::errs() << err.AsCString();
   abort();



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

[Lldb-commits] [PATCH] D77295: [lldb/Core] Fix a race in the Communication class

2020-04-23 Thread Pavel Labath via Phabricator via lldb-commits 
labath added a comment.

Cool, I've also pushed 9321255b 
 that 
fixes up other occurrences of this pattern, and adds an assertion to prevent it 
from happening again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77295



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

[Lldb-commits] [PATCH] D78712: [lldb/Host] Improve error messages on unowned read files

2020-04-23 Thread Shafik Yaghmour via Phabricator via lldb-commits 
shafik added inline comments.



Comment at: lldb/source/Host/common/FileSystem.cpp:504
+  FileSpec file(file_path.getSingleStringRef());
+  return Readable(file_path) && Instance().Open(file, File::eOpenOptionRead);
+}

Since `file` is a `FileSpec` can't we just use `ReadableByCurrentUser(const 
FileSpec &file_spec) ` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78712



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

[Lldb-commits] [PATCH] D78697: [lldb][TypeSystemClang] Desugar an elaborated type before checking if it's a typedef or getting a typedefed type

2020-04-23 Thread Raphael Isemann via Phabricator via lldb-commits 
teemperor added a comment.

LGTM now beside some minor request for the test. Thanks for the patch!




Comment at: lldb/test/API/lang/cpp/typedef/TestCppTypedef.py:33
+expr_result = frame.EvaluateExpression("(SF)s")
+self.assertTrue(expr_result.IsValid(), "Can't evaluate an expression 
with result type `SF`")
+

I know we do this (sadly) really often in LLDB, but these static error messages 
are just not useful. If my expression fails and the error message is 
"expression failed" then that doesn't help me with debugging the issue 
(especially when it's the only thing some remote bot sends back to me after a 
commit).

You could do `self.assertTrue(expr_result.IsValid(), "Expression failed with:" 
+ expr_result.GetError().GetCString())` instead and then people see the actual 
compiler output in the error log. Same for the other expr evaluation below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78697



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

[Lldb-commits] [PATCH] D78712: [lldb/Host] Improve error messages on unowned read files

2020-04-23 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib updated this revision to Diff 259606.
mib marked an inline comment as done.
mib added a comment.

Address Shafik's comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78712

Files:
  lldb/include/lldb/Host/FileSystem.h
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Host/common/FileSystem.cpp
  lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
  lldb/test/API/commands/target/basic/TestTargetCommand.py

Index: lldb/test/API/commands/target/basic/TestTargetCommand.py
===
--- lldb/test/API/commands/target/basic/TestTargetCommand.py
+++ lldb/test/API/commands/target/basic/TestTargetCommand.py
@@ -337,6 +337,12 @@
 self.expect("target create -c '" + tf.name + "'", error=True,
 substrs=["core file '", "' is not readable"])
 
+@skipIfWindows
+@no_debug_info_test
+def test_target_create_unowned_core_file(self):
+self.expect("target create -c ~root", error=True,
+substrs=["core file '", "' is not readable"])
+
 @no_debug_info_test
 def test_target_create_nonexistent_sym_file(self):
 self.expect("target create -s doesntexist doesntexisteither", error=True,
Index: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
===
--- lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -91,20 +91,17 @@
 // Resolve any executable within a bundle on MacOSX
 Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
 
-if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec()))
+if (!FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec()))
+  error.SetErrorStringWithFormat(
+  "unable to find executable for '%s'",
+  resolved_module_spec.GetFileSpec().GetPath().c_str());
+else if (!FileSystem::Instance().ReadableByCurrentUser(
+ resolved_module_spec.GetFileSpec())) {
+  error.SetErrorStringWithFormat(
+  "executable '%s' is not readable by the current user",
+  resolved_module_spec.GetFileSpec().GetPath().c_str());
+} else
   error.Clear();
-else {
-  const uint32_t permissions = FileSystem::Instance().GetPermissions(
-  resolved_module_spec.GetFileSpec());
-  if (permissions && (permissions & eFilePermissionsEveryoneR) == 0)
-error.SetErrorStringWithFormat(
-"executable '%s' is not readable",
-resolved_module_spec.GetFileSpec().GetPath().c_str());
-  else
-error.SetErrorStringWithFormat(
-"unable to find executable for '%s'",
-resolved_module_spec.GetFileSpec().GetPath().c_str());
-}
   } else {
 if (m_remote_platform_sp) {
   error =
@@ -188,7 +185,7 @@
   }
 
   if (error.Fail() || !exe_module_sp) {
-if (FileSystem::Instance().Readable(
+if (FileSystem::Instance().ReadableByCurrentUser(
 resolved_module_spec.GetFileSpec())) {
   error.SetErrorStringWithFormat(
   "'%s' doesn't contain any '%s' platform architectures: %s",
@@ -196,7 +193,7 @@
   GetPluginName().GetCString(), arch_names.GetData());
 } else {
   error.SetErrorStringWithFormat(
-  "'%s' is not readable",
+  "'%s' is not readable by the current user",
   resolved_module_spec.GetFileSpec().GetPath().c_str());
 }
   }
Index: lldb/source/Host/common/FileSystem.cpp
===
--- lldb/source/Host/common/FileSystem.cpp
+++ lldb/source/Host/common/FileSystem.cpp
@@ -7,6 +7,8 @@
 //===--===//
 
 #include "lldb/Host/FileSystem.h"
+#include "lldb/Host/File.h"
+#include "lldb/Host/HostInfo.h"
 
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/TildeExpressionResolver.h"
@@ -491,3 +493,13 @@
 m_collector->addFile(file);
   }
 }
+
+bool FileSystem::ReadableByCurrentUser(const FileSpec &file_spec) const {
+  return Readable(file_spec) &&
+ Instance().Open(file_spec, File::eOpenOptionRead);
+}
+
+bool FileSystem::ReadableByCurrentUser(const llvm::Twine &file_path) const {
+  FileSpec file_spec(file_path.getSingleStringRef());
+  return ReadableByCurrentUser(file_spec);
+}
Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -277,9 +277,10 @@
 result.SetStatus(eReturnStatusFailed);
 return false;
   }
-  if (!FileSystem::Instance().Readable(core_file)) {
-result.AppendErrorWithFormat("core file '%s' is not readable",
-

[Lldb-commits] [PATCH] D78712: [lldb/Host] Improve error messages on unowned read files

2020-04-23 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib updated this revision to Diff 259608.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78712

Files:
  lldb/include/lldb/Host/FileSystem.h
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Host/common/FileSystem.cpp
  lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
  lldb/test/API/commands/target/basic/TestTargetCommand.py

Index: lldb/test/API/commands/target/basic/TestTargetCommand.py
===
--- lldb/test/API/commands/target/basic/TestTargetCommand.py
+++ lldb/test/API/commands/target/basic/TestTargetCommand.py
@@ -337,6 +337,12 @@
 self.expect("target create -c '" + tf.name + "'", error=True,
 substrs=["core file '", "' is not readable"])
 
+@skipIfWindows
+@no_debug_info_test
+def test_target_create_unowned_core_file(self):
+self.expect("target create -c ~root", error=True,
+substrs=["core file '", "' is not readable"])
+
 @no_debug_info_test
 def test_target_create_nonexistent_sym_file(self):
 self.expect("target create -s doesntexist doesntexisteither", error=True,
Index: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
===
--- lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
@@ -91,20 +91,17 @@
 // Resolve any executable within a bundle on MacOSX
 Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
 
-if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec()))
+if (!FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec()))
+  error.SetErrorStringWithFormat(
+  "unable to find executable for '%s'",
+  resolved_module_spec.GetFileSpec().GetPath().c_str());
+else if (!FileSystem::Instance().ReadableByCurrentUser(
+ resolved_module_spec.GetFileSpec())) {
+  error.SetErrorStringWithFormat(
+  "executable '%s' is not readable by the current user",
+  resolved_module_spec.GetFileSpec().GetPath().c_str());
+} else
   error.Clear();
-else {
-  const uint32_t permissions = FileSystem::Instance().GetPermissions(
-  resolved_module_spec.GetFileSpec());
-  if (permissions && (permissions & eFilePermissionsEveryoneR) == 0)
-error.SetErrorStringWithFormat(
-"executable '%s' is not readable",
-resolved_module_spec.GetFileSpec().GetPath().c_str());
-  else
-error.SetErrorStringWithFormat(
-"unable to find executable for '%s'",
-resolved_module_spec.GetFileSpec().GetPath().c_str());
-}
   } else {
 if (m_remote_platform_sp) {
   error =
@@ -188,7 +185,7 @@
   }
 
   if (error.Fail() || !exe_module_sp) {
-if (FileSystem::Instance().Readable(
+if (FileSystem::Instance().ReadableByCurrentUser(
 resolved_module_spec.GetFileSpec())) {
   error.SetErrorStringWithFormat(
   "'%s' doesn't contain any '%s' platform architectures: %s",
@@ -196,7 +193,7 @@
   GetPluginName().GetCString(), arch_names.GetData());
 } else {
   error.SetErrorStringWithFormat(
-  "'%s' is not readable",
+  "'%s' is not readable by the current user",
   resolved_module_spec.GetFileSpec().GetPath().c_str());
 }
   }
Index: lldb/source/Host/common/FileSystem.cpp
===
--- lldb/source/Host/common/FileSystem.cpp
+++ lldb/source/Host/common/FileSystem.cpp
@@ -7,6 +7,8 @@
 //===--===//
 
 #include "lldb/Host/FileSystem.h"
+#include "lldb/Host/File.h"
+#include "lldb/Host/HostInfo.h"
 
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/TildeExpressionResolver.h"
@@ -491,3 +493,13 @@
 m_collector->addFile(file);
   }
 }
+
+bool FileSystem::ReadableByCurrentUser(const FileSpec &file_spec) const {
+  return Readable(file_spec) &&
+ Instance().Open(file_spec, File::eOpenOptionRead);
+}
+
+bool FileSystem::ReadableByCurrentUser(const llvm::Twine &file_path) const {
+  const FileSpec file_spec(file_path.getSingleStringRef());
+  return ReadableByCurrentUser(file_spec);
+}
Index: lldb/source/Commands/CommandObjectTarget.cpp
===
--- lldb/source/Commands/CommandObjectTarget.cpp
+++ lldb/source/Commands/CommandObjectTarget.cpp
@@ -277,9 +277,10 @@
 result.SetStatus(eReturnStatusFailed);
 return false;
   }
-  if (!FileSystem::Instance().Readable(core_file)) {
-result.AppendErrorWithFormat("core file '%s' is not readable",
- core_file.GetPath().c_str());
+  if (!Fi

[Lldb-commits] [PATCH] D78712: [lldb/Host] Improve error messages on unowned read files

2020-04-23 Thread Med Ismail Bennani via Phabricator via lldb-commits 
mib marked 2 inline comments as done.
mib added inline comments.



Comment at: lldb/source/Host/common/FileSystem.cpp:504
+  FileSpec file(file_path.getSingleStringRef());
+  return Readable(file_path) && Instance().Open(file, File::eOpenOptionRead);
+}

shafik wrote:
> Since `file` is a `FileSpec` can't we just use `ReadableByCurrentUser(const 
> FileSpec &file_spec) ` here?
Good catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78712



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

[Lldb-commits] [PATCH] D78489: [lldb/DWARF] Trust CU DW_AT_low/high_pc information when building address tables

2020-04-23 Thread David Blaikie via Phabricator via lldb-commits 
dblaikie added a comment.

In D78489#1993806 , @clayborg wrote:

> One thing to think about that my flawed example above did show: dead stripped 
> functions cause problems for debug info. Both in DW_AT_ranges on the compile 
> unit and in all of the DWARF below this. We might want to check any and all 
> address ranges to ensure that they are in sections that read + execute 
> permissions and if they don't throw them out. It is easy to come up with a 
> minimal address range from the main object file prior to parsing any address 
> information and use that to weed out the bad entries. If functions have had 
> their address set to zero, then we will usually correctly weed these out as 
> zero is often part of an image base load address, but not in a section (not 
> program header or top level section in LLDB's ObjectFileELF, but in a section 
> header section) with r+x permissions. It would be nice to be able to weed 
> these addresses out so they don't cause problems possibly as a follow up 
> patch.




In D78489#1999110 , @labath wrote:

> Thanks for the review.
>
> In D78489#1998060 , @clayborg wrote:
>
> > In D78489#1996834 , @labath wrote:
> >
> > > I've done a similar benchmark to the last one, but measured memory usage 
> > > ("RssAnon" as reported by linux). One notable difference is that I
> > >
> > > Currently (information retrieved from DW_AT_ranges) we use about ~330 MB 
> > > of memory. If I switch to dwarf dies as the source, the memory goes all 
> > > the way to 2890 MB. This number is suspiciously large -- it either means 
> > > that our die freeing is not working properly, or that glibc is very bad 
> > > at releasing memory back to the OS. Given the magnitude of the increase, 
> > > i think it's a little bit of both. With line tables as the source the 
> > > memory usage is 706 MB. It's an increase from 330, but definitely smaller 
> > > than 2.8 GB. (the number 330 is kind of misleading here since we're not 
> > > considering removing that option -- it will always be used if it is 
> > > available).
> >
> >
> > Since we mmap in the entire DWARF, I am not surprised by taking up new 
> > memory because we touch those pages and won't get those back. If you remove 
> > the DIE freeing code, I will bet you see much more memory used. We 
> > definitely free the memory for the DIEs and give that back, so I would be 
> > willing to bet the increase you are seeing is from mmap loading pages in 
> > that we touch.
>
>
> I don't think it's as simple as that. "RssAnon" should not include 
> file-backed memory (which why I chose to measure it). According to `man proc`:
>
>   * RssAnon: Size of resident anonymous memory.  (since Linux 4.5).
>   * RssFile: Size of resident file mappings.  (since Linux 4.5).
>   
>
> I also don't think it's as simple as not freeing the DIE memory at all. There 
> has to be some more complex interaction going on. If I had more time, I would 
> be very interested in learning what it is, but for now I'll contend myself 
> with "it's not a problem of this patch".


FWIW, I can recommend Valgrind's massif for memory usage analysis - I think it 
works at the level of intercepting memory allocation (malloc/free/etc) level, 
so in some ways that makes it more actionable (it doesn't get sidetracked by 
the actual libc memory management implementation), but also less accurate (in 
that, yeah, your process is still going to hold that memory if your libc isn't 
releasing it back to teh OS) - but perhaps if you tune your application code 
for what massif tells you, then if there's still a big delta between that and 
the reality, see if there are tuning options for your memory allocator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78489



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

[Lldb-commits] [PATCH] D78462: get rid of PythonInteger::GetInteger()

2020-04-23 Thread Lawrence D'Anna via Phabricator via lldb-commits 
lawrence_danna updated this revision to Diff 259659.
lawrence_danna added a comment.

fix python2 projblems


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78462

Files:
  lldb/bindings/python/python-typemaps.swig
  lldb/bindings/python/python-wrapper.swig
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -123,13 +123,11 @@
   EXPECT_TRUE(major_version_field.IsAllocated());
   EXPECT_TRUE(minor_version_field.IsAllocated());
 
-  PythonInteger major_version_value =
-  major_version_field.AsType();
-  PythonInteger minor_version_value =
-  minor_version_field.AsType();
+  auto major_version_value = As(major_version_field);
+  auto minor_version_value = As(minor_version_field);
 
-  EXPECT_EQ(PY_MAJOR_VERSION, major_version_value.GetInteger());
-  EXPECT_EQ(PY_MINOR_VERSION, minor_version_value.GetInteger());
+  EXPECT_THAT_EXPECTED(major_version_value, llvm::HasValue(PY_MAJOR_VERSION));
+  EXPECT_THAT_EXPECTED(minor_version_value, llvm::HasValue(PY_MINOR_VERSION));
 }
 
 TEST_F(PythonDataObjectsTest, TestGlobalNameResolutionWithDot) {
@@ -137,16 +135,14 @@
   EXPECT_TRUE(sys_path.IsAllocated());
   EXPECT_TRUE(PythonList::Check(sys_path.get()));
 
-  PythonInteger version_major =
-  m_main_module.ResolveName("sys.version_info.major")
-  .AsType();
-  PythonInteger version_minor =
-  m_main_module.ResolveName("sys.version_info.minor")
-  .AsType();
-  EXPECT_TRUE(version_major.IsAllocated());
-  EXPECT_TRUE(version_minor.IsAllocated());
-  EXPECT_EQ(PY_MAJOR_VERSION, version_major.GetInteger());
-  EXPECT_EQ(PY_MINOR_VERSION, version_minor.GetInteger());
+  auto version_major =
+  As(m_main_module.ResolveName("sys.version_info.major"));
+
+  auto version_minor =
+  As(m_main_module.ResolveName("sys.version_info.minor"));
+
+  EXPECT_THAT_EXPECTED(version_major, llvm::HasValue(PY_MAJOR_VERSION));
+  EXPECT_THAT_EXPECTED(version_minor, llvm::HasValue(PY_MINOR_VERSION));
 }
 
 TEST_F(PythonDataObjectsTest, TestDictionaryResolutionWithDot) {
@@ -155,14 +151,14 @@
   dict.SetItemForKey(PythonString("sys"), m_sys_module);
 
   // Now use that dictionary to resolve `sys.version_info.major`
-  PythonInteger version_major =
-  PythonObject::ResolveNameWithDictionary("sys.version_info.major", dict)
-  .AsType();
-  PythonInteger version_minor =
-  PythonObject::ResolveNameWithDictionary("sys.version_info.minor", dict)
-  .AsType();
-  EXPECT_EQ(PY_MAJOR_VERSION, version_major.GetInteger());
-  EXPECT_EQ(PY_MINOR_VERSION, version_minor.GetInteger());
+  auto version_major = As(
+  PythonObject::ResolveNameWithDictionary("sys.version_info.major", dict));
+
+  auto version_minor = As(
+  PythonObject::ResolveNameWithDictionary("sys.version_info.minor", dict));
+
+  EXPECT_THAT_EXPECTED(version_major, llvm::HasValue(PY_MAJOR_VERSION));
+  EXPECT_THAT_EXPECTED(version_minor, llvm::HasValue(PY_MINOR_VERSION));
 }
 
 TEST_F(PythonDataObjectsTest, TestPythonInteger) {
@@ -176,7 +172,8 @@
   PythonInteger python_int(PyRefType::Owned, py_int);
 
   EXPECT_EQ(PyObjectType::Integer, python_int.GetObjectType());
-  EXPECT_EQ(12, python_int.GetInteger());
+  auto python_int_value = As(python_int);
+  EXPECT_THAT_EXPECTED(python_int_value, llvm::HasValue(12));
 #endif
 
   // Verify that `PythonInteger` works correctly when given a PyLong object.
@@ -187,12 +184,14 @@
 
   // Verify that you can reset the value and that it is reflected properly.
   python_long.SetInteger(40);
-  EXPECT_EQ(40, python_long.GetInteger());
+  auto e = As(python_long);
+  EXPECT_THAT_EXPECTED(e, llvm::HasValue(40));
 
   // Test that creating a `PythonInteger` object works correctly with the
   // int constructor.
   PythonInteger constructed_int(7);
-  EXPECT_EQ(7, constructed_int.GetInteger());
+  auto value = As(constructed_int);
+  EXPECT_THAT_EXPECTED(value, llvm::HasValue(7));
 }
 
 TEST_F(PythonDataObjectsTest, TestPythonBoolean) {
@@ -339,7 +338,8 @@
   PythonInteger chk_int(PyRefType::Borrowed, chk_value1.get());
   PythonString chk_str(PyRefType::Borrowed, chk_value2.get());
 
-  EXPECT_EQ(long_value0, chk_int.GetInteger());
+  auto chkint = As(chk_value1);
+  ASSERT_THAT_EXPECTED(chkint, llvm::HasValue(long_value0));
   EXPECT_EQ(string_value1, chk_str.GetString());
 }
 
@@ -367,7 +367,8 @@
   PythonInteger chk_int(PyRefType::Borrowed, chk_value1.get());
   PythonString chk_str(PyRe

[Lldb-commits] [PATCH] D78462: get rid of PythonInteger::GetInteger()

2020-04-23 Thread Lawrence D'Anna via Phabricator via lldb-commits 
lawrence_danna added a comment.

@omjavaid sorry I didn't catch that on pre-submit testing.   
`PyLong_AsLongLong` and friends do not automatically convert on python2 like 
they do on python3.   It's fixed now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78462



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

[Lldb-commits] [PATCH] D78462: get rid of PythonInteger::GetInteger()

2020-04-23 Thread Lawrence D'Anna via Phabricator via lldb-commits 
lawrence_danna updated this revision to Diff 259700.
lawrence_danna added a comment.

rebased


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78462

Files:
  lldb/bindings/python/python-typemaps.swig
  lldb/bindings/python/python-wrapper.swig
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
  lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp

Index: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
===
--- lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -123,13 +123,11 @@
   EXPECT_TRUE(major_version_field.IsAllocated());
   EXPECT_TRUE(minor_version_field.IsAllocated());
 
-  PythonInteger major_version_value =
-  major_version_field.AsType();
-  PythonInteger minor_version_value =
-  minor_version_field.AsType();
+  auto major_version_value = As(major_version_field);
+  auto minor_version_value = As(minor_version_field);
 
-  EXPECT_EQ(PY_MAJOR_VERSION, major_version_value.GetInteger());
-  EXPECT_EQ(PY_MINOR_VERSION, minor_version_value.GetInteger());
+  EXPECT_THAT_EXPECTED(major_version_value, llvm::HasValue(PY_MAJOR_VERSION));
+  EXPECT_THAT_EXPECTED(minor_version_value, llvm::HasValue(PY_MINOR_VERSION));
 }
 
 TEST_F(PythonDataObjectsTest, TestGlobalNameResolutionWithDot) {
@@ -137,16 +135,14 @@
   EXPECT_TRUE(sys_path.IsAllocated());
   EXPECT_TRUE(PythonList::Check(sys_path.get()));
 
-  PythonInteger version_major =
-  m_main_module.ResolveName("sys.version_info.major")
-  .AsType();
-  PythonInteger version_minor =
-  m_main_module.ResolveName("sys.version_info.minor")
-  .AsType();
-  EXPECT_TRUE(version_major.IsAllocated());
-  EXPECT_TRUE(version_minor.IsAllocated());
-  EXPECT_EQ(PY_MAJOR_VERSION, version_major.GetInteger());
-  EXPECT_EQ(PY_MINOR_VERSION, version_minor.GetInteger());
+  auto version_major =
+  As(m_main_module.ResolveName("sys.version_info.major"));
+
+  auto version_minor =
+  As(m_main_module.ResolveName("sys.version_info.minor"));
+
+  EXPECT_THAT_EXPECTED(version_major, llvm::HasValue(PY_MAJOR_VERSION));
+  EXPECT_THAT_EXPECTED(version_minor, llvm::HasValue(PY_MINOR_VERSION));
 }
 
 TEST_F(PythonDataObjectsTest, TestDictionaryResolutionWithDot) {
@@ -155,14 +151,14 @@
   dict.SetItemForKey(PythonString("sys"), m_sys_module);
 
   // Now use that dictionary to resolve `sys.version_info.major`
-  PythonInteger version_major =
-  PythonObject::ResolveNameWithDictionary("sys.version_info.major", dict)
-  .AsType();
-  PythonInteger version_minor =
-  PythonObject::ResolveNameWithDictionary("sys.version_info.minor", dict)
-  .AsType();
-  EXPECT_EQ(PY_MAJOR_VERSION, version_major.GetInteger());
-  EXPECT_EQ(PY_MINOR_VERSION, version_minor.GetInteger());
+  auto version_major = As(
+  PythonObject::ResolveNameWithDictionary("sys.version_info.major", dict));
+
+  auto version_minor = As(
+  PythonObject::ResolveNameWithDictionary("sys.version_info.minor", dict));
+
+  EXPECT_THAT_EXPECTED(version_major, llvm::HasValue(PY_MAJOR_VERSION));
+  EXPECT_THAT_EXPECTED(version_minor, llvm::HasValue(PY_MINOR_VERSION));
 }
 
 TEST_F(PythonDataObjectsTest, TestPythonInteger) {
@@ -176,7 +172,8 @@
   PythonInteger python_int(PyRefType::Owned, py_int);
 
   EXPECT_EQ(PyObjectType::Integer, python_int.GetObjectType());
-  EXPECT_EQ(12, python_int.GetInteger());
+  auto python_int_value = As(python_int);
+  EXPECT_THAT_EXPECTED(python_int_value, llvm::HasValue(12));
 #endif
 
   // Verify that `PythonInteger` works correctly when given a PyLong object.
@@ -187,12 +184,14 @@
 
   // Verify that you can reset the value and that it is reflected properly.
   python_long.SetInteger(40);
-  EXPECT_EQ(40, python_long.GetInteger());
+  auto e = As(python_long);
+  EXPECT_THAT_EXPECTED(e, llvm::HasValue(40));
 
   // Test that creating a `PythonInteger` object works correctly with the
   // int constructor.
   PythonInteger constructed_int(7);
-  EXPECT_EQ(7, constructed_int.GetInteger());
+  auto value = As(constructed_int);
+  EXPECT_THAT_EXPECTED(value, llvm::HasValue(7));
 }
 
 TEST_F(PythonDataObjectsTest, TestPythonBoolean) {
@@ -339,7 +338,8 @@
   PythonInteger chk_int(PyRefType::Borrowed, chk_value1.get());
   PythonString chk_str(PyRefType::Borrowed, chk_value2.get());
 
-  EXPECT_EQ(long_value0, chk_int.GetInteger());
+  auto chkint = As(chk_value1);
+  ASSERT_THAT_EXPECTED(chkint, llvm::HasValue(long_value0));
   EXPECT_EQ(string_value1, chk_str.GetString());
 }
 
@@ -367,7 +367,8 @@
   PythonInteger chk_int(PyRefType::Borrowed, chk_value1.get());
   PythonString chk_str(PyRefType::Borrowe

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

2020-04-23 Thread Shafik Yaghmour via Phabricator via lldb-commits 
shafik updated this revision to Diff 259703.
shafik marked 2 inline comments as done.
shafik added a comment.

Capitalization fixes.


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

https://reviews.llvm.org/D78000

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
  
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp

Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,35 @@
+struct B {
+  int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+  // Introduce a TypedefNameDecl
+  using Iterator = D *;
+};
+
+struct C {
+  // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+  // the DeclContext of C.
+  // We will invoke ImportContext(...) which should force the From Decl to
+  // be defined if it not already defined. We will then Import the definition
+  // to the To Decl.
+  // This will force processing of the base class of D which allows us to see
+  // base class methods such as dump().
+  D::Iterator iter;
+
+  bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int", result_value="42")
+  }
+};
+
+int main() {
+  C c;
+  D d;
+
+  c.f(&d);
+
+  return 0;
+}
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5922,6 +5922,70 @@
   EXPECT_TRUE(ToA);
 }
 
+struct ImportWithExternalSource : ASTImporterOptionSpecificTestBase {
+  ImportWithExternalSource() {
+Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+ ASTContext &FromContext, FileManager &FromFileManager,
+ bool MinimalImport,
+ const std::shared_ptr &SharedState) {
+  return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport,
+ // We use the regular lookup.
+ /*SharedState=*/nullptr);
+};
+  }
+};
+
+/// An ExternalASTSource that keeps track of the tags is completed.
+struct SourceWithCompletedTagList : clang::ExternalASTSource {
+  std::vector &CompletedTags;
+  SourceWithCompletedTagList(std::vector &CompletedTags)
+  : CompletedTags(CompletedTags) {}
+  void CompleteType(TagDecl *Tag) override {
+auto *Record = cast(Tag);
+Record->startDefinition();
+Record->completeDefinition();
+CompletedTags.push_back(Tag);
+  }
+  void
+  FindExternalLexicalDecls(const DeclContext *DC,
+   llvm::function_ref IsKindWeWant,
+   SmallVectorImpl &Result) override {}
+};
+
+TEST_P(ImportWithExternalSource, CompleteRecordBeforeImporting) {
+  // Create an empty TU.
+  TranslationUnitDecl *FromTU = getTuDecl("", Lang_CXX, "input.cpp");
+
+  // Create and add the test ExternalASTSource.
+  std::vector CompletedTags;
+  IntrusiveRefCntPtr source =
+  new SourceWithCompletedTagList(CompletedTags);
+  clang::ASTContext &Context = FromTU->getASTContext();
+  Context.setExternalSource(std::move(source));
+
+  // Create a dummy class by hand with external lexical storage.
+  IdentifierInfo &Ident = Context.Idents.get("test_class");
+  auto *Record = CXXRecordDecl::Create(
+  Context, TTK_Class, FromTU, SourceLocation(), SourceLocation(), &Ident);
+  Record->setHasExternalLexicalStorage();
+  FromTU->addDecl(Record);
+
+  // Do a minimal import of the created class.
+  EXPECT_EQ(0U, CompletedTags.size());
+  Import(Record, Lang_CXX);
+  EXPECT_EQ(0U, CompletedTags.size());
+
+  // Import the definition of the created class.
+  llvm::Error Err = findFromTU(Record)->Importer->ImportDefinition(Record);
+  EXPECT_FALSE((bool)Err);
+  consumeError(std::move(Err));
+
+  // Make sure the class was completed once.
+  EXPECT_EQ(1U, CompletedTags.size());
+  EXPECT_EQ(Record, CompletedTags.front());
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,

[Lldb-commits] [PATCH] D78588: [lldb/Core] Don't crash in GetSoftwareBreakpointTrapOpcode for unknown triples

2020-04-23 Thread Jonas Devlieghere via Phabricator via lldb-commits 
JDevlieghere updated this revision to Diff 259728.
JDevlieghere retitled this revision from "[lldb/Core] Check that ArchSpec is 
valid." to "[lldb/Core] Don't crash in GetSoftwareBreakpointTrapOpcode for 
unknown triples".

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

https://reviews.llvm.org/D78588

Files:
  lldb/source/Target/Platform.cpp


Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -1822,6 +1822,7 @@
 size_t Platform::GetSoftwareBreakpointTrapOpcode(Target &target,
  BreakpointSite *bp_site) {
   ArchSpec arch = target.GetArchitecture();
+  assert(arch.IsValid());
   const uint8_t *trap_opcode = nullptr;
   size_t trap_opcode_size = 0;
 
@@ -1918,8 +1919,7 @@
   } break;
 
   default:
-llvm_unreachable(
-"Unhandled architecture in Platform::GetSoftwareBreakpointTrapOpcode");
+return 0;
   }
 
   assert(bp_site);


Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -1822,6 +1822,7 @@
 size_t Platform::GetSoftwareBreakpointTrapOpcode(Target &target,
  BreakpointSite *bp_site) {
   ArchSpec arch = target.GetArchitecture();
+  assert(arch.IsValid());
   const uint8_t *trap_opcode = nullptr;
   size_t trap_opcode_size = 0;
 
@@ -1918,8 +1919,7 @@
   } break;
 
   default:
-llvm_unreachable(
-"Unhandled architecture in Platform::GetSoftwareBreakpointTrapOpcode");
+return 0;
   }
 
   assert(bp_site);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] def7c7f - [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

2020-04-23 Thread via lldb-commits 

Author: shafik
Date: 2020-04-23T15:18:48-07:00
New Revision: def7c7f6020530ef2deb64f786d28775b5d0e47f

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

LOG: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

In ImportContext(…) we may call into CompleteDecl(…) which if FromRecrord is not
defined will start the definition of a ToRecord but from what I can tell at 
least
one of the paths though here don't ensure we complete the definition.
For a RecordDecl this can be problematic since this means we won’t import base
classes and we won’t have any of the methods or types we inherit from these 
bases.

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

Added: 

lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py

lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp

Modified: 
clang/lib/AST/ASTImporter.cpp
clang/unittests/AST/ASTImporterTest.cpp

Removed: 




diff  --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index afd35e0137b6..ecb5ce85c204 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -8176,15 +8176,22 @@ Expected 
ASTImporter::ImportContext(DeclContext *FromDC) {
   // need it to have a definition.
   if (auto *ToRecord = dyn_cast(ToDC)) {
 auto *FromRecord = cast(FromDC);
-if (ToRecord->isCompleteDefinition()) {
-  // Do nothing.
-} else if (FromRecord->isCompleteDefinition()) {
+if (ToRecord->isCompleteDefinition())
+  return ToDC;
+
+// If FromRecord is not defined we need to force it to be.
+// Simply calling CompleteDecl(...) for a RecordDecl will break some cases
+// it will start the definition but we never finish it.
+// If there are base classes they won't be imported and we will
+// be missing anything that we inherit from those bases.
+if (FromRecord->getASTContext().getExternalSource() &&
+!FromRecord->isCompleteDefinition())
+  
FromRecord->getASTContext().getExternalSource()->CompleteType(FromRecord);
+
+if (FromRecord->isCompleteDefinition())
   if (Error Err = ASTNodeImporter(*this).ImportDefinition(
   FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
 return std::move(Err);
-} else {
-  CompleteDecl(ToRecord);
-}
   } else if (auto *ToEnum = dyn_cast(ToDC)) {
 auto *FromEnum = cast(FromDC);
 if (ToEnum->isCompleteDefinition()) {

diff  --git a/clang/unittests/AST/ASTImporterTest.cpp 
b/clang/unittests/AST/ASTImporterTest.cpp
index 9f35a86e2937..8ed6c487cb53 100644
--- a/clang/unittests/AST/ASTImporterTest.cpp
+++ b/clang/unittests/AST/ASTImporterTest.cpp
@@ -5942,6 +5942,70 @@ auto ExtendWithOptions(const T &Values, const ArgVector 
&Args) {
   return ::testing::ValuesIn(Copy);
 }
 
+struct ImportWithExternalSource : ASTImporterOptionSpecificTestBase {
+  ImportWithExternalSource() {
+Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+ ASTContext &FromContext, FileManager &FromFileManager,
+ bool MinimalImport,
+ const std::shared_ptr &SharedState) {
+  return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport,
+ // We use the regular lookup.
+ /*SharedState=*/nullptr);
+};
+  }
+};
+
+/// An ExternalASTSource that keeps track of the tags is completed.
+struct SourceWithCompletedTagList : clang::ExternalASTSource {
+  std::vector &CompletedTags;
+  SourceWithCompletedTagList(std::vector &CompletedTags)
+  : CompletedTags(CompletedTags) {}
+  void CompleteType(TagDecl *Tag) override {
+auto *Record = cast(Tag);
+Record->startDefinition();
+Record->completeDefinition();
+CompletedTags.push_back(Tag);
+  }
+  void
+  FindExternalLexicalDecls(const DeclContext *DC,
+   llvm::function_ref IsKindWeWant,
+   SmallVectorImpl &Result) override {}
+};
+
+TEST_P(ImportWithExternalSource, CompleteRecordBeforeImporting) {
+  // Create an empty TU.
+  TranslationUnitDecl *FromTU = getTuDecl("", Lang_CXX, "input.cpp");
+
+  // Create and add the test ExternalASTSource.
+  std::vector CompletedTags;
+  IntrusiveRefCntPtr source =
+  new SourceWithCompletedTagList(CompletedTags);
+  clang::ASTContext &Context = FromTU->getASTContext();
+  Context.setExternalSource(std::move(source));
+
+  // Create a dummy class by hand with external lexical storage.
+  IdentifierInfo &Ident = Context.Idents.get("test_class");
+  auto *Record = CXXRecordDecl::Create(
+  Context, TTK_Class, FromTU, SourceLocation()

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

2020-04-23 Thread Shafik Yaghmour via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGdef7c7f60205: [ASTImporter] Fix handling of not defined 
FromRecord in ImportContext(...) (authored by shafik).
Herald added projects: clang, LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D78000?vs=259703&id=259747#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78000

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
  
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp

Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp
@@ -0,0 +1,35 @@
+struct B {
+  int dump() const;
+};
+
+int B::dump() const { return 42; }
+
+// Derived class D obtains dump() method from base class B
+struct D : public B {
+  // Introduce a TypedefNameDecl
+  using Iterator = D *;
+};
+
+struct C {
+  // This will cause use to invoke VisitTypedefNameDecl(...) when Importing
+  // the DeclContext of C.
+  // We will invoke ImportContext(...) which should force the From Decl to
+  // be defined if it not already defined. We will then Import the definition
+  // to the To Decl.
+  // This will force processing of the base class of D which allows us to see
+  // base class methods such as dump().
+  D::Iterator iter;
+
+  bool f(D *DD) {
+return true; //%self.expect_expr("DD->dump()", result_type="int", result_value="42")
+  }
+};
+
+int main() {
+  C c;
+  D d;
+
+  c.f(&d);
+
+  return 0;
+}
Index: lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
===
--- /dev/null
+++ lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/TestImportBaseClassWhenClassHasDerivedMember.py
@@ -0,0 +1,4 @@
+from lldbsuite.test import lldbinline
+from lldbsuite.test import decorators
+
+lldbinline.MakeInlineTest(__file__, globals())
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -5942,6 +5942,70 @@
   return ::testing::ValuesIn(Copy);
 }
 
+struct ImportWithExternalSource : ASTImporterOptionSpecificTestBase {
+  ImportWithExternalSource() {
+Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+ ASTContext &FromContext, FileManager &FromFileManager,
+ bool MinimalImport,
+ const std::shared_ptr &SharedState) {
+  return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, MinimalImport,
+ // We use the regular lookup.
+ /*SharedState=*/nullptr);
+};
+  }
+};
+
+/// An ExternalASTSource that keeps track of the tags is completed.
+struct SourceWithCompletedTagList : clang::ExternalASTSource {
+  std::vector &CompletedTags;
+  SourceWithCompletedTagList(std::vector &CompletedTags)
+  : CompletedTags(CompletedTags) {}
+  void CompleteType(TagDecl *Tag) override {
+auto *Record = cast(Tag);
+Record->startDefinition();
+Record->completeDefinition();
+CompletedTags.push_back(Tag);
+  }
+  void
+  FindExternalLexicalDecls(const DeclContext *DC,
+   llvm::function_ref IsKindWeWant,
+   SmallVectorImpl &Result) override {}
+};
+
+TEST_P(ImportWithExternalSource, CompleteRecordBeforeImporting) {
+  // Create an empty TU.
+  TranslationUnitDecl *FromTU = getTuDecl("", Lang_CXX, "input.cpp");
+
+  // Create and add the test ExternalASTSource.
+  std::vector CompletedTags;
+  IntrusiveRefCntPtr source =
+  new SourceWithCompletedTagList(CompletedTags);
+  clang::ASTContext &Context = FromTU->getASTContext();
+  Context.setExternalSource(std::move(source));
+
+  // Create a dummy class by hand with external lexical storage.
+  IdentifierInfo &Ident = Context.Idents.get("test_class");
+  auto *Record = CXXRecordDecl::Create(
+  Context, TTK_Class, FromTU, SourceLocation(), SourceLocation(), &Ident);
+  Record->setHasExternalLexicalStorage();
+  FromTU->addDecl(Record);
+
+  // Do a minimal import of the created class.
+  EXPECT_EQ(0U, CompletedTags.size());
+  Import(Record, Lang_CXX);
+  EXPECT_EQ(0U, CompletedTags.size());
+
+  // Import the definition of the created class.
+  llvm::Error Err = findFromTU(Record)->Importer->ImportDefinition(Record);
+  EXPECT_FALSE((bool)Err);
+  consum

[Lldb-commits] [PATCH] D78791: [lldb][NFC][CallSite] Remove CallSite use

2020-04-23 Thread Mircea Trofin via Phabricator via lldb-commits 
mtrofin created this revision.
mtrofin added reviewers: dblaikie, craig.topper.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78791

Files:
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp


Index: 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
===
--- 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
+++ 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
@@ -10,7 +10,6 @@
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/BasicBlock.h"
-#include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Instruction.h"
@@ -158,12 +157,11 @@
 assert(new_func_type &&
"failed to clone functionType for Renderscript ABI fixup");
 
-llvm::CallSite call_site(call_inst);
 llvm::Function *func = call_inst->getCalledFunction();
 assert(func && "cannot resolve function in RenderScriptRuntime");
 // Copy the original call arguments
-std::vector new_call_args(call_site.arg_begin(),
- call_site.arg_end());
+std::vector new_call_args(call_inst->arg_begin(),
+ call_inst->arg_end());
 
 // Allocate enough space to store the return value of the original function
 // we pass a pointer to this allocation as the StructRet param, and then


Index: lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
===
--- lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
+++ lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
@@ -10,7 +10,6 @@
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/BasicBlock.h"
-#include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Instruction.h"
@@ -158,12 +157,11 @@
 assert(new_func_type &&
"failed to clone functionType for Renderscript ABI fixup");
 
-llvm::CallSite call_site(call_inst);
 llvm::Function *func = call_inst->getCalledFunction();
 assert(func && "cannot resolve function in RenderScriptRuntime");
 // Copy the original call arguments
-std::vector new_call_args(call_site.arg_begin(),
- call_site.arg_end());
+std::vector new_call_args(call_inst->arg_begin(),
+ call_inst->arg_end());
 
 // Allocate enough space to store the return value of the original function
 // we pass a pointer to this allocation as the StructRet param, and then
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 5948daf - [lldb][NFC][CallSite] Remove CallSite use

2020-04-23 Thread Mircea Trofin via lldb-commits 

Author: Mircea Trofin
Date: 2020-04-23T22:24:23-07:00
New Revision: 5948dafc694b5f8cb239b65f33727263d1a0bef6

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

LOG: [lldb][NFC][CallSite] Remove CallSite use

Reviewers: dblaikie, craig.topper

Subscribers: lldb-commits

Tags: #lldb

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

Added: 


Modified: 

lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp

Removed: 




diff  --git 
a/lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
 
b/lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
index 51bf5655ed12..f51190e0c82c 100644
--- 
a/lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
+++ 
b/lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
@@ -10,7 +10,6 @@
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/BasicBlock.h"
-#include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Instruction.h"
@@ -158,12 +157,11 @@ bool fixupX86StructRetCalls(llvm::Module &module) {
 assert(new_func_type &&
"failed to clone functionType for Renderscript ABI fixup");
 
-llvm::CallSite call_site(call_inst);
 llvm::Function *func = call_inst->getCalledFunction();
 assert(func && "cannot resolve function in RenderScriptRuntime");
 // Copy the original call arguments
-std::vector new_call_args(call_site.arg_begin(),
- call_site.arg_end());
+std::vector new_call_args(call_inst->arg_begin(),
+ call_inst->arg_end());
 
 // Allocate enough space to store the return value of the original function
 // we pass a pointer to this allocation as the StructRet param, and then



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

[Lldb-commits] [PATCH] D78791: [lldb][NFC][CallSite] Remove CallSite use

2020-04-23 Thread Craig Topper via Phabricator via lldb-commits 
craig.topper accepted this revision.
craig.topper added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78791



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

[Lldb-commits] [PATCH] D78791: [lldb][NFC][CallSite] Remove CallSite use

2020-04-23 Thread Mircea Trofin via Phabricator via lldb-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5948dafc694b: [lldb][NFC][CallSite] Remove CallSite use 
(authored by mtrofin).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78791

Files:
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp


Index: 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
===
--- 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
+++ 
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
@@ -10,7 +10,6 @@
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/BasicBlock.h"
-#include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Instruction.h"
@@ -158,12 +157,11 @@
 assert(new_func_type &&
"failed to clone functionType for Renderscript ABI fixup");
 
-llvm::CallSite call_site(call_inst);
 llvm::Function *func = call_inst->getCalledFunction();
 assert(func && "cannot resolve function in RenderScriptRuntime");
 // Copy the original call arguments
-std::vector new_call_args(call_site.arg_begin(),
- call_site.arg_end());
+std::vector new_call_args(call_inst->arg_begin(),
+ call_inst->arg_end());
 
 // Allocate enough space to store the return value of the original function
 // we pass a pointer to this allocation as the StructRet param, and then


Index: lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
===
--- lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
+++ lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptx86ABIFixups.cpp
@@ -10,7 +10,6 @@
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/BasicBlock.h"
-#include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Instruction.h"
@@ -158,12 +157,11 @@
 assert(new_func_type &&
"failed to clone functionType for Renderscript ABI fixup");
 
-llvm::CallSite call_site(call_inst);
 llvm::Function *func = call_inst->getCalledFunction();
 assert(func && "cannot resolve function in RenderScriptRuntime");
 // Copy the original call arguments
-std::vector new_call_args(call_site.arg_begin(),
- call_site.arg_end());
+std::vector new_call_args(call_inst->arg_begin(),
+ call_inst->arg_end());
 
 // Allocate enough space to store the return value of the original function
 // we pass a pointer to this allocation as the StructRet param, and then
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits