[Lldb-commits] [PATCH] D51816: Fix raw address breakpoints not resolving

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.
ted added reviewers: jingham, LLDB.
Herald added a subscriber: lldb-commits.

An address breakpoint of the form "b 0x1000" won't resolve if it's created 
while the process isn't running. This patch deletes Address::SectionWasDeleted, 
renames Address::SectionWasDeletedPrivate to SectionWasDeleted (and makes it 
public), and changes the section check in Breakpoint::ModulesChanged back to 
its original form


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51816

Files:
  include/lldb/Core/Address.h
  source/Breakpoint/Breakpoint.cpp
  source/Core/Address.cpp


Index: source/Core/Address.cpp
===
--- source/Core/Address.cpp
+++ source/Core/Address.cpp
@@ -281,7 +281,7 @@
 // We have a valid file range, so we can return the file based address by
 // adding the file base address to our offset
 return sect_file_addr + m_offset;
-  } else if (SectionWasDeletedPrivate()) {
+  } else if (SectionWasDeleted()) {
 // Used to have a valid section but it got deleted so the offset doesn't
 // mean anything without the section
 return LLDB_INVALID_ADDRESS;
@@ -302,7 +302,7 @@
 return sect_load_addr + m_offset;
   }
 }
-  } else if (SectionWasDeletedPrivate()) {
+  } else if (SectionWasDeleted()) {
 // Used to have a valid section but it got deleted so the offset doesn't
 // mean anything without the section
 return LLDB_INVALID_ADDRESS;
@@ -761,12 +761,6 @@
 }
 
 bool Address::SectionWasDeleted() const {
-  if (GetSection())
-return false;
-  return SectionWasDeletedPrivate();
-}
-
-bool Address::SectionWasDeletedPrivate() const {
   lldb::SectionWP empty_section_wp;
 
   // If either call to "std::weak_ptr::owner_before(...) value returns true,
Index: source/Breakpoint/Breakpoint.cpp
===
--- source/Breakpoint/Breakpoint.cpp
+++ source/Breakpoint/Breakpoint.cpp
@@ -555,7 +555,7 @@
 // address that we haven't resolved to a section yet.  So we'll have to
 // look in all the new modules to resolve this location. Otherwise, if
 // it was set in this module, re-resolve it here.
-if (section_sp && section_sp->GetModule() == module_sp) {
+if (!section_sp || section_sp->GetModule() == module_sp) {
   if (!seen)
 seen = true;
 
Index: include/lldb/Core/Address.h
===
--- include/lldb/Core/Address.h
+++ include/lldb/Core/Address.h
@@ -525,11 +525,11 @@
   bool CalculateSymbolContextLineEntry(LineEntry &line_entry) const;
 
   //--
-  // Returns true if the section should be valid, but isn't because the shared
-  // pointer to the section can't be reconstructed from a weak pointer that
-  // contains a valid weak reference to a section. Returns false if the section
-  // weak pointer has no reference to a section, or if the section is still
-  // valid
+  // Returns true if the m_section_wp once had a reference to a valid section
+  // shared pointer, but no longer does. This can happen if we have an address
+  // from a module that gets unloaded and deleted. This function should only be
+  // called if GetSection() returns an empty shared pointer and you want to
+  // know if this address used to have a valid section.
   //--
   bool SectionWasDeleted() const;
 
@@ -539,15 +539,6 @@
   //--
   lldb::SectionWP m_section_wp; ///< The section for the address, can be NULL.
   lldb::addr_t m_offset; ///< Offset into section if \a m_section_wp is 
valid...
-
-  //--
-  // Returns true if the m_section_wp once had a reference to a valid section
-  // shared pointer, but no longer does. This can happen if we have an address
-  // from a module that gets unloaded and deleted. This function should only be
-  // called if GetSection() returns an empty shared pointer and you want to
-  // know if this address used to have a valid section.
-  //--
-  bool SectionWasDeletedPrivate() const;
 };
 
 //--


Index: source/Core/Address.cpp
===
--- source/Core/Address.cpp
+++ source/Core/Address.cpp
@@ -281,7 +281,7 @@
 // We have a valid file range, so we can return the file based address by
 // adding the file base address to our offset
 return sect_file_addr + m_offset;
-  } else if (SectionWasDeletedPrivate()) {
+  } else if (SectionWasDeleted()) {
 // Used to have a valid section but it got deleted so the offset doesn't
 // mean anything without the sec

[Lldb-commits] [PATCH] D51816: Fix raw address breakpoints not resolving

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

Yes, I'll add a new test to TestAddressBreakpoints.py.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D51816



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

[Lldb-commits] [PATCH] D51816: Fix raw address breakpoints not resolving

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 164533.
ted added a comment.

Added a test to TestAddressBreakpoints.py that sets an address breakpoint 
before launch, launches, and checks to see if the breakpoint was hit.


https://reviews.llvm.org/D51816

Files:
  include/lldb/Core/Address.h
  
packages/Python/lldbsuite/test/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py
  source/Breakpoint/Breakpoint.cpp
  source/Core/Address.cpp

Index: source/Core/Address.cpp
===
--- source/Core/Address.cpp
+++ source/Core/Address.cpp
@@ -281,7 +281,7 @@
 // We have a valid file range, so we can return the file based address by
 // adding the file base address to our offset
 return sect_file_addr + m_offset;
-  } else if (SectionWasDeletedPrivate()) {
+  } else if (SectionWasDeleted()) {
 // Used to have a valid section but it got deleted so the offset doesn't
 // mean anything without the section
 return LLDB_INVALID_ADDRESS;
@@ -302,7 +302,7 @@
 return sect_load_addr + m_offset;
   }
 }
-  } else if (SectionWasDeletedPrivate()) {
+  } else if (SectionWasDeleted()) {
 // Used to have a valid section but it got deleted so the offset doesn't
 // mean anything without the section
 return LLDB_INVALID_ADDRESS;
@@ -761,12 +761,6 @@
 }
 
 bool Address::SectionWasDeleted() const {
-  if (GetSection())
-return false;
-  return SectionWasDeletedPrivate();
-}
-
-bool Address::SectionWasDeletedPrivate() const {
   lldb::SectionWP empty_section_wp;
 
   // If either call to "std::weak_ptr::owner_before(...) value returns true,
Index: source/Breakpoint/Breakpoint.cpp
===
--- source/Breakpoint/Breakpoint.cpp
+++ source/Breakpoint/Breakpoint.cpp
@@ -555,7 +555,7 @@
 // address that we haven't resolved to a section yet.  So we'll have to
 // look in all the new modules to resolve this location. Otherwise, if
 // it was set in this module, re-resolve it here.
-if (section_sp && section_sp->GetModule() == module_sp) {
+if (!section_sp || section_sp->GetModule() == module_sp) {
   if (!seen)
 seen = true;
 
Index: packages/Python/lldbsuite/test/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py
===
--- packages/Python/lldbsuite/test/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py
+++ packages/Python/lldbsuite/test/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py
@@ -97,3 +97,40 @@
 
 # The hit count for the breakpoint should now be 2.
 self.assertTrue(breakpoint.GetHitCount() == 2)
+
+
+
+def test_address_breakpoint_set_before_launch(self):
+"""Test that an address bp set before the process is launched works correctly."""
+self.build()
+
+exe = self.getBuildArtifact("a.out")
+
+# Create a target by the debugger.
+target = self.dbg.CreateTarget(exe)
+self.assertTrue(target, VALID_TARGET)
+
+# get the address of the symbol "main"
+sc_list = target.FindSymbols("main", lldb.eSymbolTypeCode)
+symbol = sc_list.GetContextAtIndex(0).GetSymbol()
+address = symbol.GetStartAddress().GetFileAddress()
+
+# BreakpointCreateBySBAddress will resolve the address, causing this
+# test to always pass, so use runCmd
+self.runCmd("break set -a " + str(address))
+
+# Disable ASLR.  This will allow us to actually test (on platforms that support this flag)
+# that the breakpoint was able to track the module.
+
+launch_info = lldb.SBLaunchInfo(None)
+flags = launch_info.GetLaunchFlags()
+flags &= ~lldb.eLaunchFlagDisableASLR
+launch_info.SetLaunchFlags(flags)
+
+error = lldb.SBError()
+
+process = target.Launch(launch_info, error)
+self.assertTrue(process, PROCESS_IS_VALID)
+self.expect("process status", STOPPED_DUE_TO_BREAKPOINT,
+substrs=["stop reason = breakpoint 1.1"])
+
Index: include/lldb/Core/Address.h
===
--- include/lldb/Core/Address.h
+++ include/lldb/Core/Address.h
@@ -525,11 +525,11 @@
   bool CalculateSymbolContextLineEntry(LineEntry &line_entry) const;
 
   //--
-  // Returns true if the section should be valid, but isn't because the shared
-  // pointer to the section can't be reconstructed from a weak pointer that
-  // contains a valid weak reference to a section. Returns false if the section
-  // weak pointer has no reference to a section, or if the section is still
-  // valid
+  // Returns true if the m_section_wp once had a reference to a valid section
+  // shared pointer, but no longer does. This can happen 

[Lldb-commits] [PATCH] D51162: [PDB] Restore AST from PDB symbols

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

Another issue:

  auto context = symbol.getRawSymbol().getName();
  auto context_size = context.rfind("::");

...

  auto from = 0;
  while (from < context_size) {
   

context_size is size_t (from std::string::rfind), but on clang 5.01, "auto from 
= 0" makes from an int. The comparison on the next line generates a warning:
comparison of integers of different signs: 'int' and 'unsigned long' 
[-Werror,-Wsign-compare] on 64 bit Linux.

The "auto from = 0" should be "size_t from = 0", since auto can't determine the 
correct type.


Repository:
  rL LLVM

https://reviews.llvm.org/D51162



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

[Lldb-commits] [PATCH] D41533: Advanced guessing of rendezvous breakpoint

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

Thanks for adding me, Pavel.

Hexagon running Linux uses this plugin. These changes lgtm.

Standalone Hexagon uses its own dyld plugin; I need to look at it and see if I 
want to pull any of these ideas into it.


https://reviews.llvm.org/D41533



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

[Lldb-commits] [PATCH] D39967: Disable breakpoints before writing memory and re-enable after.

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

Traditionally, remote gdbservers handle memory operations that overlap a 
software breakpoint. Writes get the new value saved off, and reads get the 
breakpoint opcode replaced by the saved value.


https://reviews.llvm.org/D39967



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

[Lldb-commits] [PATCH] D39967: Disable breakpoints before writing memory and re-enable after.

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In https://reviews.llvm.org/D39967#1017754, @labath wrote:

> I believe lldb-server does not work around breakpoint opcodes during writing 
> (it does for reading), but that can be fixed, of course.


It should. If it doesn't, then a write over a breakpoint would clobber it.

What should happen is lldb-server finds all breakpoint locations in a write, 
and replaces the saved opcode for each such breakpoint with the corresponding 
value in the write, then writes the data to memory. Breakpoint addresses can 
either be skipped, or have the opcode in them replaced by the breakpoint opcode 
before the write.


https://reviews.llvm.org/D39967



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

[Lldb-commits] [PATCH] D45333: WIP: [LIT] Have lit run the lldb test suite

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

@JDevlieghere  I'm seeing the same issue as @alberto_magni . The Visual Studio 
cmake generator gives errors. We're reverting it internally, but it needs to be 
fixed or reverted ASAP.

The issue could be this set of code from test/CMakeLists.txt:

  configure_file(
${CMAKE_CURRENT_SOURCE_DIR}/../lit/Suite/lit.site.cfg.in
${CMAKE_CURRENT_BINARY_DIR}/../lit/Suite/lit.site.cfg
)
  file(GENERATE
OUTPUT
${CMAKE_CURRENT_BINARY_DIR}/../lit/Suite/lit.site.cfg
INPUT
${CMAKE_CURRENT_BINARY_DIR}/../lit/Suite/lit.site.cfg
)

The equivalent from clang/test/CMakeLists.txt is:

  configure_lit_site_cfg(
${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.py.in
${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg.py
MAIN_CONFIG
${CMAKE_CURRENT_SOURCE_DIR}/lit.cfg.py
)
  
  configure_lit_site_cfg(
${CMAKE_CURRENT_SOURCE_DIR}/Unit/lit.site.cfg.py.in
${CMAKE_CURRENT_BINARY_DIR}/Unit/lit.site.cfg.py
MAIN_CONFIG
${CMAKE_CURRENT_SOURCE_DIR}/Unit/lit.cfg.py
)


Repository:
  rL LLVM

https://reviews.llvm.org/D45333



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

[Lldb-commits] [PATCH] D37154: lldb-mi: -var-update can hang when traversing complex types with pointers

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.

-var-update calls CMICmdCmdVarUpdate::ExamineSBValueForChange to check if a 
varObj has been updated. It checks that the varObj is updated, then recurses on 
all of its children. If a child is a pointer pointing back to a parent node, 
this will result in an infinite loop, and lldb-mi hanging.

The problem is exposed by 
packages/Python/lldbsuite/test/tools/lldb-mi/variable/TestMiVar.py, but this 
test is skipped everywhere.

This patch changes ExamineSBValueForChange to not traverse children of varObjs 
that are pointers.


https://reviews.llvm.org/D37154

Files:
  tools/lldb-mi/MICmdCmdVar.cpp


Index: tools/lldb-mi/MICmdCmdVar.cpp
===
--- tools/lldb-mi/MICmdCmdVar.cpp
+++ tools/lldb-mi/MICmdCmdVar.cpp
@@ -516,6 +516,13 @@
 lldb::SBValue member = vrwValue.GetChildAtIndex(i);
 if (!member.IsValid())
   continue;
+if (member.TypeIsPointerType()) {
+  if (member.GetValueDidChange()) {
+vrwbChanged = true;
+return MIstatus::success;
+  }
+  continue;
+}
 
 if (member.GetValueDidChange()) {
   vrwbChanged = true;


Index: tools/lldb-mi/MICmdCmdVar.cpp
===
--- tools/lldb-mi/MICmdCmdVar.cpp
+++ tools/lldb-mi/MICmdCmdVar.cpp
@@ -516,6 +516,13 @@
 lldb::SBValue member = vrwValue.GetChildAtIndex(i);
 if (!member.IsValid())
   continue;
+if (member.TypeIsPointerType()) {
+  if (member.GetValueDidChange()) {
+vrwbChanged = true;
+return MIstatus::success;
+  }
+  continue;
+}
 
 if (member.GetValueDidChange()) {
   vrwbChanged = true;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37154: lldb-mi: -var-update can hang when traversing complex types with pointers

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 113310.
ted added reviewers: clayborg, abidh.
ted removed a subscriber: abidh.
ted added a comment.

Added check for reference types, as Greg suggested.

Simplified change.


https://reviews.llvm.org/D37154

Files:
  tools/lldb-mi/MICmdCmdVar.cpp


Index: tools/lldb-mi/MICmdCmdVar.cpp
===
--- tools/lldb-mi/MICmdCmdVar.cpp
+++ tools/lldb-mi/MICmdCmdVar.cpp
@@ -509,8 +509,6 @@
 return MIstatus::success;
   }
 
-  lldb::SBType valueType = vrwValue.GetType();
-
   const MIuint nChildren = vrwValue.GetNumChildren();
   for (MIuint i = 0; i < nChildren; ++i) {
 lldb::SBValue member = vrwValue.GetChildAtIndex(i);
@@ -520,9 +518,14 @@
 if (member.GetValueDidChange()) {
   vrwbChanged = true;
   return MIstatus::success;
-} else if (ExamineSBValueForChange(member, vrwbChanged) && vrwbChanged)
-  // Handle composite types (i.e. struct or arrays)
-  return MIstatus::success;
+}
+
+// Handle composite types (i.e. struct or arrays)
+// Don't go down into pointers or references, to avoid a loop
+lldb::SBType valueType = member.GetType();
+if (!valueType.IsPointerType() && !valueType.IsReferenceType())
+  if (ExamineSBValueForChange(member, vrwbChanged) && vrwbChanged)
+return MIstatus::success;
   }
   vrwbChanged = false;
   return MIstatus::success;


Index: tools/lldb-mi/MICmdCmdVar.cpp
===
--- tools/lldb-mi/MICmdCmdVar.cpp
+++ tools/lldb-mi/MICmdCmdVar.cpp
@@ -509,8 +509,6 @@
 return MIstatus::success;
   }
 
-  lldb::SBType valueType = vrwValue.GetType();
-
   const MIuint nChildren = vrwValue.GetNumChildren();
   for (MIuint i = 0; i < nChildren; ++i) {
 lldb::SBValue member = vrwValue.GetChildAtIndex(i);
@@ -520,9 +518,14 @@
 if (member.GetValueDidChange()) {
   vrwbChanged = true;
   return MIstatus::success;
-} else if (ExamineSBValueForChange(member, vrwbChanged) && vrwbChanged)
-  // Handle composite types (i.e. struct or arrays)
-  return MIstatus::success;
+}
+
+// Handle composite types (i.e. struct or arrays)
+// Don't go down into pointers or references, to avoid a loop
+lldb::SBType valueType = member.GetType();
+if (!valueType.IsPointerType() && !valueType.IsReferenceType())
+  if (ExamineSBValueForChange(member, vrwbChanged) && vrwbChanged)
+return MIstatus::success;
   }
   vrwbChanged = false;
   return MIstatus::success;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37154: lldb-mi: -var-update can hang when traversing complex types with pointers

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 113317.
ted marked an inline comment as done.
ted added a comment.

Updated with Greg's suggestion.

Removed second call to GetValueDidChange() because it's handled at the top of 
the recursive call. This way we get 1 extra call to ExamineSBValueForChange() 
on a changed child, but avoid 2 calls to GetValueDidChange() for each child.


https://reviews.llvm.org/D37154

Files:
  tools/lldb-mi/MICmdCmdVar.cpp


Index: tools/lldb-mi/MICmdCmdVar.cpp
===
--- tools/lldb-mi/MICmdCmdVar.cpp
+++ tools/lldb-mi/MICmdCmdVar.cpp
@@ -509,19 +509,19 @@
 return MIstatus::success;
   }
 
-  lldb::SBType valueType = vrwValue.GetType();
-
   const MIuint nChildren = vrwValue.GetNumChildren();
   for (MIuint i = 0; i < nChildren; ++i) {
 lldb::SBValue member = vrwValue.GetChildAtIndex(i);
 if (!member.IsValid())
   continue;
 
-if (member.GetValueDidChange()) {
-  vrwbChanged = true;
-  return MIstatus::success;
-} else if (ExamineSBValueForChange(member, vrwbChanged) && vrwbChanged)
-  // Handle composite types (i.e. struct or arrays)
+// skip pointers and references to avoid infinite loop
+if (member.GetType().GetTypeFlags() &
+(lldb::eTypeIsPointer | lldb::eTypeIsReference))
+  continue;
+
+// Handle composite types (i.e. struct or arrays)
+if (ExamineSBValueForChange(member, vrwbChanged) && vrwbChanged)
   return MIstatus::success;
   }
   vrwbChanged = false;


Index: tools/lldb-mi/MICmdCmdVar.cpp
===
--- tools/lldb-mi/MICmdCmdVar.cpp
+++ tools/lldb-mi/MICmdCmdVar.cpp
@@ -509,19 +509,19 @@
 return MIstatus::success;
   }
 
-  lldb::SBType valueType = vrwValue.GetType();
-
   const MIuint nChildren = vrwValue.GetNumChildren();
   for (MIuint i = 0; i < nChildren; ++i) {
 lldb::SBValue member = vrwValue.GetChildAtIndex(i);
 if (!member.IsValid())
   continue;
 
-if (member.GetValueDidChange()) {
-  vrwbChanged = true;
-  return MIstatus::success;
-} else if (ExamineSBValueForChange(member, vrwbChanged) && vrwbChanged)
-  // Handle composite types (i.e. struct or arrays)
+// skip pointers and references to avoid infinite loop
+if (member.GetType().GetTypeFlags() &
+(lldb::eTypeIsPointer | lldb::eTypeIsReference))
+  continue;
+
+// Handle composite types (i.e. struct or arrays)
+if (ExamineSBValueForChange(member, vrwbChanged) && vrwbChanged)
   return MIstatus::success;
   }
   vrwbChanged = false;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37154: lldb-mi: -var-update can hang when traversing complex types with pointers

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

We want to go down into a pointer if it's the original parent, but not into a 
child that's a pointer. To avoid skipping children of a top-level 
pointer/reference, I think we need to do the type check in the loop.


https://reviews.llvm.org/D37154



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

[Lldb-commits] [PATCH] D37533: Fix lldb-mi test data_read_memory_bytes_global

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

Tested with clang 3.8 on top-of-tree lldb-mi on Ubuntu 14.


https://reviews.llvm.org/D37533



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

[Lldb-commits] [PATCH] D37533: Fix lldb-mi test data_read_memory_bytes_global

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.
Herald added a subscriber: aprantl.

Test was skipped because -data-evaluate-expression was thought
to not work on globals. This is not the case - the issue was clang
removes debug info for globals in cpp files that are not used.

Add a reference to the globals in question, and fix memory patter in
test to match memory pattern in testcase.


https://reviews.llvm.org/D37533

Files:
  packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
  packages/Python/lldbsuite/test/tools/lldb-mi/data/main.cpp


Index: packages/Python/lldbsuite/test/tools/lldb-mi/data/main.cpp
===
--- packages/Python/lldbsuite/test/tools/lldb-mi/data/main.cpp
+++ packages/Python/lldbsuite/test/tools/lldb-mi/data/main.cpp
@@ -17,6 +17,8 @@
 {
 char array[] = { 0x01, 0x02, 0x03, 0x04 };
 char *first_element_ptr = &array[0];
+char g = g_CharArray[0];
+char s = s_CharArray[0];
 // BP_local_array_test_inner
 return;
 }
Index: packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
===
--- packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
+++ packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
@@ -88,8 +88,6 @@
 
 @skipIfWindows  # llvm.org/pr24452: Get lldb-mi tests working on Windows
 @skipIfFreeBSD  # llvm.org/pr22411: Failure presumably due to known thread 
races
-# FIXME: the global case worked before refactoring
-@unittest2.skip("-data-evaluate-expression doesn't work on globals")
 def test_lldbmi_data_read_memory_bytes_global(self):
 """Test that -data-read-memory-bytes can access global buffers."""
 
@@ -115,7 +113,7 @@
 # Test that -data-read-memory-bytes works for char[] type (global)
 self.runCmd("-data-read-memory-bytes %#x %d" % (addr, size))
 self.expect(
-
"\^done,memory=\[{begin=\"0x0*%x\",offset=\"0x0+\",end=\"0x0*%x\",contents=\"1112131400\"}\]"
 %
+
"\^done,memory=\[{begin=\"0x0*%x\",offset=\"0x0+\",end=\"0x0*%x\",contents=\"1011121300\"}\]"
 %
 (addr, addr + size))
 
 # Get address of static char[]
@@ -127,7 +125,7 @@
 # Test that -data-read-memory-bytes works for static char[] type
 self.runCmd("-data-read-memory-bytes %#x %d" % (addr, size))
 self.expect(
-
"\^done,memory=\[{begin=\"0x0*%x\",offset=\"0x0+\",end=\"0x0*%x\",contents=\"1112131400\"}\]"
 %
+
"\^done,memory=\[{begin=\"0x0*%x\",offset=\"0x0+\",end=\"0x0*%x\",contents=\"2021222300\"}\]"
 %
 (addr, addr + size))
 
 @skipIfWindows  # llvm.org/pr24452: Get lldb-mi tests working on Windows


Index: packages/Python/lldbsuite/test/tools/lldb-mi/data/main.cpp
===
--- packages/Python/lldbsuite/test/tools/lldb-mi/data/main.cpp
+++ packages/Python/lldbsuite/test/tools/lldb-mi/data/main.cpp
@@ -17,6 +17,8 @@
 {
 char array[] = { 0x01, 0x02, 0x03, 0x04 };
 char *first_element_ptr = &array[0];
+char g = g_CharArray[0];
+char s = s_CharArray[0];
 // BP_local_array_test_inner
 return;
 }
Index: packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
===
--- packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
+++ packages/Python/lldbsuite/test/tools/lldb-mi/data/TestMiData.py
@@ -88,8 +88,6 @@
 
 @skipIfWindows  # llvm.org/pr24452: Get lldb-mi tests working on Windows
 @skipIfFreeBSD  # llvm.org/pr22411: Failure presumably due to known thread races
-# FIXME: the global case worked before refactoring
-@unittest2.skip("-data-evaluate-expression doesn't work on globals")
 def test_lldbmi_data_read_memory_bytes_global(self):
 """Test that -data-read-memory-bytes can access global buffers."""
 
@@ -115,7 +113,7 @@
 # Test that -data-read-memory-bytes works for char[] type (global)
 self.runCmd("-data-read-memory-bytes %#x %d" % (addr, size))
 self.expect(
-"\^done,memory=\[{begin=\"0x0*%x\",offset=\"0x0+\",end=\"0x0*%x\",contents=\"1112131400\"}\]" %
+"\^done,memory=\[{begin=\"0x0*%x\",offset=\"0x0+\",end=\"0x0*%x\",contents=\"1011121300\"}\]" %
 (addr, addr + size))
 
 # Get address of static char[]
@@ -127,7 +125,7 @@
 # Test that -data-read-memory-bytes works for static char[] type
 self.runCmd("-data-read-memory-bytes %#x %d" % (addr, size))
 self.expect(
-"\^done,memory=\[{begin=\"0x0*%x\",offset=\"0x0+\",end=\"0x0*%x\",contents=\"1112131400\"}\]" %
+"\^done,memory=\[{begin=\"0x0*%x\",offset=\"0x0+\",end=\"0x0*%x\",contents=\"2021222300\"}\]" %
 (addr, addr + size))
 
 @skipIfWindows  # llvm.org/pr24452: Get lldb-mi tests working on Windows

[Lldb-commits] [PATCH] D39215: Default to using in-tree clang for building test executables

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

We build lldb, clang and tools for Hexagon only, and call them hexagon-lldb, 
hexagon-clang, etc. The test infrastructure is smart enough to pick up 
hexagon-lldb-mi if we tell it to run with hexagon-lldb using --executable; will 
it be smart enough to run an in-tree hexagon-clang?

@labath, we run on Windows using hexagon-clang and hexagon-clang++; don't 
forget the embedded cases when choosing compilers and running tests.

I'm all for removing redundant variables.


https://reviews.llvm.org/D39215



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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

FYI, @jingham , this is also an issue with DYLD plugins, if the file in the 
link map starts with ".".


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D55332: [CMake] Python bindings generation polishing

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.
Herald added a project: LLDB.

@sgraenitz I've found an issue with this patch, using the Visual Studio 2015 
generator.

In scripts/CMakeLists.txt the old code (for Windows):

  if (CMAKE_CONFIGURATION_TYPES)
set(SWIG_PYTHON_DIR 
${CMAKE_BINARY_DIR}/\${CMAKE_INSTALL_CONFIG_NAME}/lib${LLVM_LIBDIR_SUFFIX}/site-packages)
  else()
set(SWIG_PYTHON_DIR 
${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX}/site-packages)
  endif()

Was changed to (for everything):

  set(SWIG_PYTHON_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/${swig_python_subdir})

Which changes /tools/lldb/scripts/cmake_install.cmake from

  if("${CMAKE_INSTALL_COMPONENT}" STREQUAL "Unspecified" OR NOT 
CMAKE_INSTALL_COMPONENT)
file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/lib" TYPE DIRECTORY FILES 
"i:/obj/${CMAKE_INSTALL_CONFIG_NAME}/lib/site-packages")
  endif()

to

  if("${CMAKE_INSTALL_COMPONENT}" STREQUAL "Unspecified" OR NOT 
CMAKE_INSTALL_COMPONENT)
file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/lib" TYPE DIRECTORY FILES 
"I:/obj/$(Configuration)/lib/site-packages")
  endif()

"$(Configuration)" is a Visual Studio variable, and not available to cmake, so 
our bots error out with this error:

  CMake Error at tools/lldb/scripts/cmake_install.cmake:31 (file):
file INSTALL cannot find "I:/obj/$(Configuration)/lib/site-packages".
  Call Stack (most recent call first):
tools/lldb/cmake_install.cmake:41 (include)
tools/cmake_install.cmake:41 (include)
cmake_install.cmake:45 (include)


Verified with the latest CMake (3.13.3).

Will switching back to the old code for Windows cause any issues?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D55332



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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D57552#1381046 , @jingham wrote:

> In D57552#1380782 , @ted wrote:
>
> > FYI, @jingham , this is also an issue with DYLD plugins, if the file in the 
> > link map starts with ".". "image search-paths add . /path/to/my/libraries" 
> > fails in this case, because "./library.so" gets changed to "library.so".
>
>
> This should fix that issue as well, since image search-paths are also 
> PathMappingLists.  I don't see any tests for this, however.  Do you have 
> something handy you could turn into a test?


I don't think this will fix it, because the problem happens in the initial 
FileSpec, before we get to search-paths.

From source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp, 
DYLDRendezvous::ReadSOEntryFromMemory():

  std::string file_path = ReadStringFromMemory(entry.path_addr);
  entry.file_spec.SetFile(file_path, false, FileSpec::Style::native);

FileSpec::SetFile normalizes the path, which will remove a leading "./", so 
"image search-paths add . /path/to/my/lib", which would work with "./mylib.so", 
won't work with "mylib.so".

I think I can write a test that uses LD_LIBRARY_PATH and a subdirectory so the 
target can load "./mylib.so", but LLDB won't be able to see it without a 
search-path.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D57552#1381126 , @jingham wrote:

> That sounds like it will have to be fixed somewhere else, and not part of 
> this patch then.  Can you file a bug to get that fixed?


Yeah, it's on my todo list. Which keeps growing!


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

https://reviews.llvm.org/D57552



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

[Lldb-commits] [PATCH] D59719: [ScriptInterpreter] Make sure that PYTHONHOME is right.

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

This doesn't look correct to me - it looks like there are 1 too many #endifs. I 
think the one at line 179 should be removed - it should have been replaced by 
the #else that is at line 180.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59719



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

[Lldb-commits] [PATCH] D60152: Fix and simplify PrepareCommandsForSourcing

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

llvm/tools/lldb/tools/driver/Driver.cpp:495:12: error: comparison of integers 
of different signs: 'ssize_t' (aka 'long') and 'size_t' (aka 'unsigned long') 
[-Werror,-Wsign-compare]

  if (nrwr != commands_size) {
   ^  ~

1 error generated.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60152



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

[Lldb-commits] [PATCH] D27380: [lldb] Update the check for Linux or FreeBSD in SymbolFileDWARF::FindFunctions.

[Ted Woodward via Phabricator via lldb-commits]

ted added inline comments.



Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2590
+(arch.GetTriple().isOSBinFormatELF() ||
  arch.GetMachine() == llvm::Triple::hexagon)) {
   SymbolContextList temp_sc_list;

clayborg wrote:
> labath wrote:
> > I think we can remove the hexagon part as well. Ted, I presume hexagon uses 
> > ELF files?
> I believe Hexagon uses ELF so it would be ok to remove the Hexagon from the 
> if.
Yes, Hexagon uses ELF, so this change should be fine.


Repository:
  rL LLVM

https://reviews.llvm.org/D27380



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

[Lldb-commits] [PATCH] D112931: Fix mixed disassembly showing source lines for "line 0"

[Ted Woodward via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGded1bad64af0: Fix mixed disassembly showing source lines for 
"line 0" (authored by ted).
Herald added a project: All.

Changed prior to commit:
  https://reviews.llvm.org/D112931?vs=383810&id=539606#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112931

Files:
  lldb/test/Shell/Commands/command-disassemble-mixed.c


Index: lldb/test/Shell/Commands/command-disassemble-mixed.c
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-disassemble-mixed.c
@@ -0,0 +1,18 @@
+// invalid mixed disassembly line
+
+// RUN: %clang -g %s -o %t
+// RUN: %lldb %t -o "dis -m -n main" -o "exit" | FileCheck %s
+
+// CHECK: int main
+// CHECK: int i
+// CHECK-NOT: invalid mixed disassembly line
+// CHECK: return 0;
+
+int main(int argc, char **argv)
+{
+  int i;
+
+  for (i=0; i < 10; ++i) ;
+
+  return 0;
+}


Index: lldb/test/Shell/Commands/command-disassemble-mixed.c
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-disassemble-mixed.c
@@ -0,0 +1,18 @@
+// invalid mixed disassembly line
+
+// RUN: %clang -g %s -o %t
+// RUN: %lldb %t -o "dis -m -n main" -o "exit" | FileCheck %s
+
+// CHECK: int main
+// CHECK: int i
+// CHECK-NOT: invalid mixed disassembly line
+// CHECK: return 0;
+
+int main(int argc, char **argv)
+{
+  int i;
+
+  for (i=0; i < 10; ++i) ;
+
+  return 0;
+}
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D155107: Add support for llvm::MCInstPrinter::setPrintBranchImmAsAddress

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.
ted added reviewers: clayborg, jingham.
Herald added a project: All.
ted requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

llvm::MCInstPrinter has an option, controlled by setPrintBranchImmAsAddress,
to print branch targets as immediate addresses instead of offsets.
Turn this on in lldb, so targets that support this flag will print addresses
instead of offsets.

This requires the address of the instruction be provided, but fortunately
it's calculated right before the call to PrintMCInst.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155107

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp


Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -59,8 +59,8 @@
 
   uint64_t GetMCInst(const uint8_t *opcode_data, size_t opcode_data_len,
  lldb::addr_t pc, llvm::MCInst &mc_inst) const;
-  void PrintMCInst(llvm::MCInst &mc_inst, std::string &inst_string,
-   std::string &comments_string);
+  void PrintMCInst(llvm::MCInst &mc_inst, lldb::addr_t pc,
+   std::string &inst_string, std::string &comments_string);
   void SetStyle(bool use_hex_immed, HexImmediateStyle hex_style);
   bool CanBranch(llvm::MCInst &mc_inst) const;
   bool HasDelaySlot(llvm::MCInst &mc_inst) const;
@@ -604,7 +604,7 @@
 
 if (inst_size > 0) {
   mc_disasm_ptr->SetStyle(use_hex_immediates, hex_style);
-  mc_disasm_ptr->PrintMCInst(inst, out_string, comment_string);
+  mc_disasm_ptr->PrintMCInst(inst, pc, out_string, comment_string);
 
   if (!comment_string.empty()) {
 AppendComment(comment_string);
@@ -1287,6 +1287,8 @@
   if (!instr_printer_up)
 return Instance();
 
+  instr_printer_up->setPrintBranchImmAsAddress(true);
+
   return Instance(
   new MCDisasmInstance(std::move(instr_info_up), std::move(reg_info_up),
std::move(subtarget_info_up), 
std::move(asm_info_up),
@@ -1328,13 +1330,13 @@
 }
 
 void DisassemblerLLVMC::MCDisasmInstance::PrintMCInst(
-llvm::MCInst &mc_inst, std::string &inst_string,
+llvm::MCInst &mc_inst, lldb::addr_t pc, std::string &inst_string,
 std::string &comments_string) {
   llvm::raw_string_ostream inst_stream(inst_string);
   llvm::raw_string_ostream comments_stream(comments_string);
 
   m_instr_printer_up->setCommentStream(comments_stream);
-  m_instr_printer_up->printInst(&mc_inst, 0, llvm::StringRef(),
+  m_instr_printer_up->printInst(&mc_inst, pc, llvm::StringRef(),
 *m_subtarget_info_up, inst_stream);
   m_instr_printer_up->setCommentStream(llvm::nulls());
   comments_stream.flush();


Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -59,8 +59,8 @@
 
   uint64_t GetMCInst(const uint8_t *opcode_data, size_t opcode_data_len,
  lldb::addr_t pc, llvm::MCInst &mc_inst) const;
-  void PrintMCInst(llvm::MCInst &mc_inst, std::string &inst_string,
-   std::string &comments_string);
+  void PrintMCInst(llvm::MCInst &mc_inst, lldb::addr_t pc,
+   std::string &inst_string, std::string &comments_string);
   void SetStyle(bool use_hex_immed, HexImmediateStyle hex_style);
   bool CanBranch(llvm::MCInst &mc_inst) const;
   bool HasDelaySlot(llvm::MCInst &mc_inst) const;
@@ -604,7 +604,7 @@
 
 if (inst_size > 0) {
   mc_disasm_ptr->SetStyle(use_hex_immediates, hex_style);
-  mc_disasm_ptr->PrintMCInst(inst, out_string, comment_string);
+  mc_disasm_ptr->PrintMCInst(inst, pc, out_string, comment_string);
 
   if (!comment_string.empty()) {
 AppendComment(comment_string);
@@ -1287,6 +1287,8 @@
   if (!instr_printer_up)
 return Instance();
 
+  instr_printer_up->setPrintBranchImmAsAddress(true);
+
   return Instance(
   new MCDisasmInstance(std::move(instr_info_up), std::move(reg_info_up),
std::move(subtarget_info_up), std::move(asm_info_up),
@@ -1328,13 +1330,13 @@
 }
 
 void DisassemblerLLVMC::MCDisasmInstance::PrintMCInst(
-llvm::MCInst &mc_inst, std::string &inst_string,
+llvm::MCInst &mc_inst, lldb::addr_t pc, std::string &inst_string,
 std::string &comments_string) {
   llvm::raw_string_ostream inst_stream(inst_string);
   llvm::raw_string_ostream comments_stream(comments_string);
 
   m_instr_printer_up->setCommentStream(comments_stream);
-  m_instr_printer_up->printInst(&mc_inst, 0, llvm::StringRef(),
+  m_instr_printer_up->printInst(&mc_

[Lldb-commits] [PATCH] D155117: Platform qemu-user: Build path to qemu automatically if not specified

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.
Herald added a project: All.
ted requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Get the path to qemu in the following order:

1. From the property platform.plugin.qemu-user.emulator-path
2. If that property is not set, from PATH, building the name of the qemu binary 
from the triple in  property platform.plugin.qemu-user.architecture
3. If that property is not set, from PATH, building the name of the qemu binary 
from the triple in the target

This will allow a user to load a target and run without setting properties,
if qemu is on the PATH and named qemu-


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155117

Files:
  lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp


Index: lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
===
--- lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
+++ lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
@@ -162,9 +162,18 @@
Target &target, Status &error) {
   Log *log = GetLog(LLDBLog::Platform);
 
+  // If platform.plugin.qemu-user.emulator-path is set, use it.
   FileSpec qemu = GetGlobalProperties().GetEmulatorPath();
-  if (!qemu)
-qemu.SetPath(("qemu-" + GetGlobalProperties().GetArchitecture()).str());
+  // If platform.plugin.qemu-user.emulator-path is not set, build the
+  // executable name from platform.plugin.qemu-user.architecture.
+  if (!qemu) {
+llvm::StringRef arch = GetGlobalProperties().GetArchitecture();
+// If platform.plugin.qemu-user.architecture is not set, build the
+// executable name from the target Triple's ArchName
+if (arch.empty())
+  arch = target.GetArchitecture().GetTriple().getArchName();
+qemu.SetPath(("qemu-" + arch).str());
+  }
   FileSystem::Instance().ResolveExecutableLocation(qemu);
 
   llvm::SmallString<0> socket_model, socket_path;


Index: lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
===
--- lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
+++ lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
@@ -162,9 +162,18 @@
Target &target, Status &error) {
   Log *log = GetLog(LLDBLog::Platform);
 
+  // If platform.plugin.qemu-user.emulator-path is set, use it.
   FileSpec qemu = GetGlobalProperties().GetEmulatorPath();
-  if (!qemu)
-qemu.SetPath(("qemu-" + GetGlobalProperties().GetArchitecture()).str());
+  // If platform.plugin.qemu-user.emulator-path is not set, build the
+  // executable name from platform.plugin.qemu-user.architecture.
+  if (!qemu) {
+llvm::StringRef arch = GetGlobalProperties().GetArchitecture();
+// If platform.plugin.qemu-user.architecture is not set, build the
+// executable name from the target Triple's ArchName
+if (arch.empty())
+  arch = target.GetArchitecture().GetTriple().getArchName();
+qemu.SetPath(("qemu-" + arch).str());
+  }
   FileSystem::Instance().ResolveExecutableLocation(qemu);
 
   llvm::SmallString<0> socket_model, socket_path;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D155117: Platform qemu-user: Build path to qemu automatically if not specified

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D155117#4505538 , @labath wrote:

> I am wondering if we actually need the second step (the architecture setting) 
> here. The main reason it exists is the usage in `GetSupportedArchitectures` 
> (which is called before a target is created) it seems like the value derived 
> from the target should always be more correct. WDYT? What are the values you 
> get in steps 2 and 3 for your use case?

I don't think we need to specify the architecture, because I think we can 
always get it from the triple. There might be a case where someone is using a 
qemu that isn't named the same as the Triple ArchName, but that case could be 
covered by emulator-path.

In my case, I'm loading a target, then running, letting the platform get 
ArchName from the target Triple, then turning that into qemu-riscv32 (or 
riscv64), and launching that from my PATH.

An example (with my experimental RISC-V ABI plugin):

> bin/lldb

(lldb) platform select qemu-user

  Platform: qemu-user
Triple: x86_64-*-linux-gnu

OS Version: 5.4.0 (5.4.0-136-generic)

  Hostname: 127.0.0.1

WorkingDir: /local/mnt/ted/8.8/riscv

  Kernel: #153~18.04.1-Ubuntu SMP Wed Nov 30 15:47:57 UTC 2022

(lldb) file ~/lldb_test/factrv32
Current executable set to '/usr2/tedwood/lldb_test/factrv32' (riscv32).
(lldb) b main
Breakpoint 1: where = factrv32`main + 28 at factorial.c:32:8, address = 
0x000104ea
(lldb) r
Process 1 launched: '/usr2/tedwood/lldb_test/factrv32' (riscv32)
Process 1 stopped

- thread #1, stop reason = breakpoint 1.1 frame #0: 0x000104ea 
factrv32`main(argc=1, argv=0x408005a4) at factorial.c:32:8 29  } 30 
 */ 31

-> 32 base = 10;

  33
  34  printf("Factorial of %d is %d\n", base, factorial(base));
  35  return 0;

(lldb) target list
Current targets:

- target #0: /usr2/tedwood/lldb_test/factrv32 ( arch=riscv32-*-linux, 
platform=qemu-user, pid=1, state=stopped )

(lldb)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155117

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

[Lldb-commits] [PATCH] D155107: Add support for llvm::MCInstPrinter::setPrintBranchImmAsAddress

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D155107#4504667 , @jasonmolenda 
wrote:

> Isn't it better to print branches within a function as an offset, given that 
> our disassembly format by default lists the offset of each instruction.  So 
> instead of looking for a 6-digit long hex address, you're looking for a 
> decimal offset in the output?  I'm not sure if you disagree with my opinion, 
> or if you have an example where it is clearer to have an address that I'm not 
> seeing.

llvm-objdump calls setPrintImmAsAddress(true). I think we should do the same. I 
also think that it looks better. @clayborg here is some riscv32 disassembly 
with and without the change:

with:

  (lldb) dis -n factorial
  factrv32`factorial:
  0x1047c <+0>:  addi   sp, sp, -32
  0x1047e <+2>:  sw ra, 28(sp)
  0x10480 <+4>:  sw s0, 24(sp)
  0x10482 <+6>:  addi   s0, sp, 32
  0x10484 <+8>:  sw a0, -16(s0)
  0x10488 <+12>: lw a0, -16(s0)
  0x1048c <+16>: li a1, 1
  0x1048e <+18>: beqa0, a1, 0x10496
  0x10492 <+22>: j  0x104a4
  0x10496 <+26>: j  0x1049a
  0x1049a <+30>: li a0, 1
  0x1049c <+32>: sw a0, -12(s0)
  0x104a0 <+36>: j  0x104c2
  0x104a4 <+40>: lw a0, -16(s0)
  0x104a8 <+44>: sw a0, -20(s0)
  0x104ac <+48>: addi   a0, a0, -1
  0x104ae <+50>: jal0x1047c
  0x104b0 <+52>: mv a1, a0
  0x104b2 <+54>: lw a0, -20(s0)
  0x104b6 <+58>: mula0, a0, a1
  0x104ba <+62>: sw a0, -12(s0)
  0x104be <+66>: j  0x104c2
  0x104c2 <+70>: lw a0, -12(s0)
  0x104c6 <+74>: lw ra, 28(sp)
  0x104c8 <+76>: lw s0, 24(sp)
  0x104ca <+78>: addi   sp, sp, 32
  0x104cc <+80>: ret

without:

  factrv32`factorial:
  0x1047c <+0>:  addi   sp, sp, -32
  0x1047e <+2>:  sw ra, 28(sp)
  0x10480 <+4>:  sw s0, 24(sp)
  0x10482 <+6>:  addi   s0, sp, 32
  0x10484 <+8>:  sw a0, -16(s0)
  0x10488 <+12>: lw a0, -16(s0)
  0x1048c <+16>: li a1, 1
  0x1048e <+18>: beqa0, a1, 8
  0x10492 <+22>: j  18
  0x10496 <+26>: j  4
  0x1049a <+30>: li a0, 1
  0x1049c <+32>: sw a0, -12(s0)
  0x104a0 <+36>: j  34
  0x104a4 <+40>: lw a0, -16(s0)
  0x104a8 <+44>: sw a0, -20(s0)
  0x104ac <+48>: addi   a0, a0, -1
  0x104ae <+50>: jal-50
  0x104b0 <+52>: mv a1, a0
  0x104b2 <+54>: lw a0, -20(s0)
  0x104b6 <+58>: mula0, a0, a1
  0x104ba <+62>: sw a0, -12(s0)
  0x104be <+66>: j  4
  0x104c2 <+70>: lw a0, -12(s0)
  0x104c6 <+74>: lw ra, 28(sp)
  0x104c8 <+76>: lw s0, 24(sp)
  0x104ca <+78>: addi   sp, sp, 32
  0x104cc <+80>: ret

Addresses 0x10492 and 0x10496 are jumps. It's much easier, IMO, to see what the 
targets are with the change than without.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155107

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

[Lldb-commits] [PATCH] D155117: Platform qemu-user: Build path to qemu automatically if not specified

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

As for the GetSupportedArchitectures case, downstream I left in the code that 
checks the property, but changed the return {}; to

  return {ArchSpec(llvm::Triple("riscv32-unknown-linux")),
  ArchSpec(llvm::Triple("riscv64-unknown-linux"))};

I don't think we want that upstream, but downstream the only time we're 
targetting riscv linux is when using qemu.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155117

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

[Lldb-commits] [PATCH] D155107: Add support for llvm::MCInstPrinter::setPrintBranchImmAsAddress

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D155107#4511967 , @clayborg wrote:

> Looks like other disassemblers already show full addresses for the branches 
> and calls (at least arm64 does from my output above), so not sure why RISCV 
> would require this setting, but x86_64 and arm64 wouldn't because it is 
> already working that way??

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp has this code, 
specifically using PrintBranchImmAsAddress to gate this behavior:

  void RISCVInstPrinter::printBranchOperand(const MCInst *MI, uint64_t Address,
unsigned OpNo,
const MCSubtargetInfo &STI,
raw_ostream &O) {
const MCOperand &MO = MI->getOperand(OpNo);
if (!MO.isImm())
  return printOperand(MI, OpNo, STI, O);
  
if (PrintBranchImmAsAddress) {
  uint64_t Target = Address + MO.getImm();
  if (!STI.hasFeature(RISCV::Feature64Bit))
Target &= 0x;
  O << formatHex(Target);
} else {
  O << MO.getImm();
}
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155107

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

[Lldb-commits] [PATCH] D155107: Add support for llvm::MCInstPrinter::setPrintBranchImmAsAddress

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D155107#4534020 , @clayborg wrote:

> So Ted, can you verify that nothing is missing in RISCV since this is already 
> the way it works for x86/x86_64 and arm32/arm64? I am thinking like Jason is 
> where I wonder if something isn't just hooked up right for RISCV somehow. Or 
> if you can verify that nothing changes for x86/x86_64 or arm32/arm64 if this 
> setting is enabled and there are no symbolication regressions in the comments 
> if this does get enabled for the architectures where it is working as 
> expected...

I can verify that this change doesn't break RISC-V disassembly. I can't verify 
that nothing is missing. In fact, our current disassembler doesn't recognize 
most change-of-flow instructions, because the RISC-V disassembler doesn't 
populate the MCInstrDesc for them correctly. There's a diff D156086 
 to handle that, though (use MCInstrAnalysis 
if it's there).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155107

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

[Lldb-commits] [PATCH] D155117: Platform qemu-user: Build path to qemu automatically if not specified

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D155117#4521393 , @labath wrote:

> In D155117#4510512 , @ted wrote:
>
>> In D155117#4505538 , @labath wrote:
>>
>>> I am wondering if we actually need the second step (the architecture 
>>> setting) here. The main reason it exists is the usage in 
>>> `GetSupportedArchitectures` (which is called before a target is created) it 
>>> seems like the value derived from the target should always be more correct. 
>>> WDYT? What are the values you get in steps 2 and 3 for your use case?
>>
>> I don't think we need to specify the architecture, because I think we can 
>> always get it from the triple. There might be a case where someone is using 
>> a qemu that isn't named the same as the Triple ArchName, but that case could 
>> be covered by emulator-path.
>
> Yeah, that's my thinking as well. Given this, I think we should drop the 
> setting part and just go with the target value directly.

Do you want me to remove the setting entirely?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155117

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

[Lldb-commits] [PATCH] D155107: Add support for llvm::MCInstPrinter::setPrintBranchImmAsAddress

[Ted Woodward via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG523110d654a2: Add support for 
llvm::MCInstPrinter::setPrintBranchImmAsAddress (authored by ted).

Changed prior to commit:
  https://reviews.llvm.org/D155107?vs=539679&id=552465#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155107

Files:
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp


Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -60,8 +60,8 @@
 
   uint64_t GetMCInst(const uint8_t *opcode_data, size_t opcode_data_len,
  lldb::addr_t pc, llvm::MCInst &mc_inst) const;
-  void PrintMCInst(llvm::MCInst &mc_inst, std::string &inst_string,
-   std::string &comments_string);
+  void PrintMCInst(llvm::MCInst &mc_inst, lldb::addr_t pc,
+   std::string &inst_string, std::string &comments_string);
   void SetStyle(bool use_hex_immed, HexImmediateStyle hex_style);
   bool CanBranch(llvm::MCInst &mc_inst) const;
   bool HasDelaySlot(llvm::MCInst &mc_inst) const;
@@ -607,7 +607,7 @@
 
 if (inst_size > 0) {
   mc_disasm_ptr->SetStyle(use_hex_immediates, hex_style);
-  mc_disasm_ptr->PrintMCInst(inst, out_string, comment_string);
+  mc_disasm_ptr->PrintMCInst(inst, pc, out_string, comment_string);
 
   if (!comment_string.empty()) {
 AppendComment(comment_string);
@@ -1290,6 +1290,8 @@
   if (!instr_printer_up)
 return Instance();
 
+  instr_printer_up->setPrintBranchImmAsAddress(true);
+
   // Not all targets may have registered createMCInstrAnalysis().
   std::unique_ptr instr_analysis_up(
   curr_target->createMCInstrAnalysis(instr_info_up.get()));
@@ -1337,13 +1339,13 @@
 }
 
 void DisassemblerLLVMC::MCDisasmInstance::PrintMCInst(
-llvm::MCInst &mc_inst, std::string &inst_string,
+llvm::MCInst &mc_inst, lldb::addr_t pc, std::string &inst_string,
 std::string &comments_string) {
   llvm::raw_string_ostream inst_stream(inst_string);
   llvm::raw_string_ostream comments_stream(comments_string);
 
   m_instr_printer_up->setCommentStream(comments_stream);
-  m_instr_printer_up->printInst(&mc_inst, 0, llvm::StringRef(),
+  m_instr_printer_up->printInst(&mc_inst, pc, llvm::StringRef(),
 *m_subtarget_info_up, inst_stream);
   m_instr_printer_up->setCommentStream(llvm::nulls());
   comments_stream.flush();


Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -60,8 +60,8 @@
 
   uint64_t GetMCInst(const uint8_t *opcode_data, size_t opcode_data_len,
  lldb::addr_t pc, llvm::MCInst &mc_inst) const;
-  void PrintMCInst(llvm::MCInst &mc_inst, std::string &inst_string,
-   std::string &comments_string);
+  void PrintMCInst(llvm::MCInst &mc_inst, lldb::addr_t pc,
+   std::string &inst_string, std::string &comments_string);
   void SetStyle(bool use_hex_immed, HexImmediateStyle hex_style);
   bool CanBranch(llvm::MCInst &mc_inst) const;
   bool HasDelaySlot(llvm::MCInst &mc_inst) const;
@@ -607,7 +607,7 @@
 
 if (inst_size > 0) {
   mc_disasm_ptr->SetStyle(use_hex_immediates, hex_style);
-  mc_disasm_ptr->PrintMCInst(inst, out_string, comment_string);
+  mc_disasm_ptr->PrintMCInst(inst, pc, out_string, comment_string);
 
   if (!comment_string.empty()) {
 AppendComment(comment_string);
@@ -1290,6 +1290,8 @@
   if (!instr_printer_up)
 return Instance();
 
+  instr_printer_up->setPrintBranchImmAsAddress(true);
+
   // Not all targets may have registered createMCInstrAnalysis().
   std::unique_ptr instr_analysis_up(
   curr_target->createMCInstrAnalysis(instr_info_up.get()));
@@ -1337,13 +1339,13 @@
 }
 
 void DisassemblerLLVMC::MCDisasmInstance::PrintMCInst(
-llvm::MCInst &mc_inst, std::string &inst_string,
+llvm::MCInst &mc_inst, lldb::addr_t pc, std::string &inst_string,
 std::string &comments_string) {
   llvm::raw_string_ostream inst_stream(inst_string);
   llvm::raw_string_ostream comments_stream(comments_string);
 
   m_instr_printer_up->setCommentStream(comments_stream);
-  m_instr_printer_up->printInst(&mc_inst, 0, llvm::StringRef(),
+  m_instr_printer_up->printInst(&mc_inst, pc, llvm::StringRef(),
 *m_subtarget_info_up, inst_stream);
   m_instr_printer_up->setCommentStream(llvm::nulls());
   comments_stream.flush();
___
lldb-commits mailing list

[Lldb-commits] [PATCH] D155117: Platform qemu-user: Build path to qemu automatically if not specified

[Ted Woodward via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbe88462cd6aa: Platform qemu-user: Build path to qemu 
automatically if not specified (authored by ted).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155117

Files:
  lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp


Index: lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
===
--- lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
+++ lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
@@ -162,9 +162,18 @@
Target &target, Status &error) {
   Log *log = GetLog(LLDBLog::Platform);
 
+  // If platform.plugin.qemu-user.emulator-path is set, use it.
   FileSpec qemu = GetGlobalProperties().GetEmulatorPath();
-  if (!qemu)
-qemu.SetPath(("qemu-" + GetGlobalProperties().GetArchitecture()).str());
+  // If platform.plugin.qemu-user.emulator-path is not set, build the
+  // executable name from platform.plugin.qemu-user.architecture.
+  if (!qemu) {
+llvm::StringRef arch = GetGlobalProperties().GetArchitecture();
+// If platform.plugin.qemu-user.architecture is not set, build the
+// executable name from the target Triple's ArchName
+if (arch.empty())
+  arch = target.GetArchitecture().GetTriple().getArchName();
+qemu.SetPath(("qemu-" + arch).str());
+  }
   FileSystem::Instance().ResolveExecutableLocation(qemu);
 
   llvm::SmallString<0> socket_model, socket_path;


Index: lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
===
--- lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
+++ lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp
@@ -162,9 +162,18 @@
Target &target, Status &error) {
   Log *log = GetLog(LLDBLog::Platform);
 
+  // If platform.plugin.qemu-user.emulator-path is set, use it.
   FileSpec qemu = GetGlobalProperties().GetEmulatorPath();
-  if (!qemu)
-qemu.SetPath(("qemu-" + GetGlobalProperties().GetArchitecture()).str());
+  // If platform.plugin.qemu-user.emulator-path is not set, build the
+  // executable name from platform.plugin.qemu-user.architecture.
+  if (!qemu) {
+llvm::StringRef arch = GetGlobalProperties().GetArchitecture();
+// If platform.plugin.qemu-user.architecture is not set, build the
+// executable name from the target Triple's ArchName
+if (arch.empty())
+  arch = target.GetArchitecture().GetTriple().getArchName();
+qemu.SetPath(("qemu-" + arch).str());
+  }
   FileSystem::Instance().ResolveExecutableLocation(qemu);
 
   llvm::SmallString<0> socket_model, socket_path;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.
Herald added subscribers: luke, sunshaoce, VincentWu, vkmr, frasercrmck, 
luismarques, apazos, sameer.abuasal, s.egerton, Jim, jocewei, PkmX, the_o, 
brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, shiva0217, 
kito-cheng, niosHD, sabuasal, simoncook, johnrusso, rbar, asb, arichardson.
Herald added a project: All.
ted requested review of this revision.
Herald added subscribers: lldb-commits, wangpc, MaskRay.
Herald added a project: LLDB.

Also default to disassembling a and m features
Some code taken from https://reviews.llvm.org/D62732 , which hasn't been
updated in a year.

Tested with 32 and 64 bit Linux user space QEMU


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159101

Files:
  lldb/source/Plugins/ABI/CMakeLists.txt
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
  lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1550,6 +1550,8 @@
 ArchSpec::eRISCV_float_abi_quad)
   features_str += "+f,+d,+q,";
 // FIXME: how do we detect features such as `+a`, `+m`?
+// Turn them on by default now, since everyone seems to use them
+features_str += "+a,+m,";
   }
 
   // We use m_disasm_up.get() to tell whether we are valid or not, so if this
Index: lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
@@ -0,0 +1,12 @@
+add_lldb_library(lldbPluginABIRISCV PLUGIN
+  ABISysV_riscv.cpp
+
+  LINK_LIBS
+lldbCore
+lldbSymbol
+lldbTarget
+lldbPluginProcessUtility
+  LINK_COMPONENTS
+Support
+TargetParser
+  )
Index: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -0,0 +1,127 @@
+//===-- ABISysV_riscv.h ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef liblldb_ABISysV_riscv_h_
+#define liblldb_ABISysV_riscv_h_
+
+// Other libraries and framework includes
+#include 
+
+#include "llvm/TargetParser/Triple.h"
+
+// Project includes
+#include "lldb/Target/ABI.h"
+#include "lldb/Target/Process.h"
+#include "lldb/lldb-private.h"
+
+class ABISysV_riscv : public lldb_private::RegInfoBasedABI {
+public:
+  ~ABISysV_riscv() override = default;
+
+  size_t GetRedZoneSize() const override { return 0; }
+
+  bool PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+  lldb::addr_t functionAddress,
+  lldb::addr_t returnAddress,
+  llvm::ArrayRef args) const override;
+
+  // Special thread plan for GDB style non-jit function calls.
+  bool
+  PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+ lldb::addr_t functionAddress, lldb::addr_t returnAddress,
+ llvm::Type &prototype,
+ llvm::ArrayRef args) const override;
+
+  bool GetArgumentValues(lldb_private::Thread &thread,
+ lldb_private::ValueList &values) const override;
+
+  lldb_private::Status
+  SetReturnValueObject(lldb::StackFrameSP &frame_sp,
+   lldb::ValueObjectSP &new_value) override;
+
+  lldb::ValueObjectSP
+  GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   lldb_private::CompilerType &type) const override;
+
+  // Specialized to work with llvm IR types.
+  lldb::ValueObjectSP GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   llvm::Type &type) const override;
+
+  bool
+  CreateFunctionEntryUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool CreateDefaultUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool RegisterIsVolatile(const lldb_private::RegisterInfo *reg_info) override;
+
+  bool CallFrameAddressIsValid(lldb::addr_t cfa) override {
+// The CFA must be 128 bit aligned, unless the E ABI is used
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+uint32_t arch_flags = arch.GetFlags();
+if (arch_flags & lldb_private::ArchSpec::eRISCV_rve)
+  return (cfa & 0x3ull) == 0;
+return (cfa & 0xfull) == 0;
+  }
+
+  bool CodeAddressIsValid(lldb::addr_t pc) override {

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted added a subscriber: labath.
ted added a comment.

With this change, I'm able to debug RISC-V 32 and 64 bit applications using 
Linux user space QEMU, using @labath 's QemuUser platform. Simple expression 
parsing with the IR Interpreter is working. Complex expression parsing with JIT 
is not tested.

Here's an example of a simple debug session:

  > bin/lldb
  (lldb) platform select qemu-user
Platform: qemu-user
  Triple: x86_64-*-linux-gnu
  OS Version: 5.4.0 (5.4.0-136-generic)
Hostname: 127.0.0.1
  WorkingDir: /local/mnt/ted/upstream/full
  Kernel: #153~18.04.1-Ubuntu SMP Wed Nov 30 15:47:57 UTC 2022
  (lldb) file ~/lldb_test/factrv32
  Current executable set to '/usr2/tedwood/lldb_test/factrv32' (riscv32).
  (lldb) b main
  Breakpoint 1: where = factrv32`main + 28 at factorial.c:32:8, address = 
0x000104ea
  (lldb) target list
  Current targets:
  * target #0: /usr2/tedwood/lldb_test/factrv32 ( arch=riscv32-*-linux, 
platform=qemu-user )
  (lldb) r
  Process 1 launched: '/usr2/tedwood/lldb_test/factrv32' (riscv32)
  Process 1 stopped
  * thread #1, stop reason = breakpoint 1.1
  frame #0: 0x000104ea factrv32`main(argc=1, argv=0x40800604) at 
factorial.c:32:8
 29   }
 30 */
 31 
  -> 32   base = 10;
 33 
 34   printf("Factorial of %d is %d\n", base, factorial(base));
 35   return 0;
  (lldb) s
  Process 1 stopped
  * thread #1, stop reason = step in
  frame #0: 0x000104ee factrv32`main(argc=1, argv=0x40800604) at 
factorial.c:34:37
 31 
 32   base = 10;
 33 
  -> 34   printf("Factorial of %d is %d\n", base, factorial(base));
 35   return 0;
 36 }
 37 
  (lldb) 
  Process 1 stopped
  * thread #1, stop reason = step in
  frame #0: 0x00010488 factrv32`factorial(i=10) at factorial.c:6:7
 3  
 4  int factorial(int i)
 5  {
  -> 6if (i == 1)
 7  return 1;
 8else
 9  return i * factorial(i - 1);
  (lldb) 
  Process 1 stopped
  * thread #1, stop reason = step in
  frame #0: 0x000104a4 factrv32`factorial(i=10) at factorial.c:9:12
 6if (i == 1)
 7  return 1;
 8else
  -> 9  return i * factorial(i - 1);
 10 }
 11 
 12 int main(int argc, char **argv)
  (lldb) 
  Process 1 stopped
  * thread #1, stop reason = step in
  frame #0: 0x00010488 factrv32`factorial(i=9) at factorial.c:6:7
 3  
 4  int factorial(int i)
 5  {
  -> 6if (i == 1)
 7  return 1;
 8else
 9  return i * factorial(i - 1);
  (lldb) 
  Process 1 stopped
  * thread #1, stop reason = step in
  frame #0: 0x000104a4 factrv32`factorial(i=9) at factorial.c:9:12
 6if (i == 1)
 7  return 1;
 8else
  -> 9  return i * factorial(i - 1);
 10 }
 11 
 12 int main(int argc, char **argv)
  (lldb) 
  Process 1 stopped
  * thread #1, stop reason = step in
  frame #0: 0x00010488 factrv32`factorial(i=8) at factorial.c:6:7
 3  
 4  int factorial(int i)
 5  {
  -> 6if (i == 1)
 7  return 1;
 8else
 9  return i * factorial(i - 1);
  (lldb) 
  Process 1 stopped
  * thread #1, stop reason = step in
  frame #0: 0x000104a4 factrv32`factorial(i=8) at factorial.c:9:12
 6if (i == 1)
 7  return 1;
 8else
  -> 9  return i * factorial(i - 1);
 10 }
 11 
 12 int main(int argc, char **argv)
  (lldb) 
  Process 1 stopped
  * thread #1, stop reason = step in
  frame #0: 0x00010488 factrv32`factorial(i=7) at factorial.c:6:7
 3  
 4  int factorial(int i)
 5  {
  -> 6if (i == 1)
 7  return 1;
 8else
 9  return i * factorial(i - 1);
  (lldb) 
  Process 1 stopped
  * thread #1, stop reason = step in
  frame #0: 0x000104a4 factrv32`factorial(i=7) at factorial.c:9:12
 6if (i == 1)
 7  return 1;
 8else
  -> 9  return i * factorial(i - 1);
 10 }
 11 
 12 int main(int argc, char **argv)
  (lldb) 
  Process 1 stopped
  * thread #1, stop reason = step in
  frame #0: 0x00010488 factrv32`factorial(i=6) at factorial.c:6:7
 3  
 4  int factorial(int i)
 5  {
  -> 6if (i == 1)
 7  return 1;
 8else
 9  return i * factorial(i - 1);
  (lldb) bt
  * thread #1, stop reason = step in
* frame #0: 0x00010488 factrv32`factorial(i=6) at factoria

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted added reviewers: jasonmolenda, labath, asb, lewis-revill, compnerd, 
simoncook, jrtc27, JDevlieghere, aprantl, sven, kasper81, tzb99.
ted added a comment.

Adding commenters from https://reviews.llvm.org/D62732 as reviewers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

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

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D159101#4625539 , @DavidSpickett 
wrote:

> Also could you provide a list of things you have tested with qemu. So I can 
> get an idea of how sure we can be any of this works / be a test plan, albeit 
> manual for anyone updating this code in future.

I'll answer this first, because it's the easiest!

I've done the following:

- Run 32 and 64 bit Linux binaries, launched on user space riscv32 and riscv64 
QEMU, using the QemuUser platform.
- Hit a breakpoint
- Continue
- Step in, step over, step out, instruction step in, instruction step out, 
including stepping all the way down and back up a 10 level recursive call
- Looked at a backtrace
- Looked at variables with frame variable
- Looked at and modified variables with the expression parser
- Read and writte memory

@jasonmolenda the step plan issue we discussed was actually a disassembler 
issue, and was fixed by D156086 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

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

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted marked 13 inline comments as done.
ted added inline comments.



Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:107
+static size_t word_size = 4U;
+static size_t reg_size = word_size;
+

DavidSpickett wrote:
> What's the reason to do this this way? It seems like adding an extra argument 
> to the constructor of ABISysV_riscv  would do the same thing unless someone 
> is calling the constructor directly but that's what CreateInstance tries to 
> prevent.
> 
> I see that mips has 2 ABI plugins, and MacOS on x86 has i386 and x86_64. The 
> former I don't think has massive differences between 32 and 64 but the latter 
> does.
> 
> So I agree with sharing the ABI here between riscv32 and riscv64 just with 
> the way it's implemented.
> 
> If it's because you use an array later, either make it a vector or better an 
> llvm::SmallVector which for rv32 will likely just use stack space for 4 byte 
> stuff.
I copied this from the ARC ABI plugin.

word_size and reg_size are used in computations, in PrepareTrivialCall and 
SetReturnValueObject.



Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:145
+  return false;
+}
+

DavidSpickett wrote:
> Looking at the comments in lldb/include/lldb/Target/ABI.h I'm not sure which 
> of these should be implemented. I think this one is what most plugins provide.
> 
> One way to figure this out is to figure out what actually needs this. Return 
> false from both and try a bunch of things to see if it fails, run an 
> expression, step in and out etc.
> 
> I'd be more comfortable having one not implemented if we know how the other 
> one gets used.
The first one is used for calling functions via JIT. The second is used for 
calling functions via the IR Interpreter. I didn't want to enable JIT, so I 
took the Hexagon implementation (Hexagon doesn't support JIT in lldb, but can 
call functions with the IR interpreter) and reworked it for RISC-V.

Here's a function call on riscv64:
(lldb) re r pc
  pc = 0x000106b2 factrv64`main + 28 at factorial.c:32:8  
factrv64`main + 28 at factorial.c:32:8
(lldb) p factorial(3)
(int) 6




Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:310
+
+raw_value >>= 32;
+reg_info =

DavidSpickett wrote:
> I see this came from Arc which appears to be 32 bit, so this needs to change 
> for rv64?
> 
> If it doesn't, add comments above to note where rv64 would exit before 
> getting here.
Yes - on rv64, a 128 bit scalar can be passed in ARG1 and ARG2. Get the next 
chunk of data from data and write that.



Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:1554
+// Turn them on by default now, since everyone seems to use them
+features_str += "+a,+m,";
   }

DavidSpickett wrote:
> You might want to take the lead from AArch64 here:
> ```
> // If any AArch64 variant, enable latest ISA with all extensions.
> ```
> If "+all" doesn't already work for riscv then you don't have to go and make 
> that work right now.
> 
> But in general we decided that much like llvm-objdump, we'll try to 
> disassemble any possible encoding. If the user happens to point the 
> disassembler at garbage that looks like a fancy extension on a cpu from 20 
> years ago, that's on them.
While I like the "turn on the latest" philosophy in general, for RISC-V we 
don't want to do that. It's modular architecture means features can be turned 
on and off when a core is designed, so one core might have +d (floating point 
double), while an newer core might not have any floating point at all. I'm 
inclined to leave the features as they are now, with a and m turned on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

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

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 554505.
ted added a comment.

Updated with David Spickett's comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

Files:
  lldb/source/Plugins/ABI/CMakeLists.txt
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
  lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1550,6 +1550,8 @@
 ArchSpec::eRISCV_float_abi_quad)
   features_str += "+f,+d,+q,";
 // FIXME: how do we detect features such as `+a`, `+m`?
+// Turn them on by default now, since everyone seems to use them
+features_str += "+a,+m,";
   }
 
   // We use m_disasm_up.get() to tell whether we are valid or not, so if this
Index: lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
@@ -0,0 +1,12 @@
+add_lldb_library(lldbPluginABIRISCV PLUGIN
+  ABISysV_riscv.cpp
+
+  LINK_LIBS
+lldbCore
+lldbSymbol
+lldbTarget
+lldbPluginProcessUtility
+  LINK_COMPONENTS
+Support
+TargetParser
+  )
Index: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -0,0 +1,127 @@
+//===-- ABISysV_riscv.h -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef liblldb_ABISysV_riscv_h_
+#define liblldb_ABISysV_riscv_h_
+
+// Other libraries and framework includes
+#include 
+
+#include "llvm/TargetParser/Triple.h"
+
+// Project includes
+#include "lldb/Target/ABI.h"
+#include "lldb/Target/Process.h"
+#include "lldb/lldb-private.h"
+
+class ABISysV_riscv : public lldb_private::RegInfoBasedABI {
+public:
+  ~ABISysV_riscv() override = default;
+
+  size_t GetRedZoneSize() const override { return 0; }
+
+  bool PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+  lldb::addr_t functionAddress,
+  lldb::addr_t returnAddress,
+  llvm::ArrayRef args) const override;
+
+  // Special thread plan for GDB style non-jit function calls.
+  bool
+  PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+ lldb::addr_t functionAddress, lldb::addr_t returnAddress,
+ llvm::Type &prototype,
+ llvm::ArrayRef args) const override;
+
+  bool GetArgumentValues(lldb_private::Thread &thread,
+ lldb_private::ValueList &values) const override;
+
+  lldb_private::Status
+  SetReturnValueObject(lldb::StackFrameSP &frame_sp,
+   lldb::ValueObjectSP &new_value) override;
+
+  lldb::ValueObjectSP
+  GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   lldb_private::CompilerType &type) const override;
+
+  // Specialized to work with llvm IR types.
+  lldb::ValueObjectSP GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   llvm::Type &type) const override;
+
+  bool
+  CreateFunctionEntryUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool CreateDefaultUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool RegisterIsVolatile(const lldb_private::RegisterInfo *reg_info) override;
+
+  bool CallFrameAddressIsValid(lldb::addr_t cfa) override {
+// The CFA must be 128 bit aligned, unless the E ABI is used
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+uint32_t arch_flags = arch.GetFlags();
+if (arch_flags & lldb_private::ArchSpec::eRISCV_rve)
+  return (cfa & 0x3ull) == 0;
+return (cfa & 0xfull) == 0;
+  }
+
+  bool CodeAddressIsValid(lldb::addr_t pc) override {
+// Calls can use the least significant bit to store auxiliary information,
+// so no strict check is done for alignment.
+
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+
+//  & 2 set is a fault if C extension is not used.
+uint32_t arch_flags = arch.GetFlags();
+if (!(arch_flags & lldb_private::ArchSpec::eRISCV_rvc) && (pc & 2))
+  return false;
+
+// Make sure 64 bit addr_t only has lower 32 bits set on riscv32
+llvm::Triple::ArchTyp

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D159101#4627705 , @DavidSpickett 
wrote:

> I have some vague idea that maybe we could put a hack in the test suite to 
> use the qemu-user platform instead of lldb-server. To at least give it a go, 
> but I haven't tried it myself yet.

Downstream I've got a hack to the qemu-user platform that allows it to be 
automatically selected for riscv32/64-unknown-linux. I didn't think it would be 
appropriate upstream. So I can launch lldb with a riscv binary and do a run, 
and it launches qemu-riscv32/64. I'm planning on running the test suite on 
upstream + this patch, with that hack, and seeing what happens.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

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

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D159101#4627705 , @DavidSpickett 
wrote:

> Great. Could you include that in the commit message? Seems like we get a lot 
> of questions about how mature risc-v support is, so we can point to this as a 
> milestone for that.

Will do.




Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:107
+static size_t word_size = 4U;
+static size_t reg_size = word_size;
+

jasonmolenda wrote:
> ted wrote:
> > DavidSpickett wrote:
> > > What's the reason to do this this way? It seems like adding an extra 
> > > argument to the constructor of ABISysV_riscv  would do the same thing 
> > > unless someone is calling the constructor directly but that's what 
> > > CreateInstance tries to prevent.
> > > 
> > > I see that mips has 2 ABI plugins, and MacOS on x86 has i386 and x86_64. 
> > > The former I don't think has massive differences between 32 and 64 but 
> > > the latter does.
> > > 
> > > So I agree with sharing the ABI here between riscv32 and riscv64 just 
> > > with the way it's implemented.
> > > 
> > > If it's because you use an array later, either make it a vector or better 
> > > an llvm::SmallVector which for rv32 will likely just use stack space for 
> > > 4 byte stuff.
> > I copied this from the ARC ABI plugin.
> > 
> > word_size and reg_size are used in computations, in PrepareTrivialCall and 
> > SetReturnValueObject.
> I'd prefer that the CreateInstance method initialize an ivar with the word 
> size based on the passed-in ArchSpec, or save the ArchSpec in an ivar.  
> risc-v seems to have many variants so I worry about saving the full ArchSpec 
> when ABI::CreateInstance is being called, possibly where we might not have 
> the most specific ArchSpec for this process?  But 32-bit v. 64-bit will 
> surely be constant.  When I was reading the patch later I'd see references to 
> `reg_size` and scratch my head where that variable was coming from.
> 
> This static will not work correctly if you have an lldb connected to an rv32 
> target and an rv64 target simultaneously.
> 
> Didn't the earlier ABI have a `isRV64` ivar (should have been m_is_rv64 but 
> whatever) to solve this?
This is a very good point - debugging both rv32 and rv64 programs at the same 
time could give incorrect results. I think setting an ivar with 32-bit vs 
64-bit in CreateInstance, and setting local variables based on that is a good 
idea.



Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:638
+bool ABISysV_riscv::CreateDefaultUnwindPlan(UnwindPlan &unwind_plan) {
+  return false;
+}

jasonmolenda wrote:
> We were emailing a couple of months ago and I think I suggested something 
> like this, based on the previous ABI patch
> 
> ```
> bool ABISysV_riscv::CreateDefaultUnwindPlan(UnwindPlan &unwind_plan) {
>   unwind_plan.Clear();
>   unwind_plan.SetRegisterKind(eRegisterKindGeneric);
> 
>   uint32_t pc_reg_num = LLDB_REGNUM_GENERIC_PC;
>   uint32_t fp_reg_num = LLDB_REGNUM_GENERIC_FP;
> 
>   UnwindPlan::RowSP row(new UnwindPlan::Row);
> 
>   // Define the CFA as the current frame pointer value.
>   row->GetCFAValue().SetIsRegisterPlusOffset(fp_reg_num, 0);
>   row->SetOffset(0);
> 
>   int ptr_size = 4;
>   if (isRV64)
> ptr_size = 8;
> 
>   // Assume the ra reg (return pc) and caller's frame pointer 
>   // have been spilled to stack already.
>   row->SetRegisterLocationToAtCFAPlusOffset(fp_reg_num, ptr_size * -2, true);
>   row->SetRegisterLocationToAtCFAPlusOffset(pc_reg_num, ptr_size * -1, true);
> 
>   unwind_plan.AppendRow(row);
>   unwind_plan.SetSourceName("riscv default unwind plan");
>   unwind_plan.SetSourcedFromCompiler(eLazyBoolNo);
>   unwind_plan.SetUnwindPlanValidAtAllInstructions(eLazyBoolNo);
>   return true;
> }
> ```
The RISC-V ABI says the frame pointer is optional, and I haven't found a way to 
tell if the FP (register s0) is being used as an FP, or if it's a generic 
callee-saved register. 

Debugging an rv32 program that doesn't use the FP works with the current 
CreateDefaultUnwindPlan. I can step in, step over, step out and get a 
backtrace. Is there anything else I need to test?



Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h:79-80
+uint32_t arch_flags = arch.GetFlags();
+if (!(arch_flags & lldb_private::ArchSpec::eRISCV_rvc))
+  if (pc & 2)
+return false;

jasonmolenda wrote:
> DavidSpickett wrote:
> > Combine this into one if.
> Maybe more clear like
> ```
> if (arch_flags | ArchSpec::eRISCV_rvc && pc & 2) 
>   return false;
> ```
> 
> It's too bad ArchSpec doesn't have a `Flags()` method which returns a Flags 
> object, so this could be  `if (arch.Flags().Test(ArchSpec::eRISCV_rvc) && pc 
> & 2)`.
> 
> Suppose it would be just as easy to do 
> ```
>   Flags arch_flags(arch.GetFlags();
>   if (arch_flags.Test(ArchSpec::eRISCV_rvc) && pc & 2)
> return false;
> ```
I like this idea.

Addin

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 555155.
ted marked an inline comment as done.
ted added a comment.

Implement Jason Molenda's request to use an ivar instead of static variables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

Files:
  lldb/source/Plugins/ABI/CMakeLists.txt
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
  lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1550,6 +1550,8 @@
 ArchSpec::eRISCV_float_abi_quad)
   features_str += "+f,+d,+q,";
 // FIXME: how do we detect features such as `+a`, `+m`?
+// Turn them on by default now, since everyone seems to use them
+features_str += "+a,+m,";
   }
 
   // We use m_disasm_up.get() to tell whether we are valid or not, so if this
Index: lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
@@ -0,0 +1,12 @@
+add_lldb_library(lldbPluginABIRISCV PLUGIN
+  ABISysV_riscv.cpp
+
+  LINK_LIBS
+lldbCore
+lldbSymbol
+lldbTarget
+lldbPluginProcessUtility
+  LINK_COMPONENTS
+Support
+TargetParser
+  )
Index: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -0,0 +1,130 @@
+//===-- ABISysV_riscv.h -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef liblldb_ABISysV_riscv_h_
+#define liblldb_ABISysV_riscv_h_
+
+// Other libraries and framework includes
+#include 
+
+#include "llvm/TargetParser/Triple.h"
+
+// Project includes
+#include "lldb/Target/ABI.h"
+#include "lldb/Target/Process.h"
+#include "lldb/lldb-private.h"
+
+class ABISysV_riscv : public lldb_private::RegInfoBasedABI {
+public:
+  ~ABISysV_riscv() override = default;
+
+  size_t GetRedZoneSize() const override { return 0; }
+
+  bool PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+  lldb::addr_t functionAddress,
+  lldb::addr_t returnAddress,
+  llvm::ArrayRef args) const override;
+
+  // Special thread plan for GDB style non-jit function calls.
+  bool
+  PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+ lldb::addr_t functionAddress, lldb::addr_t returnAddress,
+ llvm::Type &prototype,
+ llvm::ArrayRef args) const override;
+
+  bool GetArgumentValues(lldb_private::Thread &thread,
+ lldb_private::ValueList &values) const override;
+
+  lldb_private::Status
+  SetReturnValueObject(lldb::StackFrameSP &frame_sp,
+   lldb::ValueObjectSP &new_value) override;
+
+  lldb::ValueObjectSP
+  GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   lldb_private::CompilerType &type) const override;
+
+  // Specialized to work with llvm IR types.
+  lldb::ValueObjectSP GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   llvm::Type &type) const override;
+
+  bool
+  CreateFunctionEntryUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool CreateDefaultUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool RegisterIsVolatile(const lldb_private::RegisterInfo *reg_info) override;
+
+  bool CallFrameAddressIsValid(lldb::addr_t cfa) override {
+// The CFA must be 128 bit aligned, unless the E ABI is used
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+uint32_t arch_flags = arch.GetFlags();
+if (arch_flags & lldb_private::ArchSpec::eRISCV_rve)
+  return (cfa & 0x3ull) == 0;
+return (cfa & 0xfull) == 0;
+  }
+
+  void SetIsRV64(bool is_rv64) { m_is_rv64 = is_rv64; }
+
+  bool CodeAddressIsValid(lldb::addr_t pc) override {
+// Calls can use the least significant bit to store auxiliary information,
+// so no strict check is done for alignment.
+
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+
+//  & 2 set is a fault if C extension is not used.
+uint32_t arch_flags = arch.GetFlags();
+if (!(arch_flags & lldb_private::ArchSpec::eRISCV_r

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

@DavidSpickett update on testing: I'm running tests. I found an issue in the IR 
Interpreter when calling class methods. Somehow a 32 bit pointer gets the upper 
32 bits in the 64 bit uint it's stored in set to 0x, which causes an 
assert. The assert should probably be an error, and not crash, but for now I'm 
skipping the tests that cause this. Progress is slow, but steady.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

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

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 557049.
ted added a comment.

Fix crash when there was no process.
Remove bugs in function call setup when using IR interpreter.
Change flag tests to use lldb_private::Flags for flags testing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

Files:
  lldb/source/Plugins/ABI/CMakeLists.txt
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
  lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1550,6 +1550,8 @@
 ArchSpec::eRISCV_float_abi_quad)
   features_str += "+f,+d,+q,";
 // FIXME: how do we detect features such as `+a`, `+m`?
+// Turn them on by default now, since everyone seems to use them
+features_str += "+a,+m,";
   }
 
   // We use m_disasm_up.get() to tell whether we are valid or not, so if this
Index: lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
@@ -0,0 +1,12 @@
+add_lldb_library(lldbPluginABIRISCV PLUGIN
+  ABISysV_riscv.cpp
+
+  LINK_LIBS
+lldbCore
+lldbSymbol
+lldbTarget
+lldbPluginProcessUtility
+  LINK_COMPONENTS
+Support
+TargetParser
+  )
Index: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -0,0 +1,131 @@
+//===-- ABISysV_riscv.h -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef liblldb_ABISysV_riscv_h_
+#define liblldb_ABISysV_riscv_h_
+
+// Other libraries and framework includes
+#include 
+
+#include "llvm/TargetParser/Triple.h"
+
+// Project includes
+#include "lldb/Target/ABI.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Utility/Flags.h"
+#include "lldb/lldb-private.h"
+
+class ABISysV_riscv : public lldb_private::RegInfoBasedABI {
+public:
+  ~ABISysV_riscv() override = default;
+
+  size_t GetRedZoneSize() const override { return 0; }
+
+  bool PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+  lldb::addr_t functionAddress,
+  lldb::addr_t returnAddress,
+  llvm::ArrayRef args) const override;
+
+  // Special thread plan for GDB style non-jit function calls.
+  bool
+  PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+ lldb::addr_t functionAddress, lldb::addr_t returnAddress,
+ llvm::Type &prototype,
+ llvm::ArrayRef args) const override;
+
+  bool GetArgumentValues(lldb_private::Thread &thread,
+ lldb_private::ValueList &values) const override;
+
+  lldb_private::Status
+  SetReturnValueObject(lldb::StackFrameSP &frame_sp,
+   lldb::ValueObjectSP &new_value) override;
+
+  lldb::ValueObjectSP
+  GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   lldb_private::CompilerType &type) const override;
+
+  // Specialized to work with llvm IR types.
+  lldb::ValueObjectSP GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   llvm::Type &type) const override;
+
+  bool
+  CreateFunctionEntryUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool CreateDefaultUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool RegisterIsVolatile(const lldb_private::RegisterInfo *reg_info) override;
+
+  bool CallFrameAddressIsValid(lldb::addr_t cfa) override {
+// The CFA must be 128 bit aligned, unless the E ABI is used
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+lldb_private::Flags arch_flags = arch.GetFlags();
+if (arch_flags.Test(lldb_private::ArchSpec::eRISCV_rve))
+  return (cfa & 0x3ull) == 0;
+return (cfa & 0xfull) == 0;
+  }
+
+  void SetIsRV64(bool is_rv64) { m_is_rv64 = is_rv64; }
+
+  bool CodeAddressIsValid(lldb::addr_t pc) override {
+// Calls can use the least significant bit to store auxiliary information,
+// so no strict check is done for alignment.
+
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+
+//  & 2 set is a fault if C extension is not used.
+l

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted added inline comments.



Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:133
+ABISysV_riscv::CreateInstance(ProcessSP process_sp, const ArchSpec &arch) {
+  if (process_sp) {
+process_sp->SetCanInterpretFunctionCalls(true);

DavidSpickett wrote:
> Do you know why this check is needed? Is there a specific test that hits this?
> 
> The other ABI plugins seem to sidestep this by not having to `->` on it, but 
> that could just be luck.
Without that check, certain tests crashed when they instantiated the ABI plugin 
without a process.



Comment at: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp:135
+process_sp->SetCanInterpretFunctionCalls(true);
+process_sp->SetCanJIT(false);
+  }

jasonmolenda wrote:
> These should not be set in the ABI, please remove these.  These are probably 
> correct for the qemu environment you're testing against, but if we're 
> connected to a remote stub that can allocate memory (_M packet), we can JIT 
> code. 
I can remove these, but for the current environment they're correct. We don't 
have function calling for JIT, or RuntimeDyld set up for RISC-V.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

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

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 557321.
ted added a comment.

Remove disabling JIT and enabling IR Interpreter function calls, as requested
by Jason Molenda.

Added default unwind plan as suggested by Jason Molenda.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

Files:
  lldb/source/Plugins/ABI/CMakeLists.txt
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
  lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1550,6 +1550,8 @@
 ArchSpec::eRISCV_float_abi_quad)
   features_str += "+f,+d,+q,";
 // FIXME: how do we detect features such as `+a`, `+m`?
+// Turn them on by default now, since everyone seems to use them
+features_str += "+a,+m,";
   }
 
   // We use m_disasm_up.get() to tell whether we are valid or not, so if this
Index: lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
@@ -0,0 +1,12 @@
+add_lldb_library(lldbPluginABIRISCV PLUGIN
+  ABISysV_riscv.cpp
+
+  LINK_LIBS
+lldbCore
+lldbSymbol
+lldbTarget
+lldbPluginProcessUtility
+  LINK_COMPONENTS
+Support
+TargetParser
+  )
Index: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -0,0 +1,131 @@
+//===-- ABISysV_riscv.h -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef liblldb_ABISysV_riscv_h_
+#define liblldb_ABISysV_riscv_h_
+
+// Other libraries and framework includes
+#include 
+
+#include "llvm/TargetParser/Triple.h"
+
+// Project includes
+#include "lldb/Target/ABI.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Utility/Flags.h"
+#include "lldb/lldb-private.h"
+
+class ABISysV_riscv : public lldb_private::RegInfoBasedABI {
+public:
+  ~ABISysV_riscv() override = default;
+
+  size_t GetRedZoneSize() const override { return 0; }
+
+  bool PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+  lldb::addr_t functionAddress,
+  lldb::addr_t returnAddress,
+  llvm::ArrayRef args) const override;
+
+  // Special thread plan for GDB style non-jit function calls.
+  bool
+  PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+ lldb::addr_t functionAddress, lldb::addr_t returnAddress,
+ llvm::Type &prototype,
+ llvm::ArrayRef args) const override;
+
+  bool GetArgumentValues(lldb_private::Thread &thread,
+ lldb_private::ValueList &values) const override;
+
+  lldb_private::Status
+  SetReturnValueObject(lldb::StackFrameSP &frame_sp,
+   lldb::ValueObjectSP &new_value) override;
+
+  lldb::ValueObjectSP
+  GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   lldb_private::CompilerType &type) const override;
+
+  // Specialized to work with llvm IR types.
+  lldb::ValueObjectSP GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   llvm::Type &type) const override;
+
+  bool
+  CreateFunctionEntryUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool CreateDefaultUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool RegisterIsVolatile(const lldb_private::RegisterInfo *reg_info) override;
+
+  bool CallFrameAddressIsValid(lldb::addr_t cfa) override {
+// The CFA must be 128 bit aligned, unless the E ABI is used
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+lldb_private::Flags arch_flags = arch.GetFlags();
+if (arch_flags.Test(lldb_private::ArchSpec::eRISCV_rve))
+  return (cfa & 0x3ull) == 0;
+return (cfa & 0xfull) == 0;
+  }
+
+  void SetIsRV64(bool is_rv64) { m_is_rv64 = is_rv64; }
+
+  bool CodeAddressIsValid(lldb::addr_t pc) override {
+// Calls can use the least significant bit to store auxiliary information,
+// so no strict check is done for alignment.
+
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+
+//  & 2 set is a fault if C extension is not used.
+lldb_priv

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 557322.
ted marked 3 inline comments as done.
ted added a comment.

Change "word_size = m_is_rv64 ? 8 : 4" to "word_size = reg_size"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

Files:
  lldb/source/Plugins/ABI/CMakeLists.txt
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
  lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1550,6 +1550,8 @@
 ArchSpec::eRISCV_float_abi_quad)
   features_str += "+f,+d,+q,";
 // FIXME: how do we detect features such as `+a`, `+m`?
+// Turn them on by default now, since everyone seems to use them
+features_str += "+a,+m,";
   }
 
   // We use m_disasm_up.get() to tell whether we are valid or not, so if this
Index: lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
@@ -0,0 +1,12 @@
+add_lldb_library(lldbPluginABIRISCV PLUGIN
+  ABISysV_riscv.cpp
+
+  LINK_LIBS
+lldbCore
+lldbSymbol
+lldbTarget
+lldbPluginProcessUtility
+  LINK_COMPONENTS
+Support
+TargetParser
+  )
Index: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -0,0 +1,131 @@
+//===-- ABISysV_riscv.h -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef liblldb_ABISysV_riscv_h_
+#define liblldb_ABISysV_riscv_h_
+
+// Other libraries and framework includes
+#include 
+
+#include "llvm/TargetParser/Triple.h"
+
+// Project includes
+#include "lldb/Target/ABI.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Utility/Flags.h"
+#include "lldb/lldb-private.h"
+
+class ABISysV_riscv : public lldb_private::RegInfoBasedABI {
+public:
+  ~ABISysV_riscv() override = default;
+
+  size_t GetRedZoneSize() const override { return 0; }
+
+  bool PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+  lldb::addr_t functionAddress,
+  lldb::addr_t returnAddress,
+  llvm::ArrayRef args) const override;
+
+  // Special thread plan for GDB style non-jit function calls.
+  bool
+  PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+ lldb::addr_t functionAddress, lldb::addr_t returnAddress,
+ llvm::Type &prototype,
+ llvm::ArrayRef args) const override;
+
+  bool GetArgumentValues(lldb_private::Thread &thread,
+ lldb_private::ValueList &values) const override;
+
+  lldb_private::Status
+  SetReturnValueObject(lldb::StackFrameSP &frame_sp,
+   lldb::ValueObjectSP &new_value) override;
+
+  lldb::ValueObjectSP
+  GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   lldb_private::CompilerType &type) const override;
+
+  // Specialized to work with llvm IR types.
+  lldb::ValueObjectSP GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   llvm::Type &type) const override;
+
+  bool
+  CreateFunctionEntryUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool CreateDefaultUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool RegisterIsVolatile(const lldb_private::RegisterInfo *reg_info) override;
+
+  bool CallFrameAddressIsValid(lldb::addr_t cfa) override {
+// The CFA must be 128 bit aligned, unless the E ABI is used
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+lldb_private::Flags arch_flags = arch.GetFlags();
+if (arch_flags.Test(lldb_private::ArchSpec::eRISCV_rve))
+  return (cfa & 0x3ull) == 0;
+return (cfa & 0xfull) == 0;
+  }
+
+  void SetIsRV64(bool is_rv64) { m_is_rv64 = is_rv64; }
+
+  bool CodeAddressIsValid(lldb::addr_t pc) override {
+// Calls can use the least significant bit to store auxiliary information,
+// so no strict check is done for alignment.
+
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+
+//  & 2 set is a fault if C extension is not used.
+lldb_private::Flags arch_flags(arch.GetFlags());
+if (!a

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 557323.
ted marked 4 inline comments as done.
ted added a comment.

Fix comment about "a" registers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

Files:
  lldb/source/Plugins/ABI/CMakeLists.txt
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
  lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1550,6 +1550,8 @@
 ArchSpec::eRISCV_float_abi_quad)
   features_str += "+f,+d,+q,";
 // FIXME: how do we detect features such as `+a`, `+m`?
+// Turn them on by default now, since everyone seems to use them
+features_str += "+a,+m,";
   }
 
   // We use m_disasm_up.get() to tell whether we are valid or not, so if this
Index: lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
@@ -0,0 +1,12 @@
+add_lldb_library(lldbPluginABIRISCV PLUGIN
+  ABISysV_riscv.cpp
+
+  LINK_LIBS
+lldbCore
+lldbSymbol
+lldbTarget
+lldbPluginProcessUtility
+  LINK_COMPONENTS
+Support
+TargetParser
+  )
Index: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -0,0 +1,131 @@
+//===-- ABISysV_riscv.h -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef liblldb_ABISysV_riscv_h_
+#define liblldb_ABISysV_riscv_h_
+
+// Other libraries and framework includes
+#include 
+
+#include "llvm/TargetParser/Triple.h"
+
+// Project includes
+#include "lldb/Target/ABI.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Utility/Flags.h"
+#include "lldb/lldb-private.h"
+
+class ABISysV_riscv : public lldb_private::RegInfoBasedABI {
+public:
+  ~ABISysV_riscv() override = default;
+
+  size_t GetRedZoneSize() const override { return 0; }
+
+  bool PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+  lldb::addr_t functionAddress,
+  lldb::addr_t returnAddress,
+  llvm::ArrayRef args) const override;
+
+  // Special thread plan for GDB style non-jit function calls.
+  bool
+  PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+ lldb::addr_t functionAddress, lldb::addr_t returnAddress,
+ llvm::Type &prototype,
+ llvm::ArrayRef args) const override;
+
+  bool GetArgumentValues(lldb_private::Thread &thread,
+ lldb_private::ValueList &values) const override;
+
+  lldb_private::Status
+  SetReturnValueObject(lldb::StackFrameSP &frame_sp,
+   lldb::ValueObjectSP &new_value) override;
+
+  lldb::ValueObjectSP
+  GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   lldb_private::CompilerType &type) const override;
+
+  // Specialized to work with llvm IR types.
+  lldb::ValueObjectSP GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   llvm::Type &type) const override;
+
+  bool
+  CreateFunctionEntryUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool CreateDefaultUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool RegisterIsVolatile(const lldb_private::RegisterInfo *reg_info) override;
+
+  bool CallFrameAddressIsValid(lldb::addr_t cfa) override {
+// The CFA must be 128 bit aligned, unless the E ABI is used
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+lldb_private::Flags arch_flags = arch.GetFlags();
+if (arch_flags.Test(lldb_private::ArchSpec::eRISCV_rve))
+  return (cfa & 0x3ull) == 0;
+return (cfa & 0xfull) == 0;
+  }
+
+  void SetIsRV64(bool is_rv64) { m_is_rv64 = is_rv64; }
+
+  bool CodeAddressIsValid(lldb::addr_t pc) override {
+// Calls can use the least significant bit to store auxiliary information,
+// so no strict check is done for alignment.
+
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+
+//  & 2 set is a fault if C extension is not used.
+lldb_private::Flags arch_flags(arch.GetFlags());
+if (!arch_flags.Test(lldb_private::Arch

[Lldb-commits] [PATCH] D159101: [RISC-V] Add RISC-V ABI plugin

[Ted Woodward via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG847de9c33277: [RISC-V] Add RISC-V ABI plugin (authored by 
ted).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159101

Files:
  lldb/source/Plugins/ABI/CMakeLists.txt
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.cpp
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
  lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp

Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1576,6 +1576,8 @@
 ArchSpec::eRISCV_float_abi_quad)
   features_str += "+f,+d,+q,";
 // FIXME: how do we detect features such as `+a`, `+m`?
+// Turn them on by default now, since everyone seems to use them
+features_str += "+a,+m,";
   }
 
   // We use m_disasm_up.get() to tell whether we are valid or not, so if this
Index: lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/CMakeLists.txt
@@ -0,0 +1,12 @@
+add_lldb_library(lldbPluginABIRISCV PLUGIN
+  ABISysV_riscv.cpp
+
+  LINK_LIBS
+lldbCore
+lldbSymbol
+lldbTarget
+lldbPluginProcessUtility
+  LINK_COMPONENTS
+Support
+TargetParser
+  )
Index: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
===
--- /dev/null
+++ lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -0,0 +1,131 @@
+//===-- ABISysV_riscv.h -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef liblldb_ABISysV_riscv_h_
+#define liblldb_ABISysV_riscv_h_
+
+// Other libraries and framework includes
+#include 
+
+#include "llvm/TargetParser/Triple.h"
+
+// Project includes
+#include "lldb/Target/ABI.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Utility/Flags.h"
+#include "lldb/lldb-private.h"
+
+class ABISysV_riscv : public lldb_private::RegInfoBasedABI {
+public:
+  ~ABISysV_riscv() override = default;
+
+  size_t GetRedZoneSize() const override { return 0; }
+
+  bool PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+  lldb::addr_t functionAddress,
+  lldb::addr_t returnAddress,
+  llvm::ArrayRef args) const override;
+
+  // Special thread plan for GDB style non-jit function calls.
+  bool
+  PrepareTrivialCall(lldb_private::Thread &thread, lldb::addr_t sp,
+ lldb::addr_t functionAddress, lldb::addr_t returnAddress,
+ llvm::Type &prototype,
+ llvm::ArrayRef args) const override;
+
+  bool GetArgumentValues(lldb_private::Thread &thread,
+ lldb_private::ValueList &values) const override;
+
+  lldb_private::Status
+  SetReturnValueObject(lldb::StackFrameSP &frame_sp,
+   lldb::ValueObjectSP &new_value) override;
+
+  lldb::ValueObjectSP
+  GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   lldb_private::CompilerType &type) const override;
+
+  // Specialized to work with llvm IR types.
+  lldb::ValueObjectSP GetReturnValueObjectImpl(lldb_private::Thread &thread,
+   llvm::Type &type) const override;
+
+  bool
+  CreateFunctionEntryUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool CreateDefaultUnwindPlan(lldb_private::UnwindPlan &unwind_plan) override;
+
+  bool RegisterIsVolatile(const lldb_private::RegisterInfo *reg_info) override;
+
+  bool CallFrameAddressIsValid(lldb::addr_t cfa) override {
+// The CFA must be 128 bit aligned, unless the E ABI is used
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+lldb_private::Flags arch_flags = arch.GetFlags();
+if (arch_flags.Test(lldb_private::ArchSpec::eRISCV_rve))
+  return (cfa & 0x3ull) == 0;
+return (cfa & 0xfull) == 0;
+  }
+
+  void SetIsRV64(bool is_rv64) { m_is_rv64 = is_rv64; }
+
+  bool CodeAddressIsValid(lldb::addr_t pc) override {
+// Calls can use the least significant bit to store auxiliary information,
+// so no strict check is done for alignment.
+
+lldb_private::ArchSpec arch = GetProcessSP()->GetTarget().GetArchitecture();
+
+//  & 2 set is a fault if C extension is not used.
+lldb_private::

[Lldb-commits] [PATCH] D159550: Remove unused #include

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.
Herald added subscribers: luke, sunshaoce, frasercrmck, luismarques, apazos, 
sameer.abuasal, s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, 
MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, niosHD, sabuasal, 
simoncook, johnrusso, rbar, asb.
Herald added a project: All.
ted requested review of this revision.
Herald added subscribers: lldb-commits, wangpc, MaskRay.
Herald added a project: LLDB.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159550

Files:
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h


Index: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
===
--- lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
+++ lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -10,8 +10,6 @@
 #define liblldb_ABISysV_riscv_h_
 
 // Other libraries and framework includes
-#include 
-
 #include "llvm/TargetParser/Triple.h"
 
 // Project includes


Index: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
===
--- lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
+++ lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -10,8 +10,6 @@
 #define liblldb_ABISysV_riscv_h_
 
 // Other libraries and framework includes
-#include 
-
 #include "llvm/TargetParser/Triple.h"
 
 // Project includes
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D159550: Remove unused #include

[Ted Woodward via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb14153490134: Remove unused #include (authored by ted).
Herald added a subscriber: JDevlieghere.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159550

Files:
  lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h


Index: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
===
--- lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
+++ lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -10,8 +10,6 @@
 #define liblldb_ABISysV_riscv_h_
 
 // Other libraries and framework includes
-#include 
-
 #include "llvm/TargetParser/Triple.h"
 
 // Project includes


Index: lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
===
--- lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
+++ lldb/source/Plugins/ABI/RISCV/ABISysV_riscv.h
@@ -10,8 +10,6 @@
 #define liblldb_ABISysV_riscv_h_
 
 // Other libraries and framework includes
-#include 
-
 #include "llvm/TargetParser/Triple.h"
 
 // Project includes
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D159550: Remove unused #include

[Ted Woodward via Phabricator via lldb-commits]

ted added a subscriber: jasonmolenda.
ted added a comment.

Remove unused #include pointed out by @jasonmolenda in D159101 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159550

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

[Lldb-commits] [PATCH] D109633: [lldb-vscode] Fix focus thread when previous thread exits

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.
ted added a reviewer: clayborg.
ted added a project: LLDB.
Herald added a subscriber: JDevlieghere.
ted requested review of this revision.

The thread that Visual Studio Code displays on a stop is called the focus 
thread. When the previous focus thread exits and we stop in a new thread, 
lldb-vscode does not tell vscode to set the new thread as the focus thread, so 
it selects the first thread in the thread list.

This patch changes lldb-vscode to tell vscode that the new thread is the focus 
thread. It also includes a test that verifies the DAP stop message for this 
case contains the correct values.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109633

Files:
  lldb/test/API/tools/lldb-vscode/correct-thread/Makefile
  lldb/test/API/tools/lldb-vscode/correct-thread/TestVSCode_correct_thread.py
  lldb/test/API/tools/lldb-vscode/correct-thread/main.c
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -232,13 +232,17 @@
   // set it as the focus thread if below if needed.
   lldb::tid_t first_tid_with_reason = LLDB_INVALID_THREAD_ID;
   uint32_t num_threads_with_reason = 0;
+  bool focus_thread_exists = false;
   for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
 lldb::SBThread thread = process.GetThreadAtIndex(thread_idx);
 const lldb::tid_t tid = thread.GetThreadID();
 const bool has_reason = ThreadHasStopReason(thread);
 // If the focus thread doesn't have a stop reason, clear the thread ID
-if (tid == g_vsc.focus_tid && !has_reason)
-  g_vsc.focus_tid = LLDB_INVALID_THREAD_ID;
+if (tid == g_vsc.focus_tid) {
+  focus_thread_exists = true;
+  if (!has_reason)
+g_vsc.focus_tid = LLDB_INVALID_THREAD_ID;
+}
 if (has_reason) {
   ++num_threads_with_reason;
   if (first_tid_with_reason == LLDB_INVALID_THREAD_ID)
@@ -246,10 +250,10 @@
 }
   }
 
-  // We will have cleared g_vsc.focus_tid if he focus thread doesn't
-  // have a stop reason, so if it was cleared, or wasn't set, then set the
-  // focus thread to the first thread with a stop reason.
-  if (g_vsc.focus_tid == LLDB_INVALID_THREAD_ID)
+  // We will have cleared g_vsc.focus_tid if he focus thread doesn't have
+  // a stop reason, so if it was cleared, or wasn't set, or doesn't exist,
+  // then set the focus thread to the first thread with a stop reason.
+  if (!focus_thread_exists || g_vsc.focus_tid == LLDB_INVALID_THREAD_ID)
 g_vsc.focus_tid = first_tid_with_reason;
 
   // If no threads stopped with a reason, then report the first one so
Index: lldb/test/API/tools/lldb-vscode/correct-thread/main.c
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/correct-thread/main.c
@@ -0,0 +1,23 @@
+#include 
+#include 
+
+int state_var;
+
+void *thread (void *in)
+{
+  state_var++; // break here
+  return NULL;
+}
+
+int main(int argc, char **argv)
+{
+  pthread_t t1, t2;
+
+  pthread_create(&t1, NULL, *thread, NULL);
+  pthread_join(t1, NULL);
+  pthread_create(&t2, NULL, *thread, NULL);
+  pthread_join(t2, NULL);
+
+  printf("state_var is %d\n", state_var);
+  return 0;
+}
Index: lldb/test/API/tools/lldb-vscode/correct-thread/TestVSCode_correct_thread.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/correct-thread/TestVSCode_correct_thread.py
@@ -0,0 +1,47 @@
+"""
+Test lldb-vscode setBreakpoints request
+"""
+
+from __future__ import print_function
+
+import unittest2
+import vscode
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import lldbvscode_testcase
+
+
+class TestVSCode_correct_thread(lldbvscode_testcase.VSCodeTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIfWindows
+@skipIfRemote
+def test_correct_thread(self):
+'''
+Tests that the correct thread is selected if we continue from
+a thread that goes away and hit a breakpoint in another thread.
+In this case, the selected thread should be the thread that
+just hit the breakpoint, and not the first thread in the list.
+'''
+program = self.getBuildArtifact("a.out")
+self.build_and_launch(program)
+source = 'main.c'
+breakpoint_line = line_number(source, '// break here')
+lines = [breakpoint_line]
+# Set breakpoint in the thread function
+breakpoint_ids = self.set_source_breakpoints(source, lines)
+self.assertEqual(len(breakpoint_ids), len(lines),
+"expect correct numb

[Lldb-commits] [PATCH] D109738: [lldb] Fix bug 38317 - Address breakpoints don't work if set before the process launches

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

I created the original bug because a change made LLDB stop working on address 
breakpoints if they were created before the program is run. Setting a 
breakpoint on an address is a typical case with embedded applications, 
especially if you want to debug startup code.

I took this patch and applied it to the latest downstream Hexagon repo. It 
behaved as expected - I set a breakpoint at the address of main, ran, and it 
stopped at the breakpoint. Before this patch, the breakpoint wouldn't be 
resolved.

Thanks, Vadim!

(lldb) p main
(int (*)(int, unsigned char **)) $0 = 0x5128
(lldb) b 0x5128
Breakpoint 1: address = 0x5128
(lldb) br l
Current breakpoints:
1: address = 0x5128, locations = 1

  1.1: address = 0x5128, unresolved, hit count = 0 

(lldb) r
Process 1 launched: '/usr2/tedwood/lldb_test/factorial' (hexagon)
Process 1 stopped

- thread #1, name = 'T1', stop reason = breakpoint 1.1 frame #0: 0x5128 
factorial`main(argc=-1161904401, argv=0x5bac) at factorial.c:13 10   } 11 
12   int main(int argc, char **argv)

-> 13   {

  14 unsigned base;
  15  
  16   /*

(lldb) re r pc

  pc = 0x5128  factorial`main at factorial.c:13

(lldb) br l
Current breakpoints:
1: address = 0x5128, locations = 1, resolved = 1, hit count = 1

  1.1: address = 0x5128, resolved, hardware, hit count = 1 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109738

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

[Lldb-commits] [PATCH] D109738: [lldb] Fix bug 38317 - Address breakpoints don't work if set before the process launches

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D109738#3000608 , @jingham wrote:

> BTW, do you know what change made this stop working?

https://github.com/llvm-mirror/lldb/commit/43793406 in the old multirepo. In 
the monorepo, it's https://github.com/llvm/llvm-project/commit/5ec7cd7 .

We've got a conflict between "clean up old breakpoint addresses left behind 
when a section is deleted" vs "keep around breakpoint addressed that are set 
explicitly, but aren't part of a loaded section".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109738

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

[Lldb-commits] [PATCH] D109633: [lldb-vscode] Fix focus thread when previous thread exits

[Ted Woodward via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG17589538aaef: [lldb-vscode] Fix focus thread when previous 
thread exits (authored by ted).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109633

Files:
  lldb/test/API/tools/lldb-vscode/correct-thread/Makefile
  lldb/test/API/tools/lldb-vscode/correct-thread/TestVSCode_correct_thread.py
  lldb/test/API/tools/lldb-vscode/correct-thread/main.c
  lldb/tools/lldb-vscode/lldb-vscode.cpp

Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -232,13 +232,17 @@
   // set it as the focus thread if below if needed.
   lldb::tid_t first_tid_with_reason = LLDB_INVALID_THREAD_ID;
   uint32_t num_threads_with_reason = 0;
+  bool focus_thread_exists = false;
   for (uint32_t thread_idx = 0; thread_idx < num_threads; ++thread_idx) {
 lldb::SBThread thread = process.GetThreadAtIndex(thread_idx);
 const lldb::tid_t tid = thread.GetThreadID();
 const bool has_reason = ThreadHasStopReason(thread);
 // If the focus thread doesn't have a stop reason, clear the thread ID
-if (tid == g_vsc.focus_tid && !has_reason)
-  g_vsc.focus_tid = LLDB_INVALID_THREAD_ID;
+if (tid == g_vsc.focus_tid) {
+  focus_thread_exists = true;
+  if (!has_reason)
+g_vsc.focus_tid = LLDB_INVALID_THREAD_ID;
+}
 if (has_reason) {
   ++num_threads_with_reason;
   if (first_tid_with_reason == LLDB_INVALID_THREAD_ID)
@@ -246,10 +250,10 @@
 }
   }
 
-  // We will have cleared g_vsc.focus_tid if he focus thread doesn't
-  // have a stop reason, so if it was cleared, or wasn't set, then set the
-  // focus thread to the first thread with a stop reason.
-  if (g_vsc.focus_tid == LLDB_INVALID_THREAD_ID)
+  // We will have cleared g_vsc.focus_tid if he focus thread doesn't have
+  // a stop reason, so if it was cleared, or wasn't set, or doesn't exist,
+  // then set the focus thread to the first thread with a stop reason.
+  if (!focus_thread_exists || g_vsc.focus_tid == LLDB_INVALID_THREAD_ID)
 g_vsc.focus_tid = first_tid_with_reason;
 
   // If no threads stopped with a reason, then report the first one so
Index: lldb/test/API/tools/lldb-vscode/correct-thread/main.c
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/correct-thread/main.c
@@ -0,0 +1,23 @@
+#include 
+#include 
+
+int state_var;
+
+void *thread (void *in)
+{
+  state_var++; // break here
+  return NULL;
+}
+
+int main(int argc, char **argv)
+{
+  pthread_t t1, t2;
+
+  pthread_create(&t1, NULL, *thread, NULL);
+  pthread_join(t1, NULL);
+  pthread_create(&t2, NULL, *thread, NULL);
+  pthread_join(t2, NULL);
+
+  printf("state_var is %d\n", state_var);
+  return 0;
+}
Index: lldb/test/API/tools/lldb-vscode/correct-thread/TestVSCode_correct_thread.py
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/correct-thread/TestVSCode_correct_thread.py
@@ -0,0 +1,47 @@
+"""
+Test lldb-vscode setBreakpoints request
+"""
+
+from __future__ import print_function
+
+import unittest2
+import vscode
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import lldbvscode_testcase
+
+
+class TestVSCode_correct_thread(lldbvscode_testcase.VSCodeTestCaseBase):
+
+mydir = TestBase.compute_mydir(__file__)
+
+@skipIfWindows
+@skipIfRemote
+def test_correct_thread(self):
+'''
+Tests that the correct thread is selected if we continue from
+a thread that goes away and hit a breakpoint in another thread.
+In this case, the selected thread should be the thread that
+just hit the breakpoint, and not the first thread in the list.
+'''
+program = self.getBuildArtifact("a.out")
+self.build_and_launch(program)
+source = 'main.c'
+breakpoint_line = line_number(source, '// break here')
+lines = [breakpoint_line]
+# Set breakpoint in the thread function
+breakpoint_ids = self.set_source_breakpoints(source, lines)
+self.assertEqual(len(breakpoint_ids), len(lines),
+"expect correct number of breakpoints")
+self.continue_to_breakpoints(breakpoint_ids)
+# We're now stopped at the breakpoint in the first thread, thread #2.
+# Continue to join the first thread and hit the breakpoint in the
+# second thread, thread #3.
+self.vscode.request_continue()
+stopped_event = 

[Lldb-commits] [PATCH] D109937: [lldb] Handle malformed qfThreadInfo reply

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.
ted added a reviewer: clayborg.
ted requested review of this revision.
Herald added a project: LLDB.

If the remote gdbserver's qfThreadInfo reply has a trailing comma,
GDBRemoteCommunicationClient::GetCurrentProcessAndThreadIDs will return
an empty vector of thread ids. This will cause lldb to recurse through
three functions trying to get the list of threads, until it blows its
stack and crashes.

A trailing comma is a malformed response, but it shouldn't cause lldb to
crash. This patch will return the tids received before the malformed
response.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109937

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp


Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -2906,8 +2906,13 @@
   if (ch == 'm') {
 do {
   auto pid_tid = response.GetPidTid(LLDB_INVALID_PROCESS_ID);
-  if (!pid_tid)
-return {};
+  if (!pid_tid) {
+// if ids is empty, this is an error
+if (ids.size() == 0)
+  return {};
+// if ids is not empty, bail out from here and process ids
+break;
+  }
 
   ids.push_back(pid_tid.getValue());
   ch = response.GetChar(); // Skip the command separator


Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -2906,8 +2906,13 @@
   if (ch == 'm') {
 do {
   auto pid_tid = response.GetPidTid(LLDB_INVALID_PROCESS_ID);
-  if (!pid_tid)
-return {};
+  if (!pid_tid) {
+// if ids is empty, this is an error
+if (ids.size() == 0)
+  return {};
+// if ids is not empty, bail out from here and process ids
+break;
+  }
 
   ids.push_back(pid_tid.getValue());
   ch = response.GetChar(); // Skip the command separator
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D109937: [lldb] Handle malformed qfThreadInfo reply

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

I agree about the test. I'll work on one.




Comment at: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2911-2912
+// if ids is empty, this is an error
+if (ids.size() == 0)
+  return {};
+// if ids is not empty, bail out from here and process ids

labath wrote:
> Isn't it still possible to get the infinite recursion you mentioned, if the 
> response consists only of a single comma ?
Yes. I was wondering if the correct response to this case is to drop down and 
go through the "pid=tid=1" case from line 2931. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109937

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

[Lldb-commits] [PATCH] D109937: [lldb] Handle malformed qfThreadInfo reply

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

I've got a test written. It doesn't crash like the debugger in the wild does, 
but it does give a tid of 0 for each thread I ask about. So I can assert if the 
threads don't have the correct tid.

With the patch, the test passes (gets the tids correctly)
Without the patch and asserts enabled, the test causes an assert in 
llvm::Optional
WIthout the patch and asserts disabled, the test fails when it checks the tid.

Which leads me to 1 more question - what should I do when we have a malformed 
response, and there are no threads listed? I'm leaning towards just falling 
through to the if at line 2931, and returning a pid and tid of 1. That's what 
we do if there's and empty response.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109937

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

[Lldb-commits] [PATCH] D109937: [lldb] Handle malformed qfThreadInfo reply

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 374049.
ted added a comment.

[lldb] Handle malformed qfThreadInfo reply

Add requested test.
Refine handling of malformed reply with no valid threads.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109937

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py


Index: 
lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
===
--- /dev/null
+++ 
lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
@@ -0,0 +1,29 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestThreadInfoTrailingComma(GDBRemoteTestBase):
+
+def test(self):
+class MyResponder(MockGDBServerResponder):
+def haltReason(self):
+return "T02thread:1"
+
+def qfThreadInfo(self):
+return "m1,2,3,4,"
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+  self.runCmd("log enable gdb-remote packets")
+  self.addTearDownHook(
+lambda: self.runCmd("log disable gdb-remote packets"))
+process = self.connect(target)
+print(process.GetThreadAtIndex(0).GetFrameAtIndex(0))
+
print(process.GetThreadAtIndex(0).GetFrameAtIndex(0).FindRegister("pc"))
+self.assertEqual(process.GetThreadAtIndex(0).GetThreadID(), 1)
+self.assertEqual(process.GetThreadAtIndex(1).GetThreadID(), 2)
+self.assertEqual(process.GetThreadAtIndex(2).GetThreadID(), 3)
+self.assertEqual(process.GetThreadAtIndex(3).GetThreadID(), 4)
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -2906,8 +2906,12 @@
   if (ch == 'm') {
 do {
   auto pid_tid = response.GetPidTid(LLDB_INVALID_PROCESS_ID);
+  // If we get an invalid response, break out of the loop.
+  // If there are valid tids, they have been added to ids.
+  // If there are no valid tids, we'll fall through to the
+  // bare-iron target handling below.
   if (!pid_tid)
-return {};
+break;
 
   ids.push_back(pid_tid.getValue());
   ch = response.GetChar(); // Skip the command separator


Index: lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
===
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
@@ -0,0 +1,29 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestThreadInfoTrailingComma(GDBRemoteTestBase):
+
+def test(self):
+class MyResponder(MockGDBServerResponder):
+def haltReason(self):
+return "T02thread:1"
+
+def qfThreadInfo(self):
+return "m1,2,3,4,"
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+  self.runCmd("log enable gdb-remote packets")
+  self.addTearDownHook(
+lambda: self.runCmd("log disable gdb-remote packets"))
+process = self.connect(target)
+print(process.GetThreadAtIndex(0).GetFrameAtIndex(0))
+print(process.GetThreadAtIndex(0).GetFrameAtIndex(0).FindRegister("pc"))
+self.assertEqual(process.GetThreadAtIndex(0).GetThreadID(), 1)
+self.assertEqual(process.GetThreadAtIndex(1).GetThreadID(), 2)
+self.assertEqual(process.GetThreadAtIndex(2).GetThreadID(), 3)
+self.assertEqual(process.GetThreadAtIndex(3).GetThreadID(), 4)
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -2906,8 +2906,12 @@
   if (ch == 'm') {
 do {
   auto pid_tid = response.GetPidTid(LLDB_INVALID_PROCESS_ID);
+  // If we get an invalid response, break out of the loop.
+  // If there are valid tids, they have been added to ids.
+  // If there are no valid tids, we'll fall through to the
+  // bare-iron target handling below.
   if (!pid_tid)
-return {};
+break;
 
   ids.push_back(pid

[Lldb-commits] [PATCH] D109937: [lldb] Handle malformed qfThreadInfo reply

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 374233.
ted added a comment.

Remove unneeded prints from test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109937

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py


Index: 
lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
===
--- /dev/null
+++ 
lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
@@ -0,0 +1,27 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestThreadInfoTrailingComma(GDBRemoteTestBase):
+
+def test(self):
+class MyResponder(MockGDBServerResponder):
+def haltReason(self):
+return "T02thread:1"
+
+def qfThreadInfo(self):
+return "m1,2,3,4,"
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+  self.runCmd("log enable gdb-remote packets")
+  self.addTearDownHook(
+lambda: self.runCmd("log disable gdb-remote packets"))
+process = self.connect(target)
+self.assertEqual(process.GetThreadAtIndex(0).GetThreadID(), 1)
+self.assertEqual(process.GetThreadAtIndex(1).GetThreadID(), 2)
+self.assertEqual(process.GetThreadAtIndex(2).GetThreadID(), 3)
+self.assertEqual(process.GetThreadAtIndex(3).GetThreadID(), 4)
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -2906,8 +2906,12 @@
   if (ch == 'm') {
 do {
   auto pid_tid = response.GetPidTid(LLDB_INVALID_PROCESS_ID);
+  // If we get an invalid response, break out of the loop.
+  // If there are valid tids, they have been added to ids.
+  // If there are no valid tids, we'll fall through to the
+  // bare-iron target handling below.
   if (!pid_tid)
-return {};
+break;
 
   ids.push_back(pid_tid.getValue());
   ch = response.GetChar(); // Skip the command separator


Index: lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
===
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
@@ -0,0 +1,27 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestThreadInfoTrailingComma(GDBRemoteTestBase):
+
+def test(self):
+class MyResponder(MockGDBServerResponder):
+def haltReason(self):
+return "T02thread:1"
+
+def qfThreadInfo(self):
+return "m1,2,3,4,"
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+  self.runCmd("log enable gdb-remote packets")
+  self.addTearDownHook(
+lambda: self.runCmd("log disable gdb-remote packets"))
+process = self.connect(target)
+self.assertEqual(process.GetThreadAtIndex(0).GetThreadID(), 1)
+self.assertEqual(process.GetThreadAtIndex(1).GetThreadID(), 2)
+self.assertEqual(process.GetThreadAtIndex(2).GetThreadID(), 3)
+self.assertEqual(process.GetThreadAtIndex(3).GetThreadID(), 4)
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -2906,8 +2906,12 @@
   if (ch == 'm') {
 do {
   auto pid_tid = response.GetPidTid(LLDB_INVALID_PROCESS_ID);
+  // If we get an invalid response, break out of the loop.
+  // If there are valid tids, they have been added to ids.
+  // If there are no valid tids, we'll fall through to the
+  // bare-iron target handling below.
   if (!pid_tid)
-return {};
+break;
 
   ids.push_back(pid_tid.getValue());
   ch = response.GetChar(); // Skip the command separator
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D109937: [lldb] Handle malformed qfThreadInfo reply

[Ted Woodward via Phabricator via lldb-commits]

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

Prints were there for my debugging. They don't serve a purpose, so I've removed 
them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109937

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

[Lldb-commits] [PATCH] D109937: [lldb] Handle malformed qfThreadInfo reply

[Ted Woodward via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG953ddded1aa2: [lldb] Handle malformed qfThreadInfo reply 
(authored by ted).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109937

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py


Index: 
lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
===
--- /dev/null
+++ 
lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
@@ -0,0 +1,27 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestThreadInfoTrailingComma(GDBRemoteTestBase):
+
+def test(self):
+class MyResponder(MockGDBServerResponder):
+def haltReason(self):
+return "T02thread:1"
+
+def qfThreadInfo(self):
+return "m1,2,3,4,"
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+  self.runCmd("log enable gdb-remote packets")
+  self.addTearDownHook(
+lambda: self.runCmd("log disable gdb-remote packets"))
+process = self.connect(target)
+self.assertEqual(process.GetThreadAtIndex(0).GetThreadID(), 1)
+self.assertEqual(process.GetThreadAtIndex(1).GetThreadID(), 2)
+self.assertEqual(process.GetThreadAtIndex(2).GetThreadID(), 3)
+self.assertEqual(process.GetThreadAtIndex(3).GetThreadID(), 4)
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -2908,8 +2908,12 @@
   if (ch == 'm') {
 do {
   auto pid_tid = response.GetPidTid(LLDB_INVALID_PROCESS_ID);
+  // If we get an invalid response, break out of the loop.
+  // If there are valid tids, they have been added to ids.
+  // If there are no valid tids, we'll fall through to the
+  // bare-iron target handling below.
   if (!pid_tid)
-return {};
+break;
 
   ids.push_back(pid_tid.getValue());
   ch = response.GetChar(); // Skip the command separator


Index: lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
===
--- /dev/null
+++ lldb/test/API/functionalities/gdb_remote_client/TestThreadInfoTrailingComma.py
@@ -0,0 +1,27 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from gdbclientutils import *
+
+
+class TestThreadInfoTrailingComma(GDBRemoteTestBase):
+
+def test(self):
+class MyResponder(MockGDBServerResponder):
+def haltReason(self):
+return "T02thread:1"
+
+def qfThreadInfo(self):
+return "m1,2,3,4,"
+
+self.server.responder = MyResponder()
+target = self.dbg.CreateTarget('')
+if self.TraceOn():
+  self.runCmd("log enable gdb-remote packets")
+  self.addTearDownHook(
+lambda: self.runCmd("log disable gdb-remote packets"))
+process = self.connect(target)
+self.assertEqual(process.GetThreadAtIndex(0).GetThreadID(), 1)
+self.assertEqual(process.GetThreadAtIndex(1).GetThreadID(), 2)
+self.assertEqual(process.GetThreadAtIndex(2).GetThreadID(), 3)
+self.assertEqual(process.GetThreadAtIndex(3).GetThreadID(), 4)
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -2908,8 +2908,12 @@
   if (ch == 'm') {
 do {
   auto pid_tid = response.GetPidTid(LLDB_INVALID_PROCESS_ID);
+  // If we get an invalid response, break out of the loop.
+  // If there are valid tids, they have been added to ids.
+  // If there are no valid tids, we'll fall through to the
+  // bare-iron target handling below.
   if (!pid_tid)
-return {};
+break;
 
   ids.push_back(pid_tid.getValue());
   ch = response.GetChar(); // Skip the command separator
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D111131: [LLDB] Round XML register bitsize to byte boundary

[Ted Woodward via Phabricator via lldb-commits]

ted added inline comments.



Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4357
 
 assert(reg_info.byte_size != 0);
 registers.push_back(reg_info);

labath wrote:
> omjavaid wrote:
> > labath wrote:
> > > mgorny wrote:
> > > > omjavaid wrote:
> > > > > mgorny wrote:
> > > > > > omjavaid wrote:
> > > > > > > @mgorny the assert already exists but then we also want to allow 
> > > > > > > bit sized registers although they ll be viewed as byte length for 
> > > > > > > now.
> > > > > > Ah, right. I suppose you could skip zero-byte registers though. 
> > > > > > That should amend the assert with better release behavior.
> > > > > on a second thought, I dont see a zero sized register being sent by 
> > > > > stub as a big enough reason to abort the whole debug session unless 
> > > > > its one of GPRs. May be we skip the assert altogether and replace it 
> > > > > with an error message. 
> > > > > What do you think?
> > > > Yes, you are correct. Probably `LLDB_LOG` would go in line with how we 
> > > > handle these things.
> > > Yeah, I don't think crashing is a good response to the stub sending us 
> > > nonsensical register definitions. Though that seems like a separate 
> > > issue..
> > LLDB_LOG will hide message from user unless log is enabled. I think user 
> > must be notified that register is  zero sized and thats why you wont be 
> > able to see it in register read. Similar to the way we notify user about 
> > unhandled register attibutes like "save-restore".
> BTW, I came very close to deleting that printf when I was touching this code 
> last month.
> FWIW, my hierarchy is:
> user-facing warning (though I don't know how would that be implemented here) 
> >> log entry >> nothing >> raw printf
+1 on not crashing. The remote sending us garbage shouldn't cause us to crash - 
we should try to make sense of the garbage, and if we can't, error out 
gracefully. Better to print an error and ignore the register than crash or 
abort the session.


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

https://reviews.llvm.org/D31

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

[Lldb-commits] [PATCH] D111136: [lldb] [DynamicRegisterInfo] Support iterating over Registers()

[Ted Woodward via Phabricator via lldb-commits]

ted added inline comments.



Comment at: lldb/include/lldb/Target/DynamicRegisterInfo.h:82
+  llvm::iterator_range Registers() {
+return llvm::iterator_range(m_regs);
+  }

mgorny wrote:
> labath wrote:
> > Could this return const iterators? It seems we already have some non-const 
> > accessors, but I'd rather not propagate that..
> It can't — we're using these iterators to augment register infos ;-).
Maybe make a non-const protected, and a const public, so random plugin #37 
can't modify register info?


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

https://reviews.llvm.org/D36

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

[Lldb-commits] [PATCH] D109738: [lldb] Fix bug 38317 - Address breakpoints don't work if set before the process launches

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D109738#3052624 , @jingham wrote:

> That is clearly wrong: an address is never enough to restore a breakpoint 
> from one session to the next even if you can turn ASLR off.  After all, the 
> breakpoint could be in a dlopened library.

This is usually true.

In cases where you're debugging an embedded application that doesn't involve 
shared libraries, it's not. The main area where I see this is when debugging 
application startup code. My coworker who's responsible for that is the one who 
reported the problem originally. In that case, the address of the breakpoint is 
always valid on subsequent runs.

On the other hand, when you're running under our RTOS, the application is a 
PIE, and we support shared libraries, so the address is never valid on 
subsequent runs.

Maybe we need a flag to "break set" that means "this address is always valid; 
don't garbage collect it".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109738

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

[Lldb-commits] [PATCH] D112834: [lldb-vscode] Fix coredump load source mapping for first file

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.
ted requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

SetSourceMapFromArguments is called after the core is loaded. This means
that the source file for the crashing code won't have the source map applied.
Move the call to SetSourceMapFromArguments in request_attach to just after
the call to RunInitCommands, matching request_launch behavior.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112834

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/test/API/tools/lldb-vscode/coreFile/TestVSCode_coreFile.py
  lldb/test/API/tools/lldb-vscode/coreFile/main.c
  lldb/tools/lldb-vscode/lldb-vscode.cpp


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -616,6 +616,8 @@
   // Run any initialize LLDB commands the user specified in the launch.json
   g_vsc.RunInitCommands();
 
+  SetSourceMapFromArguments(*arguments);
+
   lldb::SBError status;
   g_vsc.SetTarget(g_vsc.CreateTargetFromArguments(*arguments, status));
   if (status.Fail()) {
@@ -657,8 +659,6 @@
 g_vsc.target = g_vsc.debugger.GetSelectedTarget();
   }
 
-  SetSourceMapFromArguments(*arguments);
-
   if (error.Success() && core_file.empty()) {
 auto attached_pid = g_vsc.target.GetProcess().GetProcessID();
 if (attached_pid == LLDB_INVALID_PROCESS_ID) {
Index: lldb/test/API/tools/lldb-vscode/coreFile/main.c
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/coreFile/main.c
@@ -0,0 +1 @@
+/* Fake source file for core dump source mapping test */
Index: lldb/test/API/tools/lldb-vscode/coreFile/TestVSCode_coreFile.py
===
--- lldb/test/API/tools/lldb-vscode/coreFile/TestVSCode_coreFile.py
+++ lldb/test/API/tools/lldb-vscode/coreFile/TestVSCode_coreFile.py
@@ -41,3 +41,18 @@
 
 self.vscode.request_next(threadId=32259)
 self.assertEquals(self.get_stackFrames(), expected_frames)
+
+@skipIfWindows
+@skipIfRemote
+def test_core_file_source_mapping(self):
+''' Test that sourceMap property is correctly applied when loading a 
core '''
+current_dir = os.path.dirname(os.path.realpath(__file__))
+exe_file = os.path.join(current_dir, "linux-x86_64.out")
+core_file = os.path.join(current_dir, "linux-x86_64.core")
+
+self.create_debug_adaptor()
+
+source_map = [["/home/labath/test", current_dir]]
+self.attach(exe_file, coreFile=core_file, sourceMap=source_map)
+
+self.assertTrue(current_dir in 
self.get_stackFrames()[0]['source']['path'])
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
@@ -506,7 +506,8 @@
initCommands=None, preRunCommands=None,
stopCommands=None, exitCommands=None,
attachCommands=None, terminateCommands=None,
-   coreFile=None, postRunCommands=None):
+   coreFile=None, postRunCommands=None,
+   sourceMap=None):
 args_dict = {}
 if pid is not None:
 args_dict['pid'] = pid
@@ -533,6 +534,8 @@
 args_dict['coreFile'] = coreFile
 if postRunCommands:
 args_dict['postRunCommands'] = postRunCommands
+if sourceMap:
+args_dict['sourceMap'] = sourceMap
 command_dict = {
 'command': 'attach',
 'type': 'request',
Index: 
lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
@@ -251,7 +251,7 @@
initCommands=None, preRunCommands=None, stopCommands=None,
exitCommands=None, attachCommands=None, coreFile=None,
disconnectAutomatically=True, terminateCommands=None,
-   postRunCommands=None):
+   postRunCommands=None, sourceMap=None):
 '''Build the default Makefile target, create the VSCode debug adaptor,
and attach to the process.
 '''
@@ -271,7 +271,8 @@
 initCommands=initCommands, preRunCommands=preRunCommands,
 stopCommands=stopCommands, exitCommands=exitCommands,
 attachCommands=attachCommands, terminateCommands=terminateCommands,
-  

[Lldb-commits] [PATCH] D112931: Fix mixed disassembly showing source lines for "line 0"

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.
ted requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

"line 0" in a DWARF linetable means something that doesn't have associated
source. The code for mixed disassembly has a comment indicating that
"line 0" should be skipped, but the wrong value was returned. Fix the return
value and add a test to check that we don't incorrectly show source lines
from the beginning of the file.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112931

Files:
  lldb/source/Core/Disassembler.cpp
  lldb/test/Shell/Commands/command-disassemble-mixed.c


Index: lldb/test/Shell/Commands/command-disassemble-mixed.c
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-disassemble-mixed.c
@@ -0,0 +1,18 @@
+// invalid mixed disassembly line
+
+// RUN: %clang -g %s -o %t
+// RUN: %lldb %t -o "dis -m -n main" | FileCheck %s
+
+// CHECK: int main
+// CHECK: int i
+// CHECK-NOT: invalid mixed disassembly line
+// CHECK: return 0;
+
+int main(int argc, char **argv)
+{
+  int i;
+
+  for (i=0; i < 10; ++i) ;
+
+  return 0;
+}
Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -243,7 +243,7 @@
 
   // Skip any line #0 entries - they are implementation details
   if (line.line == 0)
-return false;
+return true;
 
   ThreadSP thread_sp = exe_ctx.GetThreadSP();
   if (thread_sp) {


Index: lldb/test/Shell/Commands/command-disassemble-mixed.c
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-disassemble-mixed.c
@@ -0,0 +1,18 @@
+// invalid mixed disassembly line
+
+// RUN: %clang -g %s -o %t
+// RUN: %lldb %t -o "dis -m -n main" | FileCheck %s
+
+// CHECK: int main
+// CHECK: int i
+// CHECK-NOT: invalid mixed disassembly line
+// CHECK: return 0;
+
+int main(int argc, char **argv)
+{
+  int i;
+
+  for (i=0; i < 10; ++i) ;
+
+  return 0;
+}
Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -243,7 +243,7 @@
 
   // Skip any line #0 entries - they are implementation details
   if (line.line == 0)
-return false;
+return true;
 
   ThreadSP thread_sp = exe_ctx.GetThreadSP();
   if (thread_sp) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112931: Fix mixed disassembly showing source lines for "line 0"

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 383810.
ted added a comment.

Fix test to exit lldb


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112931

Files:
  lldb/source/Core/Disassembler.cpp
  lldb/test/Shell/Commands/command-disassemble-mixed.c


Index: lldb/test/Shell/Commands/command-disassemble-mixed.c
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-disassemble-mixed.c
@@ -0,0 +1,18 @@
+// invalid mixed disassembly line
+
+// RUN: %clang -g %s -o %t
+// RUN: %lldb %t -o "dis -m -n main" -o "exit" | FileCheck %s
+
+// CHECK: int main
+// CHECK: int i
+// CHECK-NOT: invalid mixed disassembly line
+// CHECK: return 0;
+
+int main(int argc, char **argv)
+{
+  int i;
+
+  for (i=0; i < 10; ++i) ;
+
+  return 0;
+}
Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -243,7 +243,7 @@
 
   // Skip any line #0 entries - they are implementation details
   if (line.line == 0)
-return false;
+return true;
 
   ThreadSP thread_sp = exe_ctx.GetThreadSP();
   if (thread_sp) {


Index: lldb/test/Shell/Commands/command-disassemble-mixed.c
===
--- /dev/null
+++ lldb/test/Shell/Commands/command-disassemble-mixed.c
@@ -0,0 +1,18 @@
+// invalid mixed disassembly line
+
+// RUN: %clang -g %s -o %t
+// RUN: %lldb %t -o "dis -m -n main" -o "exit" | FileCheck %s
+
+// CHECK: int main
+// CHECK: int i
+// CHECK-NOT: invalid mixed disassembly line
+// CHECK: return 0;
+
+int main(int argc, char **argv)
+{
+  int i;
+
+  for (i=0; i < 10; ++i) ;
+
+  return 0;
+}
Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -243,7 +243,7 @@
 
   // Skip any line #0 entries - they are implementation details
   if (line.line == 0)
-return false;
+return true;
 
   ThreadSP thread_sp = exe_ctx.GetThreadSP();
   if (thread_sp) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D112834: [lldb-vscode] Fix coredump load source mapping for first file

[Ted Woodward via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1c05c52de217: [lldb-vscode] Fix coredump load source mapping 
for first file (authored by ted).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112834

Files:
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
  lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
  lldb/test/API/tools/lldb-vscode/coreFile/TestVSCode_coreFile.py
  lldb/test/API/tools/lldb-vscode/coreFile/main.c
  lldb/tools/lldb-vscode/lldb-vscode.cpp


Index: lldb/tools/lldb-vscode/lldb-vscode.cpp
===
--- lldb/tools/lldb-vscode/lldb-vscode.cpp
+++ lldb/tools/lldb-vscode/lldb-vscode.cpp
@@ -616,6 +616,8 @@
   // Run any initialize LLDB commands the user specified in the launch.json
   g_vsc.RunInitCommands();
 
+  SetSourceMapFromArguments(*arguments);
+
   lldb::SBError status;
   g_vsc.SetTarget(g_vsc.CreateTargetFromArguments(*arguments, status));
   if (status.Fail()) {
@@ -657,8 +659,6 @@
 g_vsc.target = g_vsc.debugger.GetSelectedTarget();
   }
 
-  SetSourceMapFromArguments(*arguments);
-
   if (error.Success() && core_file.empty()) {
 auto attached_pid = g_vsc.target.GetProcess().GetProcessID();
 if (attached_pid == LLDB_INVALID_PROCESS_ID) {
Index: lldb/test/API/tools/lldb-vscode/coreFile/main.c
===
--- /dev/null
+++ lldb/test/API/tools/lldb-vscode/coreFile/main.c
@@ -0,0 +1 @@
+/* Fake source file for core dump source mapping test */
Index: lldb/test/API/tools/lldb-vscode/coreFile/TestVSCode_coreFile.py
===
--- lldb/test/API/tools/lldb-vscode/coreFile/TestVSCode_coreFile.py
+++ lldb/test/API/tools/lldb-vscode/coreFile/TestVSCode_coreFile.py
@@ -41,3 +41,18 @@
 
 self.vscode.request_next(threadId=32259)
 self.assertEquals(self.get_stackFrames(), expected_frames)
+
+@skipIfWindows
+@skipIfRemote
+def test_core_file_source_mapping(self):
+''' Test that sourceMap property is correctly applied when loading a 
core '''
+current_dir = os.path.dirname(os.path.realpath(__file__))
+exe_file = os.path.join(current_dir, "linux-x86_64.out")
+core_file = os.path.join(current_dir, "linux-x86_64.core")
+
+self.create_debug_adaptor()
+
+source_map = [["/home/labath/test", current_dir]]
+self.attach(exe_file, coreFile=core_file, sourceMap=source_map)
+
+self.assertTrue(current_dir in 
self.get_stackFrames()[0]['source']['path'])
Index: lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/vscode.py
@@ -506,7 +506,8 @@
initCommands=None, preRunCommands=None,
stopCommands=None, exitCommands=None,
attachCommands=None, terminateCommands=None,
-   coreFile=None, postRunCommands=None):
+   coreFile=None, postRunCommands=None,
+   sourceMap=None):
 args_dict = {}
 if pid is not None:
 args_dict['pid'] = pid
@@ -533,6 +534,8 @@
 args_dict['coreFile'] = coreFile
 if postRunCommands:
 args_dict['postRunCommands'] = postRunCommands
+if sourceMap:
+args_dict['sourceMap'] = sourceMap
 command_dict = {
 'command': 'attach',
 'type': 'request',
Index: 
lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
===
--- lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
+++ lldb/packages/Python/lldbsuite/test/tools/lldb-vscode/lldbvscode_testcase.py
@@ -251,7 +251,7 @@
initCommands=None, preRunCommands=None, stopCommands=None,
exitCommands=None, attachCommands=None, coreFile=None,
disconnectAutomatically=True, terminateCommands=None,
-   postRunCommands=None):
+   postRunCommands=None, sourceMap=None):
 '''Build the default Makefile target, create the VSCode debug adaptor,
and attach to the process.
 '''
@@ -271,7 +271,8 @@
 initCommands=initCommands, preRunCommands=preRunCommands,
 stopCommands=stopCommands, exitCommands=exitCommands,
 attachCommands=attachCommands, terminateCommands=terminateCommands,
-coreFile=coreFile, postRunCommands=postRunCommands)
+coreFile=coreFile, postRunCommands=postRunCommands,
+sourceMap=sourceMap)
 if not (re

[Lldb-commits] [PATCH] D112931: Fix mixed disassembly showing source lines for "line 0"

[Ted Woodward via Phabricator via lldb-commits]

ted added inline comments.



Comment at: lldb/test/Shell/Commands/command-disassemble-mixed.c:11-18
+int main(int argc, char **argv)
+{
+  int i;
+
+  for (i=0; i < 10; ++i) ;
+
+  return 0;

clayborg wrote:
> are we guaranteed to get some debug info with line zero in it with this 
> example?
I tried this with x86 Linux clang 12.0 and our internal Hexagon clang; both 
gave me a line zero.

I ran this test on Ubuntu 18 using x86 Linux clang 12.0 and the test failed 
without the change and passed with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112931

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

[Lldb-commits] [PATCH] D112931: Fix mixed disassembly showing source lines for "line 0"

[Ted Woodward via Phabricator via lldb-commits]

ted added inline comments.



Comment at: lldb/test/Shell/Commands/command-disassemble-mixed.c:11-18
+int main(int argc, char **argv)
+{
+  int i;
+
+  for (i=0; i < 10; ++i) ;
+
+  return 0;

labath wrote:
> ted wrote:
> > clayborg wrote:
> > > are we guaranteed to get some debug info with line zero in it with this 
> > > example?
> > I tried this with x86 Linux clang 12.0 and our internal Hexagon clang; both 
> > gave me a line zero.
> > 
> > I ran this test on Ubuntu 18 using x86 Linux clang 12.0 and the test failed 
> > without the change and passed with it.
> Nonetheless, it would be better to test this via a .s file with some explicit 
> .line directives. That way you could check for the actual command output 
> instead of the absence of some string.
> 
> (Negative tests like this are very brittle, as they can be rendered 
> ineffective by a change in the capitalization of the error message.)
The test is specifically "the first line of the source file is not displayed 
when we do dis -m". The only way I can think of testing this is a negative 
test.  If anyone can think of a better way, I'm happy to do that.
Fortunately, the message we're testing against is contained in the test, so it 
avoids the brittleness of changes to text in the tool.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112931

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

[Lldb-commits] [PATCH] D112931: Fix mixed disassembly showing source lines for "line 0"

[Ted Woodward via Phabricator via lldb-commits]

ted added inline comments.



Comment at: lldb/test/Shell/Commands/command-disassemble-mixed.c:11-18
+int main(int argc, char **argv)
+{
+  int i;
+
+  for (i=0; i < 10; ++i) ;
+
+  return 0;

labath wrote:
> ted wrote:
> > labath wrote:
> > > ted wrote:
> > > > clayborg wrote:
> > > > > are we guaranteed to get some debug info with line zero in it with 
> > > > > this example?
> > > > I tried this with x86 Linux clang 12.0 and our internal Hexagon clang; 
> > > > both gave me a line zero.
> > > > 
> > > > I ran this test on Ubuntu 18 using x86 Linux clang 12.0 and the test 
> > > > failed without the change and passed with it.
> > > Nonetheless, it would be better to test this via a .s file with some 
> > > explicit .line directives. That way you could check for the actual 
> > > command output instead of the absence of some string.
> > > 
> > > (Negative tests like this are very brittle, as they can be rendered 
> > > ineffective by a change in the capitalization of the error message.)
> > The test is specifically "the first line of the source file is not 
> > displayed when we do dis -m". The only way I can think of testing this is a 
> > negative test.  If anyone can think of a better way, I'm happy to do that.
> > Fortunately, the message we're testing against is contained in the test, so 
> > it avoids the brittleness of changes to text in the tool.
> I missed the fact that this is a string coming from the test. That makes it 
> better indeed, though I'd still recommend a .s file, as that also removes the 
> dependence on the compiler producing a particular debug_line sequence.
> 
> Regarding the negative test, that's easy to avoid with a .s file, as you can 
> assert exact command output. Something like this ought to do it (the test 
> input should be alright, the assertions are made up).
> ```
> # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o %t
> # RUN: lldb %t -o "disassemble -m -n main" | FileCheck %s
> 
> # CHECK: disassemble -m -n main
> # CHECK-NEXT: first line of disassembly
> # CHECK-NEXT: second line of disassembly
> # CHECK-NEXT: etc.
> 
> .text
> .globl  main
> .type   main,@function
> main:
> .file   1 "" "mytest.s"
> .loc1 10
> nop
> .loc1 0
> nop
> .loc1 20
> nop
> .Lfunc_end0:
> .size   main, .Lfunc_end0-main
> # -- End function
> .section.debug_abbrev,"",@progbits
> .byte   1   # Abbreviation Code
> .byte   17  # DW_TAG_compile_unit
> .byte   0   # DW_CHILDREN_no
> .byte   37  # DW_AT_producer
> .byte   8   # DW_FORM_string
> .byte   19  # DW_AT_language
> .byte   5   # DW_FORM_data2
> .byte   3   # DW_AT_name
> .byte   8   # DW_FORM_string
> .byte   16  # DW_AT_stmt_list
> .byte   23  # DW_FORM_sec_offset
> .byte   17  # DW_AT_low_pc
> .byte   1   # DW_FORM_addr
> .byte   18  # DW_AT_high_pc
> .byte   6   # DW_FORM_data4
> .byte   0   # EOM(1)
> .byte   0   # EOM(2)
> .byte   0   # EOM(3)
> .section.debug_info,"",@progbits
> .Lcu_begin0:
> .long   .Ldebug_info_end0-.Ldebug_info_start0 # Length of Unit
> .Ldebug_info_start0:
> .short  4   # DWARF version number
> .long   .debug_abbrev   # Offset Into Abbrev. Section
> .byte   8   # Address Size (in bytes)
> .byte   1   # Abbrev [1] 0xb:0x1f 
> DW_TAG_compile_unit
> .asciz  "Hand-written DWARF"# DW_AT_producer
> .short  12  # DW_AT_language
> .asciz  "mytest.s"  # DW_AT_name
> .long   .Lline_table_start0 # DW_AT_stmt_list
> .quad   main# DW_AT_low_pc
> .long   .Lfunc_end0-main# DW_AT_high_pc
> .Ldebug_info_end0:
> .section.debug_line,"",@progbits
> .Lline_table_start0:
> 
My issues with the above:

  - I want to test a c source file, not an assembly file.
  - This assumes the user's toolchain supports x86-64, which many downstream 
toolchains do not. I'd rather the test be core-agnostic, which is why I didn't 
check any actual disassembly.






Repository:
  rG LLVM Github

[Lldb-commits] [PATCH] D111209: Don't push null ExecutionContext on CommandInterpreter exectx stack

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

@jasonmolenda I discovered the same issue in another way - create a python 
command and load it with "command script import".  Here is test.py:

  def __lldb_init_module(debugger, dict):
  debugger.HandleCommand(
  'command script add -f test.test test')
  print("test command loaded")
  
  def test(debugger, register, result, dict):
  result.Clear()
  res = lldb.SBCommandReturnObject()
  command = 'target create /bin/ls'
  debugger.GetCommandInterpreter().HandleCommand(command, res)
  command = 'target modules search-paths add . ' + '/tmp'
  debugger.GetCommandInterpreter().HandleCommand(command, res)
  output = res.GetOutput()
  print(output)
  
  print('test done')

The 2nd HandleCommand uses the execution context of the original command, which 
doesn't have a target, so the search-paths add fails with no target.

(lldb) command script import test
test command loaded
(lldb) test
Current executable set to '/bin/ls' (x86_64).

test command called
error: error: invalid target, create a target using the 'target create' command

That should be fairly easy to turn into a Shell test, with "REQUIRES: python", 
because the error happens before the search-path is parsed, so it doesn't have 
to be valid.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111209

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

[Lldb-commits] [PATCH] D112931: Fix mixed disassembly showing source lines for "line 0"

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

@jasonmolenda friendly ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112931

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

[Lldb-commits] [PATCH] D106832: [lldb] [gdb-remote client] Avoid zero padding PID/TID in H packet

[Ted Woodward via Phabricator via lldb-commits]

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

LGTM. Thanks Michał!


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

https://reviews.llvm.org/D106832

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

[Lldb-commits] [PATCH] D72513: Don't fail step out if remote server doesn't implement qMemoryRegionInfo

[Ted Woodward via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6fd818c5a9c5: Don't fail step out if remote server 
doesn't implement qMemoryRegionInfo (authored by ted).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72513

Files:
  lldb/source/Target/ThreadPlanStepOut.cpp


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -130,11 +130,9 @@
 uint32_t permissions = 0;
 if (!m_thread.GetProcess()->GetLoadAddressPermissions(m_return_addr,
   permissions)) {
-  m_constructor_errors.Printf("Return address (0x%" PRIx64
-  ") permissions not found.",
-  m_return_addr);
-  LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast(this),
-m_constructor_errors.GetData());
+  LLDB_LOGF(log, "ThreadPlanStepOut(%p): Return address (0x%" PRIx64
+") permissions not found.", static_cast(this),
+m_return_addr);
 } else if (!(permissions & ePermissionsExecutable)) {
   m_constructor_errors.Printf("Return address (0x%" PRIx64
   ") did not point to executable memory.",


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -130,11 +130,9 @@
 uint32_t permissions = 0;
 if (!m_thread.GetProcess()->GetLoadAddressPermissions(m_return_addr,
   permissions)) {
-  m_constructor_errors.Printf("Return address (0x%" PRIx64
-  ") permissions not found.",
-  m_return_addr);
-  LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast(this),
-m_constructor_errors.GetData());
+  LLDB_LOGF(log, "ThreadPlanStepOut(%p): Return address (0x%" PRIx64
+") permissions not found.", static_cast(this),
+m_return_addr);
 } else if (!(permissions & ePermissionsExecutable)) {
   m_constructor_errors.Printf("Return address (0x%" PRIx64
   ") did not point to executable memory.",
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D84966: Remove special Hexagon packet traversal code

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.
ted added reviewers: clayborg, jingham, labath.
Herald added a project: LLDB.
ted requested review of this revision.
Herald added a subscriber: JDevlieghere.

On Hexagon, breakpoints need to be on the first instruction of a packet.
When the LLVM disassembler for Hexagon returned 32 bit instructions, we
needed code to find the start of the current packet. Now that the LLVM
disassembler for Hexagon returns packets instead of instructions, we always
have the first instruction of the packet. Remove the packet traversal code
because it can cause problems when the next packet has more than one
instruction.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84966

Files:
  lldb/include/lldb/Core/Disassembler.h
  lldb/source/Core/Disassembler.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/ThreadPlanStepRange.cpp

Index: lldb/source/Target/ThreadPlanStepRange.cpp
===
--- lldb/source/Target/ThreadPlanStepRange.cpp
+++ lldb/source/Target/ThreadPlanStepRange.cpp
@@ -327,10 +327,9 @@
   if (instructions == nullptr)
 return false;
   else {
-Target &target = GetThread().GetProcess()->GetTarget();
 const bool ignore_calls = GetKind() == eKindStepOverRange;
 uint32_t branch_index =
-instructions->GetIndexOfNextBranchInstruction(pc_index, target,
+instructions->GetIndexOfNextBranchInstruction(pc_index,
   ignore_calls, 
   &m_found_calls);
 
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -5948,7 +5948,7 @@
   }
 
   uint32_t branch_index =
-  insn_list->GetIndexOfNextBranchInstruction(insn_offset, target,
+  insn_list->GetIndexOfNextBranchInstruction(insn_offset,
  false /* ignore_calls*/,
  nullptr);
   if (branch_index == UINT32_MAX) {
Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -990,17 +990,15 @@
 
 uint32_t
 InstructionList::GetIndexOfNextBranchInstruction(uint32_t start,
- Target &target,
  bool ignore_calls,
  bool *found_calls) const {
   size_t num_instructions = m_instructions.size();
 
   uint32_t next_branch = UINT32_MAX;
-  size_t i;
   
   if (found_calls)
 *found_calls = false;
-  for (i = start; i < num_instructions; i++) {
+  for (size_t i = start; i < num_instructions; i++) {
 if (m_instructions[i]->DoesBranch()) {
   if (ignore_calls && m_instructions[i]->IsCall()) {
 if (found_calls)
@@ -1012,42 +1010,6 @@
 }
   }
 
-  // Hexagon needs the first instruction of the packet with the branch. Go
-  // backwards until we find an instruction marked end-of-packet, or until we
-  // hit start.
-  if (target.GetArchitecture().GetTriple().getArch() == llvm::Triple::hexagon) {
-// If we didn't find a branch, find the last packet start.
-if (next_branch == UINT32_MAX) {
-  i = num_instructions - 1;
-}
-
-while (i > start) {
-  --i;
-
-  Status error;
-  uint32_t inst_bytes;
-  bool prefer_file_cache = false; // Read from process if process is running
-  lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
-  target.ReadMemory(m_instructions[i]->GetAddress(), prefer_file_cache,
-&inst_bytes, sizeof(inst_bytes), error, &load_addr);
-  // If we have an error reading memory, return start
-  if (!error.Success())
-return start;
-  // check if this is the last instruction in a packet bits 15:14 will be
-  // 11b or 00b for a duplex
-  if (((inst_bytes & 0xC000) == 0xC000) ||
-  ((inst_bytes & 0xC000) == 0x)) {
-// instruction after this should be the start of next packet
-next_branch = i + 1;
-break;
-  }
-}
-
-if (next_branch == UINT32_MAX) {
-  // We couldn't find the previous packet, so return start
-  next_branch = start;
-}
-  }
   return next_branch;
 }
 
Index: lldb/include/lldb/Core/Disassembler.h
===
--- lldb/include/lldb/Core/Disassembler.h
+++ lldb/include/lldb/Core/Disassembler.h
@@ -279,9 +279,6 @@
   /// @param[in] start
   /// The instruction index of the first instruction to check.
   ///
-  /// @param[in] target
-  /// A LLDB target object that is used to resolve addresses.
-  ///
   /// @param[in] ignore_calls
   /// It true, then fine the first branch instruction that isn't
   /// a funct

[Lldb-commits] [PATCH] D84966: Remove special Hexagon packet traversal code

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 282640.
ted added a comment.

Fixed formatting flagged by Lint: Pre-merge checks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84966

Files:
  lldb/include/lldb/Core/Disassembler.h
  lldb/source/Core/Disassembler.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/ThreadPlanStepRange.cpp

Index: lldb/source/Target/ThreadPlanStepRange.cpp
===
--- lldb/source/Target/ThreadPlanStepRange.cpp
+++ lldb/source/Target/ThreadPlanStepRange.cpp
@@ -327,13 +327,9 @@
   if (instructions == nullptr)
 return false;
   else {
-Target &target = GetThread().GetProcess()->GetTarget();
 const bool ignore_calls = GetKind() == eKindStepOverRange;
-uint32_t branch_index =
-instructions->GetIndexOfNextBranchInstruction(pc_index, target,
-  ignore_calls, 
-  &m_found_calls);
-
+uint32_t branch_index = instructions->GetIndexOfNextBranchInstruction(
+pc_index, ignore_calls, &m_found_calls);
 Address run_to_address;
 
 // If we didn't find a branch, run to the end of the range.
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -5947,10 +5947,8 @@
 return retval;
   }
 
-  uint32_t branch_index =
-  insn_list->GetIndexOfNextBranchInstruction(insn_offset, target,
- false /* ignore_calls*/,
- nullptr);
+  uint32_t branch_index = insn_list->GetIndexOfNextBranchInstruction(
+  insn_offset, false /* ignore_calls*/, nullptr);
   if (branch_index == UINT32_MAX) {
 return retval;
   }
Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -990,17 +990,15 @@
 
 uint32_t
 InstructionList::GetIndexOfNextBranchInstruction(uint32_t start,
- Target &target,
  bool ignore_calls,
  bool *found_calls) const {
   size_t num_instructions = m_instructions.size();
 
   uint32_t next_branch = UINT32_MAX;
-  size_t i;
   
   if (found_calls)
 *found_calls = false;
-  for (i = start; i < num_instructions; i++) {
+  for (size_t i = start; i < num_instructions; i++) {
 if (m_instructions[i]->DoesBranch()) {
   if (ignore_calls && m_instructions[i]->IsCall()) {
 if (found_calls)
@@ -1012,42 +1010,6 @@
 }
   }
 
-  // Hexagon needs the first instruction of the packet with the branch. Go
-  // backwards until we find an instruction marked end-of-packet, or until we
-  // hit start.
-  if (target.GetArchitecture().GetTriple().getArch() == llvm::Triple::hexagon) {
-// If we didn't find a branch, find the last packet start.
-if (next_branch == UINT32_MAX) {
-  i = num_instructions - 1;
-}
-
-while (i > start) {
-  --i;
-
-  Status error;
-  uint32_t inst_bytes;
-  bool prefer_file_cache = false; // Read from process if process is running
-  lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
-  target.ReadMemory(m_instructions[i]->GetAddress(), prefer_file_cache,
-&inst_bytes, sizeof(inst_bytes), error, &load_addr);
-  // If we have an error reading memory, return start
-  if (!error.Success())
-return start;
-  // check if this is the last instruction in a packet bits 15:14 will be
-  // 11b or 00b for a duplex
-  if (((inst_bytes & 0xC000) == 0xC000) ||
-  ((inst_bytes & 0xC000) == 0x)) {
-// instruction after this should be the start of next packet
-next_branch = i + 1;
-break;
-  }
-}
-
-if (next_branch == UINT32_MAX) {
-  // We couldn't find the previous packet, so return start
-  next_branch = start;
-}
-  }
   return next_branch;
 }
 
Index: lldb/include/lldb/Core/Disassembler.h
===
--- lldb/include/lldb/Core/Disassembler.h
+++ lldb/include/lldb/Core/Disassembler.h
@@ -279,9 +279,6 @@
   /// @param[in] start
   /// The instruction index of the first instruction to check.
   ///
-  /// @param[in] target
-  /// A LLDB target object that is used to resolve addresses.
-  ///
   /// @param[in] ignore_calls
   /// It true, then fine the first branch instruction that isn't
   /// a function call (a branch that calls and returns to the next
@@ -298,7 +295,6 @@
   /// found.
   //--
   uint32_t GetIndexOfNe

[Lldb-commits] [PATCH] D84966: Remove special Hexagon packet traversal code

[Ted Woodward via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3169d920ccd1: Remove special Hexagon packet traversal code 
(authored by ted).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84966

Files:
  lldb/include/lldb/Core/Disassembler.h
  lldb/source/Core/Disassembler.cpp
  lldb/source/Target/Process.cpp
  lldb/source/Target/ThreadPlanStepRange.cpp

Index: lldb/source/Target/ThreadPlanStepRange.cpp
===
--- lldb/source/Target/ThreadPlanStepRange.cpp
+++ lldb/source/Target/ThreadPlanStepRange.cpp
@@ -327,13 +327,9 @@
   if (instructions == nullptr)
 return false;
   else {
-Target &target = GetThread().GetProcess()->GetTarget();
 const bool ignore_calls = GetKind() == eKindStepOverRange;
-uint32_t branch_index =
-instructions->GetIndexOfNextBranchInstruction(pc_index, target,
-  ignore_calls, 
-  &m_found_calls);
-
+uint32_t branch_index = instructions->GetIndexOfNextBranchInstruction(
+pc_index, ignore_calls, &m_found_calls);
 Address run_to_address;
 
 // If we didn't find a branch, run to the end of the range.
Index: lldb/source/Target/Process.cpp
===
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -5947,10 +5947,8 @@
 return retval;
   }
 
-  uint32_t branch_index =
-  insn_list->GetIndexOfNextBranchInstruction(insn_offset, target,
- false /* ignore_calls*/,
- nullptr);
+  uint32_t branch_index = insn_list->GetIndexOfNextBranchInstruction(
+  insn_offset, false /* ignore_calls*/, nullptr);
   if (branch_index == UINT32_MAX) {
 return retval;
   }
Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -990,17 +990,15 @@
 
 uint32_t
 InstructionList::GetIndexOfNextBranchInstruction(uint32_t start,
- Target &target,
  bool ignore_calls,
  bool *found_calls) const {
   size_t num_instructions = m_instructions.size();
 
   uint32_t next_branch = UINT32_MAX;
-  size_t i;
   
   if (found_calls)
 *found_calls = false;
-  for (i = start; i < num_instructions; i++) {
+  for (size_t i = start; i < num_instructions; i++) {
 if (m_instructions[i]->DoesBranch()) {
   if (ignore_calls && m_instructions[i]->IsCall()) {
 if (found_calls)
@@ -1012,42 +1010,6 @@
 }
   }
 
-  // Hexagon needs the first instruction of the packet with the branch. Go
-  // backwards until we find an instruction marked end-of-packet, or until we
-  // hit start.
-  if (target.GetArchitecture().GetTriple().getArch() == llvm::Triple::hexagon) {
-// If we didn't find a branch, find the last packet start.
-if (next_branch == UINT32_MAX) {
-  i = num_instructions - 1;
-}
-
-while (i > start) {
-  --i;
-
-  Status error;
-  uint32_t inst_bytes;
-  bool prefer_file_cache = false; // Read from process if process is running
-  lldb::addr_t load_addr = LLDB_INVALID_ADDRESS;
-  target.ReadMemory(m_instructions[i]->GetAddress(), prefer_file_cache,
-&inst_bytes, sizeof(inst_bytes), error, &load_addr);
-  // If we have an error reading memory, return start
-  if (!error.Success())
-return start;
-  // check if this is the last instruction in a packet bits 15:14 will be
-  // 11b or 00b for a duplex
-  if (((inst_bytes & 0xC000) == 0xC000) ||
-  ((inst_bytes & 0xC000) == 0x)) {
-// instruction after this should be the start of next packet
-next_branch = i + 1;
-break;
-  }
-}
-
-if (next_branch == UINT32_MAX) {
-  // We couldn't find the previous packet, so return start
-  next_branch = start;
-}
-  }
   return next_branch;
 }
 
Index: lldb/include/lldb/Core/Disassembler.h
===
--- lldb/include/lldb/Core/Disassembler.h
+++ lldb/include/lldb/Core/Disassembler.h
@@ -279,9 +279,6 @@
   /// @param[in] start
   /// The instruction index of the first instruction to check.
   ///
-  /// @param[in] target
-  /// A LLDB target object that is used to resolve addresses.
-  ///
   /// @param[in] ignore_calls
   /// It true, then fine the first branch instruction that isn't
   /// a function call (a branch that calls and returns to the next
@@ -298,7 +295,6 @@
  

[Lldb-commits] [PATCH] D78801: [LLDB] Add class ProcessWasm for WebAssembly debugging

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

Hi Paulo,
@clayborg asked me to look at this, because I've worked with systems that have 
multiple address spaces. I was thinking, instead of a WebAssembly specific 
memory read, we should implement an optional generic memory read and write with 
memory space support.

So instead of qWasmMem:frame_index;addr;len, we have something like 
qMemSpaceRead:addr;space;len and qMemSpaceWrite:addr;space;len;data .

"space" is stub dependent - it's just a number, and can mean different things 
for different targets. It could be different modules, like you talk about here, 
or different physical memory areas like in a Motorola 56K DSP, or different 
ways to get at the same memory (physical/virtual/cacheable, or directly 
controlling MESI bits instead of relying on TLB entries) like I implemented in 
FSLDBG.

If we do this, other targets that want to work with memory spaces can use them 
instead of having to implement their own extensions.

About the expression problem - the IR Interpreter doesn't handle some complex 
expressions. It needs work, but most targets use JIT. On Hexagon, we only 
recently enabled JIT in the compiler, and haven't done it in the debugger yet, 
so we use the IR Interpreter for everything. Unfortunately I haven't had time 
to dive into it to make it better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78801



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

[Lldb-commits] [PATCH] D69119: Modernize the rest of the Find.* API (NFC)

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69119



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

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

I've got another failure case for this. If the remote gdbserver doesn't 
implement qMemoryRegionInfo or qXfer:memory-map:read, thread step-out will fail.

error: Could not create return address breakpoint. Return address (0x5bc0) 
permissions not found.

That comes from this code:

  if (!m_thread.GetProcess()->GetLoadAddressPermissions(m_return_addr,
permissions)) {
m_constructor_errors.Printf("Return address (0x%" PRIx64
") permissions not found.",
m_return_addr);
LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast(this),
  m_constructor_errors.GetData());
return;

GetLoadAddressPermissions does this:

  Status error(GetMemoryRegionInfo(load_addr, range_info));
  if (!error.Success())
return false;

and ProcessGDBRemote::GetMemoryRegionInfo will send a qMemoryRegionInfo. If 
that fails, it will try qXfer::memory-map:read. qMemoryRegionInfo isn't 
required:

//--
// "qMemoryRegionInfo:"
//
// BRIEF
//  Get information about the address range that contains ""
//
// PRIORITY TO IMPLEMENT
//  Medium. This is nice to have, but it isn't necessary.

Many embedded stubs do not implement this, which means thread step-out won't 
work with them anymore.

How should we fix this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71372



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

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D71372#1811594 , @labath wrote:

> In D71372#1810687 , @ted wrote:
>
> > I've got another failure case for this. If the remote gdbserver doesn't 
> > implement qMemoryRegionInfo or qXfer:memory-map:read, thread step-out will 
> > fail.
> >  
>
>
> That's a good point Ted. I think we should give targets which don't support 
> fetching permissions the benefit of the doubt, and treat all memory as 
> potentially executable. Would removing the `return` statement from the 
> `if(!GetLoadAddressPermissions)` branch solve your problem? If so, can you 
> whip up a patch for that?


Removing the return statement fixes the issue. I'll put up a patch. Keeping the 
m_constructor_errors.Printf line doesn't cause a failure; it might be useful to 
keep that in case the breakpoint can't be created for other reasons. What do 
you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71372



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

[Lldb-commits] [PATCH] D72513: Don't fail step out if remote server doesn't implement qMemoryRegionInfo

[Ted Woodward via Phabricator via lldb-commits]

ted created this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
ted added reviewers: labath, jingham, clayborg.

The return address validation in D71372  will 
fail if the memory permissions can't be determined. Many embedded stubs either 
don't implement the qMemoryRegionInfo packet, or don't have memory permissions 
at all.

Remove the return from the if clause that calls GetLoadAddressPermissions, so 
this call failing doesn't cause the step out to abort. Instead, assume that the 
memory permission check doesn't apply to this type of target.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72513

Files:
  lldb/source/Target/ThreadPlanStepOut.cpp


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -135,7 +135,6 @@
   m_return_addr);
   LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast(this),
 m_constructor_errors.GetData());
-  return;
 } else if (!(permissions & ePermissionsExecutable)) {
   m_constructor_errors.Printf("Return address (0x%" PRIx64
   ") did not point to executable memory.",


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -135,7 +135,6 @@
   m_return_addr);
   LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast(this),
 m_constructor_errors.GetData());
-  return;
 } else if (!(permissions & ePermissionsExecutable)) {
   m_constructor_errors.Printf("Return address (0x%" PRIx64
   ") did not point to executable memory.",
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

In D71372#1813703 , @labath wrote:

> In D71372#1813142 , @ted wrote:
>
> > In D71372#1811594 , @labath wrote:
> >
> > > In D71372#1810687 , @ted wrote:
> > >
> > > > I've got another failure case for this. If the remote gdbserver doesn't 
> > > > implement qMemoryRegionInfo or qXfer:memory-map:read, thread step-out 
> > > > will fail.
> > > >  
> > >
> > >
> > > That's a good point Ted. I think we should give targets which don't 
> > > support fetching permissions the benefit of the doubt, and treat all 
> > > memory as potentially executable. Would removing the `return` statement 
> > > from the `if(!GetLoadAddressPermissions)` branch solve your problem? If 
> > > so, can you whip up a patch for that?
> >
> >
> > Removing the return statement fixes the issue. I'll put up a patch. Keeping 
> > the m_constructor_errors.Printf line doesn't cause a failure; it might be 
> > useful to keep that in case the breakpoint can't be created for other 
> > reasons. What do you think?
>
>
> I don't care much either way.. Since you have this kind of a target around, 
> you can judge whether printing this error/warning after each "finish" would 
> be useful or just annoying. Another possibility would be to don't print the 
> error to the command output, but still emit something into the log...


There's no message printed after each finish; I think the message will only be 
printed if there's another error. That's fine by me.

I've uploaded the patch in D72513 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71372



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

[Lldb-commits] [PATCH] D72513: Don't fail step out if remote server doesn't implement qMemoryRegionInfo

[Ted Woodward via Phabricator via lldb-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG04488c485a88: Don't fail step out if remote server 
doesn't implement qMemoryRegionInfo (authored by ted).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72513

Files:
  lldb/source/Target/ThreadPlanStepOut.cpp


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -135,7 +135,6 @@
   m_return_addr);
   LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast(this),
 m_constructor_errors.GetData());
-  return;
 } else if (!(permissions & ePermissionsExecutable)) {
   m_constructor_errors.Printf("Return address (0x%" PRIx64
   ") did not point to executable memory.",


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -135,7 +135,6 @@
   m_return_addr);
   LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast(this),
 m_constructor_errors.GetData());
-  return;
 } else if (!(permissions & ePermissionsExecutable)) {
   m_constructor_errors.Printf("Return address (0x%" PRIx64
   ") did not point to executable memory.",
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72513: Don't fail step out if remote server doesn't implement qMemoryRegionInfo

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

Wow @jingham you posted your comment and I landed the patch at the same time!

I'll remove the m_constructor_errors.Printf line as requested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72513



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

[Lldb-commits] [PATCH] D72513: Don't fail step out if remote server doesn't implement qMemoryRegionInfo

[Ted Woodward via Phabricator via lldb-commits]

ted updated this revision to Diff 240985.
ted added a comment.

Removed the code that sets m_constructor_errors when GetLoadAddressPermissions 
returns False, as requested by @jingham .


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

https://reviews.llvm.org/D72513

Files:
  lldb/source/Target/ThreadPlanStepOut.cpp


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -130,11 +130,9 @@
 uint32_t permissions = 0;
 if (!m_thread.GetProcess()->GetLoadAddressPermissions(m_return_addr,
   permissions)) {
-  m_constructor_errors.Printf("Return address (0x%" PRIx64
-  ") permissions not found.",
-  m_return_addr);
-  LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast(this),
-m_constructor_errors.GetData());
+  LLDB_LOGF(log, "ThreadPlanStepOut(%p): Return address (0x%" PRIx64
+") permissions not found.", static_cast(this),
+m_return_addr);
 } else if (!(permissions & ePermissionsExecutable)) {
   m_constructor_errors.Printf("Return address (0x%" PRIx64
   ") did not point to executable memory.",


Index: lldb/source/Target/ThreadPlanStepOut.cpp
===
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -130,11 +130,9 @@
 uint32_t permissions = 0;
 if (!m_thread.GetProcess()->GetLoadAddressPermissions(m_return_addr,
   permissions)) {
-  m_constructor_errors.Printf("Return address (0x%" PRIx64
-  ") permissions not found.",
-  m_return_addr);
-  LLDB_LOGF(log, "ThreadPlanStepOut(%p): %s", static_cast(this),
-m_constructor_errors.GetData());
+  LLDB_LOGF(log, "ThreadPlanStepOut(%p): Return address (0x%" PRIx64
+") permissions not found.", static_cast(this),
+m_return_addr);
 } else if (!(permissions & ePermissionsExecutable)) {
   m_constructor_errors.Printf("Return address (0x%" PRIx64
   ") did not point to executable memory.",
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D72513: Don't fail step out if remote server doesn't implement qMemoryRegionInfo

[Ted Woodward via Phabricator via lldb-commits]

ted added a comment.

@jingham friendly ping


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

https://reviews.llvm.org/D72513



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