[Lldb-commits] [PATCH] D88681: [lldb] [Process/NetBSD] Fix reading FIP/FDP registers

[Pavel Labath via Phabricator via lldb-commits]

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

I'm assuming this is tested by D88583 .


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

https://reviews.llvm.org/D88681

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

[Lldb-commits] [PATCH] D88682: [lldb] [Process/NetBSD] Fix crash on unsupported i386 regs

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It sounds like the assertion could be useful (to detect the missing 
translations). Under what circumstances can an unknown register make its way to 
this function? How did you run into this code in the first place?


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

https://reviews.llvm.org/D88682

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

[Lldb-commits] [PATCH] D88681: [lldb] [Process/NetBSD] Fix reading FIP/FDP registers

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

mgorny added a comment.

Yes. We also need to fix our kernel but it's no less broken than it was before.


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

https://reviews.llvm.org/D88681

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

[Lldb-commits] [PATCH] D88682: [lldb] [Process/NetBSD] Fix crash on unsupported i386 regs

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

mgorny added a comment.

In D88682#2307911 , @labath wrote:

> It sounds like the assertion could be useful (to detect the missing 
> translations). Under what circumstances can an unknown register make its way 
> to this function? How did you run into this code in the first place?

Very good question. I've ran into it because it crashed under me. But now I see 
that x86 variant of 
`NativeRegisterContextNetBSD_x86_64::GetSetForNativeRegNum()` doesn't handle 
mpx*, so they fall into wrong regsets. I suppose I should change the conditions 
to check both lower and upper bound of each group. Thanks!


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

https://reviews.llvm.org/D88682

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

[Lldb-commits] [PATCH] D88682: [lldb] [Process/NetBSD] Fix crash on unsupported i386 regs

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

mgorny updated this revision to Diff 295753.
mgorny edited the summary of this revision.
mgorny added a comment.
Herald added a subscriber: pengfei.

Ok, replaced with a few more assorted fixes ;-).


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

https://reviews.llvm.org/D88682

Files:
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
  lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h


Index: lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
===
--- lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
+++ lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
@@ -106,7 +106,7 @@
   lldb_bnd1_i386,
   lldb_bnd2_i386,
   lldb_bnd3_i386,
-  k_last_mpxr = lldb_bnd3_i386,
+  k_last_mpxr_i386 = lldb_bnd3_i386,
 
   k_first_mpxc_i386,
   lldb_bndcfgu_i386 = k_first_mpxc_i386,
Index: lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
===
--- lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
+++ lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
@@ -375,6 +375,15 @@
   case lldb_ymm6_i386:
   case lldb_ymm7_i386:
 return lldb_ymm0_x86_64 + regnum - lldb_ymm0_i386;
+  case lldb_bnd0_i386:
+  case lldb_bnd1_i386:
+  case lldb_bnd2_i386:
+  case lldb_bnd3_i386:
+return lldb_bnd0_x86_64 + regnum - lldb_bnd0_i386;
+  case lldb_bndcfgu_i386:
+return lldb_bndcfgu_x86_64;
+  case lldb_bndstatus_i386:
+return lldb_bndstatus_x86_64;
   case lldb_dr0_i386:
   case lldb_dr1_i386:
   case lldb_dr2_i386:
@@ -386,7 +395,7 @@
 return lldb_dr0_x86_64 + regnum - lldb_dr0_i386;
   default:
 assert(false && "Unhandled i386 register.");
-return 0;
+return -1;
   }
 }
 
@@ -394,28 +403,32 @@
 int reg_num) const {
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86:
-if (reg_num <= k_last_gpr_i386)
+if (reg_num >= k_first_gpr_i386 && reg_num <= k_last_gpr_i386)
   return GPRegSet;
-else if (reg_num <= k_last_fpr_i386)
+else if (reg_num >= k_first_fpr_i386 && reg_num <= k_last_fpr_i386)
   return FPRegSet;
-else if (reg_num <= k_last_avx_i386)
+else if (reg_num >= k_first_avx_i386 && reg_num <= k_last_avx_i386)
   return XStateRegSet; // AVX
-else if (reg_num <= lldb_dr7_i386)
+else if (reg_num >= k_first_mpxr_i386 && reg_num <= k_last_mpxr_i386)
+  return -1; // MPXR
+else if (reg_num >= k_first_mpxr_i386 && reg_num <= k_last_mpxc_i386)
+  return -1; // MPXC
+else if (reg_num >= k_first_dbr_i386 && reg_num <= k_last_dbr_i386)
   return DBRegSet; // DBR
 else
   return -1;
   case llvm::Triple::x86_64:
-if (reg_num <= k_last_gpr_x86_64)
+if (reg_num >= k_first_gpr_x86_64 && reg_num <= k_last_gpr_x86_64)
   return GPRegSet;
-else if (reg_num <= k_last_fpr_x86_64)
+else if (reg_num >= k_first_fpr_x86_64 && reg_num <= k_last_fpr_x86_64)
   return FPRegSet;
-else if (reg_num <= k_last_avx_x86_64)
+else if (reg_num >= k_first_avx_x86_64 && reg_num <= k_last_avx_x86_64)
   return XStateRegSet; // AVX
-else if (reg_num <= k_last_mpxr_x86_64)
+else if (reg_num >= k_first_mpxr_x86_64 && reg_num <= k_last_mpxr_x86_64)
   return -1; // MPXR
-else if (reg_num <= k_last_mpxc_x86_64)
+else if (reg_num >= k_first_mpxr_x86_64 && reg_num <= k_last_mpxc_x86_64)
   return -1; // MPXC
-else if (reg_num <= lldb_dr7_x86_64)
+else if (reg_num >= k_first_dbr_x86_64 && reg_num <= k_last_dbr_x86_64)
   return DBRegSet; // DBR
 else
   return -1;


Index: lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
===
--- lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
+++ lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
@@ -106,7 +106,7 @@
   lldb_bnd1_i386,
   lldb_bnd2_i386,
   lldb_bnd3_i386,
-  k_last_mpxr = lldb_bnd3_i386,
+  k_last_mpxr_i386 = lldb_bnd3_i386,
 
   k_first_mpxc_i386,
   lldb_bndcfgu_i386 = k_first_mpxc_i386,
Index: lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
===
--- lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
+++ lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
@@ -375,6 +375,15 @@
   case lldb_ymm6_i386:
   case lldb_ymm7_i386:
 return lldb_ymm0_x86_64 + regnum - lldb_ymm0_i386;
+  case lldb_bnd0_i386:
+  case lldb_bnd1_i386:
+  case lldb_bnd2_i386:
+  case lldb_bnd3_i386:
+return lldb_bnd0_x86_64 + regnum - lldb_bnd0_i386;
+  case lldb_bndcfgu_i386:
+return lldb_bndcfgu_x86_64;
+  case lldb_bndstatus_i386:
+return lldb_bndstatus_x86_64;
   case lldb_dr0_i386:
   case lldb

[Lldb-commits] [PATCH] D88682: [lldb] [Process/NetBSD] Fix crash on unsupported i386 regs

[Pavel Labath via Phabricator via lldb-commits]

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

Yes, looks good. It might not be unreasonable to add asserts to these functions 
too, as I don't think we should ever have a register that doesn't belong to any 
register set.




Comment at: 
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp:408-431
+else if (reg_num >= k_first_fpr_i386 && reg_num <= k_last_fpr_i386)
   return FPRegSet;
-else if (reg_num <= k_last_avx_i386)
+else if (reg_num >= k_first_avx_i386 && reg_num <= k_last_avx_i386)
   return XStateRegSet; // AVX
-else if (reg_num <= lldb_dr7_i386)
+else if (reg_num >= k_first_mpxr_i386 && reg_num <= k_last_mpxr_i386)
+  return -1; // MPXR
+else if (reg_num >= k_first_mpxr_i386 && reg_num <= k_last_mpxc_i386)

Could you also remove all of these else's while you're in there, as per 
.


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

https://reviews.llvm.org/D88682

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

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

[Pavel Labath via lldb-commits]

On 01/10/2020 20:57, Walter wrote:
>> - I am surprised that it was not necessary to create a special process
> plugin for this purpose. I have a feeling one will be necessary sooner
> or later because of the need to customize the step/continue/etc. flows.
> Currently, this will probably produce very bizarre if one tries to
> execute those commands. The reason I'm bringing this up is because of
> the `Target::GetTrace` method being added in the next patch. If there is
> a trace-specific process class, then it might make sense for this class
> to hold the trace object instead of adding the GetTrace method on every
> Target object out there even though most targets will have that null. I
> don't know if that will really be the case, but I think it's something
> worth keeping in mind as you work on the subsequent patches.
> 
> Very good remark. Probably we'll end up doing that when we start
> implementing reverse debugging. The tricky thing we'll need to solve in
> the future is seamlessly transition between a trace and a live process.
> For example, when reverse debugging a live process, you might be stopped
> at a breakpoint in the trace, then you do "continue", it reaches the end
> of the trace, and then it resumes the live process. I still haven't
> decided what we'll propose for this, but probably we'll have to change a
> lot of the current code to make that happen nicely.

Yes, that will be interesting. Even more interesting will be the
question of how to communicate to the user the fact that even though we
have "unwound" the PC to the previous location, variables and memory
still contain the "live" values. Particularly, once we support rr-style
reverse debugging which will "unwind" memory as well..

> 
>> - I am puzzled by the TraceXXX vs. TraceXXXSettingsParser duality. The
> two classes are very tightly coupled, so it's not clear to me what is
> the advantage of separating it out this way (one could just move all the
> relevant methods directly into the Trace class. What's the reason for
> this design?
> 
> Well, there's definitely coupling, but there are two reasons. One is
> that the Trace of a live process won't need any parsing. Parsing is only
> used when loading a trace from a json file. That makes parsing one of
> the two main ways to create a Trace. And besides, I think that the
> single responsibility principle is good to follow. The Parser class does
> the parsing, and the Trace class holds an actual trace.

Yeah, I'm all for the single responsibility principle, but the way it is
implemented gives me pause. Like, if the Parser is supposed to "create"
the Trace, then why does the code do it the other way around
(Trace::CreateParser)? For one, that means that it will be possible to
"parse" (by calling CreateParser()->...) any trace that you can get your
hands on, even that of a live process. Ideally, the Parser would be the
one creating a Trace object (in it's Parse function, or something), and
after the Trace is created, it should no longer be possible to obtain
the parser. All of this could be encapsulated in the "create_callback"
that the plugin registers, and so the fact that there is a parser class
involved would be completely unknown to the rest of the code. And then
there could be a different create_callback which would create a Trace
object suitable for tracing a live process...

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

[Lldb-commits] [PATCH] D88682: [lldb] [Process/NetBSD] Fix crash on unsupported i386 regs

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

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

Fixed coding style, improved description, and added more assertions for 
unexpected registers.


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

https://reviews.llvm.org/D88682

Files:
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
  lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h

Index: lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
===
--- lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
+++ lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
@@ -106,7 +106,7 @@
   lldb_bnd1_i386,
   lldb_bnd2_i386,
   lldb_bnd3_i386,
-  k_last_mpxr = lldb_bnd3_i386,
+  k_last_mpxr_i386 = lldb_bnd3_i386,
 
   k_first_mpxc_i386,
   lldb_bndcfgu_i386 = k_first_mpxc_i386,
Index: lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
===
--- lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
+++ lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
@@ -375,6 +375,15 @@
   case lldb_ymm6_i386:
   case lldb_ymm7_i386:
 return lldb_ymm0_x86_64 + regnum - lldb_ymm0_i386;
+  case lldb_bnd0_i386:
+  case lldb_bnd1_i386:
+  case lldb_bnd2_i386:
+  case lldb_bnd3_i386:
+return lldb_bnd0_x86_64 + regnum - lldb_bnd0_i386;
+  case lldb_bndcfgu_i386:
+return lldb_bndcfgu_x86_64;
+  case lldb_bndstatus_i386:
+return lldb_bndstatus_x86_64;
   case lldb_dr0_i386:
   case lldb_dr1_i386:
   case lldb_dr2_i386:
@@ -386,7 +395,7 @@
 return lldb_dr0_x86_64 + regnum - lldb_dr0_i386;
   default:
 assert(false && "Unhandled i386 register.");
-return 0;
+return -1;
   }
 }
 
@@ -394,35 +403,40 @@
 int reg_num) const {
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86:
-if (reg_num <= k_last_gpr_i386)
+if (reg_num >= k_first_gpr_i386 && reg_num <= k_last_gpr_i386)
   return GPRegSet;
-else if (reg_num <= k_last_fpr_i386)
+if (reg_num >= k_first_fpr_i386 && reg_num <= k_last_fpr_i386)
   return FPRegSet;
-else if (reg_num <= k_last_avx_i386)
+if (reg_num >= k_first_avx_i386 && reg_num <= k_last_avx_i386)
   return XStateRegSet; // AVX
-else if (reg_num <= lldb_dr7_i386)
+if (reg_num >= k_first_mpxr_i386 && reg_num <= k_last_mpxr_i386)
+  return -1; // MPXR
+if (reg_num >= k_first_mpxr_i386 && reg_num <= k_last_mpxc_i386)
+  return -1; // MPXC
+if (reg_num >= k_first_dbr_i386 && reg_num <= k_last_dbr_i386)
   return DBRegSet; // DBR
-else
-  return -1;
+break;
   case llvm::Triple::x86_64:
-if (reg_num <= k_last_gpr_x86_64)
+if (reg_num >= k_first_gpr_x86_64 && reg_num <= k_last_gpr_x86_64)
   return GPRegSet;
-else if (reg_num <= k_last_fpr_x86_64)
+if (reg_num >= k_first_fpr_x86_64 && reg_num <= k_last_fpr_x86_64)
   return FPRegSet;
-else if (reg_num <= k_last_avx_x86_64)
+if (reg_num >= k_first_avx_x86_64 && reg_num <= k_last_avx_x86_64)
   return XStateRegSet; // AVX
-else if (reg_num <= k_last_mpxr_x86_64)
+if (reg_num >= k_first_mpxr_x86_64 && reg_num <= k_last_mpxr_x86_64)
   return -1; // MPXR
-else if (reg_num <= k_last_mpxc_x86_64)
+if (reg_num >= k_first_mpxr_x86_64 && reg_num <= k_last_mpxc_x86_64)
   return -1; // MPXC
-else if (reg_num <= lldb_dr7_x86_64)
+if (reg_num >= k_first_dbr_x86_64 && reg_num <= k_last_dbr_x86_64)
   return DBRegSet; // DBR
-else
-  return -1;
+break;
   default:
 assert(false && "Unhandled target architecture.");
 return -1;
   }
+
+  assert(false && "Register does not belong to any register set");
+  return -1;
 }
 
 Status NativeRegisterContextNetBSD_x86_64::ReadRegisterSet(uint32_t set) {
@@ -758,6 +772,10 @@
   case lldb_dr7_x86_64:
 reg_value = (uint64_t)m_dbr.dr[reg - lldb_dr0_x86_64];
 break;
+  default:
+assert(false && "Reading unknown/unsupported register");
+error.SetErrorStringWithFormat("register \"%s\" unknown/unsupported",
+   reg_info->name);
   }
 
   return error;
@@ -1034,6 +1052,7 @@
 }
 #else
 error.SetErrorString("XState not supported by the kernel");
+return error;
 #endif
 break;
   case lldb_dr0_x86_64:
@@ -1046,6 +1065,11 @@
   case lldb_dr7_x86_64:
 m_dbr.dr[reg - lldb_dr0_x86_64] = reg_value.GetAsUInt64();
 break;
+  default:
+assert(false && "Reading unknown/unsupported register");
+error.SetErrorStringWithFormat("register \"%s\" unknown/unsupported",
+   reg_info->name);
+return error;
   }
 
   return WriteRegisterSet(set);
___
lldb-c

[Lldb-commits] [PATCH] D88704: [lldb] Fix bug in fallback logic for finding the resource directory.

[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


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D88704

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

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

[Pavel Labath via Phabricator via lldb-commits]

labath created this revision.
labath added reviewers: mgorny, MaskRay.
Herald added a subscriber: emaste.
Herald added a project: LLDB.
labath requested review of this revision.
Herald added a subscriber: JDevlieghere.

ptsname is not thread-safe. ptsname_r is available on most (but not all)
systems -- use it preferentially.

In the patch I also improve the thread-safety of the ptsname fallback
path by wrapping it in a mutex. This should guarantee the safety of a
typical ptsname implementation using a single static buffer, as long as
all callers go through this function.

I also remove the error arguments, as the only way this function can
fail is if the "primary" fd is not valid. This is a programmer error as
this requirement is documented, and all callers ensure that is the case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88728

Files:
  lldb/cmake/modules/LLDBGenerateConfig.cmake
  lldb/include/lldb/Host/Config.h.cmake
  lldb/include/lldb/Host/PseudoTerminal.h
  lldb/source/Host/common/ProcessLaunchInfo.cpp
  lldb/source/Host/common/PseudoTerminal.cpp
  
lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
  lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
  lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -818,7 +818,7 @@
 // does a lot of output.
 if ((!stdin_file_spec || !stdout_file_spec || !stderr_file_spec) &&
 pty.OpenFirstAvailablePrimary(O_RDWR | O_NOCTTY, nullptr, 0)) {
-  FileSpec secondary_name{pty.GetSecondaryName(nullptr, 0)};
+  FileSpec secondary_name(pty.GetSecondaryName());
 
   if (!stdin_file_spec)
 stdin_file_spec = secondary_name;
Index: lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
===
--- lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
+++ lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
@@ -380,8 +380,7 @@
   FileSpec stdout_file_spec{};
   FileSpec stderr_file_spec{};
 
-  const FileSpec dbg_pts_file_spec{
-  launch_info.GetPTY().GetSecondaryName(NULL, 0)};
+  const FileSpec dbg_pts_file_spec{launch_info.GetPTY().GetSecondaryName()};
 
   file_action = launch_info.GetFileActionForFD(STDIN_FILENO);
   stdin_file_spec =
Index: lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
===
--- lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
+++ lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
@@ -407,25 +407,21 @@
 const int master_fd = launch_info.GetPTY().GetPrimaryFileDescriptor();
 if (master_fd != PseudoTerminal::invalid_fd) {
   // Check in case our file action open wants to open the secondary
-  const char *secondary_path =
-  launch_info.GetPTY().GetSecondaryName(NULL, 0);
-  if (secondary_path) {
-FileSpec secondary_spec(secondary_path);
-if (file_spec == secondary_spec) {
-  int secondary_fd =
-  launch_info.GetPTY().GetSecondaryFileDescriptor();
-  if (secondary_fd == PseudoTerminal::invalid_fd)
-secondary_fd =
-launch_info.GetPTY().OpenSecondary(O_RDWR, nullptr, 0);
-  if (secondary_fd == PseudoTerminal::invalid_fd) {
-error.SetErrorStringWithFormat(
-"unable to open secondary pty '%s'", secondary_path);
-return error; // Failure
-  }
-  [options setValue:[NSNumber numberWithInteger:secondary_fd]
- forKey:key];
-  return error; // Success
+  FileSpec secondary_spec(launch_info.GetPTY().GetSecondaryName());
+  if (file_spec == secondary_spec) {
+int secondary_fd =
+launch_info.GetPTY().GetSecondaryFileDescriptor();
+if (secondary_fd == PseudoTerminal::invalid_fd)
+  secondary_fd =
+  launch_info.GetPTY().OpenSecondary(O_RDWR, nullptr, 0);
+if (secondary_fd == PseudoTerminal::invalid_fd) {
+  error.SetErrorStringWithFormat(
+  "unable to open secondary pty '%s'", secondary_path);
+  return error; // Failure
 }
+[options setValue:[NSNumber numberWithInteger:secondary_fd]
+   forKey:key];
+return error; // Success
   }
 }
 Status posix_error;
Index: lldb/source/Host/common/PseudoTerminal.cpp
===

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

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

mgorny added inline comments.



Comment at: lldb/source/Host/common/PseudoTerminal.cpp:149
+  int r = ptsname_r(m_primary_fd, buf, sizeof(buf));
+  assert(r == 0);
+  return buf;

I would really feel better with a real error handling here. It shouldn't be 
hard to use `ErrorOr` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88728

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

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Host/common/PseudoTerminal.cpp:149
+  int r = ptsname_r(m_primary_fd, buf, sizeof(buf));
+  assert(r == 0);
+  return buf;

mgorny wrote:
> I would really feel better with a real error handling here. It shouldn't be 
> hard to use `ErrorOr` here.
Yeah, but what are you going to do with that value? Pass it to the caller? The 
existing callers are ignoring the error return anyway, and I don't want to add 
error handling everywhere as afaict, this function can't fail unless the user 
messes up the master state (which is not something I want to support).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88728

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

[Lldb-commits] [PATCH] D87868: [RFC] When calling the process mmap try to call all found instead of just the first one

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.
Herald added a subscriber: pengfei.

That sounds like a plan. FWIW, here's the implementation I hacked up today:

  Status NativeProcessLinux::AllocateMemory(size_t size, uint32_t permissions,
lldb::addr_t &addr) {
PopulateMemoryRegionCache();
auto region_it = llvm::find_if(m_mem_region_cache, [](const auto &pair) {
  return pair.first.GetExecutable() == MemoryRegionInfo::eYes;
});
if (region_it == m_mem_region_cache.end())
  return Status("No executable memory region found!");
addr_t exe_addr = region_it->first.GetRange().GetRangeBase();
  
NativeThreadLinux &thread = *GetThreadByID(GetID());
assert(thread.GetState() == eStateStopped);
  
int prot = PROT_NONE;
if (permissions & ePermissionsReadable)
  prot |= PROT_READ;
if (permissions & ePermissionsWritable)
  prot |= PROT_WRITE;
if (permissions & ePermissionsExecutable)
  prot |= PROT_EXEC;
  
NativeRegisterContextLinux ®_ctx = thread.GetRegisterContext();
DataBufferSP registers_sp;
reg_ctx.ReadAllRegisterValues(registers_sp);
uint8_t memory[2];
size_t bytes_read;
ReadMemory(exe_addr, memory, 2, bytes_read);
  
reg_ctx.SetPC(exe_addr);
reg_ctx.WriteRegisterFromUnsigned(lldb_rax_x86_64, SYS_mmap);
reg_ctx.WriteRegisterFromUnsigned(lldb_rdi_x86_64, 0);
reg_ctx.WriteRegisterFromUnsigned(lldb_rsi_x86_64, size);
reg_ctx.WriteRegisterFromUnsigned(lldb_rdx_x86_64, prot);
reg_ctx.WriteRegisterFromUnsigned(lldb_r10_x86_64,
  MAP_ANONYMOUS | MAP_PRIVATE);
reg_ctx.WriteRegisterFromUnsigned(lldb_r8_x86_64, -1);
reg_ctx.WriteRegisterFromUnsigned(lldb_r9_x86_64, 0);
WriteMemory(exe_addr, "\x0f\x05", 2, bytes_read);
PtraceWrapper(PTRACE_SINGLESTEP, thread.GetID(), nullptr, nullptr);
int status;
::pid_t wait_pid = llvm::sys::RetryAfterSignal(-1, ::waitpid, 
thread.GetID(),
   &status, __WALL);
assert((unsigned)wait_pid == thread.GetID());
addr = reg_ctx.ReadRegisterAsUnsigned(lldb_rax_x86_64, -ESRCH);
  
WriteMemory(exe_addr, memory, 2, bytes_read);
reg_ctx.WriteAllRegisterValues(registers_sp);
  
if (addr > -4096)
  return Status(-addr, eErrorTypePOSIX);
return Status();
  }

It needs more error handling, and generalization to non-x86 architectures, but 
it actually works, and is enough to make all but one test pass 
(TestBitfields.py seems to fail due to some data corruption -- quite puzzling).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87868

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

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

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

mgorny added inline comments.



Comment at: lldb/source/Host/common/PseudoTerminal.cpp:149
+  int r = ptsname_r(m_primary_fd, buf, sizeof(buf));
+  assert(r == 0);
+  return buf;

labath wrote:
> mgorny wrote:
> > I would really feel better with a real error handling here. It shouldn't be 
> > hard to use `ErrorOr` here.
> Yeah, but what are you going to do with that value? Pass it to the caller? 
> The existing callers are ignoring the error return anyway, and I don't want 
> to add error handling everywhere as afaict, this function can't fail unless 
> the user messes up the master state (which is not something I want to 
> support).
I get your point but I've literally wasted days because of missing error 
handling, so I'd really preferred if we wouldn't make it even worse. Though I 
guess `assert` is good enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88728

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

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: lldb/source/Host/common/PseudoTerminal.cpp:149
+  int r = ptsname_r(m_primary_fd, buf, sizeof(buf));
+  assert(r == 0);
+  return buf;

mgorny wrote:
> labath wrote:
> > mgorny wrote:
> > > I would really feel better with a real error handling here. It shouldn't 
> > > be hard to use `ErrorOr` here.
> > Yeah, but what are you going to do with that value? Pass it to the caller? 
> > The existing callers are ignoring the error return anyway, and I don't want 
> > to add error handling everywhere as afaict, this function can't fail unless 
> > the user messes up the master state (which is not something I want to 
> > support).
> I get your point but I've literally wasted days because of missing error 
> handling, so I'd really preferred if we wouldn't make it even worse. Though I 
> guess `assert` is good enough.
In some ways it's even better because it will point you straight to the place 
where the assumption is violated, whereas a propagated logic error can manifest 
itself much farther away (or not at all). :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88728

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

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Host/common/PseudoTerminal.cpp:149
+  int r = ptsname_r(m_primary_fd, buf, sizeof(buf));
+  assert(r == 0);
+  return buf;

labath wrote:
> mgorny wrote:
> > labath wrote:
> > > mgorny wrote:
> > > > I would really feel better with a real error handling here. It 
> > > > shouldn't be hard to use `ErrorOr` here.
> > > Yeah, but what are you going to do with that value? Pass it to the 
> > > caller? The existing callers are ignoring the error return anyway, and I 
> > > don't want to add error handling everywhere as afaict, this function 
> > > can't fail unless the user messes up the master state (which is not 
> > > something I want to support).
> > I get your point but I've literally wasted days because of missing error 
> > handling, so I'd really preferred if we wouldn't make it even worse. Though 
> > I guess `assert` is good enough.
> In some ways it's even better because it will point you straight to the place 
> where the assumption is violated, whereas a propagated logic error can 
> manifest itself much farther away (or not at all). :)
If `ptsname/ptsname_r` fails, buf will be uninitialized and trigger a 
use-of-uninitialized-value error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88728

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

[Lldb-commits] [PATCH] D88728: [lldb] Check for and use ptsname_r if available

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.



Comment at: lldb/source/Host/common/PseudoTerminal.cpp:149
+  int r = ptsname_r(m_primary_fd, buf, sizeof(buf));
+  assert(r == 0);
+  return buf;

MaskRay wrote:
> labath wrote:
> > mgorny wrote:
> > > labath wrote:
> > > > mgorny wrote:
> > > > > I would really feel better with a real error handling here. It 
> > > > > shouldn't be hard to use `ErrorOr` here.
> > > > Yeah, but what are you going to do with that value? Pass it to the 
> > > > caller? The existing callers are ignoring the error return anyway, and 
> > > > I don't want to add error handling everywhere as afaict, this function 
> > > > can't fail unless the user messes up the master state (which is not 
> > > > something I want to support).
> > > I get your point but I've literally wasted days because of missing error 
> > > handling, so I'd really preferred if we wouldn't make it even worse. 
> > > Though I guess `assert` is good enough.
> > In some ways it's even better because it will point you straight to the 
> > place where the assumption is violated, whereas a propagated logic error 
> > can manifest itself much farther away (or not at all). :)
> If `ptsname/ptsname_r` fails, buf will be uninitialized and trigger a 
> use-of-uninitialized-value error.
... in a -DLLVM_ENABLE_ASSERTIONS=off build.

This probably still needs some protection.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88728

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

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

[Walter via lldb-commits]

I totally agree with you, I didn't think about creating custom callbacks
=P. I'll refactor the code accordingly. Thanks, man

Il giorno ven 2 ott 2020 alle ore 01:51 Pavel Labath  ha
scritto:

> On 01/10/2020 20:57, Walter wrote:
> >> - I am surprised that it was not necessary to create a special process
> > plugin for this purpose. I have a feeling one will be necessary sooner
> > or later because of the need to customize the step/continue/etc. flows.
> > Currently, this will probably produce very bizarre if one tries to
> > execute those commands. The reason I'm bringing this up is because of
> > the `Target::GetTrace` method being added in the next patch. If there is
> > a trace-specific process class, then it might make sense for this class
> > to hold the trace object instead of adding the GetTrace method on every
> > Target object out there even though most targets will have that null. I
> > don't know if that will really be the case, but I think it's something
> > worth keeping in mind as you work on the subsequent patches.
> >
> > Very good remark. Probably we'll end up doing that when we start
> > implementing reverse debugging. The tricky thing we'll need to solve in
> > the future is seamlessly transition between a trace and a live process.
> > For example, when reverse debugging a live process, you might be stopped
> > at a breakpoint in the trace, then you do "continue", it reaches the end
> > of the trace, and then it resumes the live process. I still haven't
> > decided what we'll propose for this, but probably we'll have to change a
> > lot of the current code to make that happen nicely.
>
> Yes, that will be interesting. Even more interesting will be the
> question of how to communicate to the user the fact that even though we
> have "unwound" the PC to the previous location, variables and memory
> still contain the "live" values. Particularly, once we support rr-style
> reverse debugging which will "unwind" memory as well..
>
> >
> >> - I am puzzled by the TraceXXX vs. TraceXXXSettingsParser duality. The
> > two classes are very tightly coupled, so it's not clear to me what is
> > the advantage of separating it out this way (one could just move all the
> > relevant methods directly into the Trace class. What's the reason for
> > this design?
> >
> > Well, there's definitely coupling, but there are two reasons. One is
> > that the Trace of a live process won't need any parsing. Parsing is only
> > used when loading a trace from a json file. That makes parsing one of
> > the two main ways to create a Trace. And besides, I think that the
> > single responsibility principle is good to follow. The Parser class does
> > the parsing, and the Trace class holds an actual trace.
>
> Yeah, I'm all for the single responsibility principle, but the way it is
> implemented gives me pause. Like, if the Parser is supposed to "create"
> the Trace, then why does the code do it the other way around
> (Trace::CreateParser)? For one, that means that it will be possible to
> "parse" (by calling CreateParser()->...) any trace that you can get your
> hands on, even that of a live process. Ideally, the Parser would be the
> one creating a Trace object (in it's Parse function, or something), and
> after the Trace is created, it should no longer be possible to obtain
> the parser. All of this could be encapsulated in the "create_callback"
> that the plugin registers, and so the fact that there is a parser class
> involved would be completely unknown to the rest of the code. And then
> there could be a different create_callback which would create a Trace
> object suitable for tracing a live process...
>
> pl
>


-- 
- Walter Erquínigo Pezo
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88704: [lldb] Fix bug in fallback logic for finding the resource directory.

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG07c112574a32: [lldb] Fix bug in fallback logic for finding 
the resource directory. (authored by JDevlieghere).
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88704

Files:
  lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp


Index: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -137,14 +137,12 @@
 FileSystem::Instance().Resolve(file_spec);
 return true;
   }
-  raw_path = lldb_shlib_spec.GetPath();
 }
-raw_path.resize(rev_it - r_end);
-  } else {
-raw_path.resize(rev_it - r_end);
   }
 
   // Fall back to the Clang resource directory inside the framework.
+  raw_path = lldb_shlib_spec.GetPath();
+  raw_path.resize(rev_it - r_end);
   raw_path.append("LLDB.framework/Resources/Clang");
   file_spec.GetDirectory().SetString(raw_path.c_str());
   FileSystem::Instance().Resolve(file_spec);


Index: lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -137,14 +137,12 @@
 FileSystem::Instance().Resolve(file_spec);
 return true;
   }
-  raw_path = lldb_shlib_spec.GetPath();
 }
-raw_path.resize(rev_it - r_end);
-  } else {
-raw_path.resize(rev_it - r_end);
   }
 
   // Fall back to the Clang resource directory inside the framework.
+  raw_path = lldb_shlib_spec.GetPath();
+  raw_path.resize(rev_it - r_end);
   raw_path.append("LLDB.framework/Resources/Clang");
   file_spec.GetDirectory().SetString(raw_path.c_str());
   FileSystem::Instance().Resolve(file_spec);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 07c1125 - [lldb] Fix bug in fallback logic for finding the resource directory.

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-10-02T09:56:01-07:00
New Revision: 07c112574a324318a02ef29901a0d5aa1fd95144

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

LOG: [lldb] Fix bug in fallback logic for finding the resource directory.

Both of the if-clauses modify the raw_path variable and only one of them
was resetting the variable for the fallback. Avoid future bugs like that
by always resetting the variable.

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

Added: 


Modified: 
lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp

Removed: 




diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp 
b/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
index 8abb7e420575..b76fa6fbf690 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangHost.cpp
@@ -137,14 +137,12 @@ bool lldb_private::ComputeClangResourceDirectory(FileSpec 
&lldb_shlib_spec,
 FileSystem::Instance().Resolve(file_spec);
 return true;
   }
-  raw_path = lldb_shlib_spec.GetPath();
 }
-raw_path.resize(rev_it - r_end);
-  } else {
-raw_path.resize(rev_it - r_end);
   }
 
   // Fall back to the Clang resource directory inside the framework.
+  raw_path = lldb_shlib_spec.GetPath();
+  raw_path.resize(rev_it - r_end);
   raw_path.append("LLDB.framework/Resources/Clang");
   file_spec.GetDirectory().SetString(raw_path.c_str());
   FileSystem::Instance().Resolve(file_spec);



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

[Lldb-commits] [PATCH] D88682: [lldb] [Process/NetBSD] Fix crash on unsupported i386 regs

[Kamil Rytarowski via Phabricator via lldb-commits]

krytarowski added inline comments.



Comment at: 
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp:398
 assert(false && "Unhandled i386 register.");
-return 0;
+return -1;
   }

Use `llvm_unreachable` ? Same in other places where we add similar `assert()`.



Comment at: 
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp:414
+  return -1; // MPXR
+if (reg_num >= k_first_mpxr_i386 && reg_num <= k_last_mpxc_i386)
+  return -1; // MPXC

k_first_mpxr_i386 -> k_first_mpxc_i386 ?



Comment at: 
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp:428
   return -1; // MPXR
-else if (reg_num <= k_last_mpxc_x86_64)
+if (reg_num >= k_first_mpxr_x86_64 && reg_num <= k_last_mpxc_x86_64)
   return -1; // MPXC

k_first_mpxr_x86_64 -> k_first_mpxc_x86_64?


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

https://reviews.llvm.org/D88682

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

[Lldb-commits] [PATCH] D88681: [lldb] [Process/NetBSD] Fix reading FIP/FDP registers

[Kamil Rytarowski via Phabricator via lldb-commits]

krytarowski added inline comments.



Comment at: 
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp:954
   case lldb_foseg_x86_64:
-m_fpr.fxstate.fx_dp.fa_64 = reg_value.GetAsUInt64();
+m_fpr.fxstate.fx_dp.fa_32.fa_seg = reg_value.GetAsUInt64();
 break;

GetAsUInt32 above and GetAsUInt64 here?


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

https://reviews.llvm.org/D88681

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

[Lldb-commits] [PATCH] D88682: [lldb] [Process/NetBSD] Fix crash on unsupported i386 regs

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

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



Comment at: 
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp:398
 assert(false && "Unhandled i386 register.");
-return 0;
+return -1;
   }

krytarowski wrote:
> Use `llvm_unreachable` ? Same in other places where we add similar `assert()`.
@labath, what is the recommended practice for LLDB? Should I just replace these 
asserts with `llvm_unreachable(message)`?


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

https://reviews.llvm.org/D88682

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

[Lldb-commits] [PATCH] D88682: [lldb] [Process/NetBSD] Fix crash on unsupported i386 regs

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

mgorny updated this revision to Diff 295867.
mgorny added a comment.

Fixed wrong constant as pointed out by Kamil.


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

https://reviews.llvm.org/D88682

Files:
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
  lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h

Index: lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
===
--- lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
+++ lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
@@ -106,7 +106,7 @@
   lldb_bnd1_i386,
   lldb_bnd2_i386,
   lldb_bnd3_i386,
-  k_last_mpxr = lldb_bnd3_i386,
+  k_last_mpxr_i386 = lldb_bnd3_i386,
 
   k_first_mpxc_i386,
   lldb_bndcfgu_i386 = k_first_mpxc_i386,
Index: lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
===
--- lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
+++ lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
@@ -375,6 +375,15 @@
   case lldb_ymm6_i386:
   case lldb_ymm7_i386:
 return lldb_ymm0_x86_64 + regnum - lldb_ymm0_i386;
+  case lldb_bnd0_i386:
+  case lldb_bnd1_i386:
+  case lldb_bnd2_i386:
+  case lldb_bnd3_i386:
+return lldb_bnd0_x86_64 + regnum - lldb_bnd0_i386;
+  case lldb_bndcfgu_i386:
+return lldb_bndcfgu_x86_64;
+  case lldb_bndstatus_i386:
+return lldb_bndstatus_x86_64;
   case lldb_dr0_i386:
   case lldb_dr1_i386:
   case lldb_dr2_i386:
@@ -386,7 +395,7 @@
 return lldb_dr0_x86_64 + regnum - lldb_dr0_i386;
   default:
 assert(false && "Unhandled i386 register.");
-return 0;
+return -1;
   }
 }
 
@@ -394,35 +403,40 @@
 int reg_num) const {
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86:
-if (reg_num <= k_last_gpr_i386)
+if (reg_num >= k_first_gpr_i386 && reg_num <= k_last_gpr_i386)
   return GPRegSet;
-else if (reg_num <= k_last_fpr_i386)
+if (reg_num >= k_first_fpr_i386 && reg_num <= k_last_fpr_i386)
   return FPRegSet;
-else if (reg_num <= k_last_avx_i386)
+if (reg_num >= k_first_avx_i386 && reg_num <= k_last_avx_i386)
   return XStateRegSet; // AVX
-else if (reg_num <= lldb_dr7_i386)
+if (reg_num >= k_first_mpxr_i386 && reg_num <= k_last_mpxr_i386)
+  return -1; // MPXR
+if (reg_num >= k_first_mpxc_i386 && reg_num <= k_last_mpxc_i386)
+  return -1; // MPXC
+if (reg_num >= k_first_dbr_i386 && reg_num <= k_last_dbr_i386)
   return DBRegSet; // DBR
-else
-  return -1;
+break;
   case llvm::Triple::x86_64:
-if (reg_num <= k_last_gpr_x86_64)
+if (reg_num >= k_first_gpr_x86_64 && reg_num <= k_last_gpr_x86_64)
   return GPRegSet;
-else if (reg_num <= k_last_fpr_x86_64)
+if (reg_num >= k_first_fpr_x86_64 && reg_num <= k_last_fpr_x86_64)
   return FPRegSet;
-else if (reg_num <= k_last_avx_x86_64)
+if (reg_num >= k_first_avx_x86_64 && reg_num <= k_last_avx_x86_64)
   return XStateRegSet; // AVX
-else if (reg_num <= k_last_mpxr_x86_64)
+if (reg_num >= k_first_mpxr_x86_64 && reg_num <= k_last_mpxr_x86_64)
   return -1; // MPXR
-else if (reg_num <= k_last_mpxc_x86_64)
+if (reg_num >= k_first_mpxc_x86_64 && reg_num <= k_last_mpxc_x86_64)
   return -1; // MPXC
-else if (reg_num <= lldb_dr7_x86_64)
+if (reg_num >= k_first_dbr_x86_64 && reg_num <= k_last_dbr_x86_64)
   return DBRegSet; // DBR
-else
-  return -1;
+break;
   default:
 assert(false && "Unhandled target architecture.");
 return -1;
   }
+
+  assert(false && "Register does not belong to any register set");
+  return -1;
 }
 
 Status NativeRegisterContextNetBSD_x86_64::ReadRegisterSet(uint32_t set) {
@@ -758,6 +772,10 @@
   case lldb_dr7_x86_64:
 reg_value = (uint64_t)m_dbr.dr[reg - lldb_dr0_x86_64];
 break;
+  default:
+assert(false && "Reading unknown/unsupported register");
+error.SetErrorStringWithFormat("register \"%s\" unknown/unsupported",
+   reg_info->name);
   }
 
   return error;
@@ -1034,6 +1052,7 @@
 }
 #else
 error.SetErrorString("XState not supported by the kernel");
+return error;
 #endif
 break;
   case lldb_dr0_x86_64:
@@ -1046,6 +1065,11 @@
   case lldb_dr7_x86_64:
 m_dbr.dr[reg - lldb_dr0_x86_64] = reg_value.GetAsUInt64();
 break;
+  default:
+assert(false && "Reading unknown/unsupported register");
+error.SetErrorStringWithFormat("register \"%s\" unknown/unsupported",
+   reg_info->name);
+return error;
   }
 
   return WriteRegisterSet(set);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88681: [lldb] [Process/NetBSD] Fix reading FIP/FDP registers

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

mgorny updated this revision to Diff 295869.
mgorny marked an inline comment as done.
mgorny added a comment.

Fix mistype.


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

https://reviews.llvm.org/D88681

Files:
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp


Index: lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
===
--- lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
+++ lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
@@ -657,13 +657,13 @@
 reg_value = (uint64_t)m_fpr.fxstate.fx_opcode;
 break;
   case lldb_fiseg_x86_64:
-reg_value = (uint64_t)m_fpr.fxstate.fx_ip.fa_64;
+reg_value = (uint32_t)m_fpr.fxstate.fx_ip.fa_32.fa_seg;
 break;
   case lldb_fioff_x86_64:
 reg_value = (uint32_t)m_fpr.fxstate.fx_ip.fa_32.fa_off;
 break;
   case lldb_foseg_x86_64:
-reg_value = (uint64_t)m_fpr.fxstate.fx_dp.fa_64;
+reg_value = (uint32_t)m_fpr.fxstate.fx_dp.fa_32.fa_seg;
 break;
   case lldb_fooff_x86_64:
 reg_value = (uint32_t)m_fpr.fxstate.fx_dp.fa_32.fa_off;
@@ -945,13 +945,13 @@
 m_fpr.fxstate.fx_opcode = reg_value.GetAsUInt16();
 break;
   case lldb_fiseg_x86_64:
-m_fpr.fxstate.fx_ip.fa_64 = reg_value.GetAsUInt64();
+m_fpr.fxstate.fx_ip.fa_32.fa_seg = reg_value.GetAsUInt32();
 break;
   case lldb_fioff_x86_64:
 m_fpr.fxstate.fx_ip.fa_32.fa_off = reg_value.GetAsUInt32();
 break;
   case lldb_foseg_x86_64:
-m_fpr.fxstate.fx_dp.fa_64 = reg_value.GetAsUInt64();
+m_fpr.fxstate.fx_dp.fa_32.fa_seg = reg_value.GetAsUInt32();
 break;
   case lldb_fooff_x86_64:
 m_fpr.fxstate.fx_dp.fa_32.fa_off = reg_value.GetAsUInt32();


Index: lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
===
--- lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
+++ lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
@@ -657,13 +657,13 @@
 reg_value = (uint64_t)m_fpr.fxstate.fx_opcode;
 break;
   case lldb_fiseg_x86_64:
-reg_value = (uint64_t)m_fpr.fxstate.fx_ip.fa_64;
+reg_value = (uint32_t)m_fpr.fxstate.fx_ip.fa_32.fa_seg;
 break;
   case lldb_fioff_x86_64:
 reg_value = (uint32_t)m_fpr.fxstate.fx_ip.fa_32.fa_off;
 break;
   case lldb_foseg_x86_64:
-reg_value = (uint64_t)m_fpr.fxstate.fx_dp.fa_64;
+reg_value = (uint32_t)m_fpr.fxstate.fx_dp.fa_32.fa_seg;
 break;
   case lldb_fooff_x86_64:
 reg_value = (uint32_t)m_fpr.fxstate.fx_dp.fa_32.fa_off;
@@ -945,13 +945,13 @@
 m_fpr.fxstate.fx_opcode = reg_value.GetAsUInt16();
 break;
   case lldb_fiseg_x86_64:
-m_fpr.fxstate.fx_ip.fa_64 = reg_value.GetAsUInt64();
+m_fpr.fxstate.fx_ip.fa_32.fa_seg = reg_value.GetAsUInt32();
 break;
   case lldb_fioff_x86_64:
 m_fpr.fxstate.fx_ip.fa_32.fa_off = reg_value.GetAsUInt32();
 break;
   case lldb_foseg_x86_64:
-m_fpr.fxstate.fx_dp.fa_64 = reg_value.GetAsUInt64();
+m_fpr.fxstate.fx_dp.fa_32.fa_seg = reg_value.GetAsUInt32();
 break;
   case lldb_fooff_x86_64:
 m_fpr.fxstate.fx_dp.fa_32.fa_off = reg_value.GetAsUInt32();
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88682: [lldb] [Process/NetBSD] Fix crash on unsupported i386 regs

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.



Comment at: 
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp:398
 assert(false && "Unhandled i386 register.");
-return 0;
+return -1;
   }

mgorny wrote:
> krytarowski wrote:
> > Use `llvm_unreachable` ? Same in other places where we add similar 
> > `assert()`.
> @labath, what is the recommended practice for LLDB? Should I just replace 
> these asserts with `llvm_unreachable(message)`?
Yes, llvm_unreachable is better.


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

https://reviews.llvm.org/D88682

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

[Lldb-commits] [PATCH] D88516: [lldb] Add a "design" section to the documentation.

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere updated this revision to Diff 295873.
JDevlieghere added a comment.

Move architecture under design and rename the page to overview.


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

https://reviews.llvm.org/D88516

Files:
  lldb/docs/.htaccess
  lldb/docs/design/overview.rst
  lldb/docs/design/reproducers.rst
  lldb/docs/design/sbapi.rst
  lldb/docs/design/structureddataplugins.md
  lldb/docs/index.rst
  lldb/docs/resources/architecture.rst
  lldb/docs/resources/reproducers.rst
  lldb/docs/resources/sbapi.rst
  lldb/docs/resources/structureddataplugins.md


Index: lldb/docs/index.rst
===
--- lldb/docs/index.rst
+++ lldb/docs/index.rst
@@ -140,16 +140,23 @@
:maxdepth: 1
:caption: Development
 
-   resources/architecture
resources/contributing
resources/build
resources/test
resources/bots
-   resources/reproducers
-   resources/structureddataplugins
-   resources/sbapi
resources/caveats
 
+
+.. toctree::
+   :hidden:
+   :maxdepth: 1
+   :caption: Design
+
+   design/overview
+   design/reproducers
+   design/structureddataplugins
+   design/sbapi
+
 .. toctree::
:hidden:
:maxdepth: 1
Index: lldb/docs/design/overview.rst
===
--- lldb/docs/design/overview.rst
+++ lldb/docs/design/overview.rst
@@ -1,5 +1,5 @@
-Architecture
-
+Overview
+
 
 LLDB is a large and complex codebase. This section will help you become more
 familiar with the pieces that make up LLDB and give a general overview of the
Index: lldb/docs/.htaccess
===
--- lldb/docs/.htaccess
+++ lldb/docs/.htaccess
@@ -1,3 +1,4 @@
+# Old website redirects
 Redirect 301 /architecture/index.html 
https://lldb.llvm.org/resources/architecture.html
 Redirect 301 /cpp_reference/html/index.html 
https://lldb.llvm.org/cpp_reference/index.html
 Redirect 301 /features.html https://lldb.llvm.org/status/features.html
@@ -10,7 +11,10 @@
 Redirect 301 /tutorial.html https://lldb.llvm.org/use/tutorial.html
 Redirect 301 /varformats.html https://lldb.llvm.org/use/variable.html
 
-# Sphinx redirects
+# Current website redirects
 Redirect 301 /resources/source.html 
https://lldb.llvm.org/resources/contributing.html
 Redirect 301 /resources/download.html 
https://lldb.llvm.org/status/releases.html
 Redirect 301 /use/architecture.html 
https://lldb.llvm.org/resources/architecture.html
+Redirect 301 /resources/architecture.html 
https://lldb.llvm.org/design/overview.html
+Redirect 301 /resources/reproducers.html 
https://lldb.llvm.org/design/reproducers.html
+Redirect 301 /resources/sbapi.html https://lldb.llvm.org/design/sbapi.html


Index: lldb/docs/index.rst
===
--- lldb/docs/index.rst
+++ lldb/docs/index.rst
@@ -140,16 +140,23 @@
:maxdepth: 1
:caption: Development
 
-   resources/architecture
resources/contributing
resources/build
resources/test
resources/bots
-   resources/reproducers
-   resources/structureddataplugins
-   resources/sbapi
resources/caveats
 
+
+.. toctree::
+   :hidden:
+   :maxdepth: 1
+   :caption: Design
+
+   design/overview
+   design/reproducers
+   design/structureddataplugins
+   design/sbapi
+
 .. toctree::
:hidden:
:maxdepth: 1
Index: lldb/docs/design/overview.rst
===
--- lldb/docs/design/overview.rst
+++ lldb/docs/design/overview.rst
@@ -1,5 +1,5 @@
-Architecture
-
+Overview
+
 
 LLDB is a large and complex codebase. This section will help you become more
 familiar with the pieces that make up LLDB and give a general overview of the
Index: lldb/docs/.htaccess
===
--- lldb/docs/.htaccess
+++ lldb/docs/.htaccess
@@ -1,3 +1,4 @@
+# Old website redirects
 Redirect 301 /architecture/index.html https://lldb.llvm.org/resources/architecture.html
 Redirect 301 /cpp_reference/html/index.html https://lldb.llvm.org/cpp_reference/index.html
 Redirect 301 /features.html https://lldb.llvm.org/status/features.html
@@ -10,7 +11,10 @@
 Redirect 301 /tutorial.html https://lldb.llvm.org/use/tutorial.html
 Redirect 301 /varformats.html https://lldb.llvm.org/use/variable.html
 
-# Sphinx redirects
+# Current website redirects
 Redirect 301 /resources/source.html https://lldb.llvm.org/resources/contributing.html
 Redirect 301 /resources/download.html https://lldb.llvm.org/status/releases.html
 Redirect 301 /use/architecture.html https://lldb.llvm.org/resources/architecture.html
+Redirect 301 /resources/architecture.html https://lldb.llvm.org/design/overview.html
+Redirect 301 /resources/reproducers.html https://lldb.llvm.org/design/reproducers.html
+Redirect 301 /resources/sbapi.html https://lldb.llvm.org/design/sbapi.html

[Lldb-commits] [PATCH] D88682: [lldb] [Process/NetBSD] Fix crash on unsupported i386 regs

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

mgorny updated this revision to Diff 295875.
mgorny marked 2 inline comments as done.
mgorny edited the summary of this revision.
mgorny added a comment.

Use `llvm_unreachable()`.


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

https://reviews.llvm.org/D88682

Files:
  lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
  lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h

Index: lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
===
--- lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
+++ lldb/source/Plugins/Process/Utility/lldb-x86-register-enums.h
@@ -106,7 +106,7 @@
   lldb_bnd1_i386,
   lldb_bnd2_i386,
   lldb_bnd3_i386,
-  k_last_mpxr = lldb_bnd3_i386,
+  k_last_mpxr_i386 = lldb_bnd3_i386,
 
   k_first_mpxc_i386,
   lldb_bndcfgu_i386 = k_first_mpxc_i386,
Index: lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
===
--- lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
+++ lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
@@ -278,11 +278,8 @@
   case llvm::Triple::x86_64:
 return &g_reg_sets_x86_64[set_index];
   default:
-assert(false && "Unhandled target architecture.");
-return nullptr;
+llvm_unreachable("Unhandled target architecture.");
   }
-
-  return nullptr;
 }
 
 static constexpr int RegNumX86ToX86_64(int regnum) {
@@ -375,6 +372,15 @@
   case lldb_ymm6_i386:
   case lldb_ymm7_i386:
 return lldb_ymm0_x86_64 + regnum - lldb_ymm0_i386;
+  case lldb_bnd0_i386:
+  case lldb_bnd1_i386:
+  case lldb_bnd2_i386:
+  case lldb_bnd3_i386:
+return lldb_bnd0_x86_64 + regnum - lldb_bnd0_i386;
+  case lldb_bndcfgu_i386:
+return lldb_bndcfgu_x86_64;
+  case lldb_bndstatus_i386:
+return lldb_bndstatus_x86_64;
   case lldb_dr0_i386:
   case lldb_dr1_i386:
   case lldb_dr2_i386:
@@ -385,8 +391,7 @@
   case lldb_dr7_i386:
 return lldb_dr0_x86_64 + regnum - lldb_dr0_i386;
   default:
-assert(false && "Unhandled i386 register.");
-return 0;
+llvm_unreachable("Unhandled i386 register.");
   }
 }
 
@@ -394,35 +399,38 @@
 int reg_num) const {
   switch (GetRegisterInfoInterface().GetTargetArchitecture().GetMachine()) {
   case llvm::Triple::x86:
-if (reg_num <= k_last_gpr_i386)
+if (reg_num >= k_first_gpr_i386 && reg_num <= k_last_gpr_i386)
   return GPRegSet;
-else if (reg_num <= k_last_fpr_i386)
+if (reg_num >= k_first_fpr_i386 && reg_num <= k_last_fpr_i386)
   return FPRegSet;
-else if (reg_num <= k_last_avx_i386)
+if (reg_num >= k_first_avx_i386 && reg_num <= k_last_avx_i386)
   return XStateRegSet; // AVX
-else if (reg_num <= lldb_dr7_i386)
+if (reg_num >= k_first_mpxr_i386 && reg_num <= k_last_mpxr_i386)
+  return -1; // MPXR
+if (reg_num >= k_first_mpxc_i386 && reg_num <= k_last_mpxc_i386)
+  return -1; // MPXC
+if (reg_num >= k_first_dbr_i386 && reg_num <= k_last_dbr_i386)
   return DBRegSet; // DBR
-else
-  return -1;
+break;
   case llvm::Triple::x86_64:
-if (reg_num <= k_last_gpr_x86_64)
+if (reg_num >= k_first_gpr_x86_64 && reg_num <= k_last_gpr_x86_64)
   return GPRegSet;
-else if (reg_num <= k_last_fpr_x86_64)
+if (reg_num >= k_first_fpr_x86_64 && reg_num <= k_last_fpr_x86_64)
   return FPRegSet;
-else if (reg_num <= k_last_avx_x86_64)
+if (reg_num >= k_first_avx_x86_64 && reg_num <= k_last_avx_x86_64)
   return XStateRegSet; // AVX
-else if (reg_num <= k_last_mpxr_x86_64)
+if (reg_num >= k_first_mpxr_x86_64 && reg_num <= k_last_mpxr_x86_64)
   return -1; // MPXR
-else if (reg_num <= k_last_mpxc_x86_64)
+if (reg_num >= k_first_mpxc_x86_64 && reg_num <= k_last_mpxc_x86_64)
   return -1; // MPXC
-else if (reg_num <= lldb_dr7_x86_64)
+if (reg_num >= k_first_dbr_x86_64 && reg_num <= k_last_dbr_x86_64)
   return DBRegSet; // DBR
-else
-  return -1;
+break;
   default:
-assert(false && "Unhandled target architecture.");
-return -1;
+llvm_unreachable("Unhandled target architecture.");
   }
+
+  llvm_unreachable("Register does not belong to any register set");
 }
 
 Status NativeRegisterContextNetBSD_x86_64::ReadRegisterSet(uint32_t set) {
@@ -511,9 +519,7 @@
 reg = RegNumX86ToX86_64(reg);
 break;
   default:
-assert(false && "Unhandled target architecture.");
-error.SetErrorString("Unhandled target architecture.");
-return error;
+llvm_unreachable("Unhandled target architecture.");
   }
 
   error = ReadRegisterSet(set);
@@ -758,6 +764,8 @@
   case lldb_dr7_x86_64:
 reg_value = (uint64_t)m_dbr.dr[reg - lldb_dr0_x86_64];
 break;
+  default:
+llvm_unreachable("Reading unknown/unsupported register");
   }
 
   return error;
@@ -799,9 +807,7 @@
 reg = RegNumX86ToX86_64(reg);
 br

[Lldb-commits] [PATCH] D87868: [RFC] When calling the process mmap try to call all found instead of just the first one

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D87868#2308515 , @labath wrote:

> That sounds like a plan. FWIW, here's the implementation I hacked up today:
>
>   Status NativeProcessLinux::AllocateMemory(size_t size, uint32_t permissions,
> lldb::addr_t &addr) {
> PopulateMemoryRegionCache();
> auto region_it = llvm::find_if(m_mem_region_cache, [](const auto &pair) {
>   return pair.first.GetExecutable() == MemoryRegionInfo::eYes;
> });
> if (region_it == m_mem_region_cache.end())
>   return Status("No executable memory region found!");
> addr_t exe_addr = region_it->first.GetRange().GetRangeBase();
>   
> NativeThreadLinux &thread = *GetThreadByID(GetID());
> assert(thread.GetState() == eStateStopped);
>   
> int prot = PROT_NONE;
> if (permissions & ePermissionsReadable)
>   prot |= PROT_READ;
> if (permissions & ePermissionsWritable)
>   prot |= PROT_WRITE;
> if (permissions & ePermissionsExecutable)
>   prot |= PROT_EXEC;
>   
> NativeRegisterContextLinux ®_ctx = thread.GetRegisterContext();
> DataBufferSP registers_sp;
> reg_ctx.ReadAllRegisterValues(registers_sp);
> uint8_t memory[2];
> size_t bytes_read;
> ReadMemory(exe_addr, memory, 2, bytes_read);
>   
> reg_ctx.SetPC(exe_addr);
> reg_ctx.WriteRegisterFromUnsigned(lldb_rax_x86_64, SYS_mmap);
> reg_ctx.WriteRegisterFromUnsigned(lldb_rdi_x86_64, 0);
> reg_ctx.WriteRegisterFromUnsigned(lldb_rsi_x86_64, size);
> reg_ctx.WriteRegisterFromUnsigned(lldb_rdx_x86_64, prot);
> reg_ctx.WriteRegisterFromUnsigned(lldb_r10_x86_64,
>   MAP_ANONYMOUS | MAP_PRIVATE);
> reg_ctx.WriteRegisterFromUnsigned(lldb_r8_x86_64, -1);
> reg_ctx.WriteRegisterFromUnsigned(lldb_r9_x86_64, 0);
> WriteMemory(exe_addr, "\x0f\x05", 2, bytes_read);
> PtraceWrapper(PTRACE_SINGLESTEP, thread.GetID(), nullptr, nullptr);
> int status;
> ::pid_t wait_pid = llvm::sys::RetryAfterSignal(-1, ::waitpid, 
> thread.GetID(),
>&status, __WALL);
> assert((unsigned)wait_pid == thread.GetID());
> addr = reg_ctx.ReadRegisterAsUnsigned(lldb_rax_x86_64, -ESRCH);
>   
> WriteMemory(exe_addr, memory, 2, bytes_read);
> reg_ctx.WriteAllRegisterValues(registers_sp);
>   
> if (addr > -4096)
>   return Status(-addr, eErrorTypePOSIX);
> return Status();
>   }
>
> It needs more error handling, and generalization to non-x86 architectures, 
> but it actually works, and is enough to make all but one test pass 
> (TestBitfields.py seems to fail due to some data corruption -- quite 
> puzzling).

Very cool. That seems simple enough to allow us to try this out, at least on 
unix variants. Does PTRACE_SINGLESTEP cause _only_ the current thread to single 
step and keep all other threads paused? We can't allow any other threads making 
any progress when the process is briefly resumed in a multi-threaded process.

This nice thing is lldb-server _is_ compiled natively for each system so we can 
rely on system headers for the sys call numbers if they are available. So looks 
like this could be made to work. Did you also do the deallocate memory? I ran a 
few expressions and I am not sure that we ever use the deallocate memory 
packets to debugserver on mac, so it might not be needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87868

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

[Lldb-commits] [PATCH] D87868: [RFC] When calling the process mmap try to call all found instead of just the first one

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D87868#2309260 , @clayborg wrote:

> In D87868#2308515 , @labath wrote:
>
>> That sounds like a plan. FWIW, here's the implementation I hacked up today:
>>
>>   Status NativeProcessLinux::AllocateMemory(size_t size, uint32_t 
>> permissions,
>> lldb::addr_t &addr) {
>> PopulateMemoryRegionCache();
>> auto region_it = llvm::find_if(m_mem_region_cache, [](const auto &pair) {
>>   return pair.first.GetExecutable() == MemoryRegionInfo::eYes;
>> });
>> if (region_it == m_mem_region_cache.end())
>>   return Status("No executable memory region found!");
>> addr_t exe_addr = region_it->first.GetRange().GetRangeBase();
>>   
>> NativeThreadLinux &thread = *GetThreadByID(GetID());
>> assert(thread.GetState() == eStateStopped);
>>   
>> int prot = PROT_NONE;
>> if (permissions & ePermissionsReadable)
>>   prot |= PROT_READ;
>> if (permissions & ePermissionsWritable)
>>   prot |= PROT_WRITE;
>> if (permissions & ePermissionsExecutable)
>>   prot |= PROT_EXEC;
>>   
>> NativeRegisterContextLinux ®_ctx = thread.GetRegisterContext();
>> DataBufferSP registers_sp;
>> reg_ctx.ReadAllRegisterValues(registers_sp);
>> uint8_t memory[2];
>> size_t bytes_read;
>> ReadMemory(exe_addr, memory, 2, bytes_read);
>>   
>> reg_ctx.SetPC(exe_addr);
>> reg_ctx.WriteRegisterFromUnsigned(lldb_rax_x86_64, SYS_mmap);
>> reg_ctx.WriteRegisterFromUnsigned(lldb_rdi_x86_64, 0);
>> reg_ctx.WriteRegisterFromUnsigned(lldb_rsi_x86_64, size);
>> reg_ctx.WriteRegisterFromUnsigned(lldb_rdx_x86_64, prot);
>> reg_ctx.WriteRegisterFromUnsigned(lldb_r10_x86_64,
>>   MAP_ANONYMOUS | MAP_PRIVATE);
>> reg_ctx.WriteRegisterFromUnsigned(lldb_r8_x86_64, -1);
>> reg_ctx.WriteRegisterFromUnsigned(lldb_r9_x86_64, 0);
>> WriteMemory(exe_addr, "\x0f\x05", 2, bytes_read);
>> PtraceWrapper(PTRACE_SINGLESTEP, thread.GetID(), nullptr, nullptr);
>> int status;
>> ::pid_t wait_pid = llvm::sys::RetryAfterSignal(-1, ::waitpid, 
>> thread.GetID(),
>>&status, __WALL);
>> assert((unsigned)wait_pid == thread.GetID());
>> addr = reg_ctx.ReadRegisterAsUnsigned(lldb_rax_x86_64, -ESRCH);
>>   
>> WriteMemory(exe_addr, memory, 2, bytes_read);
>> reg_ctx.WriteAllRegisterValues(registers_sp);
>>   
>> if (addr > -4096)
>>   return Status(-addr, eErrorTypePOSIX);
>> return Status();
>>   }
>>
>> It needs more error handling, and generalization to non-x86 architectures, 
>> but it actually works, and is enough to make all but one test pass 
>> (TestBitfields.py seems to fail due to some data corruption -- quite 
>> puzzling).
>
> Very cool. That seems simple enough to allow us to try this out, at least on 
> unix variants. Does PTRACE_SINGLESTEP cause _only_ the current thread to 
> single step and keep all other threads paused? We can't allow any other 
> threads making any progress when the process is briefly resumed in a 
> multi-threaded process.
>
> This nice thing is lldb-server _is_ compiled natively for each system so we 
> can rely on system headers for the sys call numbers if they are available. So 
> looks like this could be made to work. Did you also do the deallocate memory? 
> I ran a few expressions and I am not sure that we ever use the deallocate 
> memory packets to debugserver on mac, so it might not be needed.

We deallocate memory when we remove the space we've made for expression result 
variables and the like after running expressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87868

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

[Lldb-commits] [PATCH] D88753: Fix raciness in the check for whether a stop hook has run the target

[Jim Ingham via Phabricator via lldb-commits]

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

This was originally done by checking the private state to see if it was 
"eStateRunning" or not.  If it is running we need mark the stop event we're 
currently processing as "restarted" so the code like Process::ResumeSynchronous 
will know to keep waiting for a real stop.

But that is racy because between the time that the process gets restarted and 
the time we do the check, the process could have stopped again.

I used the private state because where the stop-hooks are run we are in the 
process of computing Public state so it won't show the effect of the restart 
yet.

Instead, this patch switches to just asking the stop hook whether it ran or 
not, and then marking the stop event as restarted or not based on the answer.  
The CommandLine stop hooks know this definitively - or at least they do 
provided the command return status is returned correctly.  The Python stop 
hooks shouldn't be directly restarting the target, they should use the return 
values to request a restart.

I'll deal later on with the cases where (a) a command returns the wrong status 
and (b) somebody runs Continue/etc in a scripted stop hook even though they 
shouldn't by adding a mode to the Process where the high level functions that 
could run the target don't but instead mark an intention to continue the 
target.  We will turn that on when we start processing stop actions, and then 
when we're done with all the stop actions, check whether everybody agrees we 
should continue.

That will also solve the unfairness problem in the stop actions, where the 
first stop action that runs a "continue" command, or an SB API that continues 
the target causes all the other stop actions don't get a chance to run.  I've 
added auto-continue flags and for python return values to request running so 
that you CAN do the right thing.  But it would be better to make the right 
thing happen automatically.

But I'm leaving that for a separate patch because it's a much bigger chunk of 
work, and this should be sufficient for now - only failing when people do 
things they shouldn't do...


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88753

Files:
  lldb/include/lldb/Target/Target.h
  lldb/source/Target/Process.cpp
  lldb/source/Target/Target.cpp
  lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py

Index: lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
===
--- lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
+++ lldb/test/API/commands/target/stop-hooks/TestStopHookScripted.py
@@ -71,8 +71,6 @@
 """Test that the returning False from a stop hook works"""
 self.do_test_auto_continue(True)
 
-# Test is flakey on Linux.
-@skipIfLinux
 def do_test_auto_continue(self, return_true):
 """Test that auto-continue works."""
 # We set auto-continue to 1 but the stop hook only applies to step_out_of_me,
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2541,25 +2541,26 @@
   }
 }
 
-void Target::RunStopHooks() {
+bool Target::RunStopHooks() {
   if (m_suppress_stop_hooks)
-return;
+return false;
 
   if (!m_process_sp)
-return;
+return false;
 
   // Somebody might have restarted the process:
+  // Still return false, the return value is about US restarting the target.
   if (m_process_sp->GetState() != eStateStopped)
-return;
+return false;
 
   //  make sure we check that we are not stopped
   // because of us running a user expression since in that case we do not want
   // to run the stop-hooks
   if (m_process_sp->GetModIDRef().IsLastResumeForUserExpression())
-return;
+return false;
 
   if (m_stop_hooks.empty())
-return;
+return false;
 
   // If there aren't any active stop hooks, don't bother either.
   bool any_active_hooks = false;
@@ -2570,7 +2571,7 @@
 }
   }
   if (!any_active_hooks)
-return;
+return false;
 
   std::vector exc_ctx_with_reasons;
 
@@ -2588,7 +2589,7 @@
   // If no threads stopped for a reason, don't run the stop-hooks.
   size_t num_exe_ctx = exc_ctx_with_reasons.size();
   if (num_exe_ctx == 0)
-return;
+return false;
 
   StreamSP output_sp = m_debugger.GetAsyncOutputStream();
 
@@ -2636,22 +2637,27 @@
 output_sp->Printf("-- Thread %d\n",
   exc_ctx.GetThreadPtr()->GetIndexID());
 
-  bool this_should_stop = cur_hook_sp->HandleStop(exc_ctx, output_sp);
-  // If this hook is set to auto-continue that should override the
-  // HandleStop result...
-  if (cur_hook_sp->GetAutoContinue())
-this_should_

[Lldb-commits] [lldb] 128e999 - [lldb] Add a "design" section to the documentation.

[Jonas Devlieghere via lldb-commits]

Author: Jonas Devlieghere
Date: 2020-10-02T13:07:31-07:00
New Revision: 128e999d63c41e54d5d73c8af47e1ce401e6a200

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

LOG: [lldb] Add a "design" section to the documentation.

Create a "Design" section for the LLDB documentation. The goal is to
have design documents that describe how the LLDB internals work.

Currently similar pages  are mixed together under the "Development". The
existing pages describing the architecture, the reproducers, the
structured data plugins, and the SB API could be housed here. I hope
we'd see more pages being added here in the future.

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

Added: 
lldb/docs/design/overview.rst
lldb/docs/design/reproducers.rst
lldb/docs/design/sbapi.rst
lldb/docs/design/structureddataplugins.md

Modified: 
lldb/docs/.htaccess
lldb/docs/index.rst

Removed: 
lldb/docs/resources/architecture.rst
lldb/docs/resources/reproducers.rst
lldb/docs/resources/sbapi.rst
lldb/docs/resources/structureddataplugins.md



diff  --git a/lldb/docs/.htaccess b/lldb/docs/.htaccess
index 596f4481ab49..31b80359fb5f 100644
--- a/lldb/docs/.htaccess
+++ b/lldb/docs/.htaccess
@@ -1,3 +1,4 @@
+# Old website redirects
 Redirect 301 /architecture/index.html 
https://lldb.llvm.org/resources/architecture.html
 Redirect 301 /cpp_reference/html/index.html 
https://lldb.llvm.org/cpp_reference/index.html
 Redirect 301 /features.html https://lldb.llvm.org/status/features.html
@@ -10,7 +11,10 @@ Redirect 301 /source.html 
https://lldb.llvm.org/resources/contributing.html
 Redirect 301 /tutorial.html https://lldb.llvm.org/use/tutorial.html
 Redirect 301 /varformats.html https://lldb.llvm.org/use/variable.html
 
-# Sphinx redirects
+# Current website redirects
 Redirect 301 /resources/source.html 
https://lldb.llvm.org/resources/contributing.html
 Redirect 301 /resources/download.html 
https://lldb.llvm.org/status/releases.html
 Redirect 301 /use/architecture.html 
https://lldb.llvm.org/resources/architecture.html
+Redirect 301 /resources/architecture.html 
https://lldb.llvm.org/design/overview.html
+Redirect 301 /resources/reproducers.html 
https://lldb.llvm.org/design/reproducers.html
+Redirect 301 /resources/sbapi.html https://lldb.llvm.org/design/sbapi.html

diff  --git a/lldb/docs/resources/architecture.rst 
b/lldb/docs/design/overview.rst
similarity index 99%
rename from lldb/docs/resources/architecture.rst
rename to lldb/docs/design/overview.rst
index e87d1248d401..72eac56d6c3e 100644
--- a/lldb/docs/resources/architecture.rst
+++ b/lldb/docs/design/overview.rst
@@ -1,5 +1,5 @@
-Architecture
-
+Overview
+
 
 LLDB is a large and complex codebase. This section will help you become more
 familiar with the pieces that make up LLDB and give a general overview of the

diff  --git a/lldb/docs/resources/reproducers.rst 
b/lldb/docs/design/reproducers.rst
similarity index 100%
rename from lldb/docs/resources/reproducers.rst
rename to lldb/docs/design/reproducers.rst

diff  --git a/lldb/docs/resources/sbapi.rst b/lldb/docs/design/sbapi.rst
similarity index 99%
rename from lldb/docs/resources/sbapi.rst
rename to lldb/docs/design/sbapi.rst
index 048f6c12d972..674fd680b907 100644
--- a/lldb/docs/resources/sbapi.rst
+++ b/lldb/docs/design/sbapi.rst
@@ -1,5 +1,5 @@
-The SB API Coding Rules
-===
+Scripting Bridge API
+
 
 The SB APIs constitute the stable C++ API that lldb presents to external
 clients, and which get processed by SWIG to produce the Python bindings to

diff  --git a/lldb/docs/resources/structureddataplugins.md 
b/lldb/docs/design/structureddataplugins.md
similarity index 100%
rename from lldb/docs/resources/structureddataplugins.md
rename to lldb/docs/design/structureddataplugins.md

diff  --git a/lldb/docs/index.rst b/lldb/docs/index.rst
index 77c3dbbf6f74..909089f3cebe 100644
--- a/lldb/docs/index.rst
+++ b/lldb/docs/index.rst
@@ -140,16 +140,23 @@ interesting areas to contribute to lldb.
:maxdepth: 1
:caption: Development
 
-   resources/architecture
resources/contributing
resources/build
resources/test
resources/bots
-   resources/reproducers
-   resources/structureddataplugins
-   resources/sbapi
resources/caveats
 
+
+.. toctree::
+   :hidden:
+   :maxdepth: 1
+   :caption: Design
+
+   design/overview
+   design/reproducers
+   design/structureddataplugins
+   design/sbapi
+
 .. toctree::
:hidden:
:maxdepth: 1



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

[Lldb-commits] [PATCH] D88516: [lldb] Add a "design" section to the documentation.

[Jonas Devlieghere via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG128e999d63c4: [lldb] Add a "design" section to the 
documentation. (authored by JDevlieghere).
Herald added a project: LLDB.

Changed prior to commit:
  https://reviews.llvm.org/D88516?vs=295873&id=295897#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88516

Files:
  lldb/docs/.htaccess
  lldb/docs/design/overview.rst
  lldb/docs/design/reproducers.rst
  lldb/docs/design/sbapi.rst
  lldb/docs/design/structureddataplugins.md
  lldb/docs/index.rst
  lldb/docs/resources/architecture.rst
  lldb/docs/resources/reproducers.rst
  lldb/docs/resources/sbapi.rst
  lldb/docs/resources/structureddataplugins.md


Index: lldb/docs/index.rst
===
--- lldb/docs/index.rst
+++ lldb/docs/index.rst
@@ -140,16 +140,23 @@
:maxdepth: 1
:caption: Development
 
-   resources/architecture
resources/contributing
resources/build
resources/test
resources/bots
-   resources/reproducers
-   resources/structureddataplugins
-   resources/sbapi
resources/caveats
 
+
+.. toctree::
+   :hidden:
+   :maxdepth: 1
+   :caption: Design
+
+   design/overview
+   design/reproducers
+   design/structureddataplugins
+   design/sbapi
+
 .. toctree::
:hidden:
:maxdepth: 1
Index: lldb/docs/design/sbapi.rst
===
--- lldb/docs/design/sbapi.rst
+++ lldb/docs/design/sbapi.rst
@@ -1,5 +1,5 @@
-The SB API Coding Rules
-===
+Scripting Bridge API
+
 
 The SB APIs constitute the stable C++ API that lldb presents to external
 clients, and which get processed by SWIG to produce the Python bindings to
Index: lldb/docs/design/overview.rst
===
--- lldb/docs/design/overview.rst
+++ lldb/docs/design/overview.rst
@@ -1,5 +1,5 @@
-Architecture
-
+Overview
+
 
 LLDB is a large and complex codebase. This section will help you become more
 familiar with the pieces that make up LLDB and give a general overview of the
Index: lldb/docs/.htaccess
===
--- lldb/docs/.htaccess
+++ lldb/docs/.htaccess
@@ -1,3 +1,4 @@
+# Old website redirects
 Redirect 301 /architecture/index.html 
https://lldb.llvm.org/resources/architecture.html
 Redirect 301 /cpp_reference/html/index.html 
https://lldb.llvm.org/cpp_reference/index.html
 Redirect 301 /features.html https://lldb.llvm.org/status/features.html
@@ -10,7 +11,10 @@
 Redirect 301 /tutorial.html https://lldb.llvm.org/use/tutorial.html
 Redirect 301 /varformats.html https://lldb.llvm.org/use/variable.html
 
-# Sphinx redirects
+# Current website redirects
 Redirect 301 /resources/source.html 
https://lldb.llvm.org/resources/contributing.html
 Redirect 301 /resources/download.html 
https://lldb.llvm.org/status/releases.html
 Redirect 301 /use/architecture.html 
https://lldb.llvm.org/resources/architecture.html
+Redirect 301 /resources/architecture.html 
https://lldb.llvm.org/design/overview.html
+Redirect 301 /resources/reproducers.html 
https://lldb.llvm.org/design/reproducers.html
+Redirect 301 /resources/sbapi.html https://lldb.llvm.org/design/sbapi.html


Index: lldb/docs/index.rst
===
--- lldb/docs/index.rst
+++ lldb/docs/index.rst
@@ -140,16 +140,23 @@
:maxdepth: 1
:caption: Development
 
-   resources/architecture
resources/contributing
resources/build
resources/test
resources/bots
-   resources/reproducers
-   resources/structureddataplugins
-   resources/sbapi
resources/caveats
 
+
+.. toctree::
+   :hidden:
+   :maxdepth: 1
+   :caption: Design
+
+   design/overview
+   design/reproducers
+   design/structureddataplugins
+   design/sbapi
+
 .. toctree::
:hidden:
:maxdepth: 1
Index: lldb/docs/design/sbapi.rst
===
--- lldb/docs/design/sbapi.rst
+++ lldb/docs/design/sbapi.rst
@@ -1,5 +1,5 @@
-The SB API Coding Rules
-===
+Scripting Bridge API
+
 
 The SB APIs constitute the stable C++ API that lldb presents to external
 clients, and which get processed by SWIG to produce the Python bindings to
Index: lldb/docs/design/overview.rst
===
--- lldb/docs/design/overview.rst
+++ lldb/docs/design/overview.rst
@@ -1,5 +1,5 @@
-Architecture
-
+Overview
+
 
 LLDB is a large and complex codebase. This section will help you become more
 familiar with the pieces that make up LLDB and give a general overview of the
Index: lldb/docs/.htaccess
===
--- lldb/docs/.htaccess
+++ lldb/doc

[Lldb-commits] [PATCH] D88753: Fix raciness in the check for whether a stop hook has run the target

[Jonas Devlieghere via Phabricator via lldb-commits]

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

LGTM




Comment at: lldb/source/Target/Target.cpp:2648-2651
+if (cur_hook_sp->GetAutoContinue())
+  this_should_stop = false;
+else
+  this_should_stop = true;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88753

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

[Lldb-commits] [PATCH] D88769: [trace] Scaffold "thread trace dump instructions"

[walter erquinigo via Phabricator via lldb-commits]

wallace created this revision.
wallace added reviewers: clayborg, labath.
Herald added subscribers: lldb-commits, dang, mgorny.
Herald added a reviewer: JDevlieghere.
Herald added a project: LLDB.
wallace requested review of this revision.

As per the discussion in the RFC, we'll implement both

  thread thread dump [instructions | functions]

This is the first step in implementing the "instructions" dumping command.

It includes:

- A minimal ProcessTrace plugin for representing processes from a trace file. I 
noticed that it was a required step to mimic how core-based processes are 
initialized, e.g. ProcessElfCore and ProcessMinidump. I haven't had the need to 
create ThreadTrace yet, though. So far HistoryThread seems good enough.
- The command handling itself in CommandObjectThread, which outputs a 
placeholder text instead of the actual instructions. I'll do that part in the 
next diff.
- Tests


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88769

Files:
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/Target/Trace.h
  lldb/source/Commands/CommandObjectThread.cpp
  lldb/source/Commands/Options.td
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/Trace/CMakeLists.txt
  lldb/source/Plugins/Process/Trace/ProcessTrace.cpp
  lldb/source/Plugins/Process/Trace/ProcessTrace.h
  lldb/source/Plugins/Process/Utility/HistoryThread.cpp
  lldb/source/Plugins/Process/Utility/HistoryThread.h
  lldb/source/Target/Target.cpp
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceSettingsParser.cpp
  lldb/test/API/commands/trace/TestTraceDumpInstructions.py
  lldb/test/API/commands/trace/TestTraceLoad.py

Index: lldb/test/API/commands/trace/TestTraceLoad.py
===
--- lldb/test/API/commands/trace/TestTraceLoad.py
+++ lldb/test/API/commands/trace/TestTraceLoad.py
@@ -35,6 +35,10 @@
 
 self.assertEqual("6AA9A4E2-6F28-2F33-377D-59FECE874C71-5B41261A", module.GetUUIDString())
 
+# check that the Process and Thread objects were created correctly
+self.expect("thread info", substrs=["tid = 3842849"])
+self.expect("thread list", substrs=["Process 1234 stopped", "tid = 3842849"])
+
 
 def testLoadInvalidTraces(self):
 src_dir = self.getSourceDir()
Index: lldb/test/API/commands/trace/TestTraceDumpInstructions.py
===
--- /dev/null
+++ lldb/test/API/commands/trace/TestTraceDumpInstructions.py
@@ -0,0 +1,49 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+from lldbsuite.test.decorators import *
+
+class TestTraceDumpInstructions(TestBase):
+
+mydir = TestBase.compute_mydir(__file__)
+NO_DEBUG_INFO_TESTCASE = True
+
+def setUp(self):
+TestBase.setUp(self)
+if 'intel-pt' not in configuration.enabled_plugins:
+self.skipTest("The intel-pt test plugin is not enabled")
+
+def testErrorMessages(self):
+# We first check the output when there are no targets
+self.expect("thread trace dump instructions", 
+substrs=["error: invalid target, create a target using the 'target create' command"],
+error=True)
+
+# We now check the output when there's a non-running target
+self.expect("target create " + os.path.join(self.getSourceDir(), "intelpt-trace", "a.out"))
+
+self.expect("thread trace dump instructions", 
+substrs=["error: invalid process"],
+error=True)
+
+# Now we check the output when there's a running target without a trace
+self.expect("b main")
+self.expect("r")
+
+self.expect("thread trace dump instructions", 
+substrs=["error: no trace in this target"])
+
+def testDumpInstructions(self):
+self.expect("trace load -v " + os.path.join(self.getSourceDir(), "intelpt-trace", "trace.json"),
+substrs=["intel-pt"])
+
+self.expect("thread trace dump instructions",
+substrs=['placeholder trace of tid 3842849, count = 10, offset = 0'])
+
+# We check if we can pass count and offset
+self.expect("thread trace dump instructions -c 5 -o 10",
+substrs=['placeholder trace of tid 3842849, count = 5, offset = 10'])
+
+# We check if we can access the thread by index id
+self.expect("thread trace dump instructions 1",
+substrs=['placeholder trace of tid 3842849, count = 10, offset = 0'])
Index: lldb/source/Target/TraceSettingsParser.cpp
===
--- lldb/source/Target/TraceSettingsParser.cpp
+++ lldb/source/Target/TraceSettingsParser.cpp
@@ -65,7 +65,12 @@
   NormalizePath(spec);
   m_thread_to_trace_file_map[process_sp->GetID()][tid] = spec;
 
-  ThreadSP thread_sp(new HistoryThread(*process_sp, tid, /*callstack*/ {}));
+  // For now we are setting an invalid PC.

[Lldb-commits] [PATCH] D88387: Create "skinny corefiles" for Mach-O with process save-core / reading

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 295963.
jasonmolenda added a comment.
Herald added a subscriber: dang.

This patch update incorporates most of Greg's suggestions.  Most importantly, I 
isolated all of the loading of the binaries specified in the corefile down into 
ObjectFileMachO so I didn't have to add so much special case API to the 
ObjectFile base API.

Greg made a case in direct email for the usefulness of a full coredump -- e.g. 
people may save a skinny core on one mac system, and send it to someone on a 
different mac system where the system libraries are all different, and no 
backtraces would be meaningful.  This patch implements a -f option to process 
save-core to force a full coredump, if the system defaults to skinny (as it 
will on macos).

The alternative we were discussing is a hybrid approach where the core dumper 
saves all of the memory pages that cover the binary, for images that are 
currently executing on a thread.  We'd need the full symbol table, mach-o 
header, and text which is executing, so it's probably easier to just copy the 
whole thing in than to try to pare it back.  This is an interesting idea, and 
most of the groundwork is laid for it - we are already computing which 
libraries are actively executing and noting that in the corefile metadata so 
lldb can do more expensive searches for those binary images.

I switched to using the RangeDataVector container, but it has a little bug in 
the CombineConsecutiveEntriesWithEqualData where consecutive entries with the 
same Data value are combined, even if their ranges are not consecutive.  So you 
could get a vast address range claimed if a user process page and a shared 
cache page had the same protections.  Fixing that and writing a test case, I 
found a second problem where RangeDataVector isn't setting its 'upper_bound' 
field properly for this use case and searches don't work as they should.  At 
which point I booted it out of the patch and went back to the hand-rolled 
container that I'd started with; I'll try to look at RangeDataVector more 
closely next week and switch to using it in this method.  It is a cleaner and 
simpler approach.

I also got rid of the "executing uuids" LC_NOTE - as Greg pointed out, I have a 
perfectly good unused field for alignment in the binary image entries already, 
using that to flag executing images was even easier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88387

Files:
  lldb/docs/lldb-gdb-remote.txt
  lldb/include/lldb/Core/PluginManager.h
  lldb/include/lldb/Symbol/ObjectFile.h
  lldb/include/lldb/Target/MemoryRegionInfo.h
  lldb/include/lldb/lldb-private-interfaces.h
  lldb/source/API/SBProcess.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/Options.td
  lldb/source/Core/PluginManager.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
  lldb/test/API/macosx/skinny-corefile/Makefile
  lldb/test/API/macosx/skinny-corefile/TestSkinnyCorefile.py
  lldb/test/API/macosx/skinny-corefile/main.c
  lldb/test/API/macosx/skinny-corefile/present.c
  lldb/test/API/macosx/skinny-corefile/present.h
  lldb/test/API/macosx/skinny-corefile/to-be-removed.c
  lldb/test/API/macosx/skinny-corefile/to-be-removed.h
  lldb/tools/debugserver/source/DNBDefs.h
  lldb/tools/debugserver/source/MacOSX/MachVMMemory.cpp
  lldb/tools/debugserver/source/RNBRemote.cpp

Index: lldb/tools/debugserver/source/RNBRemote.cpp
===
--- lldb/tools/debugserver/source/RNBRemote.cpp
+++ lldb/tools/debugserver/source/RNBRemote.cpp
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -4422,7 +4423,7 @@
 __FILE__, __LINE__, p, "Invalid address in qMemoryRegionInfo packet");
   }
 
-  DNBRegionInfo region_info = {0, 0, 0};
+  DNBRegionInfo region_info;
   DNBProcessMemoryRegionInfo(m_ctx.ProcessID(), address, ®ion_info);
   std::ostringstream ostrm;
 
@@ -4442,6 +4443,18 @@
 if (region_info.permissions & eMemoryPermissionsExecutable)
   ostrm << 'x';
 ostrm << ';';
+
+ostrm << "dirty-pages:";
+if (region_info.dirty_pages.size() > 0) {
+  bool first = true;
+  for (nub_addr_t addr : region_info.dirty_pages) {
+if (!first)
+  ostrm << ",";
+first = false;
+ostrm << "0x" << std::hex << addr;
+  }
+}
+ostrm << ";";
   }
   return SendPacket(ostrm.str());
 }
@@ -4963,6 +4976,8 @@
   strm << "default_packet_timeout:10;";
 #endif
 
+  strm <<

[Lldb-commits] [PATCH] D88387: Create "skinny corefiles" for Mach-O with process save-core / reading

[Jason Molenda via Phabricator via lldb-commits]

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



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6223
+  range_info.GetDirtyPageList();
+  if (dirty_page_list.hasValue()) {
+for (addr_t dirtypage : dirty_page_list.getValue()) {

clayborg wrote:
> If we add a bool argument, we might need to return an error if the 
> lldb-server doesn't support the dirty page list stuff. Some regions won't 
> have dirty pages, but we might need add detection for any dirty pages and 
> then error out at the end if user requested a minimal core file
in the qMemoryRegionInfo packet response, a remote stub that can identify dirty 
pages should include a dirty-pages: key-value entry in every response where 
they're supported, even if it's an empty list.  I clarified this in the docs.  
That's how we'll detect the difference between "no dirty pages" and "dirty 
pages not supported".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88387

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

[Lldb-commits] [PATCH] D88387: Create "skinny corefiles" for Mach-O with process save-core / reading

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:6223
+  range_info.GetDirtyPageList();
+  if (dirty_page_list.hasValue()) {
+for (addr_t dirtypage : dirty_page_list.getValue()) {

jasonmolenda wrote:
> clayborg wrote:
> > If we add a bool argument, we might need to return an error if the 
> > lldb-server doesn't support the dirty page list stuff. Some regions won't 
> > have dirty pages, but we might need add detection for any dirty pages and 
> > then error out at the end if user requested a minimal core file
> in the qMemoryRegionInfo packet response, a remote stub that can identify 
> dirty pages should include a dirty-pages: key-value entry in every response 
> where they're supported, even if it's an empty list.  I clarified this in the 
> docs.  That's how we'll detect the difference between "no dirty pages" and 
> "dirty pages not supported".
Oh, and once we're inside lldb, I have the dirty page list returned as an 
Optional vector.  For a memory region where we don't have any dirty page 
information, there's no vector.  For a memory region where we have dirty page 
information -- and there are no pages, an empty vector is returned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88387

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