[Lldb-commits] [PATCH] D53175: [dotest] Make a missing FileCheck binary a warning, not an error

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I've been meaning to look at this - for the command line style "cd test; 
./dotest.py" approach, we could use similar logic to what 
packages/Python/lldbsuite/test/dotest.py does in getOutputPaths() where it 
finds an lldb binary in the "typical" location.  for an Xcode style build, this 
would be in a directory like ../llvm-build/Release+Asserts/x86_64/bin/FileCheck 
 where the build-style ("Release+Asserts") is going to vary but I'm guessing 
that the build configuration of the FileCheck binary picked is not important.


https://reviews.llvm.org/D53175



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

[Lldb-commits] [PATCH] D53175: [dotest] Make a missing FileCheck binary a warning, not an error

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

(obviously this would only be the behavior if a filecheck path was not 
specified)


https://reviews.llvm.org/D53175



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

[Lldb-commits] [PATCH] D53305: Simplify LocateDSYMInVincinityOfExecutable a bit, and call Symbols::DownloadObjectAndSymbolFile for .dSYM.yaa archives

[Jason Molenda via Phabricator via lldb-commits]

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

Host/common/Symbols.cpp has a method LocateDSYMInVincinityOfExecutable which 
looks for a dSYM next to a binary ("foo" + "foo.dSYM") and it looks for a dSYM 
next to a bundle ("foo.framework/foo" + "foo.framework.dSYM").  It constructed 
these two paths differently - one with strcat's one with FileSpec component 
adding, which isn't great.

This patch adds a helper function LookForDsymNextToExecutablePath which only 
knows how to look next to a given FileSpec for a dSYM.  It also knows to look 
for a .dSYM.yaa file -- yaa is a compressed archive format used inside Apple 
akin to a zip file.  Instead of having lldb manage a cache directory & 
expanding it there, I'm calling over to Symbols::DownloadObjectAndSymbolFile() 
which knows how to expand the archive on the local filesystem and give lldb the 
path to the expanded dSYM.

There aren't any tests here because the testsuite in dSYM mode exercises this 
code well.  I tried intentionally breaking the 
bundle-parent-directory-searching code and picked up more than a dozen fails, 
for instance.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53305

Files:
  source/Host/common/Symbols.cpp

Index: source/Host/common/Symbols.cpp
===
--- source/Host/common/Symbols.cpp
+++ source/Host/common/Symbols.cpp
@@ -65,96 +65,134 @@
   return false;
 }
 
+// Given a binary exec_fspec, and a ModuleSpec with an architecture/uuid,
+// return true if there is a matching dSYM bundle next to the exec_fspec,
+// and return that value in dsym_fspec.  
+// If there is a .dSYM.yaa compressed archive next to the exec_fspec, 
+// call through Symbols::DownloadObjectAndSymbolFile to download the
+// expanded/uncompressed dSYM and return that filepath in dsym_fspec.
+
+static bool LookForDsymNextToExecutablePath(const ModuleSpec &mod_spec,
+const FileSpec &exec_fspec,
+FileSpec &dsym_fspec) {
+  ConstString filename = exec_fspec.GetFilename();
+  FileSpec dsym_directory = exec_fspec;
+  dsym_directory.RemoveLastPathComponent();
+
+  std::string dsym_filename = filename.AsCString();
+  dsym_filename += ".dSYM";
+  dsym_directory.AppendPathComponent(dsym_filename);
+  dsym_directory.AppendPathComponent("Contents");
+  dsym_directory.AppendPathComponent("Resources");
+  dsym_directory.AppendPathComponent("DWARF");
+  
+  if (dsym_directory.Exists()) {
+
+// See if the binary name exists in the dSYM DWARF
+// subdir.
+FileSpec dsym_fspec = dsym_directory;
+dsym_fspec.AppendPathComponent(filename.AsCString());
+if (dsym_fspec.Exists()
+&& FileAtPathContainsArchAndUUID(dsym_fspec, 
+ mod_spec.GetArchitecturePtr(),
+ mod_spec.GetUUIDPtr())) {
+  return true;
+}
+
+// See if we have "../CF.framework" - so we'll look for
+// CF.framework.dSYM/Contents/Resources/DWARF/CF
+// We need to drop the last suffix after '.' to match 
+// 'CF' in the DWARF subdir.
+std::string binary_name (filename.AsCString());
+auto last_dot = binary_name.find_last_of('.');
+if (last_dot != std::string::npos) {
+  binary_name.erase(last_dot);
+  dsym_fspec = dsym_directory;
+  dsym_fspec.AppendPathComponent(binary_name);
+  if (dsym_fspec.Exists()
+  && FileAtPathContainsArchAndUUID(dsym_fspec, 
+   mod_spec.GetArchitecturePtr(),
+   mod_spec.GetUUIDPtr())) {
+return true;
+  }
+}
+  } 
+  
+  // See if we have a .dSYM.yaa next to this executable path.
+  FileSpec dsym_yaa_fspec = exec_fspec;
+  dsym_yaa_fspec.RemoveLastPathComponent();
+  std::string dsym_yaa_filename = filename.AsCString();
+  dsym_yaa_filename += ".dSYM.yaa";
+  dsym_yaa_fspec.AppendPathComponent(dsym_yaa_filename);
+
+  if (dsym_yaa_fspec.Exists()) {
+ModuleSpec mutable_mod_spec = mod_spec;
+if (Symbols::DownloadObjectAndSymbolFile (mutable_mod_spec, true)
+&& mutable_mod_spec.GetSymbolFileSpec().Exists()) {
+  dsym_fspec = mutable_mod_spec.GetSymbolFileSpec();
+  return true;
+}
+  }
+
+  return false;
+}
+
+// Given a ModuleSpec with a FileSpec and optionally uuid/architecture
+// filled in, look for a .dSYM bundle next to that binary.  Returns true
+// if a .dSYM bundle is found, and that path is returned in the dsym_fspec
+// FileSpec.
+//
+// This routine looks a few directory layers above the given exec_path -
+// exec_path might be /System/Library/Frameworks/CF.framework/CF and the
+// dSYM might be /System/Library/Frameworks/CF.framework.dSYM.
+//
+// If there is a .dSYM.yaa compressed archive found next to the binary,
+// we'll call DownloadObjectAndSymbolFile to expand it into a plain .dSYM
+
 static bool LocateDSYM

[Lldb-commits] [PATCH] D50155: Delete MacOSXFrameBackchain unwind logic (NFC)

[Jason Molenda via Phabricator via lldb-commits]

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

Sorry for not replying to this.  Yes the code is unused - it was the initial 
unwinder implementation when lldb is very young, before we had our current one. 
 The main reason to keep it around is (as you say) an example of the simplest 
unwinder someone might use when porting lldb to a new platform.  I don't feel 
strongly about it one way or the other, I'm fine with removing it if that's 
what you want to do.


https://reviews.llvm.org/D50155



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

[Lldb-commits] [PATCH] D53435: [x86] Fix issues with a realigned stack in MSVC compiled applications

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Thanks for the ping aleksandr, I have somehow managed to set up my mail rules 
so I didn't see this.  I'll try to look it over later today.  Yes, the x86 
instruction profiler has definitely overgrown over the years - the ARM64 
instruction profiler is probably a better model, but this code is pretty stable 
so it hasn't been a priority to do anything to it.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53435



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

[Lldb-commits] [PATCH] D53435: [x86] Fix issues with a realigned stack in MSVC compiled applications

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Thanks Aleksandr, this all looks good.  I'd like to see a definition of AFA 
(and we can say "CFA is DWARF's Canonical Frame Address" in the same spot) 
maybe in UnwindPlan.h, non-Windows people (ok, me) will not know what that is 
referring to.  I was a little worried about and_rsp_pattern_p() but we're not 
recording the and'ed value.  One long standing problem of the UnwindPlan 
intermediate representation is that it doesn't have any way to represent an 
alignment involved in the location computation (short of a DWARF expression).  
This is a problem on AArch32 / armv7 where the NEON registers are saved on 
higher alignment boundaries than the stack pointer on function call.  (if the 
alignment is less-than or equal-to the stack frame, then there's no align 
instructions needed in the prologue.)  But you're not doing anything like that, 
no worries.




Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.h:195
 
   // Get the CFA register for a given frame.
+  bool ReadFrameAddress(lldb::RegisterKind register_kind,

Comment update, e.g. Get the Frame Address register for a given frame.



Comment at: 
source/Plugins/UnwindAssembly/x86/x86AssemblyInspectionEngine.cpp:399
+// movq %rsp, %rbx [0x48 0x8b 0xdc] or [0x48 0x89 0xe3]
+// movl %esp, %ebx [0x8b 0xdc] or [0x89 0xe3]
+bool x86AssemblyInspectionEngine::mov_rsp_rbx_pattern_p() {

thanks for using with at&t syntax in these comments ;) 



Comment at: source/Symbol/UnwindPlan.cpp:202
+  m_afa_value.Dump(s, unwind_plan, thread);
+
   s.Printf(" => ");

What do you think about only printing the AFA= entry if it is != 
LLDB_INVALID_ADDRESS?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53435



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

[Lldb-commits] [PATCH] D54056: Add SetAllowJIT (the SBExpressionOptions equivalent of "expression --allow-jit")

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

To me --jit sounds like an imperative (i.e. don't use the IR interpreter, jit 
and execute this expression), whereas --allow-jit seems closer to the behavior 
here.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54056



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

[Lldb-commits] [PATCH] D54460: Don't keep a global ABI plugin per architecture

[Jason Molenda via Phabricator via lldb-commits]

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

LGTM, my guess is that this was Greg's code originally.  He may have made a 
singleton object out of caution.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54460



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

[Lldb-commits] [PATCH] D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.

LGTM.




Comment at: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp:104
+if (image_infos_address != m_maybe_image_infos_address)
+  did_exec = true;
+  }

Do you want to copy the image_infos_address value into 
m_maybe_image_infos_address so we can detect the next exec?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55399



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

[Lldb-commits] [PATCH] D55399: If we still have all_image_infos use it in DynamicLoaderMacOSDYLD to detect exec's

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I don't feel strongly about it, your choice.  But if I misunderstood it this 
time, I'll likely misunderstand it again in the future. :)


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55399



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

[Lldb-commits] [PATCH] D55607: Make crashlog.py work when a .dSYM is present, but a binary is missing

[Jason Molenda via Phabricator via lldb-commits]

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

Yep, looks good.  It would be nice if we found a dSYM with a Spotlight search 
(mdfind) if we could look NEXT to the dSYM bundle and see if there is a real 
binary, and load that.  But this is a good first step, and it gets us the 
source-level information for symbolicating the crash.  More advanced crashlog 
users may want to look at the assembly instructions around the crash site, and 
for that we need the actual binary.


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

https://reviews.llvm.org/D55607



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

[Lldb-commits] [PATCH] D46669: Retrieve the deployment target when retrieving an object file's triple

[Jason Molenda via Phabricator via lldb-commits]

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

Looks good.


https://reviews.llvm.org/D46669



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

[Lldb-commits] [PATCH] D46934: Make ObjectFileMachO work on non-darwin platforms

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.

Thanks for untangling this Pavel, I hadn't noticed we weren't building this 
plugin for non-darwin systems.  I'd agree with Adrian's comment that we should 
have a constants like LLDB_KERNEL_SUCCESS / LLDB_KERNEL_INVALID_ARGUMENT, I 
think the place where -1 was being returned currently would be considered 
incorrect (it's probably my fault for all I know ;)


https://reviews.llvm.org/D46934



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

[Lldb-commits] [PATCH] D47495: Support relative paths with less than two components in DBGSourcePathRemapping

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

This looks good.  My original change in r311622 to add this "remove the last 
two file path components" should have explicitly said that this is only done 
for DBGVersion == 2 to make it clearer for people reading the code.

At this point the parsing code behaves like:

DBGVersion 1 (or missing DBGVersion) - ignore the DBGSourcePathRemapping 
dictionary
DBGVersion 2 - Use DBGSourcePathRemapping, but chop off two components from 
both source and destination (assumption: the last two components are 
PROJECT-NAME/PROJECT-VERSION-NUMBER)
DBGVersion 3 - Use DBGSourcePathRemapping without any modifications

These interpretations of the DBGSourcePathRemapping / DBGVersion have been a 
little ad-hoc, as we've discovered issues in deployed plists that needed to be 
accommodated by lldb.


https://reviews.llvm.org/D47495



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

[Lldb-commits] [PATCH] D47539: [Platform] Accept arbitrary kext variants

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

LGTM.  If we added more knowledge specifically about kext bundle layouts, we 
could restrict which files we test to see if they are valid binaries - but we'd 
need to parse the Info.plist at the top (to get the CFBundleExecutable name, 
and look for variations on that prefix) and we'd need to handle shallow/deep 
kext bundles.  Given how few files are in kext bundles besides the kexts 
themselves (a couple of plists), this is code is much simpler than encoding all 
that extra specifics about kexts.


https://reviews.llvm.org/D47539



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

[Lldb-commits] [PATCH] D48302: Search for kext variants is searching from parent directory when it should not be

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: JDevlieghere.
Herald added a subscriber: llvm-commits.

There's a small perf issue after the changes to r334205; the patch is intended 
to search all the executables in a kext bundle to discover variants, e.g.

/System/Library/Extensions/AppleUSB.kext

everything under the AppleUSB.kext subdir should be searched.  But the patch 
used the FileSpec's GetDirectory method, so we ended up recursively searching 
all the kexts in the parent directory.

Functionally, these are equivalent - recursively searching from the parent dir 
will yield the same result because we require a binary's UUID match before we 
use it.  But it has a high performance cost if we search from the wrong base 
directory.

rdar://problem/41227170


Repository:
  rL LLVM

https://reviews.llvm.org/D48302

Files:
  source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
  source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h


Index: source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
===
--- source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
+++ source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
@@ -128,7 +128,7 @@
   bool recurse);
 
   static std::vector
-  SearchForExecutablesRecursively(const lldb_private::ConstString &dir);
+  SearchForExecutablesRecursively(const std::string &dir);
 
   static void AddKextToMap(PlatformDarwinKernel *thisp,
const lldb_private::FileSpec &file_spec);
Index: source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -780,10 +780,10 @@
 }
 
 std::vector
-PlatformDarwinKernel::SearchForExecutablesRecursively(const ConstString &dir) {
+PlatformDarwinKernel::SearchForExecutablesRecursively(const std::string &dir) {
   std::vector executables;
   std::error_code EC;
-  for (llvm::sys::fs::recursive_directory_iterator it(dir.GetStringRef(), EC),
+  for (llvm::sys::fs::recursive_directory_iterator it(dir.c_str(), EC),
end;
it != end && !EC; it.increment(EC)) {
 auto status = it->status();
@@ -800,7 +800,7 @@
 const FileSpec &kext_bundle_path, const lldb_private::UUID &uuid,
 const ArchSpec &arch, ModuleSP &exe_module_sp) {
   for (const auto &exe_file :
-   SearchForExecutablesRecursively(kext_bundle_path.GetDirectory())) {
+   SearchForExecutablesRecursively(kext_bundle_path.GetPath())) {
 if (exe_file.Exists()) {
   ModuleSpec exe_spec(exe_file);
   exe_spec.GetUUID() = uuid;


Index: source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
===
--- source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
+++ source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h
@@ -128,7 +128,7 @@
   bool recurse);
 
   static std::vector
-  SearchForExecutablesRecursively(const lldb_private::ConstString &dir);
+  SearchForExecutablesRecursively(const std::string &dir);
 
   static void AddKextToMap(PlatformDarwinKernel *thisp,
const lldb_private::FileSpec &file_spec);
Index: source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
===
--- source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -780,10 +780,10 @@
 }
 
 std::vector
-PlatformDarwinKernel::SearchForExecutablesRecursively(const ConstString &dir) {
+PlatformDarwinKernel::SearchForExecutablesRecursively(const std::string &dir) {
   std::vector executables;
   std::error_code EC;
-  for (llvm::sys::fs::recursive_directory_iterator it(dir.GetStringRef(), EC),
+  for (llvm::sys::fs::recursive_directory_iterator it(dir.c_str(), EC),
end;
it != end && !EC; it.increment(EC)) {
 auto status = it->status();
@@ -800,7 +800,7 @@
 const FileSpec &kext_bundle_path, const lldb_private::UUID &uuid,
 const ArchSpec &arch, ModuleSP &exe_module_sp) {
   for (const auto &exe_file :
-   SearchForExecutablesRecursively(kext_bundle_path.GetDirectory())) {
+   SearchForExecutablesRecursively(kext_bundle_path.GetPath())) {
 if (exe_file.Exists()) {
   ModuleSpec exe_spec(exe_file);
   exe_spec.GetUUID() = uuid;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I could imagine there being a bug in this ProcessGDBRemote::UpdateThreadIDList 
-> ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue sequence when 
talking to a gdb-remote stub that does not implement the lldb-enhancement 
jThreadsInfo.  If jThreadsInfo isn't implemented, we fall back to looking for 
the thread-pcs: and threads: key-value pairs in the stop packet that we 
received.  We clear m_thread_pcs, then look if thread-pcs: is present (that's 
one bug) to add all the thread pc values.  After that, we look for the threads: 
list and if it is present, we call UpdateThreadIDsFromStopReplyThreadsValue 
which clears m_thread_pcs again (bug #2) and then adds any threads listed to 
the m_thread_ids.

I could believe there are problems here if we're talking to a gdb-remote that 
doesn't support jThreadsInfo but does provide threads: or thread-pcs: maybe?  
It'd be interesting to see what the packet log for a stop packet is showing, 
and what exactly the gdb-remote stub is.


https://reviews.llvm.org/D48868



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

[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

(just to be clear, the m_thread_pcs.clear() in 
ProcessGDBRemote::UpdateThreadIDList that I called a bug -- today we only have 
two ways of populating that, jThreadsInfo or the thread-pcs: value in the stop 
packet.  So clearing it unconditionally here, and then populating it from 
thread_pcs: seems reasonable.)


https://reviews.llvm.org/D48868



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

[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I think the m_thread_pcs.clear() in UpdateThreadIDList() is to clear out any 
old thread pc values that we might have populated in the past.  The only way I 
can see this happening is if the remote stub SOMETIMES returned the thread-pcs: 
list in the stop packet.  UpdateThreadPCsFromStopReplyThreadsValue() which we 
call if we did get a thread-pcs: in this stop reply packet, clears m_thread_pcs 
so the only reason to have this also in UpdateThreadIDList() is if we had a 
stub that was inconsistent.

I agree that the m_thread_pcs.clear() in 
UpdateThreadIDsFromStopReplyThreadsValue() is wrong, and your patch here is 
good.

I don't have strong feelings about removing the m_thread_pcs() in 
UpdateThreadIDList(), but I think I would prefer leaving it in to changing it 
unless we know it's causing problems.  Does this cause problems in your 
environment?


https://reviews.llvm.org/D48868



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

[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet

[Jason Molenda via Phabricator via lldb-commits]

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

looks good.


https://reviews.llvm.org/D48868



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

[Lldb-commits] [PATCH] D48868: [LLDB] In ProcessGDBRemote::UpdateThreadIDList(), the thread PCs should not be cleared after they are updated from the stop reply packet

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a subscriber: ramana-nvr.
jasonmolenda added a comment.

That's a good point Pavel.  I tried to write one (below) but I never saw what 
the original failure mode was.  Venkata, can you help to make a test case that 
fails before the patch and works after?  Or explain what bug was being fixed 
exactly?  I could see that the code was wrong from reading it, but I never 
understood how you got to this.

Index: 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestStopPCs.py
==

- 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestStopPCs.py 
  (nonexistent)

+++ 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestStopPCs.py 
(working copy)
@@ -0,0 +1,45 @@
+from __future__ import print_function
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestThreadSelectionBug(GDBRemoteTestBase):
+def test(self):
+class MyResponder(MockGDBServerResponder):
+def haltReason(self):
+return 
"T02thread:1ff0d;threads:1ff0d,2ff0d;thread-pcs:10001bc00,10002bc00;"
+
+def threadStopInfo(self, threadnum):
+if threadnum == 0x1ff0d:
+return 
"T02thread:1ff0d;threads:1ff0d,2ff0d;thread-pcs:10001bc00,10002bc00;0:0,1:00bc010001;"
+if threadnum == 0x2ff0d:
+return 
"T00thread:2ff0d;threads:1ff0d,2ff0d;thread-pcs:10001bc00,10002bc00;0:0,1:00bc020001;"
+
+def qXferRead(self, obj, annex, offset, length):
+if annex == "target.xml":
+return """
+
+  i386:x86-64
+  
+
+
+  
+""", False
+else:
+return None, False
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+  self.runCmd("log enable gdb-remote packets")
+process = self.connect(target)
+
+self.assertEqual(process.GetNumThreads(), 2)
+th0 = process.GetThreadAtIndex(0)
+th1 = process.GetThreadAtIndex(1)
+self.assertEqual(th0.GetThreadID(), 0x1ff0d)
+self.assertEqual(th1.GetThreadID(), 0x2ff0d)
+self.assertEqual(th0.GetFrameAtIndex(0).GetPC(), 0x10001bc00)
+self.assertEqual(th1.GetFrameAtIndex(0).GetPC(), 0x10002bc00)

Index: 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
=

- 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
(revision 337215)

+++ 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
  (working copy)
@@ -130,6 +130,8 @@

  return self.QEnableErrorStrings()
  if packet == "?":
  return self.haltReason()

+if packet == "s":
+return self.haltReason()

  if packet[0] == "H":
  return self.selectThread(packet[1], int(packet[2:], 16))
  if packet[0:6] == "qXfer:":

@@ -144,6 +146,9 @@

  return self.vAttach(int(pid, 16))
  if packet[0] == "Z":
  return self.setBreakpoint(packet)

+if packet.startswith("qThreadStopInfo"):
+threadnum = int (packet[15:], 16)
+return self.threadStopInfo(threadnum)

  return self.other(packet)
   
  def interrupt(self):

@@ -204,6 +209,9 @@

  def setBreakpoint(self, packet):
  raise self.UnexpectedPacketException()
   

+def threadStopInfo(self, threadnum):
+return ""
+

  def other(self, packet):
  # empty string means unsupported
  return ""


Repository:
  rL LLVM

https://reviews.llvm.org/D48868



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

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Haven't had time to read through the main part of the patch, but I think the 
changes to debugserver's JSON parser would be good additions even if you've 
switched away from using it.  The debugserver JSON parser is a copy of lldb's 
with all the StringRef etc llvm classes removed quickly, so it's always been a 
bit of a mess, I should have forked it from before any of the llvm stuff 
started being integrated.


https://reviews.llvm.org/D50365



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

[Lldb-commits] [PATCH] D40812: Remove no-op null checks, NFC

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

all:
echo 'int foo() { return 5; }' > mylib.c
echo "int foo() __attribute__((weak_import)); int printf(const char *, 
...); int main() { if (foo) printf (\"have foo() %d\\\n\", foo()); else 
printf(\"do not have foo\\\n\"); return 0; }" > main.c
clang -dynamiclib -o libmylib.dylib mylib.c
clang main.c -L. -lmylib
./a.out
echo 'int bar() { return 5; }' > mylib.c
clang -dynamiclib -o libmylib.dylib mylib.c
./a.out


https://reviews.llvm.org/D40812



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

[Lldb-commits] [PATCH] D40812: Remove no-op null checks, NFC

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I don't think that is correct.  here's an example in the form of a makefile.  
the main binary calls foo() which is weak linked from a library.  In the first 
run, foo() is present in the library.  In the second run, the library is 
rebuilt so foo() is absent.  The behavior of the program differs.

In this case, we needed an lldb which was built on a system with libcompression 
to be able to run on a system without libcompression.  Although for Apple's 
purposes, we are no longer supporting lldb on systems without libcompression so 
it's not really needed any longer for our uses.


https://reviews.llvm.org/D40812



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

[Lldb-commits] [PATCH] D40812: Remove no-op null checks, NFC

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

The output looks like this:

% make
echo 'int foo() { return 5; }' > mylib.c
echo "int foo() __attribute__((weak_import)); int printf(const char *, ...); 
int main() { if (foo) printf (\"have foo() %d\\\n\", foo()); else printf(\"do 
not have foo\\\n\"); return 0; }" > main.c
clang -dynamiclib -o libmylib.dylib mylib.c
clang main.c -L. -lmylib
./a.out
have foo() 5
echo 'int bar() { return 5; }' > mylib.c
clang -dynamiclib -o libmylib.dylib mylib.c
./a.out
do not have foo


https://reviews.llvm.org/D40812



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

[Lldb-commits] [PATCH] D40812: Remove no-op null checks, NFC

[Jason Molenda via Phabricator via lldb-commits]

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

Ah I see, I bet I assumed it was marked weak but it never was.  patch looks 
good to me.


https://reviews.llvm.org/D40812



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

[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I guess I don't have an opinion on this one.  The correct way to pass 
environment variables to the inferior is through 
SBLaunchInfo::SetEnvironmentEntries or in cmd line lldb, process launch -v 
ENV=val.  A test that assumes an environment variable set in lldb will end up 
in the inferior process isn't going to work when the process is launched on a 
remote target, is it?

Whether llgs or debugserver should be forwarding their environment variables on 
by default - it seems fragile to me.  But maybe there's a use case that needs 
this behavior to be supported?  I guess it's valid to test that it actually 
behaves that way, at least for processes launched on the local system.

(I apologize for not keeping up with lldb-commits the past week while this was 
work was being done -- I'm in the middle of a task where I've had to 
concentrate on that & pause reading emails etc for a bit. ;)


https://reviews.llvm.org/D41352



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

[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)

[Jason Molenda via Phabricator via lldb-commits]

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

Thanks for the background Pavel.  I'm fine with changing the default behavior.  
I don't see this having any impact on users of debugserver, and if it does, we 
can add a flag like Greg suggests here.


https://reviews.llvm.org/D41352



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

[Lldb-commits] [PATCH] D41962: Fix TestYMMRegisters for older machines without AVX2

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

fwiw I'm working on upstreaming on zmm (avx512) patches that we have locally 
(there's one testsuite fail I still need to find time to fix) and the 
TestZMMRegister.py test that ChrisB wrote to test this is written as 
skip-unless-darwin, and there's a new skipUnlessFeature() method added to 
decorators.py which runs sysctl to detect hardware features (in this case, 
hw.optional.avx512f) which, I suspect, is an even more mac-specific way of 
doing this.  While Adrian's approach would be gcc/clang specific, it would def 
be better than depending on a sysctl.


Repository:
  rL LLVM

https://reviews.llvm.org/D41962



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

[Lldb-commits] [PATCH] D41962: Fix TestYMMRegisters for older machines without AVX2

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I suppose a possible alternative would be to figure out the avx2 / avx512 
features manually based on the cpuid instead of letting the compiler do it for 
us.  e.g. 
https://stackoverflow.com/questions/1666093/cpuid-implementations-in-c and then 
checking the bits as e.g. described in https://en.wikipedia.org/wiki/CPUID .  
Bummer to do it so low level if we can delegate this to the compiler though.


Repository:
  rL LLVM

https://reviews.llvm.org/D41962



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

[Lldb-commits] [PATCH] D41962: Fix TestYMMRegisters for older machines without AVX2

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

In https://reviews.llvm.org/D41962#975168, @aprantl wrote:

> > there's a new skipUnlessFeature() method added to decorators.py which runs 
> > sysctl to detect hardware features (in this case, hw.optional.avx512f)
>
> How does one execute a program like `sysctl` on the remote? I have seen code 
> in TestLldbGdbServer.py that uses `platform get-file /proc/cpuinfo` to 
> achieve something similar for Linux, but that works without executing a new 
> process.


this skipUnlessFeature sysctl check was all performed on the system running the 
testsuite.  Checking whether the feature exists in the program (the approach 
you're taking) is more correct.  We usually do host != target testsuite runs 
for arm devices, but there's no reason why someone couldn't do a macos x 
freebsd testsuite run and the sysctl check would be invalid in that case.


https://reviews.llvm.org/D41962



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

[Lldb-commits] [PATCH] D42206: If kevent() is interrupted by signal (or is being debugged) and we get EINTR, retry

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: davide.
jasonmolenda added a project: LLDB.
Herald added subscribers: llvm-commits, JDevlieghere.

I hit this while trying to debug lldb-server.  When lldb-server is waiting for 
connections, it will be in MainLoop::RunImpl::Poll(), in the kevent() call.  On 
darwin systems, attaching to the process (or interrupting it to add a 
breakpoint or detach) will interrupt the system call it is sitting in.  We will 
get back an error return value (-1) and errno will be set to EINTR.

Currently we flag this as an error and lldb-server exits with

error: IO object is not valid.

which makes it a bit difficult to debug.

This section of code is only used on the *BSD systems and Darwin (it's 
#ifdef'ed HAVE_SYS_EVENT_H).  I tested it on Darwin and on Linux (before 
noticing that linux doesn't have sys/event.h) but don't have access to a *BSD 
system.  I don't expect any problems handling the interrupted kevent() call on 
those systems as I'm doing here.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42206

Files:
  source/Host/common/MainLoop.cpp


Index: source/Host/common/MainLoop.cpp
===
--- source/Host/common/MainLoop.cpp
+++ source/Host/common/MainLoop.cpp
@@ -105,8 +105,11 @@
   for (auto &fd : loop.m_read_fds)
 EV_SET(&in_events[i++], fd.first, EVFILT_READ, EV_ADD, 0, 0, 0);
 
-  num_events = kevent(loop.m_kqueue, in_events.data(), in_events.size(),
-  out_events, llvm::array_lengthof(out_events), nullptr);
+  do {
+errno = 0;
+num_events = kevent(loop.m_kqueue, in_events.data(), in_events.size(),
+out_events, llvm::array_lengthof(out_events), nullptr);
+  } while (num_events == -1 && errno == EINTR);
 
   if (num_events < 0)
 return Status("kevent() failed with error %d\n", num_events);


Index: source/Host/common/MainLoop.cpp
===
--- source/Host/common/MainLoop.cpp
+++ source/Host/common/MainLoop.cpp
@@ -105,8 +105,11 @@
   for (auto &fd : loop.m_read_fds)
 EV_SET(&in_events[i++], fd.first, EVFILT_READ, EV_ADD, 0, 0, 0);
 
-  num_events = kevent(loop.m_kqueue, in_events.data(), in_events.size(),
-  out_events, llvm::array_lengthof(out_events), nullptr);
+  do {
+errno = 0;
+num_events = kevent(loop.m_kqueue, in_events.data(), in_events.size(),
+out_events, llvm::array_lengthof(out_events), nullptr);
+  } while (num_events == -1 && errno == EINTR);
 
   if (num_events < 0)
 return Status("kevent() failed with error %d\n", num_events);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D42206: If kevent() is interrupted by signal (or is being debugged) and we get EINTR, retry

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Hmmm, testing this would require launching lldb-server, attaching to it from a 
debugger, detaching, and seeing if lldb-server is still running.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42206



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

[Lldb-commits] [PATCH] D42210: Re-enable logging on the child side of a fork() in lldb-server platform mode

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: labath.
jasonmolenda added a project: LLDB.
Herald added a subscriber: llvm-commits.

When lldb-server is in platform mode and waiting for a connection, when it 
receives a connection it fork()'s and then runs an lldb-server/debugserver to 
respond to that connection.  The amount of things done between a fork() and an 
exec() should be very small, but if there is a bug in lldb-server and you're 
trying to use logging to debug it, the fact that logging does not happen in the 
child process makes it challenging to understand what is going on.

The fact that the logging re-enablement in the child will only happen when done 
by hand makes me more comfortable with this.

Pavel, do you think you're the best person to talk to about this?  I've tested 
this on darwin and on ubuntu and both exhibit the same behavior.  e.g.

terminal1% ./a.out

terminal2% bin/lldb-server platform --verbose --log-channels 'lldb 
all:gdb-remote all' --server --listen '*:0' -m 11120 -M 11250

terminal3% lldb
(lldb) pla sel remote-linux
(lldb) pla connect connect://localhost:0
(lldb) pla pro att -n a.out

without this patch, the only logging you'll see is lldb-server waiting for a 
connection, receiving a connection, and that's about it.


Repository:
  rL LLVM

https://reviews.llvm.org/D42210

Files:
  tools/lldb-server/lldb-platform.cpp


Index: tools/lldb-server/lldb-platform.cpp
===
--- tools/lldb-server/lldb-platform.cpp
+++ tools/lldb-server/lldb-platform.cpp
@@ -328,6 +328,10 @@
 // Parent will continue to listen for new connections.
 continue;
   } else {
+// We lose the logging on the child side of the fork, re-enable it.  
+if (!LLDBServerUtilities::SetupLogging(log_file, log_channels, 0))
+  return -1;
+
 // Child process will handle the connection and exit.
 g_server = 0;
 // Listening socket is owned by parent process.


Index: tools/lldb-server/lldb-platform.cpp
===
--- tools/lldb-server/lldb-platform.cpp
+++ tools/lldb-server/lldb-platform.cpp
@@ -328,6 +328,10 @@
 // Parent will continue to listen for new connections.
 continue;
   } else {
+// We lose the logging on the child side of the fork, re-enable it.  
+if (!LLDBServerUtilities::SetupLogging(log_file, log_channels, 0))
+  return -1;
+
 // Child process will handle the connection and exit.
 g_server = 0;
 // Listening socket is owned by parent process.
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D42206: If kevent() is interrupted by signal (or is being debugged) and we get EINTR, retry

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I tried sending signals to lldb-server via kill() and the signal handler caught 
them, the bit of code I had printing out the return value & errno value never 
got executed.  The only way I was able to repo this was by debugging 
lldb-server.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42206



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

[Lldb-commits] [PATCH] D42210: Re-enable logging on the child side of a fork() in lldb-server platform mode

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Jim remembers some problem with logging across a fork()'ed process, Pavel does 
this ring a bell?  This change might be completely bogus -- but it's very 
difficult to debug the child side of an lldb-server platform today.


Repository:
  rL LLVM

https://reviews.llvm.org/D42210



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

[Lldb-commits] [PATCH] D42215: [CMake] Make check-lldb work with LLDB_CODESIGN_IDENTITY=''

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

That's a good suggestion.  We have some bots using the system debugserver -- 
and once in a while there's a change to both lldb and debugserver, and the bots 
running the older prebuilt debugserver will fail any tests for that patchset.


https://reviews.llvm.org/D42215



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

[Lldb-commits] [PATCH] D42336: Fix memory leaks in TestArm64InstEmulation

[Jason Molenda via Phabricator via lldb-commits]

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

Thanks!


https://reviews.llvm.org/D42336



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

[Lldb-commits] [PATCH] D42828: Fix for read-past-end-of-array buglet in ProcessElfCore.cpp while reading linux notes

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: labath.
jasonmolenda added a project: LLDB.
Herald added a subscriber: llvm-commits.

I caught this while running the testsuite against lldb built with address 
sanitizer (ASAN) enabled - it found one problem when running the 
TestLinuxCore.py test.  The ELFLinuxPrPsInfo structure has two fixed width 
strings in it, pr_fname (16 chars) and pr_psargs (80 chars).  They are not 
required to be nul (\0) terminated, and in the case of ppc64le, pr_fname is not 
-

(lldb) p prpsinfo
(ELFLinuxPrPsInfo) $1 = {

  pr_fname = {
[0] = 'l'
[1] = 'i'
[2] = 'n'
[3] = 'u'
[4] = 'x'
[5] = '-'
[6] = 'p'
[7] = 'p'
[8] = 'c'
[9] = '6'
[10] = '4'
[11] = 'l'
[12] = 'e'
[13] = '.'
[14] = 'o'
[15] = 'u'
  }

When we copy this into a std::string,

thread_data.name = prpsinfo.pr_fname;

the read goes off the end of the array.  It goes into the next element on the 
structure, pr_psargs, so it's unlikely to crash, but it's an easy one to fix so 
I think we should.

TestLinuxCore.py's do_test() could also get passed in the expected thread name 
and verify that it was set correctly), that would have caught this without 
using ASAN.  But given that ASAN did catch it, I'm pretty happy with it as-is.


Repository:
  rL LLVM

https://reviews.llvm.org/D42828

Files:
  source/Plugins/Process/elf-core/ProcessElfCore.cpp


Index: source/Plugins/Process/elf-core/ProcessElfCore.cpp
===
--- source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -665,7 +665,7 @@
   Status status = prpsinfo.Parse(note.data, arch);
   if (status.Fail())
 return status.ToError();
-  thread_data.name = prpsinfo.pr_fname;
+  thread_data.name.assign (prpsinfo.pr_fname, sizeof (prpsinfo.pr_fname));
   SetID(prpsinfo.pr_pid);
   break;
 }


Index: source/Plugins/Process/elf-core/ProcessElfCore.cpp
===
--- source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -665,7 +665,7 @@
   Status status = prpsinfo.Parse(note.data, arch);
   if (status.Fail())
 return status.ToError();
-  thread_data.name = prpsinfo.pr_fname;
+  thread_data.name.assign (prpsinfo.pr_fname, sizeof (prpsinfo.pr_fname));
   SetID(prpsinfo.pr_pid);
   break;
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43048: [lldb-test/WIP] Allow a way to test autocompletion

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

No, the unwind unittests that exist today should stay written as unit tests.  
These are testing the conversion of native unwind formats -- for instance, 
eh_frame, compact unwind, or instruction analysis -- into the intermediate 
UnwindPlan representation in lldb.  They are runtime invariant, unit tests are 
the best approach to these.  If there were anything to say about these, it 
would be that we need more testing here - armv7 (AArch32) into UnwindPlan is 
not tested.  eh_frame and compact_unwind into UnwindPlan is not tested.

The part of unwind that is difficult to test is the runtime unwind behavior, 
and FileCheck style tests don't make that easier in any way.  We need a live 
register context, stack memory, symbols and UnwindPlans to test this correctly 
-- we either need a full ProcessMock with SymbolVendorMock etc, or we need 
corefiles, or we need tests with hand-written assembly code.

I'm leaning towards starting to write C test cases with hand written assembly 
routines that set up their stacks in unusual ways to test the unwinder, I've 
written unwind testsuites in the past in this way and it works well but it does 
mean that you need to be on the arch (and possibly even the OS) that the test 
is written in -- bots will show the failures for everyone after a commit goes 
in, at least.  But these will be written as SB API tests.


https://reviews.llvm.org/D43048



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

[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

On Darwin we load all the libraries that the binary links against 
pre-execution, if possible.  So I see:

% lldb a.out
(lldb) ima li libfoo.dylib
[  0] 35C5FE62-5327-3335-BBCF-5369CB07D1D6 0x 
/Volumes/ssdhome/jmolenda/k/svn/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/libfoo.dylib

  
/Volumes/ssdhome/jmolenda/k/svn/lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/libfoo.dylib.dSYM/Contents/Resources/DWARF/libfoo.dylib

(lldb) br s -f foo.cpp -p BR_foo
Breakpoint 1: where = libfoo.dylib`Foo::Foo() + 18 at foo.cpp:4, address = 
0x0f52
(lldb) br li
Current breakpoints:
1: source regex = "BR_foo", exact_match = 0, locations = 1

  1.1: where = libfoo.dylib`Foo::Foo() + 18 at foo.cpp:4, address = 
libfoo.dylib[0x0f52], unresolved, hit count = 0 


https://reviews.llvm.org/D43419



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

[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Yeah, it has a location because we're resolved it to a File address 
(section+offset), and when the dylib actually gets loaded we'll resolve that to 
a load address and insert the breakpoint instruction.


https://reviews.llvm.org/D43419



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

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I think we're being a little hasty here.  Greg's last suggestion is worth 
investigation -- how many tests would actually be run for this new variant?

Greg, maybe you could remove the make clean targets from 
packages/Python/lldbsuite/test/plugins/builder_base.py (I think that'll do it), 
do a testsuite run to create all the test binaries, and then find out how how 
many tests are actually going to be run here?

Pavel is right that the explosion of debug variations will quickly become 
untenable, even with bots, and I'm not sure what a good solution to that is.  
Running the debug information parsers against the wide variety of programs we 
include in the testsuite is a good way to test them.  This is a problem we 
should discuss, but if the practical impact of adding the extra testing for 
.debug_types is that we run 50 tests through an additional variant, I think it 
is premature to draw the line here and say this must be solved now.


https://reviews.llvm.org/D32167



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

[Lldb-commits] [PATCH] D44139: Update selected thread after loading mach core

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.

Looks good, thanks for doing this Jonas.  I thought there would be a problem 
with ThreadMachCore::CalculateStopInfo() in 
source/Plugins/Process/mach-core/ThreadMachCore.cpp which will mark every 
thread that comes from the actual core file has having a stop reason -- won't 
that conflict with your OperatingSystemPlugIn which is providing a stop reason?


https://reviews.llvm.org/D44139



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

[Lldb-commits] [PATCH] D44139: Update selected thread after loading mach core

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

(or rather, not "conflict with the OperatingSystemPlugIn stop reason" -- but 
would make it confusing to users.  I think it might be best to remove 
ThreadMachCore::CalculateStopInfo.)


https://reviews.llvm.org/D44139



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

[Lldb-commits] [PATCH] D45011: Prevent double release of mach ports

[Jason Molenda via Phabricator via lldb-commits]

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

valgrind/asan/msan aren't going to help - this is a pretty obscure stuff.  We 
could verify by hand that we're not leaking a port if this code was actually 
executed via lsmp(1) on macos, but Jim knows more about our MIG use than I do & 
I'm sure he's right that this code is never called.  LGTM.


https://reviews.llvm.org/D45011



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

[Lldb-commits] [PATCH] D45298: [debugserver] Fix LC_BUILD_VERSION load command handling.

[Jason Molenda via Phabricator via lldb-commits]

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

Looks good to me.




Comment at: packages/Python/lldbsuite/test/decorators.py:359
+output = subprocess.check_output(["xcodebuild", "-showsdks"], 
stderr=DEVNULL)
+if re.search('%ssimulator' % platform, output):
+return None

It might be helpful for anyone debugging this if examples of the names this is 
matching were included in a comment.  iphonesimulator, appletvsimulator, 
watchsimulator.  Different capitalization is used in so many places (and 
whether to include "os" suffix or not, and whether to include "apple" prefix or 
not) that it can be hard to know what's expected unless you run the command 
yourself.



Comment at: tools/debugserver/source/MacOSX/MachProcess.mm:584
+cmd == LC_VERSION_MIN_IPHONEOS || cmd == LC_VERSION_MIN_MACOSX;
+#if defined(LC_VERSION_MIN_TVOS)
+  lc_cmd_known |= cmd == LC_VERSION_MIN_TVOS;

Not important, but we're requiring a MacOS SDK of 10.11 or newer and the tvos 
and watchos SDKs were available by then; we could remove these ifdefs.  
(LC_BUILD_VERSION is newer than that & needs to be kept around.)



Comment at: tools/debugserver/source/MacOSX/MachProcess.mm:588
+#if defined(LC_VERSION_MIN_WATCHOS)
+  lc_cmd_known |= cmd == LC_VERSION_MIN_WATCHOS;
+#endif

It's equivalent, no big deal, but this is a bitwise | not a logical ||.


https://reviews.llvm.org/D45298



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

[Lldb-commits] [PATCH] D45348: Don't return error for settings set .experimental. settings that are absent

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: jingham.
jasonmolenda added a project: LLDB.
Herald added a subscriber: llvm-commits.

setting paths that include .experimental. are intended for settings that may be 
promoted to "real" settings in the future, or may be removed.  When users put 
these settings in their ~/.lldbinit files, we don't want to emit an error if 
the setting has gone away.  And if the setting has been promoted to a real 
setting, we want to do the right thing.

The first part of that -- not emitting an error -- did not work correctly.  I 
fixed that, and added tests to check all of these behaviors.


Repository:
  rL LLVM

https://reviews.llvm.org/D45348

Files:
  packages/Python/lldbsuite/test/settings/TestSettings.py
  source/Interpreter/OptionValueProperties.cpp


Index: source/Interpreter/OptionValueProperties.cpp
===
--- source/Interpreter/OptionValueProperties.cpp
+++ source/Interpreter/OptionValueProperties.cpp
@@ -207,12 +207,23 @@
   llvm::StringRef value) {
   Status error;
   const bool will_modify = true;
+  llvm::SmallVector components;
+  name.split(components, '.');
+  bool name_contains_experimental = false;
+  for (const auto &part : components)
+if (Properties::IsSettingExperimental(part))
+  name_contains_experimental = true;
+
+  
   lldb::OptionValueSP value_sp(GetSubValue(exe_ctx, name, will_modify, error));
   if (value_sp)
 error = value_sp->SetValueFromString(value, op);
   else {
-if (error.AsCString() == nullptr)
+// Don't set an error if the path contained .experimental. - those are
+// allowed to be missing and should silently fail.
+if (name_contains_experimental == false && error.AsCString() == nullptr) {
   error.SetErrorStringWithFormat("invalid value path '%s'", 
name.str().c_str());
+}
   }
   return error;
 }
Index: packages/Python/lldbsuite/test/settings/TestSettings.py
===
--- packages/Python/lldbsuite/test/settings/TestSettings.py
+++ packages/Python/lldbsuite/test/settings/TestSettings.py
@@ -524,3 +524,51 @@
  "target.process.extra-startup-command",
  "target.process.thread.step-avoid-regexp",
  "target.process.thread.trace-thread"])
+
+# settings under an ".experimental" domain should have two properties:
+#   1. If the name does not exist with "experimental" in the name path,
+#  the name lookup should try to find it without "experimental".  So
+#  a previously-experimental setting that has been promoted to a 
+#  "real" setting will still be set by the original name.
+#   2. Changing a setting with .experimental., name, where the setting
+#  does not exist either with ".experimental." or without, should
+#  not generate an error.  So if an experimental setting is removed,
+#  people who may have that in their ~/.lldbinit files should not see
+#  any errors.
+def test_experimental_settings(self):
+cmdinterp = self.dbg.GetCommandInterpreter()
+result = lldb.SBCommandReturnObject()
+
+# Set target.arg0 to a known value, check that we can retrieve it via
+# the actual name and via .experimental.
+cmdinterp.HandleCommand("settings set target.arg0 first-value", result)
+self.assertEqual(result.Succeeded(), True)
+cmdinterp.HandleCommand("settings show target.arg0", result)
+self.assertEqual(result.Succeeded(), True)
+self.assertTrue("first-value" in result.GetOutput())
+cmdinterp.HandleCommand("settings show target.experimental.arg0", 
result)
+self.assertEqual(result.Succeeded(), True)
+self.assertTrue("first-value" in result.GetOutput())
+
+# Set target.arg0 to a new value via a target.experimental.arg0 name,
+# verify that we can read it back via both .experimental., and not.
+cmdinterp.HandleCommand("settings set target.experimental.arg0 
second-value", result)
+self.assertEqual(result.Succeeded(), True)
+cmdinterp.HandleCommand("settings show target.arg0", result)
+self.assertEqual(result.Succeeded(), True)
+self.assertTrue("second-value" in result.GetOutput())
+cmdinterp.HandleCommand("settings show target.experimental.arg0", 
result)
+self.assertEqual(result.Succeeded(), True)
+self.assertTrue("second-value" in result.GetOutput())
+
+# showing & setting an undefined .experimental. setting should 
generate no errors.
+cmdinterp.HandleCommand("settings show 
target.experimental.setting-which-does-not-exist", result)
+self.assertEqual(result.Succeeded(), True)
+self.assertEqual(result.GetOutput().rstrip(), "")
+cmdinterp.HandleCommand("settings set 
target.e

[Lldb-commits] [PATCH] D45348: Don't return error for settings set .experimental. settings that are absent

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 141473.
jasonmolenda added a comment.

rewrote the test cases in terms of self.expect.  Haven't looked at being 
totally correct with flagging paths with .experimental. as never-errored.


https://reviews.llvm.org/D45348

Files:
  packages/Python/lldbsuite/test/settings/TestSettings.py
  source/Interpreter/OptionValueProperties.cpp


Index: source/Interpreter/OptionValueProperties.cpp
===
--- source/Interpreter/OptionValueProperties.cpp
+++ source/Interpreter/OptionValueProperties.cpp
@@ -207,12 +207,23 @@
   llvm::StringRef value) {
   Status error;
   const bool will_modify = true;
+  llvm::SmallVector components;
+  name.split(components, '.');
+  bool name_contains_experimental = false;
+  for (const auto &part : components)
+if (Properties::IsSettingExperimental(part))
+  name_contains_experimental = true;
+
+  
   lldb::OptionValueSP value_sp(GetSubValue(exe_ctx, name, will_modify, error));
   if (value_sp)
 error = value_sp->SetValueFromString(value, op);
   else {
-if (error.AsCString() == nullptr)
+// Don't set an error if the path contained .experimental. - those are
+// allowed to be missing and should silently fail.
+if (name_contains_experimental == false && error.AsCString() == nullptr) {
   error.SetErrorStringWithFormat("invalid value path '%s'", 
name.str().c_str());
+}
   }
   return error;
 }
Index: packages/Python/lldbsuite/test/settings/TestSettings.py
===
--- packages/Python/lldbsuite/test/settings/TestSettings.py
+++ packages/Python/lldbsuite/test/settings/TestSettings.py
@@ -524,3 +524,41 @@
  "target.process.extra-startup-command",
  "target.process.thread.step-avoid-regexp",
  "target.process.thread.trace-thread"])
+
+# settings under an ".experimental" domain should have two properties:
+#   1. If the name does not exist with "experimental" in the name path,
+#  the name lookup should try to find it without "experimental".  So
+#  a previously-experimental setting that has been promoted to a 
+#  "real" setting will still be set by the original name.
+#   2. Changing a setting with .experimental., name, where the setting
+#  does not exist either with ".experimental." or without, should
+#  not generate an error.  So if an experimental setting is removed,
+#  people who may have that in their ~/.lldbinit files should not see
+#  any errors.
+def test_experimental_settings(self):
+cmdinterp = self.dbg.GetCommandInterpreter()
+result = lldb.SBCommandReturnObject()
+
+# Set target.arg0 to a known value, check that we can retrieve it via
+# the actual name and via .experimental.
+self.expect('settings set target.arg0 first-value')
+self.expect('settings show target.arg0', substrs=['first-value'])
+self.expect('settings show target.experimental.arg0', 
substrs=['first-value'], error=False)
+
+# Set target.arg0 to a new value via a target.experimental.arg0 name,
+# verify that we can read it back via both .experimental., and not.
+self.expect('settings set target.experimental.arg0 second-value', 
error=False)
+self.expect('settings show target.arg0', substrs=['second-value'])
+self.expect('settings show target.experimental.arg0', 
substrs=['second-value'], error=False)
+
+# showing & setting an undefined .experimental. setting should 
generate no errors.
+self.expect('settings show 
target.experimental.setting-which-does-not-exist', patterns=['^\s$'], 
error=False)
+self.expect('settings set 
target.experimental.setting-which-does-not-exist true', error=False)
+
+# A domain component before .experimental. which does not exist should 
give an error
+# But the code does not yet do that.
+# self.expect('settings set 
target.setting-which-does-not-exist.experimental.arg0 true', error=True)
+
+# finally, confirm that trying to set a setting that does not exist 
still fails.
+# (SHOWING a setting that does not exist does not currently yield an 
error.)
+self.expect('settings set target.setting-which-does-not-exist true', 
error=True)


Index: source/Interpreter/OptionValueProperties.cpp
===
--- source/Interpreter/OptionValueProperties.cpp
+++ source/Interpreter/OptionValueProperties.cpp
@@ -207,12 +207,23 @@
   llvm::StringRef value) {
   Status error;
   const bool will_modify = true;
+  llvm::SmallVector components;
+  name.split(components, '.');
+  bool name_contains_experimental = false;
+  for (const auto &part : components

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

My two cents,

Why print the '### Start STATISTICS dump ###' header/footer when printing the 
results?  I don't think we demarcate result output like that anywhere else in 
lldb.  I don't think it adds anything for the user.

I would probably name the command statistics and have a built in "stats" alias.

Should statistics disable print the results, or should there be a 'statistics 
status' or 'statistics show' or 'statistics dump' command to show the 
statistics results accumulated so far?


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D45547: [Command] Implement `stats`

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Ah, no you couldn't set up a command alias like that.  Still, if the full name 
was statistics, you could type 'stat' and it would match.  'stats' wouldn't, 
though.

I do think decoupling the disabling action from the dumping action would be an 
improvement.  You may want to dump the current statistics multiple times during 
a sequence of commands, without disabling / re-enabling every time.  And 
dropping the header/footer from the results.


https://reviews.llvm.org/D45547



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

[Lldb-commits] [PATCH] D31823: Update LLDB Host to support IPv6 over TCP

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Looks good.  I'd recommend adding clayborg as a reviewer and giving him a 
couple days to comment on this if he has time.  He wrote all of this code 
originally.




Comment at: include/lldb/Host/common/TCPSocket.h:55
+
+  std::map m_listen_sockets;
 };

zturner wrote:
> Any particular reason you're using a `std::map` instead of a `DenseMap` or 
> other similar LLVM structure?  I was under the impression that the number of 
> times where you should actually use `std::map` are pretty small.
llvm docs say, "Also, because DenseMap allocates space for a large number of 
key/value pairs (it starts with 64 by default), it will waste a lot of space if 
your keys or values are large."  I think std::map is the right choice.  When 
it's a tossup between an llvm container class and an STL container class, my 
opinion is always to go with the standard container class that everyone is 
familiar with already.  If there's a distinct benefit to a given llvm container 
class for a specific scenario, that's fine, but it just increases the barrier 
to entry for people outside the llvm community to use these classes pervasively.



Comment at: source/Host/common/Socket.cpp:258
 regex_match.GetMatchAtIndex(host_and_port.data(), 2, port_str)) {
+  // IPv6 addresses are wrapped in [] when specified with ports
+  if (host_str.front() == '[' && host_str.back() == ']')

Ah, I'd originally said I thought we should just tack the :portnum on to the 
end of the string, search backwards to find it (because the [] characters are 
metacharacters in unix shells).  but I see that RFC8986 says this is how things 
like this are specified, like http://[2001:db8:1f70::999:de8:7648:6e8]:100/ so 
cool with that.


https://reviews.llvm.org/D31823



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

[Lldb-commits] [PATCH] D31880: Fix libc++ vector data formatter (bug #32553)

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

This all looks good to me, thanks for doing this Pavel.  Tamas asked in an 
earlier comment, "The previous version of the data formatter was triggering for 
std::vector> as well. Jason, do you know why was it the 
case? Do we need that functionality because of a broken compiler version or can 
it be removed?"

I have no idea why that was there.  I could dig around in the svn commit 
history to try to figure it out, but it may have been unnecessary to begin 
with.  If in doubt, we can drop it and put it back in if we hear of any 
problems.


https://reviews.llvm.org/D31880



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

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

[Jason Molenda via Phabricator via lldb-commits]

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

Looks fine to me, I don't know the debug_types sections that you're supporting 
but I read over the code and don't have any problems.




Comment at: source/Plugins/SymbolFile/DWARF/DWARFDIE.h:92
+  // .debug_info or the .debug_types section so anyone extracting data from
+  // a DIE must use the correct data.
+  //

DWARFDIE::GetData() handles the difference between debug_info / debug_types so 
the callers of this method don't need to worry about that, correct?  Maybe the 
comment could say

  // Get the data that contains the attribute values for this DIE. Support
  // for .debug_types means that any DIE can have its data either in the
  // .debug_info or the .debug_types section; this method will return the
  // correct section data.

or something.


https://reviews.llvm.org/D32167



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

[Lldb-commits] [PATCH] D32022: Fix backtrace of noreturn functions situated at the end of a module

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Sorry Pavel, I kept meaning to look at this patch this week but hadn't yet.  I 
will later today.  When I read over your description of the problem, it sounded 
like a good fix - haven't looked at the code yet.


https://reviews.llvm.org/D32022



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

[Lldb-commits] [PATCH] D32316: Change UniqueCStringMap to use ConstString as the key

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I'd recommend Greg Clayton as a reviewer.


Repository:
  rL LLVM

https://reviews.llvm.org/D32316



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

[Lldb-commits] [PATCH] D32316: Change UniqueCStringMap to use ConstString as the key

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

You should also add lldb-commits as a subscriber, fwiw, so 
comments/questions/etc are cc:'ed to the -commits list.


Repository:
  rL LLVM

https://reviews.llvm.org/D32316



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

[Lldb-commits] [PATCH] D32022: Fix backtrace of noreturn functions situated at the end of a module

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Hi Pavel, I'd document the new flag in include/lldb/Core/Address.h where we 
have documentation for the other flags being used.

It seems like we're fixing this a little indirectly, and I'm not sure it's the 
best approach.  I want to make sure I understand the situation correctly.

Say we have an object file with three sections

s1 0..99  (offset 0, size 100)
s2 100..199 (offset 100, size 100)
s3 200.299 (offset 200, size 100)

We have a noreturn function whose last instruction ends on the last byte of s2, 
so the saved return address is offset 200.  Your change makes it so that when 
we say "is 200 contained within 100..199", we will say yes, it is.  This gets 
us the correct Section for our symbol context and when we back up the pc value 
by one (the "decr_pc_and_recompute_addr_range = true" bit in 
RegisterContextLLDB::InitializeNonZerothFrame) so when we back up the offset 
within the section by 1, we're good.

The current behavior is that we pick the wrong section (s3) and somehow backing 
up the pc isn't working.

Maybe we can accomplish the same fix by looking at the if 
(decr_pc_and_recompute_addr_range) block of code?  Is 
decr_pc_and_recompute_addr_range not being set for this case?  Are we not 
correctly backing up the pc into the Section boundary correctly?

I have access to a linux machine so I can play with this myself, but it will 
take me a while to set up lldb on that system, could you let me know if you 
looked at this approach?  I know RegisterContextLLDB is complicated and it's 
easy to miss things - or it may be that you tried this approach and it didn't 
look possible.   (and I haven't had to touch this code in a few years so I may 
have forgotten about some "new Section is the same as the old Section" sanity 
check or something in there... I don't see it right now, but I may have missed 
it.)


https://reviews.llvm.org/D32022



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

[Lldb-commits] [PATCH] D32022: Fix backtrace of noreturn functions situated at the end of a module

[Jason Molenda via Phabricator via lldb-commits]

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

Pavel, my apologies for not following up on this, we had a big push to get 
ready for a beta release/conference this week, but it wasn't fair to leave you 
hanging with this change.  Please commit.  I'd like to play with this a little 
when I get free time (hahahaha) to see if I can't isolate the change a little 
more, but it'll be on me to play with the corefile test case on my own time 
instead of blocking you.


https://reviews.llvm.org/D32022



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

[Lldb-commits] [PATCH] D34199: Tweak SysV_arm64 function entry unwind plan

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.

Looks good to me.


https://reviews.llvm.org/D34199



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

[Lldb-commits] [PATCH] D34322: [lldb] Correctly escape newlines and backslashes in the JSON serializer

[Jason Molenda via Phabricator via lldb-commits]

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

LGTM.


https://reviews.llvm.org/D34322



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

[Lldb-commits] [PATCH] D34274: Remove home-grown thread-local storage wrappers

[Jason Molenda via Phabricator via lldb-commits]

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

We have thread local storage support on all our current darwin platforms.


https://reviews.llvm.org/D34274



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

[Lldb-commits] [PATCH] D34750: [UnwindAssembly/x86] Add support for "lea imm(%ebp), %esp" pattern

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Yeah, looks good.  You'll only see this on i386 because the SysV ABI require 
4-byte stack alignment.  On x86_64 it's 16-byte so the compiler doesn't need to 
emit the AND instruction to realign it.  On darwin we required 16 byte 
alignment on i386 too so we never saw this problem there.


Repository:
  rL LLVM

https://reviews.llvm.org/D34750



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

[Lldb-commits] [PATCH] D34929: respect target.x86-disassembly-flavor when using `memory read`

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda closed this revision.
jasonmolenda added a comment.

I committed this for Jeffrey in r307618.


Repository:
  rL LLVM

https://reviews.llvm.org/D34929



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

[Lldb-commits] [PATCH] D36620: Fix crash on parsing gdb remote packets

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

This looks good, the documentation for isprint() say that the value of the 
argument must be representable as an unsigned char, or EOF.


https://reviews.llvm.org/D36620



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

[Lldb-commits] [PATCH] D36620: Fix crash on parsing gdb remote packets

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda closed this revision.
jasonmolenda added a comment.

Sendingsource/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
Transmitting file data .done
Committing transaction...
Committed revision 311207.


https://reviews.llvm.org/D36620



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

[Lldb-commits] [PATCH] D36347: Add new script to launch lldb and set breakpoints for diagnostics all diagnostics seen.

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Sorry for missing this back in August.

I think it'd be clearer to import your python once in the startup, like

-o "script import $module" \

Multiple imports are a no-op IIUC so it's harmless to re-import the module 
every time the breakpoint is hit (I'm guessing it's only hit once, for that 
matter), but I think it's clearer to have this on its own line.

For what it's worth, if you use breakpoint command add -F python-function, your 
function is passed in the frame as well as the SBBreakpointLocation.  See 'help 
breakpoint add' for more information about this; I used this in a breakpoint 
python command I wrote a while back,

def handle_pthread_create_return(frame, bp_loc, dict):

  global pthreads_being_created
  
  thread_index_id = frame.GetThread().GetIndexID()

[...]

it might be a good idea to name the breakpoint you're adding and then you can 
delete it explicitly, in case the user is doing something unexpected with 
breakpoints.  e.g.

% ~/k/svn/lldb/build/Debug/lldb /tmp/a.out
(lldb) target create "/tmp/a.out"
Current executable set to '/tmp/a.out' (x86_64).
(lldb) br s -n main -N worker
Breakpoint 1: where = a.out`main, address = 0x00010ec0
(lldb) br del worker
1 breakpoints deleted; 0 breakpoint locations disabled.
(lldb) br li
No breakpoints currently set.
(lldb)


https://reviews.llvm.org/D36347



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

[Lldb-commits] [PATCH] D39199: [lldbtests] Handle errors instead of crashing

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I think this is a good idea, thanks for writing the patch Davide.


https://reviews.llvm.org/D39199



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

[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

> There was a talk at cppcon a few weeks ago from someone at backtrace.io who 
> had some insightful metrics on debugger performance memory consumption, and 
> LLDB had ~2x the memory consumption of GDB.

I haven't seen the paper, but my guess is that this is on linux and lldb wasn't 
using any accelerator tables for the dwarf of their inferior test program.

gdb was designed before there were any accelerator tables, so in their absence, 
it will create "partial symbol tables" with the address ranges and globally 
visible names of each CU.  When details are needed for a specific CU, the 
psymtab is expanded into a symbol table, symtab.  DWARF is scanned on initial 
load to create the psymtab names, and then when the symtab is created, it is 
ingested a CU at a time.  nb: my knowledge of lldb is a decade old at this 
point, I have not kept up with the project.

lldb was designed with the assumption that we could get these global symbol 
names from accelerator tables - we would map them into our address space and 
search for names in them.  When we are interested in a specific type or 
function or variable, we will read only the DWARF related to that 
type/function/variable.

In the absence of accelerator tables, I am willing to believe that lldb 
exhibits poor memory and startup performance -- that was not something we (at 
apple) concentrated on the performance of.  It's a completely valid area for 
future work in lldb -- adopting to using accelerator tables other than apple's 
vendor specific ones.

I suspect, in an absence of concrete information, that the measurement of 
lldb's memory use being primarily strings, and the perf issues mentioned in 
this paper, are all due to this non-performant code path on Linux.  If we see 
these memory issues when there are accelerator tables, then that's a big 
problem and I'll be shocked if we can actually be out-performed by gdb.  They 
were the base line that we designed ourselves to do better than.

We can expend a lot of energy on making string tables smaller, and of course I 
wouldn't object to people working in that direction.  But I think the real 
solution is to get lldb on non-darwin platforms to be using the accelerator 
tables that exist and allowing the full design of lldb to work as is intended.  
 My two cents.


https://reviews.llvm.org/D31172



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

[Lldb-commits] [PATCH] D57011: Refactor HAVE_LIBCOMPRESSION and related code in GDBRemoteCommunication

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

I'll test this later today.  The cleanup looks good; the goal of the most 
recent changes was to make HAVE_LIBCOMPESSION always enabled on Darwin systems 
- the setting would occasionally get lost in the Xcode project settings and 
we'd have slow channels using uncompressed communication until we looked into 
the perf regressions.  After this sequence happened twice, I wanted to make it 
always-on (it also needed to be conditional for a while because these API were 
only added to Darwin ~4 years ago)


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

https://reviews.llvm.org/D57011



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

[Lldb-commits] [PATCH] D57011: Refactor HAVE_LIBCOMPRESSION and related code in GDBRemoteCommunication

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Sorry for the delay in checking this out.  It works fine.  You need to add a 
#include "lldb/Host/Config.h" to GDBRemoteCommunication.h.


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

https://reviews.llvm.org/D57011



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

[Lldb-commits] [PATCH] D57745: [x64] Process the B field of the REX prefix correctly for the PUSH and POP instructions

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda accepted this revision.
jasonmolenda added a comment.

The change looks good to me, thanks for fixing this.  I'm not sure you're 
testing what you meant to test in TestPopRBPWithREX because you're 
pushing/popping r13.  You could get the unwind state at byte offset 2 (after 
the push r13 has executed) and see that you can GetRegisterInfo() r13, and the 
unwind state at byte offset 4 and verify that you can't GetRegisterInfo() r13.  
That's a good test to make sure we handle the B bit correctly.




Comment at: 
unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp:1953
+  0x41, 0x55, // pushq %rbp
+  0x41, 0x5d, // popq %rbp
+  0x90// nop

These are pushing/popping r13 aren't they?  0x40 0x55 gives us register 5 
(rbp), but 0x41 0x55 gives us register 13 (r13) if I'm reading the intel 
manuals right (volume 2a, section 2.2.1.2 "More on REX Prefix Fields").


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D57745



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

[Lldb-commits] [PATCH] D57928: Fix x86 return pattern detection

[Jason Molenda via Phabricator via lldb-commits]

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

Thanks for fixing this and adding a testcase.  I don't know why 0xc9 leave was 
here; it's handled over in x86AssemblyInspectionEngine::leave_pattern_p.  Do 
you have commit access?


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

https://reviews.llvm.org/D57928



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

[Lldb-commits] [PATCH] D58347: Reinitialize UnwindTable when the SymbolFile changes

[Jason Molenda via Phabricator via lldb-commits]

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



Comment at: source/Core/Module.cpp:1451-1454
+// Clear the unwind table too, as that may also be affected by the
+// symbol file information.
+m_unwind_table.reset();
+

clayborg wrote:
> Are we sure no one is holding onto info that was given out from an existing 
> UnwindTable? What if we do a backtrace, then load symbol files and then do 
> another backtrace? Seems like we will want to keep our existing info and just 
> call m_unwind_table->Clear()?
I THINK it will be OK.  RegisterContextLLDB holds shared pointers to the 
UnwindPlans that it got from the UnwindTable, so those will stay alive.  An 
UnwindPlan doesn't refer back to the UnwindTable it came from.


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

https://reviews.llvm.org/D58347



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

[Lldb-commits] [PATCH] D58912: [debugserver] Fix IsUserReady thread filtering

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Will this hide a thread that jumps through a null function pointer?  That's the 
only user process case where a pc of 0 needs to be reported to the developer.


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

https://reviews.llvm.org/D58912



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

[Lldb-commits] [PATCH] D58912: [debugserver] Fix IsUserReady thread filtering

[Jason Molenda via Phabricator via lldb-commits]

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

Looks good, that was my only concern.


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

https://reviews.llvm.org/D58912



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

[Lldb-commits] [PATCH] D58962: Sanity check --max-gdbserver-port

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

LGTM fwiw I think the --min-gdbserver-port and --max-gdbserver-port options 
were primarily/only used for iOS etc testsuite runs, where a fixed set of ports 
is relayed between the devices.  I was having some problems with lldb-server in 
platform mode on these devices and thought it would be nice to have a 
standalone implementation of the platform protocol packets, so I wrote one a 
couple months ago and that's what we're using now for this environment.  I 
should probably upstream it, I think it would work on linux too, it doesn't use 
any of lldb/llvm so it has several areas that are primitive because I was going 
for "good enough" and moving on, e.g. it has a dumb logging class and the 
networking could use some work, etc.  I want to keep this completely free of 
lldb/llvm dependencies so it's a little bit of an oddball.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D58962



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

[Lldb-commits] [PATCH] D59030: Pass ConstString by value

[Jason Molenda via Phabricator via lldb-commits]

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

Looks good to me.




Comment at: lldb/www/architecture/varformats.html:121
virtual 
size_t
-   
GetIndexOfChildWithName (const ConstString &name) = 0;
+   
GetIndexOfChildWithName (ConstString amp;name) = 0;


big bug! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59030



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

[Lldb-commits] [PATCH] D58347: Reinitialize UnwindTable when the SymbolFile changes

[Jason Molenda via Phabricator via lldb-commits]

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

Yeah, I'm ok with this, thanks.


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

https://reviews.llvm.org/D58347



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

[Lldb-commits] [PATCH] D59495: Fix an out-of-bounds error in RegisterContextDarwin_arm64

[Jason Molenda via Phabricator via lldb-commits]

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

Sorry for not weighing in earlier, yes this is good.


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

https://reviews.llvm.org/D59495



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

[Lldb-commits] [PATCH] D55718: [ARC] Basic support in gdb-remote process plugin

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

In D55718#1442965 , @clayborg wrote:

> It would be really nice to get the GDB remote server for ARC so that it vends 
> the correct regs to begin with, but if that isn't possible I guess we need to 
> do what we are doing here. I would really like to not see more architectures 
> have to add code to ProcessGDBRemote.cpp at all.  If that isn't possible this 
> solution will need to be used.


Unrelated to this patchset, but just last week I was working on something and 
thinking about how our system of "the RSP stub is the canonical source of 
register knowledge" is probably not the best architecture.  IMO the remote stub 
should teach us (1) the name of each register, (2) the numbers it will use for 
each register, (3) the size in bits of each register, and (4) the offset into 
the g/G packet of that register.

Everything else about registers should come from lldb in either 
architecture-specific definitions (rax has 64 bits, and it has a slice register 
eax of 32 bits, an ax of 16 bits etc; it is printed as Uint64/32/16), or ABI 
definitions (rdi is arg1 in the posix x86_64 ABI; rsp is eh_frame regnum 7, 
dwarf regnum 7).  Coordination between the RSP stub and lldb should be done by 
register name, confirmed by register size (the remote stub says it has a 64-bit 
r0 and I only know about a 32-bit r0, we have a problem).

Back in 2015 I added some code in r247121 that would fill in eh_frame and dwarf 
register numbers from the ABI plugin if the remote stub didn't provide them in 
ProcessGDBRemote's AugmentRegisterInfoViaABI.  It might be better for ARC to 
hardcode these generic register additions some place like that.  The change in 
247121 was a step in the direction of having lldb be the canonical source of 
information about registers, but only doing the easy first bit.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55718



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

[Lldb-commits] [PATCH] D55718: [ARC] Basic support in gdb-remote process plugin

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

In D55718#1443487 , @clayborg wrote:

> As long as the numbers _can_ still come from the GDB server, I am all for 
> that. If they are not supplied, then we use arch defaults. That allows new 
> system/arch bringups where the info might be changing often, and also allows 
> the info to be locked down. The architecture plug-ins are a great place for 
> this. We should also move the breakpoint opcode logic into the architecture 
> plug-ins.


I have no opinion about the RSP stub overriding lldb's definitions - fine by 
me.  I'm mostly interested in making lldb work better with rando RSP 
implementations in the wild which often give us very little information about 
the registers.

Moving the breakpoint opcode into lldb is interesting, would you want to move 
away from the Z0/z0 packets for setting/removing breakpoints then?  The 
set-breakpoint packet takes an address and iirc an instruction size.  So on 
armv7 processors, we know whether we're replacing a 2-byte or 4-byte 
instruction.  I don't think we want to move the logic of breakpoint setting & 
clearing into lldb and use generic read/write memory - the RSP stub may know 
how to clear instruction caches when we're modifying instruction data, for 
instance.  So we'd make a variation of the z0 packet which takes an address and 
a byte sequence to put at that address?  tbh it seems like the RSP stub is the 
correct place for this knowledge, even though we have to provide the hack of 
"use the 2-byte breakpoint instruction" / "use the 4-byte breakpoint 
instruction" on targets like armv7 because lldb has the knowledge of the 
underlying instructions.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55718



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

[Lldb-commits] [PATCH] D59896: [ObjectFileMachO] Disable memory caching for savecore.

[Jason Molenda via Phabricator via lldb-commits]

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

Looks good.  Removing that printf is good.  Could you also remove the 
printf("mach_header:...") in the same function?

It would be nice if include/lldb/Target/Process.h had the decl for 
ReadMemoryFromInferior right after ReadMemory(), and a comment that 
ReadMemoryFromInferior does not go through the memory cache (and most callers 
should go through ReadMemory()) so people know what the difference is.


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

https://reviews.llvm.org/D59896



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

[Lldb-commits] [PATCH] D60022: [Process] Fix WriteMemory return value

[Jason Molenda via Phabricator via lldb-commits]

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



Comment at: 
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestWriteMemory.py:19-20
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+self.runCmd("log enable gdb-remote packets")
+process = self.connect(target)

davide wrote:
> I'm not sure you really need this, we can commit it if we need to debug. 
> Please note that the bots always run with `TraceOn() == True`.
no big deal, but fwiw I put this sequence in every time I write a gdb remote 
client test - it's essential for debugging any problems with them.  The amount 
of output is very small, it's not like a real debug session.


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

https://reviews.llvm.org/D60022



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

[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added reviewers: clayborg, jingham.
Herald added a subscriber: abidh.
Herald added a project: LLDB.

I'm addressing a perf issue where DynamicLoaderDarwin has been notified that a 
batch of solibs have been loaded.  It adds them to the target one-by-one with 
Target::GetSharedModule(), and then calls Target::ModulesDidLoad() to give 
breakpoints a chance to evaluate for new locations, etc.  One of the things we 
do on ModulesDidLoad is call ProcessGDBRemote::ModulesDidLoad which will send 
the qSymbol packet to the gdb remote serial protocol stub if it indicates it 
wants to know a symbol address.  Currently, because Target::GetSharedModule() 
calls ModulesDidLoad, we will send the qSymbol packet many times - an app with 
hundreds of solibs are not unusual.

This patch renames Target::GetSharedModule to Target::AddModule() which better 
reflects what it actually does -- given a ModuleSpec set of criteria, it finds 
that binary on the local system and adds it to the Target, if it isn't already 
present.  This method name has confused all of us at one point or another.  As 
DynamicLoaderWindowsDYLD notes,

  // Confusingly, there is no Target::AddSharedModule.  Instead, calling
  // GetSharedModule() with a new module will add it to the module list and
  // return a corresponding ModuleSP.

It adds a flag to Target::AddModule to suppress notification of the new Module 
(i.e. don't call ModulesDidLoad) - if the caller does this, the caller must 
call Target::ModulesDidLoad before resuming execution.  I added a description 
of the method in Target.h to make this clear.

I also had to add flag to ModuleList::Append and ModuleList::AppendIfNeeded.  I 
made these have default values because many uses of this are in a loop creating 
a standalone ModuleList, and forcing all of them to specify the notify boolean 
was nonintuitive for those users.  When a ModuleList is a part of the Target, 
it has notifier callback functions, but when it is a standalone object, it 
doesn't.

I'm trying to think of how to test this -- but the problem I'm directly trying 
to address, the qSymbol packet being sent for every solib, instead of the # of 
groups of solib's that are loaded, is something we don't track today.  I'll 
continue to think about it.

rdar://problem/48293064


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D60172

Files:
  include/lldb/Core/ModuleList.h
  include/lldb/Target/Target.h
  source/API/SBTarget.cpp
  source/Commands/CommandObjectTarget.cpp
  source/Core/DynamicLoader.cpp
  source/Core/ModuleList.cpp
  source/Expression/FunctionCaller.cpp
  source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.cpp
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
  source/Plugins/Process/Windows/Common/ProcessWindows.cpp
  source/Plugins/Process/elf-core/ProcessElfCore.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Target.cpp

Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1442,7 +1442,8 @@
"Target::SetExecutableModule (executable = '%s')",
executable_sp->GetFileSpec().GetPath().c_str());
 
-m_images.Append(executable_sp); // The first image is our executable file
+const bool notify = true;
+m_images.Append(executable_sp, notify); // The first image is our executable file
 
 // If we haven't set an architecture yet, reset our architecture based on
 // what we found in the executable module.
@@ -1482,7 +1483,8 @@
   platform_dependent_file_spec = dependent_file_spec;
 
 ModuleSpec module_spec(platform_dependent_file_spec, m_arch.GetSpec());
-ModuleSP image_module_sp(GetSharedModule(module_spec));
+const bool notify = true;
+ModuleSP image_module_sp(AddModule(module_spec, notify));
 if (image_module_sp) {
   ObjectFile *objfile = image_module_sp->GetObjectFile();
   if (objfile)
@@ -1984,8 +1986,8 @@
   return false;
 }
 
-ModuleSP Target::GetSharedModule(const ModuleSpec &module_spec,
- Status *error_ptr) {
+ModuleSP Target::AddModule(const ModuleSpec &module_spec, bool notify,
+   Status *error_ptr) {
   ModuleSP module_sp;
 
   Status error;
@@ -2118,8 +2120,9 @@
   Module *old_module_ptr = old_module_sp.get();
   old_module_sp.reset();
   ModuleList::RemoveSharedModuleIfOrphaned(old_module_ptr);
-} else
-  m_images.Append(module_sp);
+} else {
+  m_images.Append(module_sp, notify);
+}
   } else
 module_sp.reset();
 }
Index:

[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda marked 2 inline comments as done.
jasonmolenda added a comment.

Thanks for looking the patch over.




Comment at: include/lldb/Target/Target.h:535
+   bool notify,
+   Status *error_ptr = nullptr);
 

clayborg wrote:
> Pavel had questions about this error. If we specify an error when we call 
> this, is there a way to get a valid module shared pointer back and still get 
> an error? Maybe this should be one of the llvm::ErrorOr return types?
There's only a handful of AddModule callers that pass in a Status object - the 
Windows ones, the CommandObjectTarget command, and the Minidump.  I'll look 
through the Platform GetSharedModules implementations tomorrow to see who might 
put useful things in the error object, but in general either AddModule finds a 
binary that matches the ModuleSpec, and returns a shared pointer to it, or 
finds nothing and returns an empty shared pointer.  The error object may have a 
message from a Platform indicating why the search failed ("could not find any 
SDK directories" or something) but I suspect this optional Status arg isn't 
doing anything.



Comment at: source/Commands/CommandObjectTarget.cpp:399-400
+  const bool notify = true;
+  ModuleSP module_sp = target_sp->AddModule(main_module_spec,
+  notify);
   if (module_sp)

clayborg wrote:
> Remove all "const bool notify = true; "statements and Inline with comment?
> 
> ```
> ModuleSP module_sp = target_sp->AddModule(main_module_spec, 
>   true /*notify*/);
> ```
> This would apply everywhere in this patch if so.
Can do.  I don't have a preference.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60172



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

[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

OK looked things over a bit.

I think there's a case for adopting llvm::ErrorOr here, and elsewhere across, 
but today the only place we're using that return type is when the error is 
encoded in a std::error_code (aka fancy errno).  We should have an ErrorOr that 
uses a Status object to contain the error, I think that's possible?  I've seen 
people use their own objects to return errors with llvm::Expected.  In any 
event, I think a change like this is a little out of scope for what I'm doing 
in this patch right now.

After spending time looking at all the error handling codepaths related to 
AddOrCreateModule, if we simply fail to find a binary we return an empty 
ModuleSP and the platform may set a Status with an error string (e.g. 
PlatformPOSIX will say no such file) if that's non-nullptr.  Or there may be a 
genuine error - a Target that doesn't have a Platform, we found a binary but it 
is an explicitly disallowed type (e.g. eTypeStubLibrary, a long-obsolete binary 
format on Darwin), we can return an error message explaining that to the caller 
if the Status ptr is non-nullptr.

The important wrinkle with AddOrCreateModule is that most of the callers are 
deep inside lldb internal mechanisms -- the DynamicLoaders.  And for most of 
those, we're going to handle file-not-found in custom ways, we're not going to 
relay the error messages.  For instance, the user-process DynamicLoaderDarwin, 
when it fails to find a file, it will call Process::ReadModuleFromMemory().  If 
the kernel debugging DynamicLoaderDarwinKernel fails to load a kext (a solib), 
it accumulates a list of kexts that it could not find, and at the end it will 
present a sorted list of them.   These callers have no interest in the returned 
error message.

On the other hand, a caller like CommandObjectTarget, which is directly driven 
by user interaction, we must print the error message ("file not found" and 
such).

So in this particularly API, because only some callers want the error strings, 
leaving this as an optional argument seems reasonable.  An llvm::ErrorOr which 
returns a ModuleSP or a Status would be a fine - and only the callers that want 
to print the error message would retrieve the Status object, the others would 
do their normal behavior when couldn't get the ModuleSP.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60172



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

[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 193609.
jasonmolenda added a comment.

Remove const bool notify's.  Rename method to Target::GetOrCreateModule.  
Refine the method headerdoc a bit.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60172

Files:
  include/lldb/Core/ModuleList.h
  include/lldb/Target/Target.h
  source/API/SBTarget.cpp
  source/Commands/CommandObjectTarget.cpp
  source/Core/DynamicLoader.cpp
  source/Core/ModuleList.cpp
  source/Expression/FunctionCaller.cpp
  source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.cpp
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
  source/Plugins/Process/Windows/Common/ProcessWindows.cpp
  source/Plugins/Process/elf-core/ProcessElfCore.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Target.cpp

Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1442,7 +1442,8 @@
"Target::SetExecutableModule (executable = '%s')",
executable_sp->GetFileSpec().GetPath().c_str());
 
-m_images.Append(executable_sp); // The first image is our executable file
+const bool notify = true;
+m_images.Append(executable_sp, notify); // The first image is our executable file
 
 // If we haven't set an architecture yet, reset our architecture based on
 // what we found in the executable module.
@@ -1482,7 +1483,8 @@
   platform_dependent_file_spec = dependent_file_spec;
 
 ModuleSpec module_spec(platform_dependent_file_spec, m_arch.GetSpec());
-ModuleSP image_module_sp(GetSharedModule(module_spec));
+ModuleSP image_module_sp(GetOrCreateModule(module_spec, 
+   true /* notify */));
 if (image_module_sp) {
   ObjectFile *objfile = image_module_sp->GetObjectFile();
   if (objfile)
@@ -1984,8 +1986,8 @@
   return false;
 }
 
-ModuleSP Target::GetSharedModule(const ModuleSpec &module_spec,
- Status *error_ptr) {
+ModuleSP Target::GetOrCreateModule(const ModuleSpec &module_spec, bool notify,
+   Status *error_ptr) {
   ModuleSP module_sp;
 
   Status error;
@@ -2118,8 +2120,9 @@
   Module *old_module_ptr = old_module_sp.get();
   old_module_sp.reset();
   ModuleList::RemoveSharedModuleIfOrphaned(old_module_ptr);
-} else
-  m_images.Append(module_sp);
+} else {
+  m_images.Append(module_sp, notify);
+}
   } else
 module_sp.reset();
 }
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -388,7 +388,8 @@
 Status error;
 // Try and find a module with a full UUID that matches. This function will
 // add the module to the target if it finds one.
-lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, &error);
+lldb::ModuleSP module_sp = GetTarget().GetOrCreateModule(module_spec, 
+ true /* notify */, &error);
 if (!module_sp) {
   // Try and find a module without specifying the UUID and only looking for
   // the file given a basename. We then will look for a partial UUID match
@@ -400,7 +401,8 @@
   ModuleSpec basename_module_spec(module_spec);
   basename_module_spec.GetUUID().Clear();
   basename_module_spec.GetFileSpec().GetDirectory().Clear();
-  module_sp = GetTarget().GetSharedModule(basename_module_spec, &error);
+  module_sp = GetTarget().GetOrCreateModule(basename_module_spec, 
+ true /* notify */, &error);
   if (module_sp) {
 // We consider the module to be a match if the minidump UUID is a
 // prefix of the actual UUID, or if either of the UUIDs are empty.
@@ -430,7 +432,7 @@
 
   module_sp = Module::CreateModuleFromObjectFile(
   module_spec, module->base_of_image, module->size_of_image);
-  GetTarget().GetImages().Append(module_sp);
+  GetTarget().GetImages().Append(module_sp, true /* notify */);
 }
 
 bool load_addr_changed = false;
Index: source/Plugins/Process/elf-core/ProcessElfCore.cpp
===
--- source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -244,7 +244,8 @@
   exe_module_spec.GetFileSpec()

[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Thanks Pavel, I'll convert this to use Expected<> while I'm working on it.  I'm 
writing a test case right now, and looking at how DynamicPluginDarwin works 
more closely for adding & removing binaries, e.g. there's also no way to batch 
remove libraries and have a group broadcast notification be sent.  There was 
also a bug in my original patch for calling LoadScriptingResourceForModule() to 
find a python resource file in a dSYM bundle - it is currently called from 
Target::ModuleUpdated when a binary is added to the target's ModuleList, but 
I'm suppressing that call with this change, so I need to move that over to 
ModulesDidLoad.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60172



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

[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 194000.
jasonmolenda added a comment.

Added a testcase, TestModuleLoadedNotifys.

Renamed the pure virtual notifier methods in ModuleList to add 'Notify' at the 
start of their names, updated the names in subclass Target as well.  Added a 
NotifyModulesRemoved and changed ModuleList::Remove(ModuleList&) so that it 
removes all of the modules one-by-one from the collection and then calls 
NotifyModulesRemoved() so they are broadcast as a group.  I renamed the methods 
to make it clearer in Target that these are subclass implementations.

Changed Target::SetExecutableModule() so that it will load all dependent 
libraries without notifying, and send a group notification about all of them 
once they've been added.

Moved the call to LoadScriptingResourceForModule from Target::NotifyModuleAdded 
to Target::ModulesDidLoad so DynamicLoader plugins can call ModulesDidLoad with 
the full list of libraries and still get scripting files loaded.

I tried changing Target::GetOrCreateModule to return Expected but I 
wasn't thrilled with the double-dereference of the returned value when I 
started updating callers.  It wasn't horrible, but it looked messy.  I should 
go back and try again but my first shot at a few of them didn't look great.

I want to hand-test the scripting resource loading a bit on Monday but this is 
probably about what I'm doing for now.  At this point, DynamicLoaderDarwin 
sends batched notifications on the CreateTarget() binary dependent binaries.  
It sends a batched notification when we start a real process and throw away all 
those binaries.  It sends out batched notifications when we load the binaries 
back in via dyld notifications.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60172

Files:
  include/lldb/Core/ModuleList.h
  include/lldb/Target/Target.h
  
packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/Makefile
  
packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/TestModuleLoadedNotifys.py
  
packages/Python/lldbsuite/test/functionalities/target-new-solib-notifications/main.cpp
  source/API/SBTarget.cpp
  source/Commands/CommandObjectTarget.cpp
  source/Core/DynamicLoader.cpp
  source/Core/ModuleList.cpp
  source/Expression/FunctionCaller.cpp
  source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.cpp
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
  source/Plugins/Process/Windows/Common/ProcessWindows.cpp
  source/Plugins/Process/elf-core/ProcessElfCore.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Target.cpp

Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1442,7 +1442,8 @@
"Target::SetExecutableModule (executable = '%s')",
executable_sp->GetFileSpec().GetPath().c_str());
 
-m_images.Append(executable_sp); // The first image is our executable file
+const bool notify = true;
+m_images.Append(executable_sp, notify); // The first image is our executable file
 
 // If we haven't set an architecture yet, reset our architecture based on
 // what we found in the executable module.
@@ -1470,6 +1471,7 @@
 }
 
 if (executable_objfile && load_dependents) {
+  ModuleList added_modules;
   executable_objfile->GetDependentModules(dependent_files);
   for (uint32_t i = 0; i < dependent_files.GetSize(); i++) {
 FileSpec dependent_file_spec(
@@ -1482,13 +1484,16 @@
   platform_dependent_file_spec = dependent_file_spec;
 
 ModuleSpec module_spec(platform_dependent_file_spec, m_arch.GetSpec());
-ModuleSP image_module_sp(GetSharedModule(module_spec));
+ModuleSP image_module_sp(GetOrCreateModule(module_spec, 
+ false /* notify */));
 if (image_module_sp) {
+  added_modules.AppendIfNeeded (image_module_sp, false);
   ObjectFile *objfile = image_module_sp->GetObjectFile();
   if (objfile)
 objfile->GetDependentModules(dependent_files);
 }
   }
+  ModulesDidLoad(added_modules);
 }
   }
 }
@@ -1606,20 +1611,19 @@
   return false;
 }
 
-void Target::WillClearList(const ModuleList &module_list) {}
+void Target::NotifyWillClearList(const ModuleList &module_list) {}
 
-void Target::ModuleAdded(const ModuleList &module_list,
+void Target::NotifyModuleAdded(const ModuleList &module_list,
  const ModuleSP &module_sp) {
   // A module is being added to this target for the first time
   if (m_valid) {
 ModuleList my_mo

[Lldb-commits] [PATCH] D60655: Fix typo in ArmUnwindInfo::GetUnwindPlan

[Jason Molenda via Phabricator via lldb-commits]

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

Agreed, this change is correct.  I double checked this in the 
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0038b/IHI0038B_ehabi.pdf 
doc (mod 24 Nov 2015) and the only difference in Section 9.3 "Frame unwinding 
instructions" Table 4 and this code is in the comment - they now refer to this 
as 'Pop VFP double-precision registers D[8]-D[8+nnn] saved (as if) by VPUSH 
(see remark d)'.   But there's no benefit in updating this one comment while 
the others are using the older comments (this code was originally written 
against the 30 Nov 2012 version of the doc).

We don't have any tests for the ARM.exidx exception handling information today, 
it would be take some work to add that.  It's a format like darwin's compact 
unwind or less correctly, eh_frame, only defined for 32-bit arm.  I would 
commit this obvious fix without a test.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D60655



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

[Lldb-commits] [PATCH] D60829: FuncUnwinders: remove "current_offset" from function arguments

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Thanks for digging in to this Pavel.  I haven't had time to look over the patch 
today, but I'd like to; I'll make time for it in the next couple days if that's 
OK.


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

https://reviews.llvm.org/D60829



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

[Lldb-commits] [PATCH] D60949: UnwindPlan: pretty-print dwarf expressions

[Jason Molenda via Phabricator via lldb-commits]

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

very nice.


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

https://reviews.llvm.org/D60949



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

[Lldb-commits] [PATCH] D60957: Read ObjC class names in one large read, instead of reading them individually

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: jingham.
Herald added subscribers: teemperor, abidh.
Herald added a project: LLDB.

There's a perf problem with Objective-C programs as we add more classes to the 
Darwin libraries over time - when lldb goes to run its first expression on one 
of these systems, it will load the ObjC class names from the run time, both the 
shared cache libraries and any app-contributed classes.  lldb has two 
expressions it runs in the inferior to collect this information into a buffer 
allocated by lldb - returning the isa pointers and a hash of each class name 
used to id them.  lldb would then read the names of the classes out of memory, 
which were not localized to one region of memory, and this has become a larger 
and larger performance issue.

This patch modifies:

1. The two jitted functions, g_get_dynamic_class_info_body and 
g_get_shared_cache_class_info_body, now take the address of an allocated string 
pool buffer and size.  If the address of the string pool is 0, no names are 
copied.  The functions will copy the class names in to the string pool buffer 
and add an offset to the (isa, hash) that were previously being returned.  If 
an entry in the (isa, hash, stroffset) array does not have an entry in the 
string pool, UINT32_MAX is used.  If lldb's pre-allocated string pool buffer is 
too small, entries that did not fit will get UINT32_MAX and lldb will read the 
class names the old slow way.

2. Modifies the two methods that call these jitted functions, 
AppleObjCRuntimeV2::UpdateISAToDescriptorMapDynamic and 
AppleObjCRuntimeV2::UpdateISAToDescriptorMapSharedCache, to allocate the string 
pool buffer, pass the address and size to the jitted expression, scan the array 
of (isa, hash, stroffset) tuples to find the highest stroffset, read that part 
of the string pool out of the inferior with a single read [*], pass the buffer 
to ParseClassInfoArray, deallocate the stringpool from the inferior.

[X] nb: I just saw an unintended behavior on the last class name as I was 
writing this.  Given that it uses the highest stroffset it finds to read the 
string pool back out of the inferior, the final class name won't be copied up.  
The test in ParseClassInfoArray will not

3. Modifies AppleObjCRuntimeV2::ParseClassInfoArray to grab the class name from 
the stringpool buffer if the stroffset is != UINT32_MAX and is within the 
copied buffer size.

[X] nb: I just saw an unintended behavior on the last class name as I was 
writing this.  Given that it uses the highest stroffset it finds to read the 
string pool back out of the inferior, the final class name won't be copied up.  
The test in ParseClassInfoArray to check that the stroffset is within the 
bounds of the copied buffer should mean that we read the last class name out of 
the inferior aka the old method.  I'll double check this is handled correctly 
tomorrow.

This patch also includes a change that Frederic Riss wrote the other month but 
hadn't upstreamed yet, where we detect swift names in the shared cache and 
don't compute the hash in the jitted function, doing it up in lldb later.

Testing the patch shows no testsuite difference on Mac native.  As an 
experiment, I intentionally introduced a bug where the class names in the 
string pool were corrupted and it caused the testsuite failures I expected.  
From a performance point of view, this shows the packet behavior I was aiming 
for.

rdar://problem/27798609


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D60957

Files:
  source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
  source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h

Index: source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
===
--- source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
+++ source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
@@ -297,7 +297,8 @@
   UpdateISAToDescriptorMapDynamic(RemoteNXMapTable &hash_table);
 
   uint32_t ParseClassInfoArray(const lldb_private::DataExtractor &data,
-   uint32_t num_class_infos);
+   uint32_t num_class_infos, 
+   lldb::DataBufferSP strpool_buffer_sp);
 
   DescriptorMapUpdateResult UpdateISAToDescriptorMapSharedCache();
 
Index: source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
===
--- source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -79,10 +79,11 @@
 extern "C"
 {
 size_t strlen(const char *);
-char *strncpy (char * s1, const char * s2, size_t n);
+char *strcpy (char * dst, const char * src);
 int printf(const char * format, ...);
 }
 #define DEBUG_PRINTF(fm

[Lldb-commits] [PATCH] D60829: FuncUnwinders: remove "current_offset" from function arguments

[Jason Molenda via Phabricator via lldb-commits]

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

Sorry for the delay in looking at this.  It looks like a good change, an 
improvement on the original for sure.  I've never seen multiple eh_frame FDEs 
for a single function, so that's fine.  The .ARM.exidx format and Apple's 
compact unwind format can theoretically have multiple different unwind plans 
for a single function - but I've never seen it done in practice.  More 
commonly, a single unwind plan will be used for multiple functions that have 
the same unwind state.  (these two formats don't describe prologue/epilogues - 
they only describe the unwind state in the middle of the function where 
exceptions can be thrown)


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

https://reviews.llvm.org/D60829



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

[Lldb-commits] [PATCH] D27289: Return "thread-pcs" in jstopinfo on Linux/Android.

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

For what it's worth, this change was part of the changes we made to reduce 
packet traffic for "private stops".  When a high-level "step" or "next" command 
is done, we instruction step (or fast-step with Jim's fast-step code when it 
sees a sequence of instructions that cannot branch - we set a breakpoint and 
continue instead of instruction stepping) until we reach the end of the 
stepping range and then we do a "public stop".  Given that the private stops 
are more numerous, we spent a bunch of time looking at everything we could 
eliminate for those private stops.  Getting all the pc values into the stop 
packet (aka T or ? packet) was a big one.  For multi-threaded programs we also 
added a "jstopinfo" which gives the stop reasons for all threads that had a 
stop reason, e.g.

T05thread:90834;threads:90834,90845,90846,90847,90848,90849,9084a,9084b,9084c,9084d,9084e,9084f,90850,90851;thread-pcs:10ac4,7fffb3eb1fda,7fffb3eb1fda,7fffb3eb1fda,7fffb3eb1fda,7fffb3eb1fda,7fffb3eb1fda,7fffb3eb1fda,7fffb3eb1fda,7fffb3eb1fda,7fffb3eb1fda,7fffb3eb1fda,7fffb3eb1fda,7fffb3eb1fda;jstopinfo:5b7b22746964223a3539313932342c226d6574797065223a362c226d6564617461223a5b322c305d2c22726561736f6e223a22657863657074696f6e227d5d;metype:6;mecount:2;medata:2;medata:0;memory:0x7fff5fbff8e0=30f9bf5fff7fb20c0100;memory:0x7fff5fbff930=40f9bf5fff7f5532d8b3ff7f;

(I elided the expedited registers)

The jstopinfo above is ascii-hex encoding of the JSON,

[{"tid":591924,"metype":6,"medata":[2,0],"reason":"exception"}]

else lldb would iterate over all the threads to see if any of them stopped for 
some reason (e.g. hitting a breakpoint) while we were instruction stepping.  If 
you don't see lldb doing that thread-stop-reason checking for your platform, it 
won't be needed.

(and looking at this, it's giving the stop reasoon for the thread that hit a 
breakpoint -- which we already get from the T packet.  Hmmm, I see another tiny 
perf win in the near future! ;)

We're also sending up 4 words of stack memory which might be the start of this 
function's stack frame and it's caller's stack frame.  For a simple "find the 
pc of the caller stack frame" in lldb, this can remove a couple of memory reads 
as the thread stepping logic needs to know who the caller stack frame is.

We also did some work on the public stop side, adding a jThreadsInfo packet 
which gives us all the information about all of the threads.  We get all the 
expedited registers, the thread name, the stop reasons, information about the 
darwin libdispatch queues, and expedited stack memory to help speed up a stack 
walk (I don't remember offhand if debugserver tries to walk the entire stack 
using the simple follow-the-frame-chain-in-stack-memory technique or if it caps 
the stack walk.  but our main IDE, Xcode, needs the full stack for its UI so we 
wanted to fetch that in one big packet so we can give the pc and function names 
without any additional packet traffic.)


https://reviews.llvm.org/D27289



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

[Lldb-commits] [PATCH] D27289: Return "thread-pcs" in jstopinfo on Linux/Android.

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

@labath ah I see I hadn't looked at the lldb-server packets so I didn't know 
you had jThreadsInfo, good to hear.  Yes, if your target is built mostly 
-fomit-frame-pointer, lldb-server won't be able to do a stack walk without 
reading eh_frame or the arm unwind info that Tamas added a year or so ago.  
We've always avoided having lldb-server/debugserver know anything about files 
and symbols else the memory footprint will grow and we need these stubs to run 
in low-memory environments; I'm not sure how it could do a stalk walk.  It's a 
pretty big perf hit for lldb to walk to stacks of modern apps that have tens of 
threads on every public stop. :(

Agree with not sending the registers in the jstopinfo kv pair in the T/? packet 
- \it's such a space-inefficient encoding compared to the usual kv pairs of 
info in the T packet, as weirdly as they're formatted.  JSON requires we use 
base 10 for numbers, and then ascii-hex encoding it doubles it (I played with 
the idea of using base64 for jstopinfo originally, but instead worked on 
including the smallest amount of info we needed).

If we were going to be serious about the T packet format, I think we'd add a 
new JT packet (or whatever) which is all the information in straight JSON, and 
some request from lldb to enable JT packets instead of T packets.  But it 
doesn't seem like a pressing concern, and the more we deviate from standard 
gdb-remote protocol (even if it's optional deviation with fallback to standard 
packets), I think it makes lldb more fragile for interop.


https://reviews.llvm.org/D27289



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

[Lldb-commits] [PATCH] D27289: Return "thread-pcs" in jstopinfo on Linux/Android.

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

@labath intersting idea, sending a blob of stack memory above the current stack 
pointer reg value to accelerate an unwind.  If you have a lot of small stack 
frames, it could be a performance benefit.  Common/simple methods do their work 
in 32-64 bytes of stack space (IMO) so we could look at the perf implications 
of sending up the 64 bytes of stack above $sp in the T packet.  We're locked 
into ascii for the T packet so we're talking about 128 characters plus 16 for 
the address.  It would only be needed on platforms where a frame pointer 
register is not used by default.  If you look at your packet logs, I bet lldb 
is doing a memory read on private stops as it tries to figure out what the 
caller function is - you could eliminate that packet with this, assuming 
smallish stack frames.

As mentioned, for wireless gdb-remote protocol (bluetooth, wifi) the 
performance characteristics are very different than for wired connections, but 
if we sent up that blob of stack data only on platforms without a fp reg used 
by default, it wouldn't impact the apple environments.

Agreed, I think we're all waiting for the addition of one more thing to the T 
packet to finally push us off of it, and to make up a new stop packet.  Greg's 
addition of jstopinfo almost did it for me personally. :)  The only thing I 
don't like about data sent in JSON is the base 10 requirement for numbers.  If 
we want to send up 64 bytes of stack memory, should we send that as an array of 
bytes?  Or an array of 64-bit ints that have been byteswapped by debugserver to 
big-endian format?  Right now debugserver sends up two words of memory at the 
frame pointer value - so 16 bytes on a 64-bit process - in native endian order, 
like

memory:0x7fff5fbffa90=a0fabf5fff7fad95588fff7f;


https://reviews.llvm.org/D27289



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

[Lldb-commits] [PATCH] D27459: Straw-man proposal for new logging syntax

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

A couple of thoughts / two cents.

I don't mind the current if (log) { ... } style of logging, even with the 
PRIx64's and having to do filepath.GetPath().c_str() and all that.  I like 
being able to do extra work in a if (log) block of code -- create a 
SymbolContext for something or whatever, and as I'm debugging that section of 
code I like to be able to put a breakpoint there and look more closely.  If 
this is all in a preprocessor macro, a breakpoint is going to stop me on the 
if() and then I'm trying to step through a macro?  And if my computation is 
down in a lambda I need to figure out how to put a breakpoint in that or step 
into that somehow to look at the values being computed.  How do I create a 
SymbolContext my_symctx(...); and then include that in what I log if I have to 
do all of this in the formatv arguments?

I'm not thrilled with the formatv reinvention of format specification.  The 
printf formatters are a bizarre little invention, but it's a bizarre invention 
that we've all worked with for decades and everyone who works in C knows well.  
The formatv format specification may be superior, but I don't want to see 
innovation here, I want clarity with the linga franca that every C programmer 
knows.  I think the barrier to adopting something non-standard is very high.

The formatv format specification string seems to explicitly mention the 
ordering of the arguments, like llvm::formatv("{0} {1} {2}", first, second, 
third).  I'm guessing this allows you to print the arguments in different order 
than they appear?  Like  llvm::formatv("{0} {2} {2} {1}", first, second, 
third)?  What's the benefit of this vrs. the uncertainty of which arguments are 
used in different parts of the string (short of counting them out by hand)?  If 
I have a formatv format specification like "{0} {1} {2} {3} {4}" and I want to 
insert an argument between 2 and 3, I need to renumber 3 and 4 now?  Or do I do 
"{0} {1} {2} {5} {3} {4}" and throw my argument on to the end?

Simply *ability* to put the arguments in any order is my concern.  Of course 
I'm never going to do it, but I'm going to have to work on code where other 
people have done it.

I don't personally want to see the file / function / line numbers in my 
logging.  It's fine if that's an option that other people can enable if they 
want it.  There's going to be a growth in the size of our text for the constant 
strings for every log point having its filename and function, but it will 
probably not be significant (maybe the strings can be coalesced, I wouldn't bet 
one way or the other.)

If formatv could take something a LOT closer to printf, I'd be better with it.  
Something like the python string formatters would be a real improvement - just 
%d for base 10 numbers, not having to worry about the size of the argument in 
the format string, for instance, and would still be readable & understandable 
by any C programmer out there.

If we're going to start using this alternate logging scheme, eventually there 
will be a push to use it everywhere, or someone will go through all the other 
classes in the code base and switch it out without consultation.  We need to 
all sign on to this idea before it goes into the code base at all.


https://reviews.llvm.org/D27459



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

[Lldb-commits] [PATCH] D28035: Stop limiting the number of TSan backtrace size to 8

[Jason Molenda via Phabricator via lldb-commits]

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

Looks fine to me, the only reason I could think to cap it is if the method used 
to backtrace the thread could get stuck in a loop & provide infinite number of 
frames.  but we could have crashed earlier than this if that happened, so I'm 
not concerned.


https://reviews.llvm.org/D28035



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