[Lldb-commits] [PATCH] D62756: Be consistent when adding names and mangled names to the index

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Being consistent definitely sounds like a good idea. Since this does change 
behavior somewhat, I'm wondering whether it would make sense to add a test 
here. The thing that's not clear to me is whether this change in behavior is 
only theoretical, or if it can also happen in a real-world scenario. We can 
always hand-construct an DIE which will have a linkage name, but no "normal" 
name, but I don't see how would this ever happen in practice. How did you 
initially discover this inconsistency?

> We might think about adding a feature where if there is a mangled name and no 
> simple name, chop up the demangled name like we do in the ObjectFile plug-ins 
> when we see mangled names so we extract the basename out and then could use 
> this as the simple name. This might help expressions find things correctly by 
> basename

My worry about that is two-fold:

- it adds a demangling step (a pretty expensive operation) to something that is 
already the biggest bottleneck in loading DWARF
- this isn't something that we would do when using accelerator tables, so this 
will result in inconsistent behavior in these two cases

To understand the tradeoffs here, it would be good to understand under what 
circumstances can this situation occur...




Comment at: source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp:114
+static inline bool AddMangled(const char *name, const char *mangled) {
+  if (mangled == nullptr || name == mangled)
+return false;

aprantl wrote:
> Even in a static function I feel weird comparing two char* by pointer value. 
> Can you use llvm::StringRefs instead? then we don't need to call the unsafe 
> strcmp either and just trust that operator== is implemented efficiently.
Since this is *the* hottest piece of code when not using the accelerator 
tables, I think it makes sense to spend some time to squeeze every last bit of 
performace from it. The problem I see with constructing a StringRef is that it 
will force a linear scan of the c string regardless of the result of 
comparison. Since (I expect) the most comon result of this function will be 
"true", it means we will end up adding this string to the lookup tables, which 
will mean another linear scan due to the construction of ConstString.

Though, I guess that means that if the most frequent (>95%) result here is 
"unequal", we can just optimistically ConstString-ify both names, and rely on 
the ConstString operator== to do the fast comparison. That may result in less 
overall branches in the code and faster execution, but this is just a 
speculation on my part.


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

https://reviews.llvm.org/D62756



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

[Lldb-commits] [PATCH] D62755: [Target] Remove Process::GetCPPLanguageRuntime

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I agree with your reasoning about language runtime dependencies, and I think 
this is the right way to accomplish this.

However, I just want to add that from an lldb-server POV, even the fact that we 
pull in the `Process` class into it's dependency graph is a bug. So, what I'd 
do (and what I have been doing so far, but mainly as a passtime effort) was to 
cut the dependencies way lower that this. Ideally, I wouldn't want to include 
anything from the `Target` library in lldb-server, possibly by moving some 
stuff that lldb-server does need to use (MemoryRegionInfo) out of it.

But, you know, there are multiple angles from which you can attack this 
problem, and if this results in dependency cleanups in the rest of lldb, then I 
don't see a reason to not proceed here.




Comment at: include/lldb/Target/CPPLanguageRuntime.h:47
+  static CPPLanguageRuntime *GetCPPLanguageRuntime(Process &process) {
+return static_cast(
+process.GetLanguageRuntime(lldb::eLanguageTypeC_plus_plus));

It might be nice to add some glue so we could write 
`llvm::cast_or_null(...)` here. The main advantage of that 
being that we'd automatically get an assert firing if `GetLanguageRuntime` ever 
returns something which is *not* a CPPLanguageRuntime.


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

https://reviews.llvm.org/D62755



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

[Lldb-commits] [PATCH] D62788: [lldb-server unittest] Add missing teardown logic

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.

> Not sure why this isn't failing the build bots.. (unless they're running 
> without asserts?).

The build bots (and lit, in general) runs each of the unit tests in isolation 
(so it first gets a list of all tests, then runs the executable with 
`--gtest_filter=TestName` for each of the tests in turn). This means issues 
like this don't show up there.

However, it's good to fix them nonetheless.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62788



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

[Lldb-commits] [PATCH] D62788: [lldb-server unittest] Add missing teardown logic

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I should add (since I'm guessing you found this problem while trying to add 
unit tests for your lldb-server changes) a bit of history about these tests. 
There are two kinds of tests for lldb-server. The python tests 
(test/testcases/tools/lldb-server) are older ones. The c++ ones were added in 
an attempt to fix the issues which made the python tests hard to understand. 
However, the person who was doing that left the project, so the were never 
quite finished. In fact, I've been lately considering just removing them and 
instead to try to fix the python issues incrementally in-place.

I don't think this has any immediate impact on what you're doing now -- feel 
free to use whichever method you find the easiest to write the tests you need. 
But if, after doing that, you have any feedback about one test method or the 
other, I'd very much like to hear it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62788



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

[Lldb-commits] [PATCH] D62796: [Target] Generalize some behavior in Target::SymbolsDidLoad

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Target/Target.cpp:1670-1672
+  if (LanguageRuntime *objc_runtime =
+  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC))
 objc_runtime->SymbolsDidLoad(module_list);

Same question about runtime iteration as in the other review.


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

https://reviews.llvm.org/D62796



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

[Lldb-commits] [PATCH] D62795: [Target] Move ObjCLanguageRuntime::LookupRuntimeSymbol into LanguageRuntime

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

If this is supposed to be truly language-agnostic, shouldn't we be iterating 
over all language plugins and asking all of them for the "runtime symbol" (not 
that I know what a runtime symbol is, really)?

Otherwise, this is still language-specific behavior, even though it's not 
visible to the linker because it's hidden by an enum and a virtual interface.


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

https://reviews.llvm.org/D62795



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

[Lldb-commits] [PATCH] D62797: [Expression] Add PersistentExpressionState::SetCompilerTypeFromPersistentDecl

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The idea seems right, but I'm not too familiar with this part of the code. 
Someone else ought to look at this too.




Comment at: include/lldb/Expression/ExpressionVariable.h:235-236
 
+  virtual bool SetCompilerTypeFromPersistentDecl(ConstString type_name,
+ CompilerType &compiler_type) 
= 0;
+

It would be better if this would return `Optional`(or 
`Expected`)``


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

https://reviews.llvm.org/D62797



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

[Lldb-commits] [PATCH] D62499: Create a generic handler for Xfer packets

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for writing the test, I have a bunch of more comments on the test 
itself, but hopefully this will be the last iteration. :)




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2766
 // Grab the auxv data.
 auto buffer_or_error = m_debugged_process_up->GetAuxvData();
 if (!buffer_or_error) {

aadsm wrote:
> @labath, I was thinking about the auto rule and I'm not sure how it would 
> play here. I remember having to check the GetAuvData to know what this was 
> returning. However, it's quite verbose but I have found instances like this 
> in the codebase:
> ```
> ErrorOr> buf =
>   llvm::WritableMemoryBuffer::getNewMemBuffer(auxv_size);
> ```
Yeah, for more complex types like this, the readability question gets a bit 
fuzzier. Right now, I think I'd spell this type out fully, but even I'm not 
fully consistent about that (I'm pretty sure I originally wrote the `auto` line 
on the LHS). Overall, I wouldn't worry too much about this particular case.



Comment at: lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp:1
+//===-- CommunicationServerTest.cpp -*- C++ 
-*-===//
+//

Given the current layout of the code, a better place for this would be 
`unittests/Process/gdb-remote`, because that's where the class it is testing 
lives, and it is also the place where the "client" unit tests are located. The 
"client" unit tests are more similar to this than the "lldb-server" "unit" 
tests, because the latter test the lldb-server as a whole, whereas the client 
tests do mocking and other unittest-y stuff.

(Also, please name it GDBRemoteCommunicationServerTest, based on the 
class-under-test name)



Comment at: 
lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp:40-42
+for (size_t i = 0; i < dst_len; i++) {
+  packet.append(1, *(static_cast(dst) + i));
+}

aadsm wrote:
> Is there a better way to do this? I found the copy function but it accepts a 
> `char *`, not a `const char *`.
I'm not sure which copy function you meant, but I'd just do something like 
`packets.emplace_back(static_cast(dst), dst_len)`



Comment at: 
lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp:47
+
+  PacketsSP m_packets_sp;
+};

You could just make this class take a `std::vector &`, and make 
the MockServer class responsible for owning the packet array and handing it out 
via `GetPackets` as `llvm::ArrayRef`.

Shared pointers are used much more frequently than they need to be in lldb. If 
you look at the llvm codebase, you'll see that it uses shared ownership 
extremely rarely. For simple types like this it doesn't matter, but once you 
start having objects with non-trivial destructors, and cross-referencing them 
via shared pointers, it becomes hard to reason about what are the possible 
orders destruction of various things. That's why it's better to just not use 
them at all, if possible.



Comment at: 
lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp:50
+
+struct MockServer : public GDBRemoteCommunicationServer {
+  MockServer()

I'd make this a class, according to 
.



Comment at: 
lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp:59-69
+  PacketResult SendErrorResponse(uint8_t error) {
+return GDBRemoteCommunicationServer::SendErrorResponse(error);
+  }
+
+  PacketResult SendErrorResponse(const Status &error) {
+return GDBRemoteCommunicationServer::SendErrorResponse(error);
+  }

It looks like you could replace these by `using 
GDBRemoteCommunicationServer::SendErrorResponse`



Comment at: 
lldb/unittests/tools/lldb-server/tests/CommunicationServerTest.cpp:81-82
+
+  EXPECT_EQ(server.GetPackets()->size(), 1UL);
+  EXPECT_EQ(server.GetPackets()->at(0), std::string("$E42#ab"));
+}

`EXPECT_THAT(server.GetPackets(), testing::ElementsAre("$E42#ab"));`

Besides being a one-liner, this will give you better error messages if we ever 
have more than one packet here (it will print out the packets instead of just 
saying "1 != 2").


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62499



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

[Lldb-commits] [PATCH] D62500: Add support to read aux vector values

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.



Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:629-631
+  llvm::Optional vdso_base =
+  m_auxv->GetAuxValue(AuxVector::AUXV_AT_SYSINFO_EHDR);
+  if (vdso_base)

```
if (llvm::Optional vdso_base =
  m_auxv->GetAuxValue(AuxVector::AUXV_AT_SYSINFO_EHDR))
  m_vdso_base = *vdso_base;
```
(and similar elsewhere)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62500



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

[Lldb-commits] [PATCH] D62502: Implement xfer:libraries-svr4:read packet

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Regarding the test, I am wondering whether requiring the `/proc/%d/maps` and 
the linker rendezvous structure to match isn't too strict of a requirement. 
Dynamic linkers (and particularly android dynamic linkers) do a lot of crazy 
stuff, and trying to assert this invariant exposes us to all kinds of 
"optimizations" that the linkers do or may do in the future. I'm wondering if 
it wouldn't be better to just build an executable with one or two dependent 
libraries, and then just assert that their entries are present in the library 
list. However, I'm not completely sure on this, so I'd like to hear what you 
think about that..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62502



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

[Lldb-commits] [PATCH] D62714: [FormatEntity] Ignore ASCII escape sequences when colors are disabled.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/trunk/include/lldb/Core/FormatEntity.h:44
   ParentString,
-  InsertString,
+  EscapeCode,
   Root,

InsertString is used for more than just escape codes so it is now not named 
correctly. For a variable formatter string like "(${var.x}, ${var.y})", the "(" 
is a string, and ", " and ")" are InsertString objects. Any constant string 
that is part of a formatter. Can you change the name back?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62714



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

[Lldb-commits] [PATCH] D62732: [WIP][RISCV] Initial port of LLDB for RISC-V

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

You are following all of the patterns for all of the architectures. It would be 
nice for us to cleanup DynamicRegisterInfo.cpp, Platform.cpp, and Thread.cpp 
eventually so we don't need to modify these files when a few arch is added by 
abstracting things into lldb_private::Architecture, but that is beyond the 
scope of this change.




Comment at: lldb/source/Plugins/ABI/SysV-riscv/ABISysV_riscv.cpp:41
+ eFormatHex,
+ {0, 0, LLDB_INVALID_REGNUM, 0, 0},
+ nullptr,

No need to fill in the "eRegisterKindProcessPlugin" field of each register 
info. That is designed to be the number that any process plug-in (probably 
ProcessGDBRemote.cpp) fills in, so best to not fill this in. The orders are: EH 
frame, DWARF, generic, process plug-in and then LLDB. So I would change all of 
these to:
```
 {0, 0, LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, 0},
```



Comment at: lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:624-635
+case llvm::Triple::riscv32:
+case llvm::Triple::riscv64:
+  for (auto ® : m_regs) {
+if (strcmp(reg.name, "x1") == 0)
+  reg.kinds[eRegisterKindGeneric] = LLDB_REGNUM_GENERIC_RA;
+else if (strcmp(reg.name, "x2") == 0)
+  reg.kinds[eRegisterKindGeneric] = LLDB_REGNUM_GENERIC_SP;

Seems like you filled all of this info in the registers defs already in 
ABISysV_riscv.cpp? Do we need this here? Some platforms weren't setting these 
correctly and this code was meant to correct that.



Comment at: lldb/source/Target/Platform.cpp:1914-1922
+  case llvm::Triple::riscv32:
+  case llvm::Triple::riscv64: {
+// FIXME: Use ebreak when c_ebreak is not available.
+static const uint8_t g_riscv_c_opcode[] = {0x02, 0x90}; // c_ebreak
+trap_opcode = g_riscv_c_opcode;
+trap_opcode_size = sizeof(g_riscv_c_opcode);
+break;

This would be great to factor out into lldb_private::Architecture eventually. 
You are correctly following the patterns that are in LLDB. I will see if I can 
get this to happen soon so we don't run into these issues again.



Comment at: lldb/source/Target/Thread.cpp:2060-2084
 switch (machine) {
 case llvm::Triple::x86_64:
 case llvm::Triple::x86:
 case llvm::Triple::arm:
 case llvm::Triple::aarch64:
 case llvm::Triple::thumb:
 case llvm::Triple::mips:

Seems like the original logic before your mods is not correct. Not sure the 
default case ever gets used here. The arch of x86_64, x86, arm, aarch64 or 
thumb should all be caught before we get to the default statement for apple 
targets. I will add Jason Molenda to the change to get a comment on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62732



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

[Lldb-commits] [PATCH] D62812: [llvm] [CodeView] Move Triple::ArchType → CPUType mapping from LLDB

[Michał Górny via Phabricator via lldb-commits]

mgorny created this revision.
mgorny added reviewers: labath, mtrent, compnerd.
Herald added a project: LLVM.

Since neither the CodeView::CPUType nor Triple::ArchType constants are
specific to LLDB, it makes no sense to maintain their mapping there.
Instead, introduce a small inline function to provide the mapping
in CodeView, and provide a convenience overload of getRegisterNames().


https://reviews.llvm.org/D62812

Files:
  lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
  llvm/include/llvm/DebugInfo/CodeView/CodeView.h
  llvm/include/llvm/DebugInfo/CodeView/EnumTables.h


Index: llvm/include/llvm/DebugInfo/CodeView/EnumTables.h
===
--- llvm/include/llvm/DebugInfo/CodeView/EnumTables.h
+++ llvm/include/llvm/DebugInfo/CodeView/EnumTables.h
@@ -10,6 +10,7 @@
 #define LLVM_DEBUGINFO_CODEVIEW_ENUMTABLES_H
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/BinaryFormat/COFF.h"
 #include "llvm/DebugInfo/CodeView/CodeView.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -38,6 +39,11 @@
 ArrayRef>
 getImageSectionCharacteristicNames();
 
+inline ArrayRef>
+getRegisterNames(Triple::ArchType ArchType) {
+  return getRegisterNames(getCPUTypeForArchType(ArchType));
+}
+
 } // end namespace codeview
 } // end namespace llvm
 
Index: llvm/include/llvm/DebugInfo/CodeView/CodeView.h
===
--- llvm/include/llvm/DebugInfo/CodeView/CodeView.h
+++ llvm/include/llvm/DebugInfo/CodeView/CodeView.h
@@ -16,6 +16,7 @@
 #include 
 #include 
 
+#include "llvm/ADT/Triple.h"
 #include "llvm/Support/Endian.h"
 
 namespace llvm {
@@ -138,6 +139,16 @@
   D3D11_Shader = 0x100,
 };
 
+/// Convert llvm::Triple::ArchType value into llvm::codeview::CPUType.
+inline CPUType getCPUTypeForArchType(const Triple::ArchType &ArchType) {
+  switch (ArchType) {
+  case Triple::ArchType::aarch64:
+return CPUType::ARM64;
+  default:
+return CPUType::X64;
+  }
+}
+
 /// These values correspond to the CV_CFL_LANG enumeration, and are documented
 /// here: https://msdn.microsoft.com/en-us/library/bw3aekw6.aspx
 enum SourceLanguage : uint8_t {
Index: 
lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
@@ -25,19 +25,8 @@
 
 static uint32_t ResolveLLDBRegisterNum(llvm::StringRef reg_name, 
llvm::Triple::ArchType arch_type) {
   // lookup register name to get lldb register number
-  llvm::codeview::CPUType cpu_type;
-  switch (arch_type) {
-case llvm::Triple::ArchType::aarch64:
-  cpu_type = llvm::codeview::CPUType::ARM64;
-  break;
-
-default:
-  cpu_type = llvm::codeview::CPUType::X64;
-  break;
-  }
-
   llvm::ArrayRef> register_names =
-  llvm::codeview::getRegisterNames(cpu_type);
+  llvm::codeview::getRegisterNames(arch_type);
   auto it = llvm::find_if(
   register_names,
   [®_name](const llvm::EnumEntry ®ister_entry) {


Index: llvm/include/llvm/DebugInfo/CodeView/EnumTables.h
===
--- llvm/include/llvm/DebugInfo/CodeView/EnumTables.h
+++ llvm/include/llvm/DebugInfo/CodeView/EnumTables.h
@@ -10,6 +10,7 @@
 #define LLVM_DEBUGINFO_CODEVIEW_ENUMTABLES_H
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/BinaryFormat/COFF.h"
 #include "llvm/DebugInfo/CodeView/CodeView.h"
 #include "llvm/Support/ScopedPrinter.h"
@@ -38,6 +39,11 @@
 ArrayRef>
 getImageSectionCharacteristicNames();
 
+inline ArrayRef>
+getRegisterNames(Triple::ArchType ArchType) {
+  return getRegisterNames(getCPUTypeForArchType(ArchType));
+}
+
 } // end namespace codeview
 } // end namespace llvm
 
Index: llvm/include/llvm/DebugInfo/CodeView/CodeView.h
===
--- llvm/include/llvm/DebugInfo/CodeView/CodeView.h
+++ llvm/include/llvm/DebugInfo/CodeView/CodeView.h
@@ -16,6 +16,7 @@
 #include 
 #include 
 
+#include "llvm/ADT/Triple.h"
 #include "llvm/Support/Endian.h"
 
 namespace llvm {
@@ -138,6 +139,16 @@
   D3D11_Shader = 0x100,
 };
 
+/// Convert llvm::Triple::ArchType value into llvm::codeview::CPUType.
+inline CPUType getCPUTypeForArchType(const Triple::ArchType &ArchType) {
+  switch (ArchType) {
+  case Triple::ArchType::aarch64:
+return CPUType::ARM64;
+  default:
+return CPUType::X64;
+  }
+}
+
 /// These values correspond to the CV_CFL_LANG enumeration, and are documented
 /// here: https://msdn.microsoft.com/en-us/library/bw3aekw6.aspx
 enum SourceLanguage : uint8_t {
Index: lldb/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
===
--- lldb/source/Plugins/Symbol

[Lldb-commits] [PATCH] D62743: Add color to the default thread and frame format.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Attaching my versions in case you like anything that I did:

  settings set frame-format frame #${frame.index}: 
${ansi.fg.yellow}${frame.pc}${ansi.normal}{ 
${ansi.bold}${ansi.underline}${ansi.fg.cyan}${module.file.basename}${ansi.normal}${ansi.fg.cyan}
 ${function.name-with-args}{${function.pc-offset}}${ansi.normal}}{ 
${ansi.fg.purple}at 
${ansi.bold}${ansi.fg.purple}${line.file.basename}:${line.number}}${ansi.normal}\n
  settings set thread-format thread #${thread.index}: 
${ansi.bold}${ansi.fg.yellow}tid${ansi.normal}${ansi.fg.yellow} = 
${thread.id}${ansi.normal}{, ${ansi.fg.yellow}${frame.pc}${ansi.normal}}{ 
${ansi.bold}${ansi.underline}${ansi.fg.cyan}${module.file.basename}${ansi.normal}${ansi.fg.cyan}
 ${function.name-with-args}{${function.pc-offset}}${ansi.normal}}{, 
${ansi.bold}${ansi.fg.purple}stop reason${ansi.normal}${ansi.fg.purple} = 
${thread.stop-reason}${ansi.normal}}{, ${ansi.bold}${ansi.fg.cyan}name = 
${ansi.normal}${ansi.fg.cyan}${thread.name}${ansi.normal}}{, 
${ansi.bold}${ansi.fg.green}queue = 
${ansi.normal}${ansi.fg.green}${thread.queue}${ansi.normal}}\n{Return value: 
${thread.return-value}}


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

https://reviews.llvm.org/D62743



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

[Lldb-commits] [PATCH] D62788: [lldb-server unittest] Add missing teardown logic

[António Afonso via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362406: [lldb-server unittest] Add missing teardown logic 
(authored by aadsm, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62788?vs=202581&id=202726#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62788

Files:
  lldb/trunk/unittests/tools/lldb-server/tests/TestBase.h


Index: lldb/trunk/unittests/tools/lldb-server/tests/TestBase.h
===
--- lldb/trunk/unittests/tools/lldb-server/tests/TestBase.h
+++ lldb/trunk/unittests/tools/lldb-server/tests/TestBase.h
@@ -25,6 +25,11 @@
 lldb_private::HostInfo::Initialize();
   }
 
+  static void TearDownTestCase() {
+lldb_private::HostInfo::Terminate();
+lldb_private::FileSystem::Terminate();
+  }
+
   static std::string getInferiorPath(llvm::StringRef Name) {
 llvm::SmallString<64> Path(LLDB_TEST_INFERIOR_PATH);
 llvm::sys::path::append(Path, Name + LLDB_TEST_INFERIOR_SUFFIX);


Index: lldb/trunk/unittests/tools/lldb-server/tests/TestBase.h
===
--- lldb/trunk/unittests/tools/lldb-server/tests/TestBase.h
+++ lldb/trunk/unittests/tools/lldb-server/tests/TestBase.h
@@ -25,6 +25,11 @@
 lldb_private::HostInfo::Initialize();
   }
 
+  static void TearDownTestCase() {
+lldb_private::HostInfo::Terminate();
+lldb_private::FileSystem::Terminate();
+  }
+
   static std::string getInferiorPath(llvm::StringRef Name) {
 llvm::SmallString<64> Path(LLDB_TEST_INFERIOR_PATH);
 llvm::sys::path::append(Path, Name + LLDB_TEST_INFERIOR_SUFFIX);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D62634: Improve DWARF parsing and accessing by 1% to 2%

[Pavel Labath via Phabricator via lldb-commits]

labath added a subscriber: dblaikie.
labath added a comment.

Actually, there was a subtle change of behavior here. Before this change, if we 
tried to parse a DIE using an abbreviation table from a different file, we 
would most likely fail immediately because of the fail-safe in 
GetAbbreviationDeclarationPtr. Now, we just do a blind read and return bogus 
data.

Now, normally this shouldn't matter, but in case of split-dwarf, things start 
to get interesting. Lldb has this assumption that when we are reading a debug 
info entry (which is not the CU DIE), we are actually reading it from the dwo 
compile unit and not the skeleton unit in the main file. This means it uses the 
dwo abbreviation list and everything. Now, as far as I can tell, LLDB is kind 
of right here. The DWARF5 (in v4 split dwarf was a non-standard extension) spec 
says "A skeleton compilation unit has no children." (3.1.2 Skeleton Compilation 
Unit Entries). And indeed, gcc produces compilation units with no children. 
Clang, on the other hand, seems to be putting some DIEs into the main CU, 
**if** the compilation results in functions being inlined.  And it seems clang 
has been doing this since at least 7.0 (I haven't tried older versions).

So it seems we have two problems here:

1. We need to figure out whether there's a bug in clang and fix it. + @dblaikie 
for help with that
2. Depending on the outcome of the first item, we need to do something in lldb 
to handle this. If this is a clang bug (and my feeling is that it is), then the 
best solution might be to just start ignoring any non-CU dies from the main 
file in split-dwarf scenario

@dblaikie: Do you agree with my assessment above? If this is indeed a clang 
bug, any chance you could help with debugging the clang/llvm side of things?

The simplest reproducer for this would be something like:

  inline __attribute__((always_inline)) int foo(int x) { return x; }
  
  int main(int argc) { return foo(argc); }

`$ clang++ a.cc -c -o a.o -g -gsplit-dwarf`


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

https://reviews.llvm.org/D62634



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

[Lldb-commits] [PATCH] D62796: [Target] Generalize some behavior in Target::SymbolsDidLoad

[Greg Clayton via Phabricator via lldb-commits]

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

Seems like we need a Process::SymbolsDidLoad(...) function that we can call. It 
should iterate over all loaded language runtimes (only loaded ones) and forward 
the list.




Comment at: source/Target/Target.cpp:1670
 if (m_process_sp) {
-  LanguageRuntime *runtime =
-  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC);
-  if (runtime) {
-ObjCLanguageRuntime *objc_runtime = (ObjCLanguageRuntime *)runtime;
+  if (LanguageRuntime *objc_runtime =
+  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC))

labath wrote:
> Same question about runtime iteration as in the other review.
Seems like we should add a Process::SymbolsDidLoad() method that matches this 
method's signature and just call that. Process would iterate across all loaded 
language runtimes and call SymbolsDidLoad on any?


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

https://reviews.llvm.org/D62796



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

[Lldb-commits] [PATCH] D62634: Improve DWARF parsing and accessing by 1% to 2%

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D62634#1527533 , @labath wrote:

> Actually, there was a subtle change of behavior here. Before this change, if 
> we tried to parse a DIE using an abbreviation table from a different file, we 
> would most likely fail immediately because of the fail-safe in 
> GetAbbreviationDeclarationPtr. Now, we just do a blind read and return bogus 
> data.
>
> Now, normally this shouldn't matter, but in case of split-dwarf, things start 
> to get interesting. Lldb has this assumption that when we are reading a debug 
> info entry (which is not the CU DIE), we are actually reading it from the dwo 
> compile unit and not the skeleton unit in the main file. This means it uses 
> the dwo abbreviation list and everything. Now, as far as I can tell, LLDB is 
> kind of right here. The DWARF5 (in v4 split dwarf was a non-standard 
> extension) spec says "A skeleton compilation unit has no children." (3.1.2 
> Skeleton Compilation Unit Entries). And indeed, gcc produces compilation 
> units with no children. Clang, on the other hand, seems to be putting some 
> DIEs into the main CU, **if** the compilation results in functions being 
> inlined.  And it seems clang has been doing this since at least 7.0 (I 
> haven't tried older versions).
>
> So it seems we have two problems here:
>
> 1. We need to figure out whether there's a bug in clang and fix it. + 
> @dblaikie for help with that


This is intentional behavior in clang (controllable under the 
-f[no-]split-dwarf-inlining flag, and modified by the 
-f[no-]debug-info-for-profiling flag).

This extra debug info is used for online symbolication (in the absence of .dwo 
files) - such as for the sanitizers (accurate symbolication is necessary for 
correctness in msan, due to msan's necessary use of blacklisting to avoid 
certain false positives) and some forms of sample based profiling collection.

In the default mode, clang includes, as you noted, just the subprograms that 
have inlined subroutines in them in this split-dwarf-inlining data.
In -fdebug-info-for-profiling all subprograms are described in the skeleton CU 
with some minimal attributes (they don't need parameters, local 
variables/scopes, etc) necessary to do certain profile things I don't know lots 
about.

Chances are it's probably best for a debugger to ignore these entries.

> 2. Depending on the outcome of the first item, we need to do something in 
> lldb to handle this. If this is a clang bug (and my feeling is that it is), 
> then the best solution might be to just start ignoring any non-CU dies from 
> the main file in split-dwarf scenario
> 
>   @dblaikie: Do you agree with my assessment above? If this is indeed a clang 
> bug, any chance you could help with debugging the clang/llvm side of things?
> 
>   The simplest reproducer for this would be something like: ``` inline 
> __attribute__((always_inline)) int foo(int x) { return x; }
> 
>   int main(int argc) { return foo(argc); } ``` `$ clang++ a.cc -c -o a.o -g 
> -gsplit-dwarf`




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

https://reviews.llvm.org/D62634



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

[Lldb-commits] [PATCH] D62743: Add color to the default thread and frame format.

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Sorry, I got more comments :-)




Comment at: lldb/source/Core/Debugger.cpp:124
+#define FILE_COLOR "${ansi.fg.yellow}"
+#define STOP_COLOR "${ansi.fg.red}$"
+

Thanks, but I was thinking more about something that could be replaced 
dynamically to support dark and light terminals in the same LLDB binary. Int 
hits form, this probably doesn't add much because ...



Comment at: lldb/source/Core/Debugger.cpp:141
   "{, ${thread.info.trace_messages} messages}" 
\
-  "{, stop reason = ${thread.stop-reason}}"
\
+  "{, stop reason = " STOP_COLOR " ${thread.stop-reason} ${ansi.normal}}"  
\
   "{\\nReturn value: ${thread.return-value}}"  
\

... because we probably want a macro for the entire "stop reason" string, not 
just its colors ...



Comment at: lldb/source/Core/Debugger.cpp:151
   "{, ${thread.info.trace_messages} messages}" 
\
-  "{, stop reason = ${thread.stop-reason}}"
\
+  "{, stop reason = " STOP_COLOR " ${thread.stop-reason} ${ansi.normal}}"  
\
   "{\\nReturn value: ${thread.return-value}}"  
\

... and use it here, too.


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

https://reviews.llvm.org/D62743



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

[Lldb-commits] [PATCH] D62634: Improve DWARF parsing and accessing by 1% to 2%

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In D62634#1527575 , @dblaikie wrote:

> This is intentional behavior in clang (controllable under the 
> -f[no-]split-dwarf-inlining flag, and modified by the 
> -f[no-]debug-info-for-profiling flag).
>
> This extra debug info is used for online symbolication (in the absence of 
> .dwo files) - such as for the sanitizers (accurate symbolication is necessary 
> for correctness in msan, due to msan's necessary use of blacklisting to avoid 
> certain false positives) and some forms of sample based profiling collection.
>
> In the default mode, clang includes, as you noted, just the subprograms that 
> have inlined subroutines in them in this split-dwarf-inlining data.
>  In -fdebug-info-for-profiling all subprograms are described in the skeleton 
> CU with some minimal attributes (they don't need parameters, local 
> variables/scopes, etc) necessary to do certain profile things I don't know 
> lots about.


I think we have to tolerate the msan part.  However, the profile feedback 
workflow could (should) surely be taught to read a .dwo file.  The point of 
-fdebug-info-for-profiling was so you don't have to emit limited/full debug 
info in the .o file just to make profiling work; but if you're using split 
DWARF then you're doing limited/full anyway, and the feedback loop happens in 
the context of a build environment so the .dwo can be asserted to be available. 
 So in split DWARF mode, we should not be emitting debug-info-for-profiling 
into the skeleton.

> Chances are it's probably best for a debugger to ignore these entries.

This stuff is going to show up in the wild, so yeah.  You know you're looking 
at a skeleton, so don't bother looking at any children.


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

https://reviews.llvm.org/D62634



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

Re: [Lldb-commits] [PATCH] D62634: Improve DWARF parsing and accessing by 1% to 2%

[Greg Clayton via lldb-commits]

> On Jun 3, 2019, at 8:33 AM, Pavel Labath via Phabricator 
>  wrote:
> 
> labath added a subscriber: dblaikie.
> labath added a comment.
> 
> Actually, there was a subtle change of behavior here. Before this change, if 
> we tried to parse a DIE using an abbreviation table from a different file, we 
> would most likely fail immediately because of the fail-safe in 
> GetAbbreviationDeclarationPtr. Now, we just do a blind read and return bogus 
> data.

This seems just like a straight up bug we need to fix in LLDB. It shouldn't 
affect or require any changes from any compilers. Seems like a lldbassert that 
maybe verifies that the abbreviation we use is valid for the current CU in 
debug builds would be nice so we can trigger this bug when we run the test 
suite or locally against an example program would be good. Depending on that 
fact that we have a mismatch in the abbrev index seems fragile. Most 
.debug_abbrev tables start out with a DW_TAG_compile_unit. So if the abbrev 
index magically matches, that still doesn't mean the we will be decoding the 
right data even if the index matches.
> 
> Now, normally this shouldn't matter, but in case of split-dwarf, things start 
> to get interesting. Lldb has this assumption that when we are reading a debug 
> info entry (which is not the CU DIE), we are actually reading it from the dwo 
> compile unit and not the skeleton unit in the main file. This means it uses 
> the dwo abbreviation list and everything. Now, as far as I can tell, LLDB is 
> kind of right here. The DWARF5 (in v4 split dwarf was a non-standard 
> extension) spec says "A skeleton compilation unit has no children." (3.1.2 
> Skeleton Compilation Unit Entries). And indeed, gcc produces compilation 
> units with no children. Clang, on the other hand, seems to be putting some 
> DIEs into the main CU, **if** the compilation results in functions being 
> inlined.  And it seems clang has been doing this since at least 7.0 (I 
> haven't tried older versions).
> 
> So it seems we have two problems here:
> 
> 1. We need to figure out whether there's a bug in clang and fix it. + 
> @dblaikie for help with that
> 2. Depending on the outcome of the first item, we need to do something in 
> lldb to handle this. If this is a clang bug (and my feeling is that it is), 
> then the best solution might be to just start ignoring any non-CU dies from 
> the main file in split-dwarf scenario
> 
> @dblaikie: Do you agree with my assessment above? If this is indeed a clang 
> bug, any chance you could help with debugging the clang/llvm side of things?
> 
> The simplest reproducer for this would be something like:
> 
>  inline __attribute__((always_inline)) int foo(int x) { return x; }
> 
>  int main(int argc) { return foo(argc); }
> 
> `$ clang++ a.cc -c -o a.o -g -gsplit-dwarf`

I don't believe we need to fix or change anything in the compiler since this is 
just a bug in LLDB. If LLDB was relying on incorrect behavior before that is 
bad, but we should just fix the issues in LLDB.

Greg

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

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

[Lldb-commits] [PATCH] D62755: [Target] Remove Process::GetCPPLanguageRuntime

[Alex Langford via Phabricator via lldb-commits]

xiaobai marked an inline comment as done.
xiaobai added a comment.

In D62755#1527004 , @labath wrote:

> However, I just want to add that from an lldb-server POV, even the fact that 
> we pull in the `Process` class into it's dependency graph is a bug.


Agreed

> So, what I'd do (and what I have been doing so far, but mainly as a passtime 
> effort) was to cut the dependencies way lower that this. Ideally, I wouldn't 
> want to include anything from the `Target` library in lldb-server, possibly 
> by moving some stuff that lldb-server does need to use (MemoryRegionInfo) out 
> of it.

Yeah, I'd like to see this happen as well. My efforts are twofold: Remove 
non-plugin libraries dependencies on plugin libraries, and move things out of 
the core libraries into plugins (and possibly new non-plugin libraries, if it 
makes sense). There are many dependency issues that I'm not yet aware of. I'm 
hoping to uncover more as I keep decoupling.




Comment at: include/lldb/Target/CPPLanguageRuntime.h:47
+  static CPPLanguageRuntime *GetCPPLanguageRuntime(Process &process) {
+return static_cast(
+process.GetLanguageRuntime(lldb::eLanguageTypeC_plus_plus));

labath wrote:
> It might be nice to add some glue so we could write 
> `llvm::cast_or_null(...)` here. The main advantage of 
> that being that we'd automatically get an assert firing if 
> `GetLanguageRuntime` ever returns something which is *not* a 
> CPPLanguageRuntime.
I had thought about doing that, but there's an assertion in GetLanguageRuntime 
that achieves the same thing imo. I do think that we could move towards a 
`llvm::cast_or_null` implementation though.


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

https://reviews.llvm.org/D62755



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

[Lldb-commits] [PATCH] D62502: Implement xfer:libraries-svr4:read packet

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:37-39
+  lldb::addr_t link_map;
+  lldb::addr_t base_addr;
+  lldb::addr_t ld_addr;

These seem very linux specific. Why do we need 3 addresses here? Seems like we 
should just need one. Or is this the structure of the 
"xfer:libraries-svr4:read" that is required to be returned? If so, maybe we 
rename "SharedLibraryInfo" to "SVR4ModuleInfo"?



Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:40
+  lldb::addr_t ld_addr;
+  bool main;
+  lldb::addr_t next;

Why is "main" important? Hopefully the dynamic linker can figure out what is 
what without needing to know this? Seems verify linux specific. Or is this 
"xfer:libraries-svr4:read" specific?



Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:41
+  bool main;
+  lldb::addr_t next;
+};

Why do we need this "next" member here? We get a list of these from:
```
virtual Status GetLoadedSharedLibraries(std::vector 
&library_list);
```
Seems like we shouldn't need this. Darwin based systems doesn't store the 
shared library list as a linked list. Hopefully not "xfer:libraries-svr4:read" 
specific?



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2191
+  char name_buffer[PATH_MAX];
+  error = ReadMemory(link_map.l_name, &name_buffer, sizeof(name_buffer),
+ bytes_read);

Use ReadCStringFromMemory?



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2247
+library_list.push_back(info);
+link_map = info.next;
+  }

Seems this we are using "info.next" just because it is a linked list on linux. 
Can we remove "next" and just add an extra "next" reference parameter to 
ReadSharedLibraryInfo(link_map, info, next);?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62502



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

[Lldb-commits] [PATCH] D62812: [llvm] [CodeView] Move Triple::ArchType → CPUType mapping from LLDB

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd added inline comments.



Comment at: llvm/include/llvm/DebugInfo/CodeView/CodeView.h:145
+  switch (ArchType) {
+  case Triple::ArchType::aarch64:
+return CPUType::ARM64;

I that `aarch64_be` and `aarch64_32` should be included in this.



Comment at: llvm/include/llvm/DebugInfo/CodeView/CodeView.h:147
+return CPUType::ARM64;
+  default:
+return CPUType::X64;

This is still wrong.  `x86` and `armv7` are totally legitimate values.  
Furthermore, x86 is still far more prevalent on Windows.


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

https://reviews.llvm.org/D62812



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

[Lldb-commits] [PATCH] D62795: [Target] Move ObjCLanguageRuntime::LookupRuntimeSymbol into LanguageRuntime

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

In D62795#1527035 , @labath wrote:

> If this is supposed to be truly language-agnostic, shouldn't we be iterating 
> over all language plugins and asking all of them for the "runtime symbol" 
> (not that I know what a runtime symbol is, really)?
>
> Otherwise, this is still language-specific behavior, even though it's not 
> visible to the linker because it's hidden by an enum and a virtual interface.


Mmm, yes, you're right, that should be happening. The function is 
`FindInRuntimes`, not `FindInObjCRuntime` after all. Thanks for pointing that 
out, let me change that.


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

https://reviews.llvm.org/D62795



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

[Lldb-commits] [PATCH] D62500: Add support to read aux vector values

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:91-92
 m_process ? m_process->GetID() : LLDB_INVALID_PROCESS_ID);
-
-  m_auxv.reset(new AuxVector(m_process));
+  DataExtractor auxv_data(m_process->GetAuxvData(), m_process->GetByteOrder(),
+  m_process->GetAddressByteSize());
+  m_auxv = llvm::make_unique(auxv_data);

We should change Process::GetAuxvData() to return a DataExtractor to avoid this 
kind of code.



Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:94
+  m_auxv = llvm::make_unique(auxv_data);
   if (log)
 log->Printf("DynamicLoaderPOSIXDYLD::%s pid %" PRIu64 " reloaded auxv 
data",

Process can't be NULL ever in a DynamicLoader. It should be a reference, but 
there may have been complications making that happen for some reason since if 
you have a member variables "Process &m_process;" it has to be initialized in 
the contructor? Can't remember. Since process owns the dynamic loader plug-in 
you can assume "m_process" is always valid and not NULL.



Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:181-182
 
-  m_auxv.reset(new AuxVector(m_process));
+  DataExtractor auxv_data(m_process->GetAuxvData(), m_process->GetByteOrder(),
+  m_process->GetAddressByteSize());
+  m_auxv = llvm::make_unique(auxv_data);

We should change Process::GetAuxvData() to return a DataExtractor to avoid this 
kind of code.



Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:183
+  m_process->GetAddressByteSize());
+  m_auxv = llvm::make_unique(auxv_data);
 

Use std::move?
```
m_auxv = llvm::make_unique(std::move(auxv_data));
```



Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:11
 
-using namespace lldb;
-using namespace lldb_private;
+AuxVector::AuxVector(lldb_private::DataExtractor &data) { ParseAuxv(data); }
 

Not a big fan of constructors doing parsing work. No way to return an error 
unless you build that into the AuxVector class (AuxVector::GetError()).



Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:13-20
+static bool ReadUInt(lldb_private::DataExtractor &data,
+ lldb::offset_t *offset_ptr, uint64_t *value) {
   lldb::offset_t saved_offset = *offset_ptr;
-  *value = data.GetMaxU64(offset_ptr, byte_size);
+  // We're not reading an address but an int that could be 32 or 64 bit
+  // depending on the address size, which is what GetAddress does.
+  *value = data.GetAddress(offset_ptr);
   return *offset_ptr != saved_offset;

Remove this function and just call "data.GetAddress(offset_ptr)"?



Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:24-28
+  while (true) {
+uint64_t type = 0;
+uint64_t value = 0;
+if (!ReadUInt(data, &offset, &type) || !ReadUInt(data, &offset, &value))
   break;

```
const size_t value_type_size = data.GetAddressByteSize() * 2;
while (data.ValidOffsetForDataOfSize(value_type_size)) {
  const uint64_t type = data.GetAddress(&offset);
  const uint64_t value = data.GetAddress(&offset);
  if (type == AUXV_AT_NULL)
...
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62500



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

[Lldb-commits] [PATCH] D62797: [Expression] Add PersistentExpressionState::SetCompilerTypeFromPersistentDecl

[Alex Langford via Phabricator via lldb-commits]

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



Comment at: include/lldb/Expression/ExpressionVariable.h:235-236
 
+  virtual bool SetCompilerTypeFromPersistentDecl(ConstString type_name,
+ CompilerType &compiler_type) 
= 0;
+

labath wrote:
> It would be better if this would return `Optional`(or 
> `Expected`)``
Let me try that out, I'll update this later.


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

https://reviews.llvm.org/D62797



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

[Lldb-commits] [PATCH] D62796: [Target] Generalize some behavior in Target::SymbolsDidLoad

[Alex Langford via Phabricator via lldb-commits]

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



Comment at: source/Target/Target.cpp:1670
 if (m_process_sp) {
-  LanguageRuntime *runtime =
-  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC);
-  if (runtime) {
-ObjCLanguageRuntime *objc_runtime = (ObjCLanguageRuntime *)runtime;
+  if (LanguageRuntime *objc_runtime =
+  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC))

xiaobai wrote:
> clayborg wrote:
> > labath wrote:
> > > Same question about runtime iteration as in the other review.
> > Seems like we should add a Process::SymbolsDidLoad() method that matches 
> > this method's signature and just call that. Process would iterate across 
> > all loaded language runtimes and call SymbolsDidLoad on any?
> Right, we should definitely iterate over loaded language runtimes instead of 
> just using the ObjC runtime. Will update.
Instead of adding `Process::SymbolsDidLoad`, I could instead iterate here over 
the loaded language runtimes with `Process::GetLanguageRuntimes` and call 
`LanguageRuntime::SymbolsDidLoad` on each one. What do you think? This would 
prevent us from having 3 classes with the method `SymbolsDidLoad` with the same 
signature.



Comment at: source/Target/Target.cpp:1670-1672
+  if (LanguageRuntime *objc_runtime =
+  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC))
 objc_runtime->SymbolsDidLoad(module_list);

clayborg wrote:
> labath wrote:
> > Same question about runtime iteration as in the other review.
> Seems like we should add a Process::SymbolsDidLoad() method that matches this 
> method's signature and just call that. Process would iterate across all 
> loaded language runtimes and call SymbolsDidLoad on any?
Right, we should definitely iterate over loaded language runtimes instead of 
just using the ObjC runtime. Will update.


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

https://reviews.llvm.org/D62796



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

[Lldb-commits] [PATCH] D62796: [Target] Generalize some behavior in Target::SymbolsDidLoad

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: source/Target/Target.cpp:1670
 if (m_process_sp) {
-  LanguageRuntime *runtime =
-  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC);
-  if (runtime) {
-ObjCLanguageRuntime *objc_runtime = (ObjCLanguageRuntime *)runtime;
+  if (LanguageRuntime *objc_runtime =
+  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC))

xiaobai wrote:
> xiaobai wrote:
> > clayborg wrote:
> > > labath wrote:
> > > > Same question about runtime iteration as in the other review.
> > > Seems like we should add a Process::SymbolsDidLoad() method that matches 
> > > this method's signature and just call that. Process would iterate across 
> > > all loaded language runtimes and call SymbolsDidLoad on any?
> > Right, we should definitely iterate over loaded language runtimes instead 
> > of just using the ObjC runtime. Will update.
> Instead of adding `Process::SymbolsDidLoad`, I could instead iterate here 
> over the loaded language runtimes with `Process::GetLanguageRuntimes` and 
> call `LanguageRuntime::SymbolsDidLoad` on each one. What do you think? This 
> would prevent us from having 3 classes with the method `SymbolsDidLoad` with 
> the same signature.
That is fine as long as we iterate over all loaded language runtimes I am good.


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

https://reviews.llvm.org/D62796



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

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

[Greg Clayton via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:430
 
+  lldb::addr_t GetELFImageInfoAddress();
+

This doesn't seem like it belongs in the base class. Maybe just add it to 
NativeProcessLinux.h?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62501



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

[Lldb-commits] [PATCH] D62771: [LLDBRegisterNum] Update function call llvm::codeview::getRegisterNames(CPUType) in lldb

[Wanyi Ye via Phabricator via lldb-commits]

kusmour abandoned this revision.
kusmour marked an inline comment as done.
kusmour added a comment.

D62772  toke care of this


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62771



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

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

[António Afonso via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:430
 
+  lldb::addr_t GetELFImageInfoAddress();
+

clayborg wrote:
> This doesn't seem like it belongs in the base class. Maybe just add it to 
> NativeProcessLinux.h?
oops, it certainly does not, my mistake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62501



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

[Lldb-commits] [PATCH] D62795: [Target] Move ObjCLanguageRuntime::LookupRuntimeSymbol into LanguageRuntime

[Alex Langford via Phabricator via lldb-commits]

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

Make the behavior in IRExecutionUnit actually language agnostic.

Also, reverse the order of the for loops.


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

https://reviews.llvm.org/D62795

Files:
  include/lldb/Target/LanguageRuntime.h
  include/lldb/Target/ObjCLanguageRuntime.h
  source/Expression/IRExecutionUnit.cpp


Index: source/Expression/IRExecutionUnit.cpp
===
--- source/Expression/IRExecutionUnit.cpp
+++ source/Expression/IRExecutionUnit.cpp
@@ -24,7 +24,7 @@
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Target/ExecutionContext.h"
-#include "lldb/Target/ObjCLanguageRuntime.h"
+#include "lldb/Target/LanguageRuntime.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/DataExtractor.h"
@@ -902,10 +902,8 @@
 return LLDB_INVALID_ADDRESS;
   }
 
-  ObjCLanguageRuntime *runtime = process_sp->GetObjCLanguageRuntime();
-
-  if (runtime) {
-for (const SearchSpec &spec : specs) {
+  for (const SearchSpec &spec : specs) {
+for (LanguageRuntime *runtime : process_sp->GetLanguageRuntimes()) {
   lldb::addr_t symbol_load_addr = runtime->LookupRuntimeSymbol(spec.name);
 
   if (symbol_load_addr != LLDB_INVALID_ADDRESS)
Index: include/lldb/Target/ObjCLanguageRuntime.h
===
--- include/lldb/Target/ObjCLanguageRuntime.h
+++ include/lldb/Target/ObjCLanguageRuntime.h
@@ -264,13 +264,6 @@
   virtual size_t GetByteOffsetForIvar(CompilerType &parent_qual_type,
   const char *ivar_name);
 
-  // Given the name of an Objective-C runtime symbol (e.g., ivar offset
-  // symbol), try to determine from the runtime what the value of that symbol
-  // would be. Useful when the underlying binary is stripped.
-  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
-return LLDB_INVALID_ADDRESS;
-  }
-
   bool HasNewLiteralsAndIndexing() {
 if (m_has_new_literals_and_indexing == eLazyBoolCalculate) {
   if (CalculateHasNewLiteralsAndIndexing())
Index: include/lldb/Target/LanguageRuntime.h
===
--- include/lldb/Target/LanguageRuntime.h
+++ include/lldb/Target/LanguageRuntime.h
@@ -166,6 +166,13 @@
 return false;
   }
 
+  // Given the name of a runtime symbol (e.g. in Objective-C, an ivar offset
+  // symbol), try to determine from the runtime what the value of that symbol
+  // would be. Useful when the underlying binary is stripped.
+  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
+return LLDB_INVALID_ADDRESS;
+  }
+
 protected:
   // Classes that inherit from LanguageRuntime can see and modify these
 


Index: source/Expression/IRExecutionUnit.cpp
===
--- source/Expression/IRExecutionUnit.cpp
+++ source/Expression/IRExecutionUnit.cpp
@@ -24,7 +24,7 @@
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Target/ExecutionContext.h"
-#include "lldb/Target/ObjCLanguageRuntime.h"
+#include "lldb/Target/LanguageRuntime.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/DataExtractor.h"
@@ -902,10 +902,8 @@
 return LLDB_INVALID_ADDRESS;
   }
 
-  ObjCLanguageRuntime *runtime = process_sp->GetObjCLanguageRuntime();
-
-  if (runtime) {
-for (const SearchSpec &spec : specs) {
+  for (const SearchSpec &spec : specs) {
+for (LanguageRuntime *runtime : process_sp->GetLanguageRuntimes()) {
   lldb::addr_t symbol_load_addr = runtime->LookupRuntimeSymbol(spec.name);
 
   if (symbol_load_addr != LLDB_INVALID_ADDRESS)
Index: include/lldb/Target/ObjCLanguageRuntime.h
===
--- include/lldb/Target/ObjCLanguageRuntime.h
+++ include/lldb/Target/ObjCLanguageRuntime.h
@@ -264,13 +264,6 @@
   virtual size_t GetByteOffsetForIvar(CompilerType &parent_qual_type,
   const char *ivar_name);
 
-  // Given the name of an Objective-C runtime symbol (e.g., ivar offset
-  // symbol), try to determine from the runtime what the value of that symbol
-  // would be. Useful when the underlying binary is stripped.
-  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
-return LLDB_INVALID_ADDRESS;
-  }
-
   bool HasNewLiteralsAndIndexing() {
 if (m_has_new_literals_and_indexing == eLazyBoolCalculate) {
   if (CalculateHasNewLiteralsAndIndexing())
Index: include/lldb/Target/LanguageRuntime.h
===
--- include/lldb/Target/LanguageRuntime.h
+++ include/lldb/Target/LanguageRuntime.h
@@ -166,6 +166,13 @@
 return false;
   }
 
+  // Given the name of a runti

[Lldb-commits] [PATCH] D62796: [Target] Generalize some behavior in Target::SymbolsDidLoad

[Alex Langford via Phabricator via lldb-commits]

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

Address comments


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

https://reviews.llvm.org/D62796

Files:
  include/lldb/Target/LanguageRuntime.h
  include/lldb/Target/ObjCLanguageRuntime.h
  source/Target/Target.cpp


Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -43,7 +43,6 @@
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/LanguageRuntime.h"
-#include "lldb/Target/ObjCLanguageRuntime.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/StackFrame.h"
@@ -1668,11 +1667,8 @@
 void Target::SymbolsDidLoad(ModuleList &module_list) {
   if (m_valid && module_list.GetSize()) {
 if (m_process_sp) {
-  LanguageRuntime *runtime =
-  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC);
-  if (runtime) {
-ObjCLanguageRuntime *objc_runtime = (ObjCLanguageRuntime *)runtime;
-objc_runtime->SymbolsDidLoad(module_list);
+  for (LanguageRuntime *runtime : m_process_sp->GetLanguageRuntimes()) {
+runtime->SymbolsDidLoad(module_list);
   }
 }
 
Index: include/lldb/Target/ObjCLanguageRuntime.h
===
--- include/lldb/Target/ObjCLanguageRuntime.h
+++ include/lldb/Target/ObjCLanguageRuntime.h
@@ -282,7 +282,7 @@
 return (m_has_new_literals_and_indexing == eLazyBoolYes);
   }
 
-  virtual void SymbolsDidLoad(const ModuleList &module_list) {
+  void SymbolsDidLoad(const ModuleList &module_list) override {
 m_negative_complete_class_cache.clear();
   }
 
Index: include/lldb/Target/LanguageRuntime.h
===
--- include/lldb/Target/LanguageRuntime.h
+++ include/lldb/Target/LanguageRuntime.h
@@ -143,6 +143,8 @@
 return false;
   }
 
+  virtual void SymbolsDidLoad(const ModuleList &module_list) { return; }
+
   virtual lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread,
   bool stop_others) = 
0;
 


Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -43,7 +43,6 @@
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/LanguageRuntime.h"
-#include "lldb/Target/ObjCLanguageRuntime.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/StackFrame.h"
@@ -1668,11 +1667,8 @@
 void Target::SymbolsDidLoad(ModuleList &module_list) {
   if (m_valid && module_list.GetSize()) {
 if (m_process_sp) {
-  LanguageRuntime *runtime =
-  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC);
-  if (runtime) {
-ObjCLanguageRuntime *objc_runtime = (ObjCLanguageRuntime *)runtime;
-objc_runtime->SymbolsDidLoad(module_list);
+  for (LanguageRuntime *runtime : m_process_sp->GetLanguageRuntimes()) {
+runtime->SymbolsDidLoad(module_list);
   }
 }
 
Index: include/lldb/Target/ObjCLanguageRuntime.h
===
--- include/lldb/Target/ObjCLanguageRuntime.h
+++ include/lldb/Target/ObjCLanguageRuntime.h
@@ -282,7 +282,7 @@
 return (m_has_new_literals_and_indexing == eLazyBoolYes);
   }
 
-  virtual void SymbolsDidLoad(const ModuleList &module_list) {
+  void SymbolsDidLoad(const ModuleList &module_list) override {
 m_negative_complete_class_cache.clear();
   }
 
Index: include/lldb/Target/LanguageRuntime.h
===
--- include/lldb/Target/LanguageRuntime.h
+++ include/lldb/Target/LanguageRuntime.h
@@ -143,6 +143,8 @@
 return false;
   }
 
+  virtual void SymbolsDidLoad(const ModuleList &module_list) { return; }
+
   virtual lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread,
   bool stop_others) = 0;
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D62795: [Target] Move ObjCLanguageRuntime::LookupRuntimeSymbol into LanguageRuntime

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.



Comment at: include/lldb/Target/ObjCLanguageRuntime.h:270-272
-  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
-return LLDB_INVALID_ADDRESS;
-  }

Which language has this filled in? Only Swift?


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

https://reviews.llvm.org/D62795



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

[Lldb-commits] [PATCH] D62795: [Target] Move ObjCLanguageRuntime::LookupRuntimeSymbol into LanguageRuntime

[Alex Langford via Phabricator via lldb-commits]

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



Comment at: include/lldb/Target/ObjCLanguageRuntime.h:270-272
-  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
-return LLDB_INVALID_ADDRESS;
-  }

clayborg wrote:
> Which language has this filled in? Only Swift?
Objective-C is the only thing that has this filled in at the moment, as I 
understand it.


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

https://reviews.llvm.org/D62795



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

[Lldb-commits] [PATCH] D62795: [Target] Move ObjCLanguageRuntime::LookupRuntimeSymbol into LanguageRuntime

[Greg Clayton via Phabricator via lldb-commits]

clayborg accepted this revision.
clayborg added inline comments.
This revision is now accepted and ready to land.



Comment at: include/lldb/Target/ObjCLanguageRuntime.h:270-272
-  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
-return LLDB_INVALID_ADDRESS;
-  }

xiaobai wrote:
> clayborg wrote:
> > Which language has this filled in? Only Swift?
> Objective-C is the only thing that has this filled in at the moment, as I 
> understand it.
There must be a V1 or V2 implementation somewhere? From the looks of things 
here, it seems like we are pulling it out, but that must not be what is 
happening. So the V1 and V2 impls must have this already set as "lldb::addr_t 
LookupRuntimeSymbol(ConstString name) override;"?


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

https://reviews.llvm.org/D62795



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

[Lldb-commits] [PATCH] D62795: [Target] Move ObjCLanguageRuntime::LookupRuntimeSymbol into LanguageRuntime

[Alex Langford via Phabricator via lldb-commits]

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



Comment at: include/lldb/Target/ObjCLanguageRuntime.h:270-272
-  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
-return LLDB_INVALID_ADDRESS;
-  }

clayborg wrote:
> xiaobai wrote:
> > clayborg wrote:
> > > Which language has this filled in? Only Swift?
> > Objective-C is the only thing that has this filled in at the moment, as I 
> > understand it.
> There must be a V1 or V2 implementation somewhere? From the looks of things 
> here, it seems like we are pulling it out, but that must not be what is 
> happening. So the V1 and V2 impls must have this already set as "lldb::addr_t 
> LookupRuntimeSymbol(ConstString name) override;"?
Yes, AppleObjCRuntimeV2 has an override for this method. 


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

https://reviews.llvm.org/D62795



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

[Lldb-commits] [PATCH] D62797: [Expression] Add PersistentExpressionState::SetCompilerTypeFromPersistentDecl

[Alex Langford via Phabricator via lldb-commits]

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

Pavel's suggestion
Renamed method from "SetCompilerTypeFromPersistentDecl" to 
"GetCompilerTypeFromPersistentDecl"


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

https://reviews.llvm.org/D62797

Files:
  include/lldb/Expression/ExpressionVariable.h
  source/Commands/CMakeLists.txt
  source/Commands/CommandObjectMemory.cpp
  source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
  source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h

Index: source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
===
--- source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
+++ source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h
@@ -50,6 +50,9 @@
 return "$";
   }
 
+  llvm::Optional
+  GetCompilerTypeFromPersistentDecl(ConstString type_name) override;
+
   void RegisterPersistentDecl(ConstString name, clang::NamedDecl *decl);
 
   clang::NamedDecl *GetPersistentDecl(ConstString name);
Index: source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
===
--- source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
+++ source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
@@ -9,6 +9,7 @@
 #include "ClangPersistentVariables.h"
 
 #include "lldb/Core/Value.h"
+#include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataExtractor.h"
 #include "lldb/Utility/Log.h"
@@ -52,6 +53,21 @@
 m_next_persistent_variable_id--;
 }
 
+llvm::Optional
+ClangPersistentVariables::GetCompilerTypeFromPersistentDecl(
+ConstString type_name) {
+  CompilerType compiler_type;
+  if (clang::TypeDecl *tdecl = llvm::dyn_cast_or_null(
+  GetPersistentDecl(type_name))) {
+compiler_type.SetCompilerType(
+ClangASTContext::GetASTContext(&tdecl->getASTContext()),
+reinterpret_cast(
+const_cast(tdecl->getTypeForDecl(;
+return compiler_type;
+  }
+  return llvm::None;
+}
+
 void ClangPersistentVariables::RegisterPersistentDecl(ConstString name,
   clang::NamedDecl *decl) {
   m_persistent_decls.insert(
Index: source/Commands/CommandObjectMemory.cpp
===
--- source/Commands/CommandObjectMemory.cpp
+++ source/Commands/CommandObjectMemory.cpp
@@ -6,16 +6,14 @@
 //
 //===--===//
 
-#include "clang/AST/Decl.h"
-
 #include "CommandObjectMemory.h"
-#include "Plugins/ExpressionParser/Clang/ClangPersistentVariables.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/DumpDataExtractor.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Core/ValueObjectMemory.h"
 #include "lldb/DataFormatters/ValueObjectPrinter.h"
+#include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
@@ -25,13 +23,14 @@
 #include "lldb/Interpreter/OptionGroupValueObjectDisplay.h"
 #include "lldb/Interpreter/OptionValueString.h"
 #include "lldb/Interpreter/Options.h"
-#include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/TypeList.h"
+#include "lldb/Target/Language.h"
 #include "lldb/Target/MemoryHistory.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StackFrame.h"
+#include "lldb/Target/Target.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/DataBufferHeap.h"
@@ -472,21 +471,16 @@
 exact_match, 1, searched_symbol_files,
 type_list);
 
-  if (type_list.GetSize() == 0 && lookup_type_name.GetCString() &&
-  *lookup_type_name.GetCString() == '$') {
-if (ClangPersistentVariables *persistent_vars =
-llvm::dyn_cast_or_null(
-target->GetPersistentExpressionStateForLanguage(
-lldb::eLanguageTypeC))) {
-  clang::TypeDecl *tdecl = llvm::dyn_cast_or_null(
-  persistent_vars->GetPersistentDecl(
-  ConstString(lookup_type_name)));
-
-  if (tdecl) {
-clang_ast_type.SetCompilerType(
-ClangASTContext::GetASTContext(&tdecl->getASTContext()),
-reinterpret_cast(
-const_cast(tdecl->getTypeForDecl(;
+  if (type_list.GetSize() == 0 && lookup_type_name.GetCString()) {
+for (auto lang : Language::GetSupportedLanguages()) {
+  if (PersistentExpressionState *persistent_vars =
+  target->GetPersistentExpressionStateForLanguage(lang)) {
+if 

[Lldb-commits] [lldb] r362456 - Add support for mid-function epilogues on x86 that end in a non-local jump.

[Jason Molenda via lldb-commits]

Author: jmolenda
Date: Mon Jun  3 15:34:12 2019
New Revision: 362456

URL: http://llvm.org/viewvc/llvm-project?rev=362456&view=rev
Log:
Add support for mid-function epilogues on x86 that end in a non-local jump.

The x86 assembly inspection engine has code to support detecting a
mid-function epilogue that ends in a RET instruction; add support for 
recognizing an epilogue that ends in a JMP, and add a check that the
unwind state has been restored to the original stack setup; reinstate
the post-prologue unwind state after this JMP instruction.

The assembly inspection engine used for other architectures, 
UnwindAssemblyInstEmulation, detects mid-function epilogues by 
tracking branch instructions within the function and "forwards"
the current unwind state to the targets of the branches.  If
an epilogue unwinds the stack and exits, followed by a branch
target, we get back to the correct unwind state.  The x86 
unwinder should move to this same algorithm, or possibly even
look at implementing an x86 instruction emulation plugin and
get UnwindAssemblyInstEmulation to work for x86 too.  I added
a branch instruction recognizier method that will be necessary
if we want to switch the algorithm.

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

Modified:
lldb/trunk/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp
lldb/trunk/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.h
lldb/trunk/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp

Modified: 
lldb/trunk/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp?rev=362456&r1=362455&r2=362456&view=diff
==
--- 
lldb/trunk/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp 
(original)
+++ 
lldb/trunk/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp 
Mon Jun  3 15:34:12 2019
@@ -667,12 +667,209 @@ bool x86AssemblyInspectionEngine::mov_re
   return false;
 }
 
+// Returns true if this is a jmp instruction where we can't
+// know the destination address statically. 
+//
+// ff e0   jmpq   *%rax
+// ff e1   jmpq   *%rcx
+// ff 60 28jmpq   *0x28(%rax)
+// ff 60 60jmpq   *0x60(%rax)
+bool x86AssemblyInspectionEngine::jmp_to_reg_p() {
+  if (*m_cur_insn != 0xff)
+return false;
+
+  // The second byte is a ModR/M /4 byte, strip off the registers
+  uint8_t second_byte_sans_reg = *(m_cur_insn + 1) & ~7;
+
+  // Don't handle 0x24 disp32, because the target address is
+  // knowable statically - pc_rel_branch_or_jump_p() will
+  // return the target address.
+
+  // [reg]
+  if (second_byte_sans_reg == 0x20)
+return true;
+
+  // [reg]+disp8
+  if (second_byte_sans_reg == 0x60)
+return true;
+
+  // [reg]+disp32
+  if (second_byte_sans_reg == 0xa0)
+return true;
+
+  // reg
+  if (second_byte_sans_reg == 0xe0)
+return true;
+
+  // disp32
+  // jumps to an address stored in memory, the value can't be cached
+  // in an unwind plan.
+  if (second_byte_sans_reg == 0x24)
+return true;
+
+  // use SIB byte
+  // ff 24 fe  jmpq   *(%rsi,%rdi,8)
+  if (second_byte_sans_reg == 0x24)
+return true;
+
+  return false;
+}
+
+// Detect branches to fixed pc-relative offsets.
+// Returns the offset from the address of the next instruction
+// that may be branch/jumped to.
+//
+// Cannot determine the offset of a JMP that jumps to the address in
+// a register ("jmpq *%rax") or offset from a register value 
+// ("jmpq *0x28(%rax)"), this method will return false on those
+// instructions.
+//
+// These instructions all end in either a relative 8/16/32 bit value
+// depending on the instruction and the current execution mode of the
+// inferior process.  Once we know the size of the opcode instruction, 
+// we can use the total instruction length to determine the size of
+// the relative offset without having to compute it correctly.
+
+bool x86AssemblyInspectionEngine::pc_rel_branch_or_jump_p (
+const int instruction_length, int &offset)
+{
+  int opcode_size = 0;
+
+  uint8_t b1 = m_cur_insn[0];
+  uint8_t b2 = m_cur_insn[1];
+
+  switch (b1) {
+case 0x77: // JA/JNBE rel8
+case 0x73: // JAE/JNB/JNC rel8
+case 0x72: // JB/JC/JNAE rel8
+case 0x76: // JBE/JNA rel8
+case 0xe3: // JCXZ/JECXZ/JRCXZ rel8
+case 0x74: // JE/JZ rel8
+case 0x7f: // JG/JNLE rel8
+case 0x7d: // JGE/JNL rel8
+case 0x7c: // JL/JNGE rel8
+case 0x7e: // JNG/JLE rel8
+case 0x71: // JNO rel8
+case 0x7b: // JNP/JPO rel8
+case 0x79: // JNS rel8
+case 0x75: // JNE/JNZ rel8
+case 0x70: // JO rel8
+case 0x7a: // JP/JPE rel8
+case 0x78: // JS rel8
+case 0xeb: // JMP rel8
+case 0xe9: // JMP rel16/rel32
+  

[Lldb-commits] [PATCH] D62764: Detect x86 mid-function epilogues that end in a jump

[Phabricator via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL362456: Add support for mid-function epilogues on x86 that 
end in a non-local jump. (authored by jmolenda, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62764?vs=202518&id=202809#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62764

Files:
  lldb/trunk/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp
  lldb/trunk/source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.h
  lldb/trunk/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp

Index: lldb/trunk/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp
===
--- lldb/trunk/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp
+++ lldb/trunk/unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp
@@ -2723,3 +2723,125 @@
   EXPECT_TRUE(regloc.IsAtCFAPlusOffset());
   EXPECT_EQ(-8, regloc.GetOffset());
 }
+
+
+// Test mid-function epilogues - the unwind state post-prologue
+// should be re-instated.
+
+TEST_F(Testx86AssemblyInspectionEngine, TestDisassemblyMidFunctionEpilogues) {
+  AddressRange sample_range;
+  UnwindPlan unwind_plan(eRegisterKindLLDB);
+  std::unique_ptr engine32 = Geti386Inspector();
+  std::unique_ptr engine64 = Getx86_64Inspector();
+
+  uint8_t data[] = {
+0x55,   // <+0>: pushq %rbp
+0x48, 0x89, 0xe5,   // <+1>: movq %rsp, %rbp
+0x48, 0x83, 0xec, 0x70, // <+4>: subq $0x70, %rsp
+0x90,   // <+8>: nop   // prologue set up
+
+0x74, 0x7,  // <+9>: je 7 <+18>
+0x48, 0x83, 0xc4, 0x70, // <+11>: addq $0x70, %rsp
+0x5d,   // <+15>: popq %rbp
+0xff, 0xe0, // <+16>: jmpq *%rax  // epilogue completed
+
+0x90,   // <+18>: nop // prologue setup back
+
+0x74, 0x7,  // <+19>: je 6 <+27>
+0x48, 0x83, 0xc4, 0x70, // <+21>: addq $0x70, %rsp
+0x5d,   // <+25>: popq %rbp
+0xc3,   // <+26>: retq// epilogue completed
+
+0x90,   // <+27>: nop // prologue setup back
+
+0x48, 0x83, 0xc4, 0x70, // <+28>: addq $0x70, %rsp
+0x5d,   // <+32>: popq %rbp
+0xc3,   // <+33>: retq// epilogue completed
+
+  };
+
+  sample_range = AddressRange(0x1000, sizeof(data));
+
+  int wordsize = 4;
+  EXPECT_TRUE(engine32->GetNonCallSiteUnwindPlanFromAssembly(
+  data, sizeof(data), sample_range, unwind_plan));
+
+  // Check that we've unwound the stack after the first mid-function epilogue
+  // row:   CFA=esp +4 => esp=CFA+0 eip=[CFA-4]
+  UnwindPlan::RowSP row_sp = unwind_plan.GetRowForFunctionOffset(16);
+  EXPECT_EQ(16ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_esp);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(wordsize, row_sp->GetCFAValue().GetOffset());
+
+  // Check that we've reinstated the stack frame setup 
+  // unwind instructions after a jmpq *%eax
+  // row:   CFA=ebp +8 => esp=CFA+0 eip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(18);
+  EXPECT_EQ(18ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_ebp);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(wordsize * 2, row_sp->GetCFAValue().GetOffset());
+
+  // Check that we've reinstated the stack frame setup 
+  // unwind instructions after a mid-function retq
+  // row:   CFA=ebp +8 => esp=CFA+0 eip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(27);
+  EXPECT_EQ(27ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_ebp);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(wordsize * 2, row_sp->GetCFAValue().GetOffset());
+
+  // After last instruction in the function, verify that
+  // the stack frame has been unwound
+  // row:   CFA=esp +4 => esp=CFA+0 eip=[CFA-4]
+  row_sp = unwind_plan.GetRowForFunctionOffset(33);
+  EXPECT_EQ(33ull, row_sp->GetOffset());
+  EXPECT_TRUE(row_sp->GetCFAValue().GetRegisterNumber() == k_esp);
+  EXPECT_TRUE(row_sp->GetCFAValue().IsRegisterPlusOffset() == true);
+  EXPECT_EQ(wordsize, row_sp->GetCFAValue().GetOffset());
+
+
+  unwind_plan.Clear();
+
+  wordsize = 8;
+  EXPECT_TRUE(engine64->GetNonCallSiteUnwindPlanFromAssembly(
+  data, sizeof(data), sample_range, unwind_plan));
+
+  // Check that we've unwound the stack after the first mid-function epilogue
+  // row:   CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8]
+  row_sp = unwind_plan.GetRowForFunctionOffset(

[Lldb-commits] [lldb] r362458 - [Target] Move ObjCLanguageRuntime::LookupRuntimeSymbol into LanguageRuntime

[Alex Langford via lldb-commits]

Author: xiaobai
Date: Mon Jun  3 15:41:48 2019
New Revision: 362458

URL: http://llvm.org/viewvc/llvm-project?rev=362458&view=rev
Log:
[Target] Move ObjCLanguageRuntime::LookupRuntimeSymbol into LanguageRuntime

Summary:
LookupRuntimeSymbol seems like a general LanguageRuntime method.
Although no other language runtime currently implements this, there's no
reason another language runtime couldn't use this.

Additionally, this breaks IRExecutionUnit's dependency on
ObjCLanguageRuntime.

Reviewers: compnerd, labath, JDevlieghere, davide

Subscribers: lldb-commits

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

Modified:
lldb/trunk/include/lldb/Target/LanguageRuntime.h
lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
lldb/trunk/source/Expression/IRExecutionUnit.cpp

Modified: lldb/trunk/include/lldb/Target/LanguageRuntime.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/LanguageRuntime.h?rev=362458&r1=362457&r2=362458&view=diff
==
--- lldb/trunk/include/lldb/Target/LanguageRuntime.h (original)
+++ lldb/trunk/include/lldb/Target/LanguageRuntime.h Mon Jun  3 15:41:48 2019
@@ -166,6 +166,13 @@ public:
 return false;
   }
 
+  // Given the name of a runtime symbol (e.g. in Objective-C, an ivar offset
+  // symbol), try to determine from the runtime what the value of that symbol
+  // would be. Useful when the underlying binary is stripped.
+  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
+return LLDB_INVALID_ADDRESS;
+  }
+
 protected:
   // Classes that inherit from LanguageRuntime can see and modify these
 

Modified: lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h?rev=362458&r1=362457&r2=362458&view=diff
==
--- lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h (original)
+++ lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h Mon Jun  3 15:41:48 
2019
@@ -264,13 +264,6 @@ public:
   virtual size_t GetByteOffsetForIvar(CompilerType &parent_qual_type,
   const char *ivar_name);
 
-  // Given the name of an Objective-C runtime symbol (e.g., ivar offset
-  // symbol), try to determine from the runtime what the value of that symbol
-  // would be. Useful when the underlying binary is stripped.
-  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
-return LLDB_INVALID_ADDRESS;
-  }
-
   bool HasNewLiteralsAndIndexing() {
 if (m_has_new_literals_and_indexing == eLazyBoolCalculate) {
   if (CalculateHasNewLiteralsAndIndexing())

Modified: lldb/trunk/source/Expression/IRExecutionUnit.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Expression/IRExecutionUnit.cpp?rev=362458&r1=362457&r2=362458&view=diff
==
--- lldb/trunk/source/Expression/IRExecutionUnit.cpp (original)
+++ lldb/trunk/source/Expression/IRExecutionUnit.cpp Mon Jun  3 15:41:48 2019
@@ -24,7 +24,7 @@
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Target/ExecutionContext.h"
-#include "lldb/Target/ObjCLanguageRuntime.h"
+#include "lldb/Target/LanguageRuntime.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/DataExtractor.h"
@@ -902,10 +902,8 @@ IRExecutionUnit::FindInRuntimes(const st
 return LLDB_INVALID_ADDRESS;
   }
 
-  ObjCLanguageRuntime *runtime = process_sp->GetObjCLanguageRuntime();
-
-  if (runtime) {
-for (const SearchSpec &spec : specs) {
+  for (const SearchSpec &spec : specs) {
+for (LanguageRuntime *runtime : process_sp->GetLanguageRuntimes()) {
   lldb::addr_t symbol_load_addr = runtime->LookupRuntimeSymbol(spec.name);
 
   if (symbol_load_addr != LLDB_INVALID_ADDRESS)


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

[Lldb-commits] [PATCH] D62795: [Target] Move ObjCLanguageRuntime::LookupRuntimeSymbol into LanguageRuntime

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362458: [Target] Move 
ObjCLanguageRuntime::LookupRuntimeSymbol into LanguageRuntime (authored by 
xiaobai, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62795

Files:
  lldb/trunk/include/lldb/Target/LanguageRuntime.h
  lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
  lldb/trunk/source/Expression/IRExecutionUnit.cpp


Index: lldb/trunk/include/lldb/Target/LanguageRuntime.h
===
--- lldb/trunk/include/lldb/Target/LanguageRuntime.h
+++ lldb/trunk/include/lldb/Target/LanguageRuntime.h
@@ -166,6 +166,13 @@
 return false;
   }
 
+  // Given the name of a runtime symbol (e.g. in Objective-C, an ivar offset
+  // symbol), try to determine from the runtime what the value of that symbol
+  // would be. Useful when the underlying binary is stripped.
+  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
+return LLDB_INVALID_ADDRESS;
+  }
+
 protected:
   // Classes that inherit from LanguageRuntime can see and modify these
 
Index: lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
===
--- lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
+++ lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
@@ -264,13 +264,6 @@
   virtual size_t GetByteOffsetForIvar(CompilerType &parent_qual_type,
   const char *ivar_name);
 
-  // Given the name of an Objective-C runtime symbol (e.g., ivar offset
-  // symbol), try to determine from the runtime what the value of that symbol
-  // would be. Useful when the underlying binary is stripped.
-  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
-return LLDB_INVALID_ADDRESS;
-  }
-
   bool HasNewLiteralsAndIndexing() {
 if (m_has_new_literals_and_indexing == eLazyBoolCalculate) {
   if (CalculateHasNewLiteralsAndIndexing())
Index: lldb/trunk/source/Expression/IRExecutionUnit.cpp
===
--- lldb/trunk/source/Expression/IRExecutionUnit.cpp
+++ lldb/trunk/source/Expression/IRExecutionUnit.cpp
@@ -24,7 +24,7 @@
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Target/ExecutionContext.h"
-#include "lldb/Target/ObjCLanguageRuntime.h"
+#include "lldb/Target/LanguageRuntime.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/DataExtractor.h"
@@ -902,10 +902,8 @@
 return LLDB_INVALID_ADDRESS;
   }
 
-  ObjCLanguageRuntime *runtime = process_sp->GetObjCLanguageRuntime();
-
-  if (runtime) {
-for (const SearchSpec &spec : specs) {
+  for (const SearchSpec &spec : specs) {
+for (LanguageRuntime *runtime : process_sp->GetLanguageRuntimes()) {
   lldb::addr_t symbol_load_addr = runtime->LookupRuntimeSymbol(spec.name);
 
   if (symbol_load_addr != LLDB_INVALID_ADDRESS)


Index: lldb/trunk/include/lldb/Target/LanguageRuntime.h
===
--- lldb/trunk/include/lldb/Target/LanguageRuntime.h
+++ lldb/trunk/include/lldb/Target/LanguageRuntime.h
@@ -166,6 +166,13 @@
 return false;
   }
 
+  // Given the name of a runtime symbol (e.g. in Objective-C, an ivar offset
+  // symbol), try to determine from the runtime what the value of that symbol
+  // would be. Useful when the underlying binary is stripped.
+  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
+return LLDB_INVALID_ADDRESS;
+  }
+
 protected:
   // Classes that inherit from LanguageRuntime can see and modify these
 
Index: lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
===
--- lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
+++ lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
@@ -264,13 +264,6 @@
   virtual size_t GetByteOffsetForIvar(CompilerType &parent_qual_type,
   const char *ivar_name);
 
-  // Given the name of an Objective-C runtime symbol (e.g., ivar offset
-  // symbol), try to determine from the runtime what the value of that symbol
-  // would be. Useful when the underlying binary is stripped.
-  virtual lldb::addr_t LookupRuntimeSymbol(ConstString name) {
-return LLDB_INVALID_ADDRESS;
-  }
-
   bool HasNewLiteralsAndIndexing() {
 if (m_has_new_literals_and_indexing == eLazyBoolCalculate) {
   if (CalculateHasNewLiteralsAndIndexing())
Index: lldb/trunk/source/Expression/IRExecutionUnit.cpp
===
--- lldb/trunk/source/Expression/IRExecutionUnit.cpp
+++ lldb/trunk/source/Expression/IRExecutionUnit.cpp
@@ -24,7 +24,7 @@
 #include "lldb/Symbol/SymbolF

[Lldb-commits] [lldb] r362461 - [Target] Generalize some behavior in Target::SymbolsDidLoad

[Alex Langford via lldb-commits]

Author: xiaobai
Date: Mon Jun  3 16:12:11 2019
New Revision: 362461

URL: http://llvm.org/viewvc/llvm-project?rev=362461&view=rev
Log:
[Target] Generalize some behavior in Target::SymbolsDidLoad

Summary:
SymbolsDidLoad is currently only implemented for ObjCLanguageRuntime,
but that doesn't mean that it couldn't be useful for other Langauges. Although
this change seems like it's generalizing for the sake of purity, this removes
Target's dependency on ObjCLanguageRuntime.

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

Modified:
lldb/trunk/include/lldb/Target/LanguageRuntime.h
lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
lldb/trunk/source/Target/Target.cpp

Modified: lldb/trunk/include/lldb/Target/LanguageRuntime.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/LanguageRuntime.h?rev=362461&r1=362460&r2=362461&view=diff
==
--- lldb/trunk/include/lldb/Target/LanguageRuntime.h (original)
+++ lldb/trunk/include/lldb/Target/LanguageRuntime.h Mon Jun  3 16:12:11 2019
@@ -143,6 +143,8 @@ public:
 return false;
   }
 
+  virtual void SymbolsDidLoad(const ModuleList &module_list) { return; }
+
   virtual lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread,
   bool stop_others) = 
0;
 

Modified: lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h?rev=362461&r1=362460&r2=362461&view=diff
==
--- lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h (original)
+++ lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h Mon Jun  3 16:12:11 
2019
@@ -275,7 +275,7 @@ public:
 return (m_has_new_literals_and_indexing == eLazyBoolYes);
   }
 
-  virtual void SymbolsDidLoad(const ModuleList &module_list) {
+  void SymbolsDidLoad(const ModuleList &module_list) override {
 m_negative_complete_class_cache.clear();
   }
 

Modified: lldb/trunk/source/Target/Target.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/Target.cpp?rev=362461&r1=362460&r2=362461&view=diff
==
--- lldb/trunk/source/Target/Target.cpp (original)
+++ lldb/trunk/source/Target/Target.cpp Mon Jun  3 16:12:11 2019
@@ -43,7 +43,6 @@
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/LanguageRuntime.h"
-#include "lldb/Target/ObjCLanguageRuntime.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/StackFrame.h"
@@ -1668,11 +1667,8 @@ void Target::ModulesDidLoad(ModuleList &
 void Target::SymbolsDidLoad(ModuleList &module_list) {
   if (m_valid && module_list.GetSize()) {
 if (m_process_sp) {
-  LanguageRuntime *runtime =
-  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC);
-  if (runtime) {
-ObjCLanguageRuntime *objc_runtime = (ObjCLanguageRuntime *)runtime;
-objc_runtime->SymbolsDidLoad(module_list);
+  for (LanguageRuntime *runtime : m_process_sp->GetLanguageRuntimes()) {
+runtime->SymbolsDidLoad(module_list);
   }
 }
 


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

[Lldb-commits] [PATCH] D62796: [Target] Generalize some behavior in Target::SymbolsDidLoad

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362461: [Target] Generalize some behavior in 
Target::SymbolsDidLoad (authored by xiaobai, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62796

Files:
  lldb/trunk/include/lldb/Target/LanguageRuntime.h
  lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
  lldb/trunk/source/Target/Target.cpp


Index: lldb/trunk/include/lldb/Target/LanguageRuntime.h
===
--- lldb/trunk/include/lldb/Target/LanguageRuntime.h
+++ lldb/trunk/include/lldb/Target/LanguageRuntime.h
@@ -143,6 +143,8 @@
 return false;
   }
 
+  virtual void SymbolsDidLoad(const ModuleList &module_list) { return; }
+
   virtual lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread,
   bool stop_others) = 
0;
 
Index: lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
===
--- lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
+++ lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
@@ -275,7 +275,7 @@
 return (m_has_new_literals_and_indexing == eLazyBoolYes);
   }
 
-  virtual void SymbolsDidLoad(const ModuleList &module_list) {
+  void SymbolsDidLoad(const ModuleList &module_list) override {
 m_negative_complete_class_cache.clear();
   }
 
Index: lldb/trunk/source/Target/Target.cpp
===
--- lldb/trunk/source/Target/Target.cpp
+++ lldb/trunk/source/Target/Target.cpp
@@ -43,7 +43,6 @@
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/LanguageRuntime.h"
-#include "lldb/Target/ObjCLanguageRuntime.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/StackFrame.h"
@@ -1668,11 +1667,8 @@
 void Target::SymbolsDidLoad(ModuleList &module_list) {
   if (m_valid && module_list.GetSize()) {
 if (m_process_sp) {
-  LanguageRuntime *runtime =
-  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC);
-  if (runtime) {
-ObjCLanguageRuntime *objc_runtime = (ObjCLanguageRuntime *)runtime;
-objc_runtime->SymbolsDidLoad(module_list);
+  for (LanguageRuntime *runtime : m_process_sp->GetLanguageRuntimes()) {
+runtime->SymbolsDidLoad(module_list);
   }
 }
 


Index: lldb/trunk/include/lldb/Target/LanguageRuntime.h
===
--- lldb/trunk/include/lldb/Target/LanguageRuntime.h
+++ lldb/trunk/include/lldb/Target/LanguageRuntime.h
@@ -143,6 +143,8 @@
 return false;
   }
 
+  virtual void SymbolsDidLoad(const ModuleList &module_list) { return; }
+
   virtual lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread,
   bool stop_others) = 0;
 
Index: lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
===
--- lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
+++ lldb/trunk/include/lldb/Target/ObjCLanguageRuntime.h
@@ -275,7 +275,7 @@
 return (m_has_new_literals_and_indexing == eLazyBoolYes);
   }
 
-  virtual void SymbolsDidLoad(const ModuleList &module_list) {
+  void SymbolsDidLoad(const ModuleList &module_list) override {
 m_negative_complete_class_cache.clear();
   }
 
Index: lldb/trunk/source/Target/Target.cpp
===
--- lldb/trunk/source/Target/Target.cpp
+++ lldb/trunk/source/Target/Target.cpp
@@ -43,7 +43,6 @@
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Target/Language.h"
 #include "lldb/Target/LanguageRuntime.h"
-#include "lldb/Target/ObjCLanguageRuntime.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/StackFrame.h"
@@ -1668,11 +1667,8 @@
 void Target::SymbolsDidLoad(ModuleList &module_list) {
   if (m_valid && module_list.GetSize()) {
 if (m_process_sp) {
-  LanguageRuntime *runtime =
-  m_process_sp->GetLanguageRuntime(lldb::eLanguageTypeObjC);
-  if (runtime) {
-ObjCLanguageRuntime *objc_runtime = (ObjCLanguageRuntime *)runtime;
-objc_runtime->SymbolsDidLoad(module_list);
+  for (LanguageRuntime *runtime : m_process_sp->GetLanguageRuntimes()) {
+runtime->SymbolsDidLoad(module_list);
   }
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D62755: [Target] Remove Process::GetCPPLanguageRuntime

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: include/lldb/Target/CPPLanguageRuntime.h:47
+  static CPPLanguageRuntime *GetCPPLanguageRuntime(Process &process) {
+return static_cast(
+process.GetLanguageRuntime(lldb::eLanguageTypeC_plus_plus));

xiaobai wrote:
> labath wrote:
> > It might be nice to add some glue so we could write 
> > `llvm::cast_or_null(...)` here. The main advantage of 
> > that being that we'd automatically get an assert firing if 
> > `GetLanguageRuntime` ever returns something which is *not* a 
> > CPPLanguageRuntime.
> I had thought about doing that, but there's an assertion in 
> GetLanguageRuntime that achieves the same thing imo. I do think that we could 
> move towards a `llvm::cast_or_null` implementation though.
I think would be worthwhile too :-) 


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

https://reviews.llvm.org/D62755



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

[Lldb-commits] [PATCH] D62755: [Target] Remove Process::GetCPPLanguageRuntime

[Jim Ingham via Phabricator via lldb-commits]

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

This deals with my objections so I'm marking as accepted because that's the 
only way I can see to unblock.  Seems like you were still discussing return 
types, and I don't want to preempt that, however...


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

https://reviews.llvm.org/D62755



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

[Lldb-commits] [PATCH] D62702: [ABI] Fix SystemV ABI to handle nested aggregate type returned in register

[Wanyi Ye via Phabricator via lldb-commits]

kusmour updated this revision to Diff 202824.
kusmour added a comment.

1. Limit the `TestReturnValue` for nested struct and class (cpp support) to 
only x86_64
2. This patch somehow fix the bug: pr36870 
 for Systerm V ABI (windows is 
waiting for this change to go in). So I change the test to allow passing on 
SysV-x86_64


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62702

Files:
  lldb/include/lldb/Symbol/ClangASTContext.h
  lldb/include/lldb/Symbol/TypeSystem.h
  lldb/packages/Python/lldbsuite/test/functionalities/return-value/Makefile
  
lldb/packages/Python/lldbsuite/test/functionalities/return-value/TestReturnValue.py
  lldb/packages/Python/lldbsuite/test/functionalities/return-value/call-func.c
  lldb/packages/Python/lldbsuite/test/functionalities/return-value/call-func.cpp
  lldb/packages/Python/lldbsuite/test/lang/cpp/trivial_abi/TestTrivialABI.py
  lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
  lldb/source/Symbol/ClangASTContext.cpp

Index: lldb/source/Symbol/ClangASTContext.cpp
===
--- lldb/source/Symbol/ClangASTContext.cpp
+++ lldb/source/Symbol/ClangASTContext.cpp
@@ -3911,6 +3911,14 @@
   return GetCanonicalQualType(type)->isVoidType();
 }
 
+bool ClangASTContext::CanPassInRegisters(const CompilerType &type) {
+  if (auto *record_decl = 
+  ClangASTContext::GetAsRecordDecl(type)) {
+return record_decl->canPassInRegisters();
+  }
+  return false;
+}
+
 bool ClangASTContext::SupportsLanguage(lldb::LanguageType language) {
   return ClangASTContextSupportsLanguage(language);
 }
Index: lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
===
--- lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
+++ lldb/source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
@@ -30,6 +30,8 @@
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/Status.h"
 
+#include 
+
 using namespace lldb;
 using namespace lldb_private;
 
@@ -1558,6 +1560,55 @@
   return return_valobj_sp;
 }
 
+// The compiler will flatten the nested aggregate type into single
+// layer and push the value to stack
+// This helper function will flatten an aggregate type
+// and return true if it can be returned in register(s) by value
+// return false if the aggregate is in memory
+static bool FlattenAggregateType(
+Thread &thread, ExecutionContext &exe_ctx,
+CompilerType &return_compiler_type,
+uint32_t data_byte_offset,
+std::vector &aggregate_field_offsets,
+std::vector &aggregate_compiler_types) {
+
+  const uint32_t num_children = return_compiler_type.GetNumFields();
+  for (uint32_t idx = 0; idx < num_children; ++idx) {
+std::string name;
+bool is_signed;
+uint32_t count;
+bool is_complex;
+
+uint64_t field_bit_offset = 0;
+CompilerType field_compiler_type = return_compiler_type.GetFieldAtIndex(
+idx, name, &field_bit_offset, nullptr, nullptr);
+llvm::Optional field_bit_width =
+  field_compiler_type.GetBitSize(&thread);
+
+// if we don't know the size of the field (e.g. invalid type), exit
+if (!field_bit_width || *field_bit_width == 0) {
+  return false;
+}
+
+uint32_t field_byte_offset = field_bit_offset / 8 + data_byte_offset;
+
+const uint32_t field_type_flags = field_compiler_type.GetTypeInfo();
+if (field_compiler_type.IsIntegerOrEnumerationType(is_signed) ||
+field_compiler_type.IsPointerType() ||
+field_compiler_type.IsFloatingPointType(count, is_complex)) {
+  aggregate_field_offsets.push_back(field_byte_offset);
+  aggregate_compiler_types.push_back(field_compiler_type);
+} else if (field_type_flags & eTypeHasChildren) {
+  if (!FlattenAggregateType(thread, exe_ctx, field_compiler_type,
+field_byte_offset, aggregate_field_offsets,
+aggregate_compiler_types)) {
+return false;
+  }
+}
+  }
+  return true;
+}
+
 ValueObjectSP ABISysV_x86_64::GetReturnValueObjectImpl(
 Thread &thread, CompilerType &return_compiler_type) const {
   ValueObjectSP return_valobj_sp;
@@ -1580,10 +1631,17 @@
   if (return_compiler_type.IsAggregateType()) {
 Target *target = exe_ctx.GetTargetPtr();
 bool is_memory = true;
-if (*bit_width <= 128) {
-  ByteOrder target_byte_order = target->GetArchitecture().GetByteOrder();
+std::vector aggregate_field_offsets;
+std::vector aggregate_compiler_types;
+if (return_compiler_type.GetTypeSystem()->CanPassInRegisters(
+  return_compiler_type) &&
+  *bit_width <= 128 &&
+  FlattenAggregateType(thread, exe_ctx, return_compiler_type,
+  0, aggregate_field_offsets,
+  aggregate_compiler_types)) {
+  ByteOrder byte_order = target->GetArchit

[Lldb-commits] [PATCH] D62500: Add support to read aux vector values

[António Afonso via Phabricator via lldb-commits]

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



Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:11
 
-using namespace lldb;
-using namespace lldb_private;
+AuxVector::AuxVector(lldb_private::DataExtractor &data) { ParseAuxv(data); }
 

clayborg wrote:
> Not a big fan of constructors doing parsing work. No way to return an error 
> unless you build that into the AuxVector class (AuxVector::GetError()).
That is true but ParseAuxv doesn't generate errors. It just reads as much data 
as it exists in the data extractor until there's no more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62500



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

[Lldb-commits] [PATCH] D62755: [Target] Remove Process::GetCPPLanguageRuntime

[Alex Langford via Phabricator via lldb-commits]

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



Comment at: include/lldb/Target/CPPLanguageRuntime.h:47
+  static CPPLanguageRuntime *GetCPPLanguageRuntime(Process &process) {
+return static_cast(
+process.GetLanguageRuntime(lldb::eLanguageTypeC_plus_plus));

JDevlieghere wrote:
> xiaobai wrote:
> > labath wrote:
> > > It might be nice to add some glue so we could write 
> > > `llvm::cast_or_null(...)` here. The main advantage of 
> > > that being that we'd automatically get an assert firing if 
> > > `GetLanguageRuntime` ever returns something which is *not* a 
> > > CPPLanguageRuntime.
> > I had thought about doing that, but there's an assertion in 
> > GetLanguageRuntime that achieves the same thing imo. I do think that we 
> > could move towards a `llvm::cast_or_null` implementation though.
> I think would be worthwhile too :-) 
Would y'all mind if I did that in a follow up commit? I think it is 
worthwhile/valuable, but I'd like it to be in a separate commit if possible.


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

https://reviews.llvm.org/D62755



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

[Lldb-commits] [PATCH] D62502: Implement xfer:libraries-svr4:read packet

[António Afonso via Phabricator via lldb-commits]

aadsm marked 4 inline comments as done.
aadsm added a comment.

@labath

> Regarding the test, I am wondering whether requiring the /proc/%d/maps and 
> the linker rendezvous structure to match isn't too strict of a requirement. 
> Dynamic linkers (and particularly android dynamic linkers) do a lot of crazy 
> stuff, and trying to assert this invariant exposes us to all kinds of 
> "optimizations" that the linkers do or may do in the future. I'm wondering if 
> it wouldn't be better to just build an executable with one or two dependent 
> libraries, and then just assert that their entries are present in the library 
> list. However, I'm not completely sure on this, so I'd like to hear what you 
> think about that..

Yeah, that was actually my first idea for the test but then realized we already 
had that main.c all set up and ready to use so I decide to use proc maps 
instead. I was not aware that things like that could happen (although I had a 
suspicion this might be brittle) so let me revisit this.




Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:37-39
+  lldb::addr_t link_map;
+  lldb::addr_t base_addr;
+  lldb::addr_t ld_addr;

clayborg wrote:
> These seem very linux specific. Why do we need 3 addresses here? Seems like 
> we should just need one. Or is this the structure of the 
> "xfer:libraries-svr4:read" that is required to be returned? If so, maybe we 
> rename "SharedLibraryInfo" to "SVR4ModuleInfo"?
Yes, that is a good point and yes this is the data the SVR4 returns. Given this 
structure it makes more sense to move this into the NativeProcessLinux instead 
and name it more specifically.



Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:40
+  lldb::addr_t ld_addr;
+  bool main;
+  lldb::addr_t next;

clayborg wrote:
> Why is "main" important? Hopefully the dynamic linker can figure out what is 
> what without needing to know this? Seems verify linux specific. Or is this 
> "xfer:libraries-svr4:read" specific?
It is packet specific yes. And I just realized I'm not putting that info in the 
XML I'm returning, will update.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2191
+  char name_buffer[PATH_MAX];
+  error = ReadMemory(link_map.l_name, &name_buffer, sizeof(name_buffer),
+ bytes_read);

clayborg wrote:
> Use ReadCStringFromMemory?
ReadCStringFromMemory doesn't exist here yet. That's on the next diff of this 
stack of diffs.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2247
+library_list.push_back(info);
+link_map = info.next;
+  }

clayborg wrote:
> Seems this we are using "info.next" just because it is a linked list on 
> linux. Can we remove "next" and just add an extra "next" reference parameter 
> to ReadSharedLibraryInfo(link_map, info, next);?
> 
Yes, this is the only reason, sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62502



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

[Lldb-commits] [PATCH] D62634: Improve DWARF parsing and accessing by 1% to 2%

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D62634#1527634 , @probinson wrote:

> In D62634#1527575 , @dblaikie wrote:
>
> > This is intentional behavior in clang (controllable under the 
> > -f[no-]split-dwarf-inlining flag, and modified by the 
> > -f[no-]debug-info-for-profiling flag).
> >
> > This extra debug info is used for online symbolication (in the absence of 
> > .dwo files) - such as for the sanitizers (accurate symbolication is 
> > necessary for correctness in msan, due to msan's necessary use of 
> > blacklisting to avoid certain false positives) and some forms of sample 
> > based profiling collection.
> >
> > In the default mode, clang includes, as you noted, just the subprograms 
> > that have inlined subroutines in them in this split-dwarf-inlining data.
> >  In -fdebug-info-for-profiling all subprograms are described in the 
> > skeleton CU with some minimal attributes (they don't need parameters, local 
> > variables/scopes, etc) necessary to do certain profile things I don't know 
> > lots about.
>
>
> I think we have to tolerate the msan part.  However, the profile feedback 
> workflow could (should) surely be taught to read a .dwo file.  The point of 
> -fdebug-info-for-profiling was so you don't have to emit limited/full debug 
> info in the .o file just to make profiling work; but if you're using split 
> DWARF then you're doing limited/full anyway, and the feedback loop happens in 
> the context of a build environment so the .dwo can be asserted to be 
> available.  So in split DWARF mode, we should not be emitting 
> debug-info-for-profiling into the skeleton.


Ah, hmm, looks like debug-info-for-profiling+split+-dwarf-inling ends up in a 
slighty hybrid state. DIFP does add the linkage name, decl line and decl file 
to subprograms, beyond just the simple name that gmlt would normally provide. 
But one of DIFP's other features doesn't trigger on the split-dwarf-inlining: 
it doesn't cause descriptions of functions without any inlining to be described.

>> Chances are it's probably best for a debugger to ignore these entries.
> 
> This stuff is going to show up in the wild, so yeah.  You know you're looking 
> at a skeleton, so don't bother looking at any children.




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

https://reviews.llvm.org/D62634



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

[Lldb-commits] [PATCH] D62500: Add support to read aux vector values

[António Afonso via Phabricator via lldb-commits]

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



Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:183
+  m_process->GetAddressByteSize());
+  m_auxv = llvm::make_unique(auxv_data);
 

clayborg wrote:
> Use std::move?
> ```
> m_auxv = llvm::make_unique(std::move(auxv_data));
> ```
The constructor receives a `lldb_private::DataExtractor &` so there's no copy 
going on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62500



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

[Lldb-commits] [PATCH] D62797: [Expression] Add PersistentExpressionState::GetCompilerTypeFromPersistentDecl

[Saleem Abdulrasool via Phabricator via lldb-commits]

compnerd accepted this revision.
compnerd added a subscriber: clayborg.
compnerd added a comment.
This revision is now accepted and ready to land.

Would be nice to get someone like @clayborg to chime in, but, I think that 
@labath also seems to think that this is fine.




Comment at: source/Commands/CommandObjectMemory.cpp:476
+for (auto lang : Language::GetSupportedLanguages()) {
+  if (PersistentExpressionState *persistent_vars =
+  target->GetPersistentExpressionStateForLanguage(lang)) {

I think that `auto` would be fine given that the 
`GetPersistentExpressionStateForLanguage` spells out the return type.



Comment at: source/Commands/CommandObjectMemory.cpp:481
+lookup_type_name)) {
+  clang_ast_type = *type;
+  break;

This seems wrong now.  You iterate over all the languages, but always set the 
`clang_ast_type`?  The type may be a Swift AST type or Clang AST type (or 
something more exotic like a rust AST type).  We should rename that to 
`m_ast_type` as per the LLDB style.  But, that makes sense to do as a follow up 
change.


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

https://reviews.llvm.org/D62797



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

[Lldb-commits] [PATCH] D62755: [Target] Remove Process::GetCPPLanguageRuntime

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Fine by me.


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

https://reviews.llvm.org/D62755



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

[Lldb-commits] [PATCH] D62797: [Expression] Add PersistentExpressionState::GetCompilerTypeFromPersistentDecl

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I have no problem with the change in general.  However, you've introduced the 
possibility of name collision between convenience type definition in various 
languages.  So it would be good to run through the persistent DECL's for ALL 
the supported languages and report collisions.  Probably also need to add a -l 
flag to pick a language?


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

https://reviews.llvm.org/D62797



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

[Lldb-commits] [PATCH] D62795: [Target] Move ObjCLanguageRuntime::LookupRuntimeSymbol into LanguageRuntime

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This looks fine to me.

For context, ObjC has symbols that point into the runtime that tell you things 
like the current offsets of the members of an ObjC class.  In debug builds the 
symbols are present, but the runtime doesn't depend on the symbols per se, 
since it reads the data directly from the runtime.  The symbol names are 
constructed from the class & ivar name so they are a convenient way to name the 
data you are looking for.  The lldb does "look for the symbol and if you don't 
find it ask the runtime if it knows where the equivalent data lives directly in 
the runtime.".  The latter task is "LookupRuntimeSymbol".

We don't do it this way in swift, instead we ask RemoteAST for this sort of 
data.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62795



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

[Lldb-commits] [PATCH] D62755: [Target] Remove Process::GetCPPLanguageRuntime

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: include/lldb/Target/CPPLanguageRuntime.h:47
+  static CPPLanguageRuntime *GetCPPLanguageRuntime(Process &process) {
+return static_cast(
+process.GetLanguageRuntime(lldb::eLanguageTypeC_plus_plus));

xiaobai wrote:
> JDevlieghere wrote:
> > xiaobai wrote:
> > > labath wrote:
> > > > It might be nice to add some glue so we could write 
> > > > `llvm::cast_or_null(...)` here. The main advantage 
> > > > of that being that we'd automatically get an assert firing if 
> > > > `GetLanguageRuntime` ever returns something which is *not* a 
> > > > CPPLanguageRuntime.
> > > I had thought about doing that, but there's an assertion in 
> > > GetLanguageRuntime that achieves the same thing imo. I do think that we 
> > > could move towards a `llvm::cast_or_null` implementation though.
> > I think would be worthwhile too :-) 
> Would y'all mind if I did that in a follow up commit? I think it is 
> worthwhile/valuable, but I'd like it to be in a separate commit if possible.
Sounds good


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

https://reviews.llvm.org/D62755



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

[Lldb-commits] [PATCH] D62502: Implement xfer:libraries-svr4:read packet

[António Afonso via Phabricator via lldb-commits]

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



Comment at: lldb/include/lldb/Host/common/NativeProcessProtocol.h:37-39
+  lldb::addr_t link_map;
+  lldb::addr_t base_addr;
+  lldb::addr_t ld_addr;

aadsm wrote:
> clayborg wrote:
> > These seem very linux specific. Why do we need 3 addresses here? Seems like 
> > we should just need one. Or is this the structure of the 
> > "xfer:libraries-svr4:read" that is required to be returned? If so, maybe we 
> > rename "SharedLibraryInfo" to "SVR4ModuleInfo"?
> Yes, that is a good point and yes this is the data the SVR4 returns. Given 
> this structure it makes more sense to move this into the NativeProcessLinux 
> instead and name it more specifically.
Now that I think more about this, SVR4 is really generic to all system v 
implementations (4.0) so it's not linux specific. Will just change the name to 
more accurately reflect what is means.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62502



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

[Lldb-commits] [PATCH] D62499: Create a generic handler for Xfer packets

[António Afonso via Phabricator via lldb-commits]

aadsm updated this revision to Diff 202852.
aadsm marked 2 inline comments as done.
aadsm added a comment.

Move test to a better place, address other comments related to tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62499

Files:
  lldb/include/lldb/Utility/StringExtractorGDBRemote.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h
  lldb/source/Utility/StringExtractorGDBRemote.cpp
  lldb/unittests/Process/gdb-remote/CMakeLists.txt
  lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
  lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h

Index: lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
===
--- lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
+++ lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h
@@ -8,9 +8,11 @@
 #ifndef lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
 #define lldb_unittests_Process_gdb_remote_GDBRemoteTestUtils_h
 
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
+#include "lldb/Utility/Connection.h"
 
 namespace lldb_private {
 namespace process_gdb_remote {
@@ -21,10 +23,38 @@
   static void TearDownTestCase();
 };
 
-struct MockServer : public GDBRemoteCommunicationServer {
+class MockConnection : public lldb_private::Connection {
+public:
+  MockConnection(std::vector *packets) { m_packets = packets; };
+
+  MOCK_METHOD2(Connect,
+   lldb::ConnectionStatus(llvm::StringRef url, Status *error_ptr));
+  MOCK_METHOD5(Read, size_t(void *dst, size_t dst_len,
+const Timeout &timeout,
+lldb::ConnectionStatus &status, Status *error_ptr));
+  MOCK_METHOD0(GetURI, std::string());
+  MOCK_METHOD0(InterruptRead, bool());
+
+  lldb::ConnectionStatus Disconnect(Status *error_ptr) {
+return lldb::eConnectionStatusSuccess;
+  };
+
+  bool IsConnected() const { return true; };
+  size_t Write(const void *dst, size_t dst_len, lldb::ConnectionStatus &status,
+   Status *error_ptr) {
+m_packets->emplace_back(static_cast(dst), dst_len);
+return dst_len;
+  };
+
+  std::vector *m_packets;
+};
+
+class MockServer : public GDBRemoteCommunicationServer {
+public:
   MockServer()
   : GDBRemoteCommunicationServer("mock-server", "mock-server.listener") {
 m_send_acks = false;
+m_send_error_strings = true;
   }
 
   PacketResult SendPacket(llvm::StringRef payload) {
@@ -37,10 +67,22 @@
sync_on_timeout);
   }
 
+  using GDBRemoteCommunicationServer::SendErrorResponse;
   using GDBRemoteCommunicationServer::SendOKResponse;
   using GDBRemoteCommunicationServer::SendUnimplementedResponse;
 };
 
+class MockServerWithMockConnection : public MockServer {
+public:
+  MockServerWithMockConnection() : MockServer() {
+SetConnection(new MockConnection(&m_packets));
+  }
+
+  llvm::ArrayRef GetPackets() { return m_packets; };
+
+  std::vector m_packets;
+};
+
 } // namespace process_gdb_remote
 } // namespace lldb_private
 
Index: lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
===
--- /dev/null
+++ lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp
@@ -0,0 +1,74 @@
+//===-- GDBRemoteGDBRemoteCommunicationServerTest.cpp *- C++
+//-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "GDBRemoteTestUtils.h"
+
+#include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h"
+#include "lldb/Utility/Connection.h"
+
+namespace lldb_private {
+namespace process_gdb_remote {
+
+TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_ErrorNumber) {
+  MockServerWithMockConnection server;
+  server.SendErrorResponse(0x42);
+
+  EXPECT_THAT(server.GetPackets(), testing::ElementsAre("$E42#ab"));
+}
+
+TEST(GDBRemoteCommunicationServerTest, SendErrorResponse_Status) {
+  MockServerWithMockConnection server;
+  Status status;
+
+  status.SetError(0x42, lldb::eErrorTypeGeneric);
+  status.SetErrorString("Test error message");
+  server.SendErrorResponse(status);
+
+  EXPECT_THAT(
+  server.GetPackets(),
+  testing::ElementsAre("$E42;54657374206572726f72206d657373616765#ad"));
+}
+
+TEST(GDBRemoteCommunicationServerTest, SendError

[Lldb-commits] [PATCH] D62500: Add support to read aux vector values

[António Afonso via Phabricator via lldb-commits]

aadsm updated this revision to Diff 202853.
aadsm marked an inline comment as done.
aadsm added a comment.
Herald added a subscriber: emaste.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62500

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.cpp
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.h
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/CMakeLists.txt
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
  lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
  lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.h
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Process/Utility/AuxVector.cpp
  lldb/source/Plugins/Process/Utility/AuxVector.h
  lldb/source/Plugins/Process/Utility/CMakeLists.txt
  lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
  lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  lldb/source/Target/Process.cpp

Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -2730,7 +2730,7 @@
   return m_dyld_up.get();
 }
 
-const lldb::DataBufferSP Process::GetAuxvData() { return DataBufferSP(); }
+DataExtractor Process::GetAuxvData() { return DataExtractor(); }
 
 JITLoaderList &Process::GetJITLoaders() {
   if (!m_jit_loaders_up) {
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -324,7 +324,7 @@
 
   bool ParsePythonTargetDefinition(const FileSpec &target_definition_fspec);
 
-  const lldb::DataBufferSP GetAuxvData() override;
+  DataExtractor GetAuxvData() override;
 
   StructuredData::ObjectSP GetExtendedInfoForThread(lldb::tid_t tid);
 
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -4046,7 +4046,7 @@
   return error;
 }
 
-const DataBufferSP ProcessGDBRemote::GetAuxvData() {
+DataExtractor ProcessGDBRemote::GetAuxvData() {
   DataBufferSP buf;
   if (m_gdb_comm.GetQXferAuxvReadSupported()) {
 std::string response_string;
@@ -4056,7 +4056,7 @@
   buf = std::make_shared(response_string.c_str(),
  response_string.length());
   }
-  return buf;
+  return DataExtractor(buf, GetByteOrder(), GetAddressByteSize());
 }
 
 StructuredData::ObjectSP
Index: lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
===
--- lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
+++ lldb/source/Plugins/Process/elf-core/ProcessElfCore.h
@@ -97,7 +97,7 @@
   lldb_private::ArchSpec GetArchitecture();
 
   // Returns AUXV structure found in the core file
-  const lldb::DataBufferSP GetAuxvData() override;
+  lldb_private::DataExtractor GetAuxvData() override;
 
   bool GetProcessInfo(lldb_private::ProcessInstanceInfo &info) override;
 
Index: lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
===
--- lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ lldb/source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -894,11 +894,11 @@
   return arch;
 }
 
-const lldb::DataBufferSP ProcessElfCore::GetAuxvData() {
+DataExtractor ProcessElfCore::GetAuxvData() {
   const uint8_t *start = m_auxv.GetDataStart();
   size_t len = m_auxv.GetByteSize();
   lldb::DataBufferSP buffer(new lldb_private::DataBufferHeap(start, len));
-  return buffer;
+  return DataExtractor(buffer, GetByteOrder(), GetAddressByteSize());
 }
 
 bool ProcessElfCore::GetProcessInfo(ProcessInstanceInfo &info) {
Index: lldb/source/Plugins/Process/Utility/CMakeLists.txt
===
--- lldb/source/Plugins/Process/Utility/CMakeLists.txt
+++ lldb/source/Plugins/Process/Utility/CMakeLists.txt
@@ -1,4 +1,5 @@
 add_lldb_library(lldbPluginProcessUtility PLUGIN
+  AuxVector.cpp
   DynamicRegisterInfo.cpp
   FreeBSDSignals.cpp
   GDBRemoteSignals.cpp
Index: lldb/source/Plugins/Process/Utility/AuxVector.h
===
--- /dev/null
+++ lldb/source/Plugins/Process/Utility/AuxVector.h
@@ -0,0 +1,73

[Lldb-commits] [PATCH] D62500: Add support to read aux vector values

[António Afonso via Phabricator via lldb-commits]

aadsm added inline comments.



Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:24-28
+  while (true) {
+uint64_t type = 0;
+uint64_t value = 0;
+if (!ReadUInt(data, &offset, &type) || !ReadUInt(data, &offset, &value))
   break;

clayborg wrote:
> ```
> const size_t value_type_size = data.GetAddressByteSize() * 2;
> while (data.ValidOffsetForDataOfSize(value_type_size)) {
>   const uint64_t type = data.GetAddress(&offset);
>   const uint64_t value = data.GetAddress(&offset);
>   if (type == AUXV_AT_NULL)
> ...
> ```
> 
nice, this is a much better loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62500



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

[Lldb-commits] [PATCH] D62501: Implement GetSharedLibraryInfoAddress

[António Afonso via Phabricator via lldb-commits]

aadsm updated this revision to Diff 202854.
aadsm added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62501

Files:
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h

Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
@@ -132,10 +132,14 @@
   llvm::Expected>
   GetSoftwareBreakpointTrapOpcode(size_t size_hint) override;
 
+  template 
+  lldb::addr_t GetELFImageInfoAddress();
+
 private:
   MainLoop::SignalHandleUP m_sigchld_handle;
   ArchSpec m_arch;
   std::unique_ptr m_aux_vector;
+  lldb::addr_t m_elf_image_info_addr = LLDB_INVALID_ADDRESS;
 
   LazyBool m_supports_mem_region = eLazyBoolCalculate;
   std::vector> m_mem_region_cache;
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -38,6 +38,7 @@
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StringExtractor.h"
+#include "llvm/BinaryFormat/ELF.h"
 #include "llvm/Support/Errno.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Threading.h"
@@ -1390,8 +1391,12 @@
 }
 
 lldb::addr_t NativeProcessLinux::GetSharedLibraryInfoAddress() {
-  // punt on this for now
-  return LLDB_INVALID_ADDRESS;
+  if (GetAddressByteSize() == 8)
+return GetELFImageInfoAddress();
+  else
+return GetELFImageInfoAddress();
 }
 
 size_t NativeProcessLinux::UpdateThreads() {
@@ -2097,3 +2102,77 @@
 
   return m_aux_vector->GetAuxValue(type);
 }
+
+template 
+lldb::addr_t NativeProcessLinux::GetELFImageInfoAddress() {
+  if (m_elf_image_info_addr != LLDB_INVALID_ADDRESS)
+return m_elf_image_info_addr;
+
+  llvm::Optional maybe_phdr_addr =
+  GetAuxValue(AuxVector::AUXV_AT_PHDR);
+  llvm::Optional maybe_phdr_entry_size =
+  GetAuxValue(AuxVector::AUXV_AT_PHENT);
+  llvm::Optional maybe_phdr_num_entries =
+  GetAuxValue(AuxVector::AUXV_AT_PHNUM);
+  if (!maybe_phdr_addr || !maybe_phdr_entry_size || !maybe_phdr_num_entries)
+return LLDB_INVALID_ADDRESS;
+  lldb::addr_t phdr_addr = *maybe_phdr_addr;
+  size_t phdr_entry_size = *maybe_phdr_entry_size;
+  size_t phdr_num_entries = *maybe_phdr_num_entries;
+  lldb::addr_t load_base = phdr_addr - sizeof(ELF_EHDR);
+
+  // Find the PT_DYNAMIC segment (.dynamic section) in the program header and
+  // what the ELF believes to be the load base.
+  lldb::addr_t elf_load_base = 0;
+  bool found_elf_load_base = false;
+  lldb::addr_t dynamic_section_addr = 0;
+  uint64_t dynamic_section_size = 0;
+  bool found_dynamic_section = false;
+  ELF_PHDR phdr_entry;
+  for (size_t i = 0; i < phdr_num_entries; i++) {
+size_t bytes_read;
+auto error = ReadMemory(phdr_addr + i * phdr_entry_size, &phdr_entry,
+sizeof(phdr_entry), bytes_read);
+if (!error.Success())
+  return LLDB_INVALID_ADDRESS;
+
+if ((!found_elf_load_base || phdr_entry.p_vaddr < elf_load_base) &&
+phdr_entry.p_type == llvm::ELF::PT_LOAD) {
+  elf_load_base = phdr_entry.p_vaddr;
+  found_elf_load_base = true;
+}
+
+if (phdr_entry.p_type == llvm::ELF::PT_DYNAMIC) {
+  dynamic_section_addr = phdr_entry.p_vaddr;
+  dynamic_section_size = phdr_entry.p_memsz;
+  found_dynamic_section = true;
+}
+  }
+
+  if (!found_elf_load_base || !found_dynamic_section)
+return LLDB_INVALID_ADDRESS;
+
+  // Find the DT_DEBUG entry in the .dynamic section
+  // The load base we got from the auxiliary vector is the source of truth. If
+  // we got a different load base from the ELF then we need to correct the
+  // .dynamic section address with the difference we found.
+  dynamic_section_addr += (load_base - elf_load_base);
+  ELF_DYN dynamic_entry;
+  size_t dynamic_num_entries = dynamic_section_size / sizeof(dynamic_entry);
+  for (size_t i = 0; i < dynamic_num_entries; i++) {
+size_t bytes_read;
+auto error = ReadMemory(dynamic_section_addr + i * sizeof(dynamic_entry),
+&dynamic_entry, sizeof(dynamic_entry), bytes_read);
+if (!error.Success())
+  return LLDB_INVALID_ADDRESS;
+// Return the &DT_DEBUG->d_ptr which points to r_debug which contains the
+// link_map.
+if (dynamic_entry.d_tag == llvm::ELF::DT_DEBUG) {
+  m_elf_image_info_addr = dynamic_section_addr + i * sizeof(dynamic_entry) +
+  sizeof(dynamic_entry.d_tag);
+  return m_elf_image_info_addr;
+}
+  }
+
+  return LLDB_INVALID_ADDRESS;
+}
___
lldb-commits mailin

[Lldb-commits] [PATCH] D62502: Implement xfer:libraries-svr4:read packet

[António Afonso via Phabricator via lldb-commits]

aadsm updated this revision to Diff 202855.
aadsm added a comment.

Address comments except the one about a different test plan.
Also removed the main property, after searching on the internet I'm not 
confident with the way I was using to detect the main executable. Since the 
"lm-main" is an optional attribute I'm not going to implement it now.
However, I'm excluding from the list the libraries without name or that are at 
base addr == 0. They shouldn't be there by the spec.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62502

Files:
  lldb/include/lldb/Host/common/NativeProcessProtocol.h
  
lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteLibrariesSvr4Support.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-server/gdbremote_testcase.py
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
  lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -2765,6 +2765,26 @@
 return std::move(*buffer_or_error);
   }
 
+#if defined(__linux__)
+  if (object == "libraries-svr4") {
+std::vector library_list;
+auto status = m_debugged_process_up->GetLoadedSVR4Libraries(library_list);
+if (!status.Success())
+  return status.ToError();
+
+StreamString response;
+response.Printf("");
+for (auto const &library : library_list) {
+  response.Printf("", library.ld_addr);
+}
+response.Printf("");
+return MemoryBuffer::getMemBufferCopy(response.GetString(), __FUNCTION__);
+  }
+#endif
+
   return llvm::make_error(
   "Xfer object not supported");
 }
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
@@ -826,6 +826,9 @@
   response.PutCString(";QPassSignals+");
   response.PutCString(";qXfer:auxv:read+");
 #endif
+#if defined(__linux__)
+  response.PutCString(";qXfer:libraries-svr4:read+");
+#endif
 
   return SendPacketNoLock(response.GetString());
 }
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.h
@@ -79,6 +79,9 @@
 
   lldb::addr_t GetSharedLibraryInfoAddress() override;
 
+  Status
+  GetLoadedSVR4Libraries(std::vector &library_list) override;
+
   size_t UpdateThreads() override;
 
   const ArchSpec &GetArchitecture() const override { return m_arch; }
@@ -135,6 +138,17 @@
   template 
   lldb::addr_t GetELFImageInfoAddress();
 
+  template  struct ELFLinkMap {
+T l_addr;
+T l_name;
+T l_ld;
+T l_next;
+T l_prev;
+  };
+  template 
+  Status ReadSVR4LibraryInfo(lldb::addr_t link_map_addr, SVR4LibraryInfo &info,
+ lldb::addr_t &next);
+
 private:
   MainLoop::SignalHandleUP m_sigchld_handle;
   ArchSpec m_arch;
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -2176,3 +2176,70 @@
 
   return LLDB_INVALID_ADDRESS;
 }
+
+template 
+Status NativeProcessLinux::ReadSVR4LibraryInfo(lldb::addr_t link_map_addr,
+   SVR4LibraryInfo &info,
+   lldb::addr_t &next) {
+  ELFLinkMap link_map;
+  size_t bytes_read;
+  auto error =
+  ReadMemory(link_map_addr, &link_map, sizeof(link_map), bytes_read);
+  if (!error.Success())
+return error;
+
+  char name_buffer[PATH_MAX];
+  error = ReadMemory(link_map.l_name, &name_buffer, sizeof(name_buffer),
+ bytes_read);
+  if (!error.Success())
+return error;
+
+  info.name = std::string(name_buffer);
+  info.link_map = link_map_addr;
+  info.base_addr = link_map.l_addr;
+  info.ld_addr = link_map.l_ld;
+
+  next = link_map.l_next;
+  return Status();
+}
+
+Status NativeProcessLinux::GetLoadedSVR4Libraries(
+std::vector &library_list) {
+  // Address of DT_DEBUG.d_ptr which points to r_debug
+  lldb::addr_t info_address = GetSharedLibraryInfoAddress();
+  if (info_address == LLDB_INVALID_ADDRESS)
+return Status("Invalid shared library info ad

[Lldb-commits] [PATCH] D62755: [Target] Remove Process::GetCPPLanguageRuntime

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.



Comment at: include/lldb/Target/CPPLanguageRuntime.h:47
+  static CPPLanguageRuntime *GetCPPLanguageRuntime(Process &process) {
+return static_cast(
+process.GetLanguageRuntime(lldb::eLanguageTypeC_plus_plus));

JDevlieghere wrote:
> xiaobai wrote:
> > JDevlieghere wrote:
> > > xiaobai wrote:
> > > > labath wrote:
> > > > > It might be nice to add some glue so we could write 
> > > > > `llvm::cast_or_null(...)` here. The main 
> > > > > advantage of that being that we'd automatically get an assert firing 
> > > > > if `GetLanguageRuntime` ever returns something which is *not* a 
> > > > > CPPLanguageRuntime.
> > > > I had thought about doing that, but there's an assertion in 
> > > > GetLanguageRuntime that achieves the same thing imo. I do think that we 
> > > > could move towards a `llvm::cast_or_null` implementation though.
> > > I think would be worthwhile too :-) 
> > Would y'all mind if I did that in a follow up commit? I think it is 
> > worthwhile/valuable, but I'd like it to be in a separate commit if possible.
> Sounds good
no problem here.


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

https://reviews.llvm.org/D62755



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

[Lldb-commits] [PATCH] D62499: Create a generic handler for Xfer packets

[Pavel Labath via Phabricator via lldb-commits]

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

Looks good to me now. Thanks for your patience.




Comment at: 
lldb/unittests/Process/gdb-remote/GDBRemoteCommunicationServerTest.cpp:1-2
+//===-- GDBRemoteGDBRemoteCommunicationServerTest.cpp *- 
C++
+//-*-===//
+//

This line wrapped accidentally.



Comment at: lldb/unittests/Process/gdb-remote/GDBRemoteTestUtils.h:28
+public:
+  MockConnection(std::vector *packets) { m_packets = packets; };
+

A reference would be slightly better, as it is implicitly non-null.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62499



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