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

[Raphael Isemann via Phabricator via lldb-commits]

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

LGTM, thanks!


Repository:
  rLLDB LLDB

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] D62499: Create a generic handler for Xfer packets

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for the update. The actual code looks mostly good, but I have some 
comments on the error handling infrastructure. I am sorry that this is taking a 
bit long, but I am trying to make sure it's not unnecessarily overcomplicated 
(in the past we've generally not paid much attention to the error codes, and it 
doesn't look like we're about to start now), but instead it makes good use of 
our existing features (like error strings), and is generally unobtrusive so the 
actual code stands out (not needing to both log and create an error object when 
something happens helps that). Since you're building this as a general tool 
that can be used for future packets (or refactors of old ones), I believe it's 
worth the trouble.




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:118
+  GDBRemoteCommunication::PacketResult result;
+  llvm::handleAllErrors(
+  std::move(error),

I am sorry, but I just remembered there's one more case to think about here. 
The `error` might actually be a `ErrorList` and contain multiple errors. It's 
not a commonly used feature (in fact, it might go away at some point), but it 
exists and if such an error happens to make it's way here it will cause us to 
send multiple error messages and completely mess up the packet synchronization.

So we should at least guard against that with 
`assert(!error.isA())`; or implement this in a way that guarantees 
only one response would be sent. One way to do that would be to just send one 
of the errors based on some semi-arbitrary priority list. Maybe something like:
```
std::unique_ptr PE;
std::unique_ptr UE;
...
llvm::handleAllErrors(std::move(error),
  [&] (std::unique_ptr E) { PE = std::move(E); },
  ...);
if (PE)
  return SendErrorResponse(...);
if (UE)
  return SendUnimplementedError(...);
...
```




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:125
+  [&](std::unique_ptr e) {
+result = SendErrorResponse(std::move(e));
+  });

I'm a bit confused. What does this call exactly? It looks to me like this will 
use the implicit (we should probably make it explicit) 
`std::unique_ptr` constructor of `Error` to call this method 
again and cause infinite recursion.

I'd suggest doing something like 
`SendErrorResponse(Status(Error(std::move(e)))`. The advantage of this is that 
the `Status` overload knows how to send an error as string (we have an protocol 
extension for that), and so will provide a better error message on the client 
side.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h:83
+public:
+  using llvm::ErrorInfo::ErrorInfo; // inherit constructors

You need to define `static char ID` here too. Otherwise the dynamic type 
detection magic will not work..



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2758-2762
   if (log)
-log->Printf(
-"GDBRemoteCommunicationServerLLGS::%s failed, no process 
available",
-__FUNCTION__);
-  return SendErrorResponse(0x10);
+log->Printf("GDBRemoteCommunicationServerLLGS::%s failed, no process "
+"available",
+__FUNCTION__);
+  return llvm::make_error(0x10);

In the spirit of the comment below, here I'd just do something like `return 
createStringError(inconvertibleErrorCode(), "No process");`



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2770
   LLDB_LOG(log, "no auxv data retrieved: {0}", ec.message());
-  return SendErrorResponse(ec.value());
+  return llvm::make_error(ec.value());
 }

I am wondering whether we actually need the `PacketError` class. Such a class 
would be useful if we actually wanted to start providing meaningful error codes 
to the other side (as we would give us tighter control over the allocation of 
error codes). However, here you're just taking random numbers and sticking them 
into the `PacketError` class, so I am not sure that adds any value.

So, what I'd do here is just delete the PacketError class, and just do a 
`return llvm::errorCodeToError(ec)` here. I'd also delete the log message as 
the error message will implicitly end up in the packet log via the error 
message extension (if you implement my suggestion `SendErrorResponse`).



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2793-2794
+return SendUnimplementedResponse("qXfer action not supported");
+  std::string buffer_key = std::string(xfer_object) + std::string(xfer_action) 
+
+   std::string(xfer_annex);
+  // Parse offset.

A slightly more efficient (and shorter) way to concatenate this would be 
`(xfer_obj

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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



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

It looks like we're not guarding the call to `LoadModules` on line 101, so I 
think we can assume that m_process is not null here (and I don't see why it 
should be -- it does not make sense to call `DidAttach` if there is no process 
around).

BTW, your guarding attempt isn't even correct as we'd always call 
`m_process->GetByteOrder()` and crash if the process was null.



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

same here.



Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:549-550
 
-  if (ModuleSP module_sp = target.GetOrCreateModule(module_spec, 
-true /* notify */)) {
+  if (ModuleSP module_sp =
+  target.GetOrCreateModule(module_spec, true /* notify */)) {
 UpdateLoadedSections(module_sp, LLDB_INVALID_ADDRESS, m_interpreter_base,

Whitespace-only change. Did you maybe run clang-format on the whole file 
instead of just the diff?

It doesn't matter that much here as there isn't many of them, but I just want 
to make sure you're aware of this in the future.



Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:38
 
-AuxVector::AuxVector(Process *process) : m_process(process) {
-  DataExtractor data;
-  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
-
-  data.SetData(GetAuxvData());
-  data.SetByteOrder(m_process->GetByteOrder());
-  data.SetAddressByteSize(m_process->GetAddressByteSize());
-
-  ParseAuxv(data);
-
-  if (log)
-DumpToLog(log);
-}
-
-AuxVector::iterator AuxVector::FindEntry(EntryType type) const {
-  for (iterator I = begin(); I != end(); ++I) {
-if (I->type == static_cast(type))
-  return I;
-  }
-
-  return end();
+uint64_t AuxVector::GetAuxValue(enum EntryType entry_type) const {
+  auto it = m_auxv_entries.find(static_cast(entry_type));

It would be better to return an Optional (or addr_t maybe ?) instead 
of using a magic value to mean "not found". It looks like at least some of 
these entries can validly be zero.



Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:50
   log->PutCString("AuxVector: ");
-  for (iterator I = begin(); I != end(); ++I) {
-log->Printf("   %s [%" PRIu64 "]: %" PRIx64, GetEntryName(*I), I->type,
-I->value);
+  for (auto it = m_auxv_entries.begin(); it != m_auxv_entries.end(); ++it) {
+log->Printf("   %s [%" PRIu64 "]: %" PRIx64,

range-based for ?


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] D62715: [NativeProcessLinux] Reuse memory read by process_vm_readv before calling ptrace

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thinking about tests, do you know of any way to create memory which is readable 
by ptrace, but it is not accessible via process_vm_readv ? I know that latest 
android phones have memory like that, but I believe this depends on selinux or 
some other mechanism which cannot be reproduced in a test.

If we can't do anything like that, we should at least have an lldb-server test 
that performs a read (`$m` packet) crossing page boundaries, where one of the 
pages is unmapped. Probably the easiest way to guarantee that a particular page 
will be unmapped is to first `mmap` 2 pages worth of memory, and then `munmap` 
one of them.




Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1457
 
-bytes_read = process_vm_readv(pid, &local_iov, 1, &remote_iov, 1, 0);
-const bool success = bytes_read == size;
+auto vm_bytes_read =
+process_vm_readv(pid, &local_iov, 1, &remote_iov, 1, 0);

llvm's policy is to *not* "almost always use auto".  And it's nice to know 
whether this returns size_t, ssize_t, or something else without looking up the 
documentation.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1467
+  if (!success) {
+addr = reinterpret_cast(reinterpret_cast(addr) +
+vm_bytes_read);

`addr += vm_bytes_read` ?
Since this is talking about addresses in the other process, I can't imagine 
that casting to a `char *` is going to make this more "correct" in any way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62715



___
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 marked 4 inline comments as done.
aadsm added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:125
+  [&](std::unique_ptr e) {
+result = SendErrorResponse(std::move(e));
+  });

labath wrote:
> I'm a bit confused. What does this call exactly? It looks to me like this 
> will use the implicit (we should probably make it explicit) 
> `std::unique_ptr` constructor of `Error` to call this method 
> again and cause infinite recursion.
> 
> I'd suggest doing something like 
> `SendErrorResponse(Status(Error(std::move(e)))`. The advantage of this is 
> that the `Status` overload knows how to send an error as string (we have an 
> protocol extension for that), and so will provide a better error message on 
> the client side.
ah, not sure what I was thinking here. I remember writing 
`SendErrorResponse(Status(Error(std::move(e)))` (or something to that effect) 
but somehow ended up with this recursion..
I need to force these errors to happen to make sure everything makes sense.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h:83
+public:
+  using llvm::ErrorInfo::ErrorInfo; // inherit constructors

labath wrote:
> You need to define `static char ID` here too. Otherwise the dynamic type 
> detection magic will not work..
Ah, I guess it would only match against the StringError because it's inheriting 
that one?



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2770
   LLDB_LOG(log, "no auxv data retrieved: {0}", ec.message());
-  return SendErrorResponse(ec.value());
+  return llvm::make_error(ec.value());
 }

labath wrote:
> I am wondering whether we actually need the `PacketError` class. Such a class 
> would be useful if we actually wanted to start providing meaningful error 
> codes to the other side (as we would give us tighter control over the 
> allocation of error codes). However, here you're just taking random numbers 
> and sticking them into the `PacketError` class, so I am not sure that adds 
> any value.
> 
> So, what I'd do here is just delete the PacketError class, and just do a 
> `return llvm::errorCodeToError(ec)` here. I'd also delete the log message as 
> the error message will implicitly end up in the packet log via the error 
> message extension (if you implement my suggestion `SendErrorResponse`).
I thought it would be nice to have a little abstraction layer around the packet 
errors overall. My purpose with the PacketError is to make it more obvious in 
the code that the number given will be sent back as an error number.
I didn't realize the numbers we were using were meaningless today though (now 
that I think of it this ec.value is really whatever GetAuxvData returns). I 
searched at the time and found a few different numbers being used: 9, 4, 10, 
etc. I guess these numbers are just lies then :D.




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:85
   lldb::StateType m_inferior_prev_state = lldb::StateType::eStateInvalid;
-  std::unique_ptr m_active_auxv_buffer_up;
+  std::unordered_map>
+  m_xfer_buffer_map;

labath wrote:
> labath wrote:
> > This could be an `llvm::StringMap`. Also, it should be enough to use 
> > `unique_ptr` here. You just need to make sure that you don't copy the value 
> > out of the map. I recommend a pattern like
> > ```
> > iter = map.find("foo");
> > if (iter == end()) {
> >   auto buffer_or_error = getFoo();
> >   if (buffer_or_error)
> > iter = map.insert("foo", std::move(*buffer_or_error))
> >   else
> > report(buffer_or_error.getError());
> > }
> > StringRef buffer = iter->second->getBuffer();
> > 
> > do_stuff(buffer);
> > 
> > if (done)
> >   map.erase(iter);
> > ```
> What about the StringMap part ? :)
completely forgot about that :D


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] D62732: [WIP][RISCV] Initial port of LLDB for RISC-V

[Simon Cook via Phabricator via lldb-commits]

simoncook created this revision.
simoncook added reviewers: asb, lewis-revill.
Herald added subscribers: benna, psnobl, jocewei, PkmX, rkruppe, the_o, 
brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, shiva0217, 
kito-cheng, niosHD, sabuasal, apazos, johnrusso, rbar, mgorny.
Herald added a project: LLDB.

This is the start of a LLDB port for both 32 and 64-bit RISC-V, implementing 
the components needed in order to get basic functionality working with our 
embedded debug server.

Currently the following things are working:

- RV32I/RV64I base registers
- breakpoints (assumes C extension is enabled)
- frame unwinding (if binary has CFI information)
- continue/step
- disassembly

Currently calling functions via the command line and disassembly based frame 
unwind is not yet implemented. I haven't yet looked at what tests should be 
implemented alongside this patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D62732

Files:
  lldb/include/lldb/Utility/ArchSpec.h
  lldb/source/API/SystemInitializerFull.cpp
  lldb/source/Plugins/ABI/CMakeLists.txt
  lldb/source/Plugins/ABI/SysV-riscv/ABISysV_riscv.cpp
  lldb/source/Plugins/ABI/SysV-riscv/ABISysV_riscv.h
  lldb/source/Plugins/ABI/SysV-riscv/CMakeLists.txt
  lldb/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
  lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
  lldb/source/Target/Platform.cpp
  lldb/source/Target/Thread.cpp
  lldb/source/Utility/ArchSpec.cpp

Index: lldb/source/Utility/ArchSpec.cpp
===
--- lldb/source/Utility/ArchSpec.cpp
+++ lldb/source/Utility/ArchSpec.cpp
@@ -210,6 +210,11 @@
 {eByteOrderLittle, 4, 4, 4, llvm::Triple::hexagon,
  ArchSpec::eCore_hexagon_hexagonv5, "hexagonv5"},
 
+{eByteOrderLittle, 4, 2, 4, llvm::Triple::riscv32,
+ ArchSpec::eCore_riscv32, "riscv32"},
+{eByteOrderLittle, 8, 2, 4, llvm::Triple::riscv64,
+ ArchSpec::eCore_riscv64, "riscv64"},
+
 {eByteOrderLittle, 4, 4, 4, llvm::Triple::UnknownArch,
  ArchSpec::eCore_uknownMach32, "unknown-mach-32"},
 {eByteOrderLittle, 8, 4, 4, llvm::Triple::UnknownArch,
@@ -446,6 +451,10 @@
  ArchSpec::eMIPSSubType_mips64r6el, 0xu, 0xu}, // mips64r6el
 {ArchSpec::eCore_hexagon_generic, llvm::ELF::EM_HEXAGON,
  LLDB_INVALID_CPUTYPE, 0xu, 0xu}, // HEXAGON
+{ArchSpec::eCore_riscv32, llvm::ELF::EM_RISCV, LLDB_INVALID_CPUTYPE,
+ 0xu, 0xu}, // riscv32
+{ArchSpec::eCore_riscv64, llvm::ELF::EM_RISCV, LLDB_INVALID_CPUTYPE,
+ 0xu, 0xu}, // riscv64
 };
 
 static const ArchDefinition g_elf_arch_def = {
Index: lldb/source/Target/Thread.cpp
===
--- lldb/source/Target/Thread.cpp
+++ lldb/source/Target/Thread.cpp
@@ -2070,6 +2070,8 @@
 case llvm::Triple::ppc:
 case llvm::Triple::ppc64:
 case llvm::Triple::ppc64le:
+case llvm::Triple::riscv32:
+case llvm::Triple::riscv64:
 case llvm::Triple::systemz:
 case llvm::Triple::hexagon:
   m_unwinder_up.reset(new UnwindLLDB(*this));
Index: lldb/source/Target/Platform.cpp
===
--- lldb/source/Target/Platform.cpp
+++ lldb/source/Target/Platform.cpp
@@ -1911,6 +1911,15 @@
 trap_opcode_size = sizeof(g_i386_opcode);
   } break;
 
+  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;
+  }
+
   default:
 llvm_unreachable(
 "Unhandled architecture in Platform::GetSoftwareBreakpointTrapOpcode");
Index: lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
===
--- lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
+++ lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
@@ -621,6 +621,18 @@
   }
   break;
 
+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;
+else if (strcmp(reg.name, "pc") == 0)
+  reg.kinds[eRegisterKindGeneric] = LLDB_REGNUM_GENERIC_PC;
+  }
+  break;
+
 default:
   break;
 }
Index: lldb/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
@@ -1198,6 +1198,11 @@
 cpu = "apple-latest";
   }
 
+  // For RISC-V, ena

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

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

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



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

labath wrote:
> It looks like we're not guarding the call to `LoadModules` on line 101, so I 
> think we can assume that m_process is not null here (and I don't see why it 
> should be -- it does not make sense to call `DidAttach` if there is no 
> process around).
> 
> BTW, your guarding attempt isn't even correct as we'd always call 
> `m_process->GetByteOrder()` and crash if the process was null.
oh no, this is embarrassing!



Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:549-550
 
-  if (ModuleSP module_sp = target.GetOrCreateModule(module_spec, 
-true /* notify */)) {
+  if (ModuleSP module_sp =
+  target.GetOrCreateModule(module_spec, true /* notify */)) {
 UpdateLoadedSections(module_sp, LLDB_INVALID_ADDRESS, m_interpreter_base,

labath wrote:
> Whitespace-only change. Did you maybe run clang-format on the whole file 
> instead of just the diff?
> 
> It doesn't matter that much here as there isn't many of them, but I just want 
> to make sure you're aware of this in the future.
odd, pretty sure I just run clang-format-diff (as I don't have auto formatting 
going on), will double check this.



Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:38
 
-AuxVector::AuxVector(Process *process) : m_process(process) {
-  DataExtractor data;
-  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
-
-  data.SetData(GetAuxvData());
-  data.SetByteOrder(m_process->GetByteOrder());
-  data.SetAddressByteSize(m_process->GetAddressByteSize());
-
-  ParseAuxv(data);
-
-  if (log)
-DumpToLog(log);
-}
-
-AuxVector::iterator AuxVector::FindEntry(EntryType type) const {
-  for (iterator I = begin(); I != end(); ++I) {
-if (I->type == static_cast(type))
-  return I;
-  }
-
-  return end();
+uint64_t AuxVector::GetAuxValue(enum EntryType entry_type) const {
+  auto it = m_auxv_entries.find(static_cast(entry_type));

labath wrote:
> It would be better to return an Optional (or addr_t maybe ?) 
> instead of using a magic value to mean "not found". It looks like at least 
> some of these entries can validly be zero.
Can do, I was trying to follow this api though: 
http://man7.org/linux/man-pages/man3/getauxval.3.html.
I think I'll go with the uint64_t though, I find it odd to return an addr_t 
because it's not an address, it's a number that happens to be the same size of 
an address.


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] D62499: Create a generic handler for Xfer packets

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp:125
+  [&](std::unique_ptr e) {
+result = SendErrorResponse(std::move(e));
+  });

aadsm wrote:
> labath wrote:
> > I'm a bit confused. What does this call exactly? It looks to me like this 
> > will use the implicit (we should probably make it explicit) 
> > `std::unique_ptr` constructor of `Error` to call this method 
> > again and cause infinite recursion.
> > 
> > I'd suggest doing something like 
> > `SendErrorResponse(Status(Error(std::move(e)))`. The advantage of this is 
> > that the `Status` overload knows how to send an error as string (we have an 
> > protocol extension for that), and so will provide a better error message on 
> > the client side.
> ah, not sure what I was thinking here. I remember writing 
> `SendErrorResponse(Status(Error(std::move(e)))` (or something to that effect) 
> but somehow ended up with this recursion..
> I need to force these errors to happen to make sure everything makes sense.
It doesn't look like it should be too hard to instantiate 
`GDBRemoteCommunicationServer` from a unit test.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h:83
+public:
+  using llvm::ErrorInfo::ErrorInfo; // inherit constructors

aadsm wrote:
> labath wrote:
> > You need to define `static char ID` here too. Otherwise the dynamic type 
> > detection magic will not work..
> Ah, I guess it would only match against the StringError because it's 
> inheriting that one?
I think it would match against *all* StringErrors, even those that are not 
PacketUnimplementedErrors.



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2770
   LLDB_LOG(log, "no auxv data retrieved: {0}", ec.message());
-  return SendErrorResponse(ec.value());
+  return llvm::make_error(ec.value());
 }

aadsm wrote:
> labath wrote:
> > I am wondering whether we actually need the `PacketError` class. Such a 
> > class would be useful if we actually wanted to start providing meaningful 
> > error codes to the other side (as we would give us tighter control over the 
> > allocation of error codes). However, here you're just taking random numbers 
> > and sticking them into the `PacketError` class, so I am not sure that adds 
> > any value.
> > 
> > So, what I'd do here is just delete the PacketError class, and just do a 
> > `return llvm::errorCodeToError(ec)` here. I'd also delete the log message 
> > as the error message will implicitly end up in the packet log via the error 
> > message extension (if you implement my suggestion `SendErrorResponse`).
> I thought it would be nice to have a little abstraction layer around the 
> packet errors overall. My purpose with the PacketError is to make it more 
> obvious in the code that the number given will be sent back as an error 
> number.
> I didn't realize the numbers we were using were meaningless today though (now 
> that I think of it this ec.value is really whatever GetAuxvData returns). I 
> searched at the time and found a few different numbers being used: 9, 4, 10, 
> etc. I guess these numbers are just lies then :D.
> 
Yeah, the only way you can assign meaning to these numbers today is if you open 
the source code and search for the occurrences of the given number. :)
That will be even easier if we switch to using strings. :)


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 added inline comments.



Comment at: lldb/source/Plugins/Process/Utility/AuxVector.cpp:38
 
-AuxVector::AuxVector(Process *process) : m_process(process) {
-  DataExtractor data;
-  Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
-
-  data.SetData(GetAuxvData());
-  data.SetByteOrder(m_process->GetByteOrder());
-  data.SetAddressByteSize(m_process->GetAddressByteSize());
-
-  ParseAuxv(data);
-
-  if (log)
-DumpToLog(log);
-}
-
-AuxVector::iterator AuxVector::FindEntry(EntryType type) const {
-  for (iterator I = begin(); I != end(); ++I) {
-if (I->type == static_cast(type))
-  return I;
-  }
-
-  return end();
+uint64_t AuxVector::GetAuxValue(enum EntryType entry_type) const {
+  auto it = m_auxv_entries.find(static_cast(entry_type));

aadsm wrote:
> labath wrote:
> > It would be better to return an Optional (or addr_t maybe ?) 
> > instead of using a magic value to mean "not found". It looks like at least 
> > some of these entries can validly be zero.
> Can do, I was trying to follow this api though: 
> http://man7.org/linux/man-pages/man3/getauxval.3.html.
> I think I'll go with the uint64_t though, I find it odd to return an addr_t 
> because it's not an address, it's a number that happens to be the same size 
> of an address.
Ah, interesting. It's a bit odd of an api as it does not allow you to 
distinguish AT_EUID not being present from somebody actually being root. I 
guess you're expected to assume that the AT_EUID field will always be present...

In any case, I think it would be better to use Optional, and not be tied to 
what is expressible in some C api.

I'm fine with uint64_t.


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] D62715: [NativeProcessLinux] Reuse memory read by process_vm_readv before calling ptrace

[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/Linux/NativeProcessLinux.cpp:1457
 
-bytes_read = process_vm_readv(pid, &local_iov, 1, &remote_iov, 1, 0);
-const bool success = bytes_read == size;
+auto vm_bytes_read =
+process_vm_readv(pid, &local_iov, 1, &remote_iov, 1, 0);

labath wrote:
> llvm's policy is to *not* "almost always use auto".  And it's nice to know 
> whether this returns size_t, ssize_t, or something else without looking up 
> the documentation.
Makes sense. I'll make sure to take a more careful read at 
https://llvm.org/docs/CodingStandards.html.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:1467
+  if (!success) {
+addr = reinterpret_cast(reinterpret_cast(addr) +
+vm_bytes_read);

labath wrote:
> `addr += vm_bytes_read` ?
> Since this is talking about addresses in the other process, I can't imagine 
> that casting to a `char *` is going to make this more "correct" in any way.
For some reason I thought that would increase addr by vm_bytes_read*4 or 8 but 
it's really just a uint64_t not a pointer. I'll also update 
ReadCStringFromMemory function on my other diff then, I believe I make the same 
logic there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62715



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

[Lldb-commits] [PATCH] D62715: [NativeProcessLinux] Reuse memory read by process_vm_readv before calling ptrace

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

aadsm added a comment.

That's a good idea for the tests, will look into that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62715



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

[Lldb-commits] [PATCH] D62715: [NativeProcessLinux] Reuse memory read by process_vm_readv before calling ptrace

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D62715#1525085 , @aadsm wrote:

> That's a good idea for the tests, will look into that.


Actually, it looks like getting `process_vm_readv` to fail is as simple as 
running `mprotect(..., PROT_NONE)` on the piece of memory. Here's my test 
program, in case you find it useful for anything:

  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  #include 
  
  long *mem;
  
  void child() {
assert(ptrace(PTRACE_TRACEME, 0, 0, 0) == 0);
raise(SIGSTOP);
_exit(0);
  }
  
  void parent(pid_t child) {
int status;
assert(waitpid(child, &status, __WALL) == child);
assert(WIFSTOPPED(status));
assert(WSTOPSIG(status) == SIGSTOP);
unsigned long myx;
struct iovec local;
local.iov_base = &myx;
local.iov_len = sizeof myx;
  
struct iovec remote;
remote.iov_base = mem;
remote.iov_len = sizeof myx;
  
ssize_t s = process_vm_readv(child, &local, 1, &remote, 1, 0);
fprintf(stderr, "readv: %zd (%d - %s)\n", s, errno, strerror(errno));
myx = ptrace(PTRACE_PEEKDATA, child, mem, 0);
fprintf(stderr, "peek: %lx (%d - %s)\n", myx, errno, strerror(errno));
_exit(0);
  }
  
  int main() {
mem = reinterpret_cast(mmap(nullptr, 0x1000, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS, 0, 0));
assert(mem != MAP_FAILED);
*mem = 0x424344454647;
assert(mprotect(mem, 0x1000, PROT_NONE) == 0);
  
pid_t pid = fork();
assert(pid != -1);
if (pid)
  parent(pid);
else
  child();
  }
  
  
  $ g++ a.cc
  $ ./a.out 
  readv: -1 (14 - Bad address)
  peek: 424344454647 (0 - Success)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62715



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

[Lldb-commits] [lldb] r362240 - [FormatEntity] Ignore ASCII escape sequences when colors are disabled.

[Jonas Devlieghere via lldb-commits]

Author: jdevlieghere
Date: Fri May 31 09:27:44 2019
New Revision: 362240

URL: http://llvm.org/viewvc/llvm-project?rev=362240&view=rev
Log:
[FormatEntity] Ignore ASCII escape sequences when colors are disabled.

This patch makes the FormatEntity honor the debugger's color settings by
not inserting ASCII escape sequences when colors are disabled.

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

Added:
lldb/trunk/lit/Settings/Inputs/main.c
lldb/trunk/lit/Settings/TestFrameFormatColor.test
lldb/trunk/lit/Settings/TestFrameFormatNoColor.test
Modified:
lldb/trunk/include/lldb/Core/FormatEntity.h
lldb/trunk/source/Core/FormatEntity.cpp

Modified: lldb/trunk/include/lldb/Core/FormatEntity.h
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/FormatEntity.h?rev=362240&r1=362239&r2=362240&view=diff
==
--- lldb/trunk/include/lldb/Core/FormatEntity.h (original)
+++ lldb/trunk/include/lldb/Core/FormatEntity.h Fri May 31 09:27:44 2019
@@ -41,7 +41,7 @@ public:
   Invalid,
   ParentNumber,
   ParentString,
-  InsertString,
+  EscapeCode,
   Root,
   String,
   Scope,

Added: lldb/trunk/lit/Settings/Inputs/main.c
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Settings/Inputs/main.c?rev=362240&view=auto
==
--- lldb/trunk/lit/Settings/Inputs/main.c (added)
+++ lldb/trunk/lit/Settings/Inputs/main.c Fri May 31 09:27:44 2019
@@ -0,0 +1,2 @@
+int foo() { return 0; }
+int main() { return foo(); }

Added: lldb/trunk/lit/Settings/TestFrameFormatColor.test
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Settings/TestFrameFormatColor.test?rev=362240&view=auto
==
--- lldb/trunk/lit/Settings/TestFrameFormatColor.test (added)
+++ lldb/trunk/lit/Settings/TestFrameFormatColor.test Fri May 31 09:27:44 2019
@@ -0,0 +1,12 @@
+# RUN: %clang -g -O0 %S/Inputs/main.c -o %t.out
+# RUN: %lldb -x -b -s %s %t.out | FileCheck %s
+settings set use-color true
+settings set -f frame-format "frame #${frame.index}: 
\`${ansi.fg.green}{${function.name-with-args}${ansi.normal}\n"
+b foo
+run
+bt
+c
+q
+
+# Check the ASCII escape code
+# CHECK: 

Added: lldb/trunk/lit/Settings/TestFrameFormatNoColor.test
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/lit/Settings/TestFrameFormatNoColor.test?rev=362240&view=auto
==
--- lldb/trunk/lit/Settings/TestFrameFormatNoColor.test (added)
+++ lldb/trunk/lit/Settings/TestFrameFormatNoColor.test Fri May 31 09:27:44 2019
@@ -0,0 +1,12 @@
+# RUN: %clang -g -O0 %S/Inputs/main.c -o %t.out
+# RUN: %lldb -x -b -s %s %t.out | FileCheck %s
+settings set use-color false
+settings set -f frame-format "frame #${frame.index}: 
\`${ansi.fg.green}{${function.name-with-args}${ansi.normal}\n"
+b foo
+run
+bt
+c
+q
+
+# Check the ASCII escape code
+# CHECK-NOT: 

Modified: lldb/trunk/source/Core/FormatEntity.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Core/FormatEntity.cpp?rev=362240&r1=362239&r2=362240&view=diff
==
--- lldb/trunk/source/Core/FormatEntity.cpp (original)
+++ lldb/trunk/source/Core/FormatEntity.cpp Fri May 31 09:27:44 2019
@@ -94,7 +94,7 @@ enum FileKind { FileError = 0, Basename,
 static_cast(llvm::array_lengthof(c)), c, true
\
   }
 #define ENTRY_STRING(n, s) 
\
-  { n, s, FormatEntity::Entry::Type::InsertString, 0, 0, nullptr, false }
+  { n, s, FormatEntity::Entry::Type::EscapeCode, 0, 0, nullptr, false }
 static FormatEntity::Entry::Definition g_string_entry[] = {
 ENTRY("*", ParentString)};
 
@@ -307,7 +307,7 @@ const char *FormatEntity::Entry::TypeToC
 ENUM_TO_CSTR(Invalid);
 ENUM_TO_CSTR(ParentNumber);
 ENUM_TO_CSTR(ParentString);
-ENUM_TO_CSTR(InsertString);
+ENUM_TO_CSTR(EscapeCode);
 ENUM_TO_CSTR(Root);
 ENUM_TO_CSTR(String);
 ENUM_TO_CSTR(Scope);
@@ -1102,8 +1102,17 @@ bool FormatEntity::Format(const Entry &e
   // FormatEntity::Entry::Definition encoding
   case Entry::Type::ParentString: // Only used for
   // FormatEntity::Entry::Definition encoding
-  case Entry::Type::InsertString: // Only used for
-  // FormatEntity::Entry::Definition encoding
+return false;
+  case Entry::Type::EscapeCode:
+if (exe_ctx) {
+  if (Target *target = exe_ctx->GetTargetPtr()) {
+Debugger &debugger = target->GetDebugger();
+if (debugger.GetUseColor()) {
+  s.PutCString(entry.string);
+  return true;
+}
+  }
+}
 return false;
 
   case Entry::Type::Root:
@@ -1896,7

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

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362240: [FormatEntity] Ignore ASCII escape sequences when 
colors are disabled. (authored by JDevlieghere, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62714?vs=202354&id=202452#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62714

Files:
  lldb/trunk/include/lldb/Core/FormatEntity.h
  lldb/trunk/lit/Settings/Inputs/main.c
  lldb/trunk/lit/Settings/TestFrameFormatColor.test
  lldb/trunk/lit/Settings/TestFrameFormatNoColor.test
  lldb/trunk/source/Core/FormatEntity.cpp

Index: lldb/trunk/source/Core/FormatEntity.cpp
===
--- lldb/trunk/source/Core/FormatEntity.cpp
+++ lldb/trunk/source/Core/FormatEntity.cpp
@@ -94,7 +94,7 @@
 static_cast(llvm::array_lengthof(c)), c, true\
   }
 #define ENTRY_STRING(n, s) \
-  { n, s, FormatEntity::Entry::Type::InsertString, 0, 0, nullptr, false }
+  { n, s, FormatEntity::Entry::Type::EscapeCode, 0, 0, nullptr, false }
 static FormatEntity::Entry::Definition g_string_entry[] = {
 ENTRY("*", ParentString)};
 
@@ -307,7 +307,7 @@
 ENUM_TO_CSTR(Invalid);
 ENUM_TO_CSTR(ParentNumber);
 ENUM_TO_CSTR(ParentString);
-ENUM_TO_CSTR(InsertString);
+ENUM_TO_CSTR(EscapeCode);
 ENUM_TO_CSTR(Root);
 ENUM_TO_CSTR(String);
 ENUM_TO_CSTR(Scope);
@@ -1102,8 +1102,17 @@
   // FormatEntity::Entry::Definition encoding
   case Entry::Type::ParentString: // Only used for
   // FormatEntity::Entry::Definition encoding
-  case Entry::Type::InsertString: // Only used for
-  // FormatEntity::Entry::Definition encoding
+return false;
+  case Entry::Type::EscapeCode:
+if (exe_ctx) {
+  if (Target *target = exe_ctx->GetTargetPtr()) {
+Debugger &debugger = target->GetDebugger();
+if (debugger.GetUseColor()) {
+  s.PutCString(entry.string);
+  return true;
+}
+  }
+}
 return false;
 
   case Entry::Type::Root:
@@ -1896,7 +1905,7 @@
 entry.number = entry_def->data;
 return error; // Success
 
-  case FormatEntity::Entry::Type::InsertString:
+  case FormatEntity::Entry::Type::EscapeCode:
 entry.type = entry_def->type;
 entry.string = entry_def->string;
 return error; // Success
@@ -2265,13 +2274,7 @@
   return error;
 }
   }
-  // Check if this entry just wants to insert a constant string value
-  // into the parent_entry, if so, insert the string with AppendText,
-  // else append the entry to the parent_entry.
-  if (entry.type == Entry::Type::InsertString)
-parent_entry.AppendText(entry.string.c_str());
-  else
-parent_entry.AppendEntry(std::move(entry));
+  parent_entry.AppendEntry(std::move(entry));
 }
   }
   break;
Index: lldb/trunk/lit/Settings/Inputs/main.c
===
--- lldb/trunk/lit/Settings/Inputs/main.c
+++ lldb/trunk/lit/Settings/Inputs/main.c
@@ -0,0 +1,2 @@
+int foo() { return 0; }
+int main() { return foo(); }
Index: lldb/trunk/lit/Settings/TestFrameFormatColor.test
===
--- lldb/trunk/lit/Settings/TestFrameFormatColor.test
+++ lldb/trunk/lit/Settings/TestFrameFormatColor.test
@@ -0,0 +1,12 @@
+# RUN: %clang -g -O0 %S/Inputs/main.c -o %t.out
+# RUN: %lldb -x -b -s %s %t.out | FileCheck %s
+settings set use-color true
+settings set -f frame-format "frame #${frame.index}: \`${ansi.fg.green}{${function.name-with-args}${ansi.normal}\n"
+b foo
+run
+bt
+c
+q
+
+# Check the ASCII escape code
+# CHECK: 
Index: lldb/trunk/lit/Settings/TestFrameFormatNoColor.test
===
--- lldb/trunk/lit/Settings/TestFrameFormatNoColor.test
+++ lldb/trunk/lit/Settings/TestFrameFormatNoColor.test
@@ -0,0 +1,12 @@
+# RUN: %clang -g -O0 %S/Inputs/main.c -o %t.out
+# RUN: %lldb -x -b -s %s %t.out | FileCheck %s
+settings set use-color false
+settings set -f frame-format "frame #${frame.index}: \`${ansi.fg.green}{${function.name-with-args}${ansi.normal}\n"
+b foo
+run
+bt
+c
+q
+
+# Check the ASCII escape code
+# CHECK-NOT: 
Index: lldb/trunk/include/lldb/Core/FormatEntity.h
===
--- lldb/trunk/include/lldb/Core/FormatEntity.h
+++ lldb/trunk/include/lldb/Core/FormatEntity.h
@@ -41,7 +41,7 @@
   Invalid,
   ParentNumber,
   ParentString,
-  InsertString,
+  EscapeCode,
   Root,
   String,
   Scope,

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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:94
+  m_process->GetAddressByteSize());
+  m_auxv.reset(new AuxVector(auxv_data));
   if (log)

`llvm::make_unique< AuxVector>(auxv_data);`



Comment at: 
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:631
 void DynamicLoaderPOSIXDYLD::EvalSpecialModulesStatus() {
-  auto I = m_auxv->FindEntry(AuxVector::AUXV_AT_SYSINFO_EHDR);
-  if (I != m_auxv->end() && I->value != 0)
-m_vdso_base = I->value;
+  auto vdso_base = m_auxv->GetAuxValue(AuxVector::AUXV_AT_SYSINFO_EHDR);
+  if (vdso_base != 0)

This shouldn't be `auto` anymore, as it's not obvious what the type is. Same 
below.



Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.h:105
   }
+  uint64_t GetAuxValue(enum AuxVector::EntryType type);
 

Newline before the function to keep things consistent.


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] D62743: Add color to the default thread and frame format.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere created this revision.
JDevlieghere added reviewers: clayborg, aprantl, teemperor, labath.
Herald added a project: LLDB.

Now that we correctly ignore ASCII escape sequences (r 362240) when colors are 
disabled, I'd like to change the the default frame and thread format to include 
color in their output, in line with the syntax highlighting that Raphael added 
a while ago. More specifically, I want to highlight the stop reason and the 
file + line/column number. With colors disabled, this of course is a no-op.

Please see the following screenshot for example output: 
https://i.imgur.com/KRZhxSz.png


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D62743

Files:
  lldb/source/Core/Debugger.cpp


Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -121,7 +121,10 @@
   "{${frame.no-debug}${function.pc-offset"
 
 #define FILE_AND_LINE  
\
-  "{ at ${line.file.basename}:${line.number}{:${line.column}}}"
+  "{ at ${ansi.fg.yellow}" 
\
+  "${line.file.basename}:${line.number}{:${line.column}"   
\
+  "}${ansi.normal}}"
+
 #define IS_OPTIMIZED "{${function.is-optimized} [opt]}"
 
 #define IS_ARTIFICIAL "{${frame.is-artificial} [artificial]}"
@@ -133,7 +136,7 @@
   "{, queue = '${thread.queue}'}"  
\
   "{, activity = '${thread.info.activity.name}'}"  
\
   "{, ${thread.info.trace_messages} messages}" 
\
-  "{, stop reason = ${thread.stop-reason}}"
\
+  "{, stop reason = ${ansi.fg.red}${thread.stop-reason}${ansi.normal}}"
\
   "{\\nReturn value: ${thread.return-value}}"  
\
   "{\\nCompleted expression: ${thread.completed-expression}}"  
\
   "\\n"
@@ -143,7 +146,7 @@
   "{, queue = '${thread.queue}'}"  
\
   "{, activity = '${thread.info.activity.name}'}"  
\
   "{, ${thread.info.trace_messages} messages}" 
\
-  "{, stop reason = ${thread.stop-reason}}"
\
+  "{, stop reason = ${ansi.fg.red}${thread.stop-reason}${ansi.normal}}"
\
   "{\\nReturn value: ${thread.return-value}}"  
\
   "{\\nCompleted expression: ${thread.completed-expression}}"  
\
   "\\n"


Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -121,7 +121,10 @@
   "{${frame.no-debug}${function.pc-offset"
 
 #define FILE_AND_LINE  \
-  "{ at ${line.file.basename}:${line.number}{:${line.column}}}"
+  "{ at ${ansi.fg.yellow}" \
+  "${line.file.basename}:${line.number}{:${line.column}"   \
+  "}${ansi.normal}}"
+
 #define IS_OPTIMIZED "{${function.is-optimized} [opt]}"
 
 #define IS_ARTIFICIAL "{${frame.is-artificial} [artificial]}"
@@ -133,7 +136,7 @@
   "{, queue = '${thread.queue}'}"  \
   "{, activity = '${thread.info.activity.name}'}"  \
   "{, ${thread.info.trace_messages} messages}" \
-  "{, stop reason = ${thread.stop-reason}}"\
+  "{, stop reason = ${ansi.fg.red}${thread.stop-reason}${ansi.normal}}"\
   "{\\nReturn value: ${thread.return-value}}"  \
   "{\\nCompleted expression: ${thread.completed-expression}}"  \
   "\\n"
@@ -143,7 +146,7 @@
   "{, queue = '${thread.queue}'}"  \
   "{, activity = '${thread.info.activity.name}'}"  \
   "{, ${thread.info.trace_messages} messages}" \
-  "{, stop reason = ${thread.stop-reason}}"\
+  "{, stop reason = ${ansi.fg.red}${thread.stop-reason}${ansi.normal}}"\
   "{\\nReturn value: ${thread.return-value}}"  \
   "{\\nCompleted expression: ${thread.completed-expression}}"  \
   "\\n"
___
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.

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

+1. I'm (obviously) in favor of this.


Repository:
  rLLDB LLDB

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] [lldb] r362259 - [Target] Remove ABI's dependence on ExpressionParser

[Alex Langford via lldb-commits]

Author: xiaobai
Date: Fri May 31 13:17:21 2019
New Revision: 362259

URL: http://llvm.org/viewvc/llvm-project?rev=362259&view=rev
Log:
[Target] Remove ABI's dependence on ExpressionParser

Modified:
lldb/trunk/source/Target/ABI.cpp

Modified: lldb/trunk/source/Target/ABI.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/ABI.cpp?rev=362259&r1=362258&r2=362259&view=diff
==
--- lldb/trunk/source/Target/ABI.cpp (original)
+++ lldb/trunk/source/Target/ABI.cpp Fri May 31 13:17:21 2019
@@ -7,10 +7,10 @@
 
//===--===//
 
 #include "lldb/Target/ABI.h"
-#include "Plugins/ExpressionParser/Clang/ClangPersistentVariables.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/Value.h"
 #include "lldb/Core/ValueObjectConstResult.h"
+#include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Symbol/CompilerType.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Target/Target.h"
@@ -142,16 +142,16 @@ ValueObjectSP ABI::GetReturnValueObject(
 case Value::eValueTypeScalar:
 case Value::eValueTypeVector:
   clang_expr_variable_sp->m_flags |=
-  ClangExpressionVariable::EVIsFreezeDried;
+  ExpressionVariable::EVIsFreezeDried;
   clang_expr_variable_sp->m_flags |=
-  ClangExpressionVariable::EVIsLLDBAllocated;
+  ExpressionVariable::EVIsLLDBAllocated;
   clang_expr_variable_sp->m_flags |=
-  ClangExpressionVariable::EVNeedsAllocation;
+  ExpressionVariable::EVNeedsAllocation;
   break;
 case Value::eValueTypeLoadAddress:
   clang_expr_variable_sp->m_live_sp = live_valobj_sp;
   clang_expr_variable_sp->m_flags |=
-  ClangExpressionVariable::EVIsProgramReference;
+  ExpressionVariable::EVIsProgramReference;
   break;
 }
 


___
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

[Saleem Abdulrasool via Phabricator via lldb-commits]

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

Actually, I think that we should extend `CompilerType` and `TypeSystem` to 
expose Clang's knowledge of whether a type is passed in a register by means of 
using `clang::RecordDecl::isPassInRegisters`


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62702



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

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

[Jim Ingham via lldb-commits]

There was a plan - which Adrian may remember more about - to have some API from 
clang where you could pass in a function signature and have it return dwarf 
expressions for the locations of all the input parameters, and of the return 
value directly on exit.  That would be the handiest form, dwarf expressions 
should be up to the task even for complex return types, and they are a form 
both sides know how to handle.  Then the whole of this "GetReturnValue" on the 
lldb side would be "call this API and and run the dwarf expression to fetch the 
value."  All the knowledge of how where values are returned would then be 
properly in clang/llvm.

I don't know what the status of this design is, however...

Jim


> On May 31, 2019, at 2:41 PM, Saleem Abdulrasool via Phabricator 
>  wrote:
> 
> compnerd requested changes to this revision.
> compnerd added a comment.
> This revision now requires changes to proceed.
> 
> Actually, I think that we should extend `CompilerType` and `TypeSystem` to 
> expose Clang's knowledge of whether a type is passed in a register by means 
> of using `clang::RecordDecl::isPassInRegisters`
> 
> 
> Repository:
>  rLLDB LLDB
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D62702/new/
> 
> https://reviews.llvm.org/D62702
> 
> 
> 

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

In D62702#1525655 , @compnerd wrote:

> Actually, I think that we should extend `CompilerType` and `TypeSystem` to 
> expose Clang's knowledge of whether a type is passed in a register by means 
> of using `clang::RecordDecl::isPassInRegisters`


Can't agree more. Thanks!!


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D62702



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

[Lldb-commits] [lldb] r362268 - [Commands] Stop hardcoding languages in CommandObjectType

[Alex Langford via lldb-commits]

Author: xiaobai
Date: Fri May 31 15:15:29 2019
New Revision: 362268

URL: http://llvm.org/viewvc/llvm-project?rev=362268&view=rev
Log:
[Commands] Stop hardcoding languages in CommandObjectType

Modified:
lldb/trunk/source/Commands/CommandObjectType.cpp

Modified: lldb/trunk/source/Commands/CommandObjectType.cpp
URL: 
http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Commands/CommandObjectType.cpp?rev=362268&r1=362267&r2=362268&view=diff
==
--- lldb/trunk/source/Commands/CommandObjectType.cpp (original)
+++ lldb/trunk/source/Commands/CommandObjectType.cpp Fri May 31 15:15:29 2019
@@ -2806,17 +2806,11 @@ public:
   return m_cmd_help_long;
 
 StreamString stream;
-// FIXME: hardcoding languages is not good
-lldb::LanguageType languages[] = {eLanguageTypeObjC,
-  eLanguageTypeC_plus_plus};
-
-for (const auto lang_type : languages) {
-  if (auto language = Language::FindPlugin(lang_type)) {
-if (const char *help = language->GetLanguageSpecificTypeLookupHelp()) {
-  stream.Printf("%s\n", help);
-}
-  }
-}
+Language::ForEach([&](Language *lang) {
+  if (const char *help = lang->GetLanguageSpecificTypeLookupHelp())
+stream.Printf("%s\n", help);
+  return true;
+});
 
 m_cmd_help_long = stream.GetString();
 return m_cmd_help_long;
@@ -2852,9 +2846,10 @@ public:
 
 if ((is_global_search =
  (m_command_options.m_language == eLanguageTypeUnknown))) {
-  // FIXME: hardcoding languages is not good
-  languages.push_back(Language::FindPlugin(eLanguageTypeObjC));
-  languages.push_back(Language::FindPlugin(eLanguageTypeC_plus_plus));
+  Language::ForEach([&](Language *lang) {
+languages.push_back(lang);
+return true;
+  });
 } else {
   languages.push_back(Language::FindPlugin(m_command_options.m_language));
 }


___
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 created this revision.
xiaobai added reviewers: compnerd, davide, JDevlieghere, jingham, clayborg, 
labath, aprantl.

I want to remove this method because I think that Process should be
language agnostic, or at least, not have knowledge about specific language
runtimes. There is "GetLanguageRuntime()" which should be used instead. If the
caller a CPPLanguageRuntime, they should cast it as needed. Ideally, this
should only happen in plugins that need C++ specific knowledge.

The next step I would like to do is remove "GetObjCLanguageRuntime()" as well.
There are a lot more instances of that function being used, so I wanted to
upload this one first to get the general reception to this idea.


https://reviews.llvm.org/D62755

Files:
  include/lldb/Target/Process.h
  source/Plugins/Language/CPlusPlus/LibCxx.cpp
  source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
  source/Target/Process.cpp


Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -1598,16 +1598,6 @@
   return runtime;
 }
 
-CPPLanguageRuntime *Process::GetCPPLanguageRuntime(bool retry_if_null) {
-  std::lock_guard guard(m_language_runtimes_mutex);
-  LanguageRuntime *runtime =
-  GetLanguageRuntime(eLanguageTypeC_plus_plus, retry_if_null);
-  if (!runtime)
-return nullptr;
-
-  return static_cast(runtime);
-}
-
 ObjCLanguageRuntime *Process::GetObjCLanguageRuntime(bool retry_if_null) {
   std::lock_guard guard(m_language_runtimes_mutex);
   LanguageRuntime *runtime =
Index: source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
===
--- source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
+++ source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
@@ -462,7 +462,8 @@
 
 ValueObjectSP AppleObjCRuntime::GetExceptionObjectForThread(
 ThreadSP thread_sp) {
-  auto cpp_runtime = m_process->GetCPPLanguageRuntime();
+  auto *cpp_runtime = static_cast(
+  m_process->GetLanguageRuntime(eLanguageTypeC_plus_plus));
   if (!cpp_runtime) return ValueObjectSP();
   auto cpp_exception = cpp_runtime->GetExceptionObjectForThread(thread_sp);
   if (!cpp_exception) return ValueObjectSP();
Index: source/Plugins/Language/CPlusPlus/LibCxx.cpp
===
--- source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -67,7 +67,8 @@
   if (process == nullptr)
 return false;
 
-  CPPLanguageRuntime *cpp_runtime = process->GetCPPLanguageRuntime();
+  CPPLanguageRuntime *cpp_runtime = static_cast(
+  process->GetLanguageRuntime(eLanguageTypeC_plus_plus));
 
   if (!cpp_runtime)
 return false;
Index: include/lldb/Target/Process.h
===
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -2184,8 +2184,6 @@
   LanguageRuntime *GetLanguageRuntime(lldb::LanguageType language,
   bool retry_if_null = true);
 
-  CPPLanguageRuntime *GetCPPLanguageRuntime(bool retry_if_null = true);
-
   ObjCLanguageRuntime *GetObjCLanguageRuntime(bool retry_if_null = true);
 
   bool IsPossibleDynamicValue(ValueObject &in_value);


Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -1598,16 +1598,6 @@
   return runtime;
 }
 
-CPPLanguageRuntime *Process::GetCPPLanguageRuntime(bool retry_if_null) {
-  std::lock_guard guard(m_language_runtimes_mutex);
-  LanguageRuntime *runtime =
-  GetLanguageRuntime(eLanguageTypeC_plus_plus, retry_if_null);
-  if (!runtime)
-return nullptr;
-
-  return static_cast(runtime);
-}
-
 ObjCLanguageRuntime *Process::GetObjCLanguageRuntime(bool retry_if_null) {
   std::lock_guard guard(m_language_runtimes_mutex);
   LanguageRuntime *runtime =
Index: source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
===
--- source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
+++ source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
@@ -462,7 +462,8 @@
 
 ValueObjectSP AppleObjCRuntime::GetExceptionObjectForThread(
 ThreadSP thread_sp) {
-  auto cpp_runtime = m_process->GetCPPLanguageRuntime();
+  auto *cpp_runtime = static_cast(
+  m_process->GetLanguageRuntime(eLanguageTypeC_plus_plus));
   if (!cpp_runtime) return ValueObjectSP();
   auto cpp_exception = cpp_runtime->GetExceptionObjectForThread(thread_sp);
   if (!cpp_exception) return ValueObjectSP();
Index: source/Plugins/Language/CPlusPlus/LibCxx.cpp
===
--- source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ source/Plugins/L

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

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

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



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2770
   LLDB_LOG(log, "no auxv data retrieved: {0}", ec.message());
-  return SendErrorResponse(ec.value());
+  return llvm::make_error(ec.value());
 }

labath wrote:
> aadsm wrote:
> > labath wrote:
> > > I am wondering whether we actually need the `PacketError` class. Such a 
> > > class would be useful if we actually wanted to start providing meaningful 
> > > error codes to the other side (as we would give us tighter control over 
> > > the allocation of error codes). However, here you're just taking random 
> > > numbers and sticking them into the `PacketError` class, so I am not sure 
> > > that adds any value.
> > > 
> > > So, what I'd do here is just delete the PacketError class, and just do a 
> > > `return llvm::errorCodeToError(ec)` here. I'd also delete the log message 
> > > as the error message will implicitly end up in the packet log via the 
> > > error message extension (if you implement my suggestion 
> > > `SendErrorResponse`).
> > I thought it would be nice to have a little abstraction layer around the 
> > packet errors overall. My purpose with the PacketError is to make it more 
> > obvious in the code that the number given will be sent back as an error 
> > number.
> > I didn't realize the numbers we were using were meaningless today though 
> > (now that I think of it this ec.value is really whatever GetAuxvData 
> > returns). I searched at the time and found a few different numbers being 
> > used: 9, 4, 10, etc. I guess these numbers are just lies then :D.
> > 
> Yeah, the only way you can assign meaning to these numbers today is if you 
> open the source code and search for the occurrences of the given number. :)
> That will be even easier if we switch to using strings. :)
But we can't return strings on the protocol though, it will have to be a `E NN`.
I'm going with your suggestions but how about this in a future patch:

Have a base `PacketError(num, message)` and then subclass that one with the 
different errors we have like `NoProcessAvailablePacketError()` that would code 
their own error number and message (or maybe we just need to have static 
functions like PacketError::createNoProcessAvailableError()?).

Then, on the client side we could add an `Optional 
GetResponseError()` to `StringExtractorGDBRemote` that would create the right 
packet error given the number, so we can print a descriptive error message on 
the lldb terminal. (maybe GDBResponseError instead of PacketError...)


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] D62756: Be consistent when adding names and mangled names to the index

[Greg Clayton via Phabricator via lldb-commits]

clayborg created this revision.
clayborg added reviewers: labath, aprantl, JDevlieghere, jingham, jasonmolenda.
Herald added a subscriber: arphaman.

We were being very inconsistent with the way we were adding names to the 
indexes.

For DW_TAG_subprogram and DW_TAG_inlined_subroutine we would not add the 
mangled name if the we didn't have a simple name. We would only add the mangled 
name if started with '_' (even if it matches the simple name that started with 
'_') or if the name differed if the mangled name didn't start with '_'.

For DW_TAG_array_type, DW_TAG_base_type, DW_TAG_class_type, DW_TAG_constant, 
DW_TAG_enumeration_type, DW_TAG_string_type, DW_TAG_structure_type, 
DW_TAG_subroutine_type, DW_TAG_typedef, DW_TAG_union_type, and 
DW_TAG_unspecified_type we would add the simple name and the mangled name if 
either existed, even if they were the same..

For DW_TAG_variable we would only follow the same rule as the DW_TAG_subprogram 
and DW_TAG_inlined_subroutine (so no simple name meant we wouldn't add the 
mangled name).

Not sure if this was on purpose or not, but wanted to post this diff for 
discussion on what the right thing to do should be.


https://reviews.llvm.org/D62756

Files:
  source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp


Index: source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -110,6 +110,16 @@
   }
 }
 
+static inline bool AddMangled(const char *name, const char *mangled) {
+  if (mangled == nullptr || name == mangled)
+return false;
+  if (name == nullptr)
+return true;
+  if (mangled[0] != name[0])
+return true;
+  return ::strcmp(name, mangled) != 0;
+}
+
 void ManualDWARFIndex::IndexUnitImpl(
 DWARFUnit &unit, const LanguageType cu_language,
 const dw_offset_t cu_offset, IndexSet &set) {
@@ -281,17 +291,9 @@
   if (!is_method && !mangled_cstr && !objc_method.IsValid(true))
 set.function_fullnames.Insert(ConstString(name), ref);
 }
-if (mangled_cstr) {
-  // Make sure our mangled name isn't the same string table entry as
-  // our name. If it starts with '_', then it is ok, else compare the
-  // string to make sure it isn't the same and we don't end up with
-  // duplicate entries
-  if (name && name != mangled_cstr &&
-  ((mangled_cstr[0] == '_') ||
-   (::strcmp(name, mangled_cstr) != 0))) {
-set.function_fullnames.Insert(ConstString(mangled_cstr), ref);
-  }
-}
+// Make sure our mangled name isn't the same as our name.
+if (AddMangled(name, mangled_cstr))
+ set.function_fullnames.Insert(ConstString(mangled_cstr), ref);
   }
   break;
 
@@ -306,10 +308,13 @@
 case DW_TAG_typedef:
 case DW_TAG_union_type:
 case DW_TAG_unspecified_type:
-  if (name && !is_declaration)
-set.types.Insert(ConstString(name), ref);
-  if (mangled_cstr && !is_declaration)
-set.types.Insert(ConstString(mangled_cstr), ref);
+  if (!is_declaration) {
+if (name)
+  set.types.Insert(ConstString(name), ref);
+// Make sure our mangled name isn't the same as our name.
+if (AddMangled(name, mangled_cstr))
+  set.types.Insert(ConstString(mangled_cstr), ref);
+  }
   break;
 
 case DW_TAG_namespace:
@@ -318,21 +323,17 @@
   break;
 
 case DW_TAG_variable:
-  if (name && has_location_or_const_value && is_global_or_static_variable) 
{
-set.globals.Insert(ConstString(name), ref);
+  if (has_location_or_const_value && is_global_or_static_variable) {
+if (name)
+  set.globals.Insert(ConstString(name), ref);
 // Be sure to include variables by their mangled and demangled names if
 // they have any since a variable can have a basename "i", a mangled
 // named "_ZN12_GLOBAL__N_11iE" and a demangled mangled name
 // "(anonymous namespace)::i"...
 
-// Make sure our mangled name isn't the same string table entry as our
-// name. If it starts with '_', then it is ok, else compare the string
-// to make sure it isn't the same and we don't end up with duplicate
-// entries
-if (mangled_cstr && name != mangled_cstr &&
-((mangled_cstr[0] == '_') || (::strcmp(name, mangled_cstr) != 0))) 
{
+// Make sure our mangled name isn't the same as our name.
+if (AddMangled(name, mangled_cstr))
   set.globals.Insert(ConstString(mangled_cstr), ref);
-}
   }
   break;
 


Index: source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -110,

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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Let the bike-shedding begin!
Feel free to ignore this, but personally, I'd only highlight the filename (or 
highlight the the line number in a different color).
Feel free to ignore this even more, but we should probably have symbolic color 
names instead of hardcoding red and such, for users with black-on-white vs. 
white-on-black (or green-on-black) terminals.


Repository:
  rLLDB LLDB

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] D62756: Be consistent when adding names and mangled names to the index

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



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;

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.


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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: source/Target/Process.cpp:1601
 
-CPPLanguageRuntime *Process::GetCPPLanguageRuntime(bool retry_if_null) {
-  std::lock_guard guard(m_language_runtimes_mutex);

so what about this mutex at the new call sites?


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 requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

This seems like carrying purity a little too far.  You haven't removed the fact 
that the using code is explicitly dialing up the C++ language runtime, you've 
just made the call-site a little harder to read.

I don't see any problem with having explicit accessors for the commonly used 
language runtimes.


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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

I tend to agree with Jim's comment.

> There is "GetLanguageRuntime()" which should be used instead.

Can you explain why that is?


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] D62759: Fix lit tests on Windows related to CR

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth created this revision.
amccarth added a reviewer: jasonmolenda.

Problem discovered in the breakpoint lit test, but probably exists in others.

lldb-test splits lines on LF.  Input files that are CR+LF separated (as is 
common on Windows) then resulted in commands being sent to LLDB that ended in 
CR, which confused the command interpreter.

This could be fixed at different levels:

1. Treat '\r' like a tab or space in the argument splitter.

2. Fix the line splitters (plural) in lldb-test.

3. Normalize the test files to LF only.

If we did only 3, I'd expect similar problems to recur, so this patch does 1 
and 2.  I may also do 3 in a separate patch later, but that's tricky because I 
believe we have some input files that MUST use CR+LF.


https://reviews.llvm.org/D62759

Files:
  lldb/source/Utility/Args.cpp
  lldb/tools/lldb-test/lldb-test.cpp


Index: lldb/tools/lldb-test/lldb-test.cpp
===
--- lldb/tools/lldb-test/lldb-test.cpp
+++ lldb/tools/lldb-test/lldb-test.cpp
@@ -312,7 +312,7 @@
   while (!Rest.empty()) {
 StringRef Line;
 std::tie(Line, Rest) = Rest.split('\n');
-Line = Line.ltrim();
+Line = Line.ltrim().rtrim();
 if (Line.empty() || Line[0] == '#')
   continue;
 
@@ -939,7 +939,7 @@
   while (!Rest.empty()) {
 StringRef Line;
 std::tie(Line, Rest) = Rest.split('\n');
-Line = Line.ltrim();
+Line = Line.ltrim().rtrim();
 
 if (Line.empty() || Line[0] == '#')
   continue;
Index: lldb/source/Utility/Args.cpp
===
--- lldb/source/Utility/Args.cpp
+++ lldb/source/Utility/Args.cpp
@@ -95,7 +95,7 @@
   bool arg_complete = false;
   do {
 // Skip over over regular characters and append them.
-size_t regular = command.find_first_of(" \t\"'`\\");
+size_t regular = command.find_first_of(" \t\r\"'`\\");
 arg += command.substr(0, regular);
 command = command.substr(regular);
 
@@ -123,6 +123,7 @@
 
 case ' ':
 case '\t':
+case '\r':
   // We are not inside any quotes, we just found a space after an argument.
   // We are done.
   arg_complete = true;


Index: lldb/tools/lldb-test/lldb-test.cpp
===
--- lldb/tools/lldb-test/lldb-test.cpp
+++ lldb/tools/lldb-test/lldb-test.cpp
@@ -312,7 +312,7 @@
   while (!Rest.empty()) {
 StringRef Line;
 std::tie(Line, Rest) = Rest.split('\n');
-Line = Line.ltrim();
+Line = Line.ltrim().rtrim();
 if (Line.empty() || Line[0] == '#')
   continue;
 
@@ -939,7 +939,7 @@
   while (!Rest.empty()) {
 StringRef Line;
 std::tie(Line, Rest) = Rest.split('\n');
-Line = Line.ltrim();
+Line = Line.ltrim().rtrim();
 
 if (Line.empty() || Line[0] == '#')
   continue;
Index: lldb/source/Utility/Args.cpp
===
--- lldb/source/Utility/Args.cpp
+++ lldb/source/Utility/Args.cpp
@@ -95,7 +95,7 @@
   bool arg_complete = false;
   do {
 // Skip over over regular characters and append them.
-size_t regular = command.find_first_of(" \t\"'`\\");
+size_t regular = command.find_first_of(" \t\r\"'`\\");
 arg += command.substr(0, regular);
 command = command.substr(regular);
 
@@ -123,6 +123,7 @@
 
 case ' ':
 case '\t':
+case '\r':
   // We are not inside any quotes, we just found a space after an argument.
   // We are done.
   arg_complete = true;
___
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#1525790 , @jingham wrote:

> This seems like carrying purity a little too far.


I disagree that it is "carrying purity a little too far". My goal is to see 
LLDB's non-plugin libraries be more language agnostic.
I have two motivations for this:

1. Better language support in LLDB, making it easier to add support for new 
languages in a clean fashion.
2. Shrinking the size of lldb-server. Though the lldb on llvm.org doesn't 
suffer from this problem too much, swift-lldb has an issue where the 
lldb-server you build is incredibly bloated. On Linux I am measuring about 
162MB and on android I measured 61MB. It is incredibly difficult to shrink the 
size without first decoupling components. Because swift-lldb is a downstream 
project, I want to make sure that the abstractions there make sense with what 
is on llvm.org, so I am doing this work here first.

> You haven't removed the fact that the using code is explicitly dialing up the 
> C++ language runtime, you've just made the call-site a little harder to read.

Right, that was not an explicit goal. There are legitimate instances where you 
need to get the C++ language runtime, I'm not saying that this is a bad thing. 
I'm saying that if you need a CPPLanguageRuntime from your process, you should 
use `GetLanguageRuntime()` with LanguageType being eLanguageTypeC_plus_plus. If 
you need to call a method from CPPLanguageRuntime (or one its derived classes) 
that isn't a part of the LanguageRuntime interface, then you should explicitly 
cast it. I also think that this should happen in Plugins that need details 
about the C++ language runtime explicitly and not in non-plugin libraries.

> I don't see any problem with having explicit accessors for the commonly used 
> language runtimes.

Language runtimes are supposed to be plugins. I don't think Process needs to 
have knowledge about specific language runtime plugins. This particular 
instance is relatively harmless, but it is a necessary part of a larger change 
to decouple specific language details from the core lldb implementation.
I recognize that CPPLanguageRuntime currently also lives in Target, but I would 
like to see it moved to "source/Plugins/LanguageRuntime/CPlusPlus/". I think 
that before I try to move it, this change makes sense as a first step.

In D62755#1525810 , @aprantl wrote:

> I tend to agree with Jim's comment.
>
> > There is "GetLanguageRuntime()" which should be used instead.
>
> Can you explain why that is?


I think my response to Jim above explains my reasoning here. If there's 
something I haven't addressed or you have other concerns, please let me know.




Comment at: source/Target/Process.cpp:1601
 
-CPPLanguageRuntime *Process::GetCPPLanguageRuntime(bool retry_if_null) {
-  std::lock_guard guard(m_language_runtimes_mutex);

aprantl wrote:
> so what about this mutex at the new call sites?
GetLanguageRuntime locks the mutex as well.


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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

> I disagree that it is "carrying purity a little too far". My goal is to see 
> LLDB's non-plugin libraries be more language agnostic.

I have two motivations for this:

> Better language support in LLDB, making it easier to add support for new 
> languages in a clean fashion.
>  Shrinking the size of lldb-server. Though the lldb on llvm.org doesn't 
> suffer from this problem too much, swift-lldb has an issue where the 
> lldb-server you build is incredibly bloated. On Linux I am measuring about 
> 162MB and on android I measured 61MB. It is incredibly difficult to shrink 
> the size without first decoupling components. Because swift-lldb is a 
> downstream project, I want to make sure that the abstractions there make 
> sense with what is on llvm.org, so I am doing this work here first.

Those are good goals. Thank you for working on this!

I don't yet see the connection between those goals and this patch, but I might 
be missing something. Would CPPLanguageRuntime need to be anything but a 
forward declaration in Process.h?


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

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

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



Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2770
   LLDB_LOG(log, "no auxv data retrieved: {0}", ec.message());
-  return SendErrorResponse(ec.value());
+  return llvm::make_error(ec.value());
 }

aadsm wrote:
> labath wrote:
> > aadsm wrote:
> > > labath wrote:
> > > > I am wondering whether we actually need the `PacketError` class. Such a 
> > > > class would be useful if we actually wanted to start providing 
> > > > meaningful error codes to the other side (as we would give us tighter 
> > > > control over the allocation of error codes). However, here you're just 
> > > > taking random numbers and sticking them into the `PacketError` class, 
> > > > so I am not sure that adds any value.
> > > > 
> > > > So, what I'd do here is just delete the PacketError class, and just do 
> > > > a `return llvm::errorCodeToError(ec)` here. I'd also delete the log 
> > > > message as the error message will implicitly end up in the packet log 
> > > > via the error message extension (if you implement my suggestion 
> > > > `SendErrorResponse`).
> > > I thought it would be nice to have a little abstraction layer around the 
> > > packet errors overall. My purpose with the PacketError is to make it more 
> > > obvious in the code that the number given will be sent back as an error 
> > > number.
> > > I didn't realize the numbers we were using were meaningless today though 
> > > (now that I think of it this ec.value is really whatever GetAuxvData 
> > > returns). I searched at the time and found a few different numbers being 
> > > used: 9, 4, 10, etc. I guess these numbers are just lies then :D.
> > > 
> > Yeah, the only way you can assign meaning to these numbers today is if you 
> > open the source code and search for the occurrences of the given number. :)
> > That will be even easier if we switch to using strings. :)
> But we can't return strings on the protocol though, it will have to be a `E 
> NN`.
> I'm going with your suggestions but how about this in a future patch:
> 
> Have a base `PacketError(num, message)` and then subclass that one with the 
> different errors we have like `NoProcessAvailablePacketError()` that would 
> code their own error number and message (or maybe we just need to have static 
> functions like PacketError::createNoProcessAvailableError()?).
> 
> Then, on the client side we could add an `Optional 
> GetResponseError()` to `StringExtractorGDBRemote` that would create the right 
> packet error given the number, so we can print a descriptive error message on 
> the lldb terminal. (maybe GDBResponseError instead of PacketError...)
Nevermind what I said about sending strings, I just noticed 
`m_send_error_strings`.


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] D62755: [Target] Remove Process::GetCPPLanguageRuntime

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

In D62755#1525890 , @aprantl wrote:

> Those are good goals. Thank you for working on this!


Thank you for taking the time to review this and discuss this with me! :)

> I don't yet see the connection between those goals and this patch, but I 
> might be missing something. Would CPPLanguageRuntime need to be anything but 
> a forward declaration in Process.h?

Apologies for not making this clearer. I view this patch as a step in moving 
all non-plugin libraries over to using the LanguageRuntime interface instead of 
using CPPLanguageRuntime and ObjCLanguageRuntime directly. This particular 
patch is a part of making Target more language agnostic, as I've been doing 
over the past few weeks. Here are some other commits that I've recently made 
towards that goal: rL362164 , rL362154 
, rL361999 
, rL360945 
.

Somewhat related, earlier this month I made the Breakpoint library language 
agnostic with rL360509 . This is what I want 
to do, but for each of the non-plugin libraries (e.g. Target).


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

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

In D62755#1525890 , @aprantl wrote:

> I don't yet see the connection between those goals and this patch, but I 
> might be missing something. Would CPPLanguageRuntime need to be anything but 
> a forward declaration in Process.h?


I forgot to address this point. CPPLanguageRuntime would not be a forward 
declaration in Process.h, nor be referred to anywhere in Process.h


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

In D62755#1525919 , @xiaobai wrote:

> In D62755#1525890 , @aprantl wrote:
>
> > I don't yet see the connection between those goals and this patch, but I 
> > might be missing something. Would CPPLanguageRuntime need to be anything 
> > but a forward declaration in Process.h?
>
>
> I forgot to address this point. CPPLanguageRuntime would not be a forward 
> declaration in Process.h, nor be referred to anywhere in Process.h


Yeah, getting CPPLanguageRuntime out of the general forward declarations does 
seem a worthy goal.  But it would be great to do it in a way that doesn't add 
so much line noise.  I added these methods because it got tiresome to write and 
read the version you are showing...

Maybe we can add a static:

CPPLanguageRuntime *GetCPPLanguageRuntime(Process &process);

to CPPLanguageRuntime.h?

If you are calling LanguageRuntime methods on the CPPLanguageRuntime, you 
shouldn't need the cast and probably shouldn't be including 
CPPLanguageRuntime.h.   So in that case, you'd just do:

#include "Target/LanguageRuntime.h"
...

  LanguageRuntime *cpp_runtime = 
process->GetLanguageRuntime(eLanguageTypeCPlusPlus)
  cpp_runtime->GenericMethod();

which is easy to type and read.

You only need to get a runtime cast as a CPPLanguageRuntime if you are planning 
to call methods that are in CPPLanguageRuntime.h, so you would be including 
that header anyway, and then you could use the convenience method from there to 
improve readability.


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

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

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



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) {

@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);
```


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] 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 202517.
kusmour added a comment.
Herald added a subscriber: teemperor.

added a virtual function `CanPassInRegister` in TypeSystem class
to provide info about type can be passed in register or not
`ClangASTContext` will refer to `clang::RecordDecl::canPassInRegister` for this 
info
and later `SwiftASTContext` can provide the equivalent 
updated the test to test on C++
and provided class test, including base class, subclass, abstract class (this 
one must be in memory)


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/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/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 (clang::RecordDecl *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] D62755: [Target] Remove Process::GetCPPLanguageRuntime

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

In D62755#1525937 , @jingham wrote:

> Yeah, getting CPPLanguageRuntime out of the general forward declarations does 
> seem a worthy goal.  But it would be great to do it in a way that doesn't add 
> so much line noise.  I added these methods because it got tiresome to write 
> and read the version you are showing...
>
> Maybe we can add a static:
>
> CPPLanguageRuntime *GetCPPLanguageRuntime(Process &process);
>
> to CPPLanguageRuntime.h?
>
> If you are calling LanguageRuntime methods on the CPPLanguageRuntime, you 
> shouldn't need the cast and probably shouldn't be including 
> CPPLanguageRuntime.h.   So in that case, you'd just do:
>
> #include "Target/LanguageRuntime.h"
>  ...
>
>   LanguageRuntime *cpp_runtime = 
> process->GetLanguageRuntime(eLanguageTypeCPlusPlus)
>   cpp_runtime->GenericMethod();
>   
>
> which is easy to type and read.
>
> You only need to get a runtime cast as a CPPLanguageRuntime if you are 
> planning to call methods that are in CPPLanguageRuntime.h, so you would be 
> including that header anyway, and then you could use the convenience method 
> from there to improve readability.


What you're saying makes sense to me. Since it's true that casting requires 
including CPPLanguageRuntime.h in the first place, adding it as a static method 
to CPPLanguageRuntime itself shouldn't be a problem. I don't think this will be 
an issue when I try to move CPPLanguageRuntime into the Plugins directory 
either, so I'm perfectly fine with this solution. I will also see if I can 
remove CPPLanguageRuntime from the forward declarations, and will try to update 
this diff tonight or tomorrow.


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] 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:549-550
 
-  if (ModuleSP module_sp = target.GetOrCreateModule(module_spec, 
-true /* notify */)) {
+  if (ModuleSP module_sp =
+  target.GetOrCreateModule(module_spec, true /* notify */)) {
 UpdateLoadedSections(module_sp, LLDB_INVALID_ADDRESS, m_interpreter_base,

aadsm wrote:
> labath wrote:
> > Whitespace-only change. Did you maybe run clang-format on the whole file 
> > instead of just the diff?
> > 
> > It doesn't matter that much here as there isn't many of them, but I just 
> > want to make sure you're aware of this in the future.
> odd, pretty sure I just run clang-format-diff (as I don't have auto 
> formatting going on), will double check this.
I have my editor to remove trailing spaces on save and this line had one, 
that's why it ended up being parsed by clang-format-diff.


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] D62764: Detect x86 mid-function epilogues that end in a jump

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a project: LLDB.
Herald added a subscriber: abidh.

This patch adds a few new instruction recognizers to the x86 assembly 
inspection engine to recognize relative branches & jumps, adds support for 
recognizing a mid-function epilogue that ends in a jump instruction, and tests 
this.

The x86 unwinder still uses the initial algorithm for detecting mid-function 
epilogues: recognizing the end of an epilogue (relatively easy on x86 when it's 
usually a RET), we re-instate the unwind state from the end of the prologue.  
This patch adds recognition of a jump outside the function as a final epilogue 
instruction.

The second attempt at recognizing mid-function epilogues is implemented in 
UnwindAssemblyInstEmulation and is a much better approach -- it recognizes 
branch instructions and forwards the unwind state to the target instruction.  
This means that a branch over a mid-function epilogue will forward the unwind 
state past the epilogue; no detection of prologues or epilogues is needed.

I've added more instruction recognition machinery than is technically necessary 
for the bug I needed to fix - but it'll be needed for the better algorithm to 
be implemented in the x86 inspection engine.  I didn't want to take on that 
larger bit of work right now, but it's something that should be done - this 
current algorithm is always going to be more fragile.

A more ambitious way to fix this would be to add an InstEmulation plugin for 
x86 and update UnwindAssemblyInstEmulation to handle all of the x86 features 
needed for unwinding, retire the x86AssemblyInspectionEngine.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D62764

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

Index: unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp
===
--- unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp
+++ 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 = unwin

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

[Alex Langford via Phabricator via lldb-commits]

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

Jim's suggestion


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

https://reviews.llvm.org/D62755

Files:
  include/lldb/Target/CPPLanguageRuntime.h
  include/lldb/Target/Process.h
  include/lldb/lldb-forward.h
  source/Plugins/Language/CPlusPlus/LibCxx.cpp
  source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
  source/Target/Process.cpp


Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -1598,16 +1598,6 @@
   return runtime;
 }
 
-CPPLanguageRuntime *Process::GetCPPLanguageRuntime(bool retry_if_null) {
-  std::lock_guard guard(m_language_runtimes_mutex);
-  LanguageRuntime *runtime =
-  GetLanguageRuntime(eLanguageTypeC_plus_plus, retry_if_null);
-  if (!runtime)
-return nullptr;
-
-  return static_cast(runtime);
-}
-
 ObjCLanguageRuntime *Process::GetObjCLanguageRuntime(bool retry_if_null) {
   std::lock_guard guard(m_language_runtimes_mutex);
   LanguageRuntime *runtime =
Index: source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
===
--- source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
+++ source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
@@ -462,7 +462,7 @@
 
 ValueObjectSP AppleObjCRuntime::GetExceptionObjectForThread(
 ThreadSP thread_sp) {
-  auto cpp_runtime = m_process->GetCPPLanguageRuntime();
+  auto *cpp_runtime = m_process->GetLanguageRuntime(eLanguageTypeC_plus_plus);
   if (!cpp_runtime) return ValueObjectSP();
   auto cpp_exception = cpp_runtime->GetExceptionObjectForThread(thread_sp);
   if (!cpp_exception) return ValueObjectSP();
Index: source/Plugins/Language/CPlusPlus/LibCxx.cpp
===
--- source/Plugins/Language/CPlusPlus/LibCxx.cpp
+++ source/Plugins/Language/CPlusPlus/LibCxx.cpp
@@ -67,7 +67,8 @@
   if (process == nullptr)
 return false;
 
-  CPPLanguageRuntime *cpp_runtime = process->GetCPPLanguageRuntime();
+  CPPLanguageRuntime *cpp_runtime =
+  CPPLanguageRuntime::GetCPPLanguageRuntime(*process);
 
   if (!cpp_runtime)
 return false;
Index: include/lldb/lldb-forward.h
===
--- include/lldb/lldb-forward.h
+++ include/lldb/lldb-forward.h
@@ -43,7 +43,6 @@
 class BroadcastEventSpec;
 class Broadcaster;
 class BroadcasterManager;
-class CPPLanguageRuntime;
 class ClangASTContext;
 class ClangASTImporter;
 class ClangASTMetadata;
Index: include/lldb/Target/Process.h
===
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -2184,8 +2184,6 @@
   LanguageRuntime *GetLanguageRuntime(lldb::LanguageType language,
   bool retry_if_null = true);
 
-  CPPLanguageRuntime *GetCPPLanguageRuntime(bool retry_if_null = true);
-
   ObjCLanguageRuntime *GetObjCLanguageRuntime(bool retry_if_null = true);
 
   bool IsPossibleDynamicValue(ValueObject &in_value);
Index: include/lldb/Target/CPPLanguageRuntime.h
===
--- include/lldb/Target/CPPLanguageRuntime.h
+++ include/lldb/Target/CPPLanguageRuntime.h
@@ -43,6 +43,11 @@
 return lldb::eLanguageTypeC_plus_plus;
   }
 
+  static CPPLanguageRuntime *GetCPPLanguageRuntime(Process &process) {
+return static_cast(
+process.GetLanguageRuntime(lldb::eLanguageTypeC_plus_plus));
+  }
+
   virtual bool IsVTableName(const char *name) = 0;
 
   bool GetObjectDescription(Stream &str, ValueObject &object) override;


Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -1598,16 +1598,6 @@
   return runtime;
 }
 
-CPPLanguageRuntime *Process::GetCPPLanguageRuntime(bool retry_if_null) {
-  std::lock_guard guard(m_language_runtimes_mutex);
-  LanguageRuntime *runtime =
-  GetLanguageRuntime(eLanguageTypeC_plus_plus, retry_if_null);
-  if (!runtime)
-return nullptr;
-
-  return static_cast(runtime);
-}
-
 ObjCLanguageRuntime *Process::GetObjCLanguageRuntime(bool retry_if_null) {
   std::lock_guard guard(m_language_runtimes_mutex);
   LanguageRuntime *runtime =
Index: source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
===
--- source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
+++ source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp
@@ -462,7 +462,7 @@
 
 ValueObjectSP AppleObjCRuntime::GetExceptionObjectForThread(
 ThreadSP thread_sp) {
-  auto cpp_runtime = m_process->GetCPPLanguageRuntime();
+  auto *cpp_runtime = m_process->Get