[Lldb-commits] [PATCH] D41745: Handle O reply packets during qRcmd

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw created this revision.
owenpshaw added a reviewer: clayborg.

Gdb servers like openocd may send many $O reply packets for the client to 
output during a qRcmd command sequence.  Currently, lldb interprets the first O 
packet as an unexpected response.  Besides generating no output, this causes 
lldb to get out of sync with future commands because it continues reading O 
packets from the first command as response to subsequent commands.

This patch handles any O packets during an qRcmd, treating the first non-O 
packet as the true response.

Looking for guidance on how best to write test cases, haven't found anything 
currently testing qRcmd but may have just missed it.

Preliminary discussion at 
http://lists.llvm.org/pipermail/lldb-dev/2018-January/013078.html


https://reviews.llvm.org/D41745

Files:
  source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -5152,12 +5152,14 @@
   packet.PutCString("qRcmd,");
   packet.PutBytesAsRawHex8(command, strlen(command));
 
-  bool send_async = true;
   StringExtractorGDBRemote response;
-  process->GetGDBRemote().SendPacketAndWaitForResponse(
-  packet.GetString(), response, send_async);
-  result.SetStatus(eReturnStatusSuccessFinishResult);
   Stream &output_strm = result.GetOutputStream();
+  process->GetGDBRemote().SendPacketHandlingOutput(
+  packet.GetString(), response,
+  [&output_strm](llvm::StringRef output) {
+output_strm << output;
+  });
+  result.SetStatus(eReturnStatusSuccessFinishResult);
   output_strm.Printf("  packet: %s\n", packet.GetData());
   const std::string &response_str = response.GetStringRef();
 
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -232,6 +232,10 @@
   PacketResult ReadPacket(StringExtractorGDBRemote &response,
   Timeout timeout, bool sync_on_timeout);
 
+  PacketResult ReadPacketHandlingOutput(StringExtractorGDBRemote &response,
+  Timeout timeout, bool sync_on_timeout,
+  std::function output_callback);
+
   // Pop a packet from the queue in a thread safe manner
   PacketResult PopPacketFromQueue(StringExtractorGDBRemote &response,
   Timeout timeout);
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -274,6 +274,24 @@
   return result;
 }
 
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunication::ReadPacketHandlingOutput(
+StringExtractorGDBRemote &response,
+Timeout timeout,
+bool sync_on_timeout,
+std::function output_callback) {
+  auto result = ReadPacket(response, timeout, sync_on_timeout);
+  while (result == PacketResult::Success && response.IsNormalResponse() &&
+ response.PeekChar() == 'O') {
+response.GetChar();
+std::string output;
+if (response.GetHexByteString(output))
+  output_callback(output);
+result = ReadPacket(response, timeout, sync_on_timeout);
+  }
+  return result;
+}
+
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunication::ReadPacket(StringExtractorGDBRemote &response,
Timeout timeout,
Index: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
===
--- source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
@@ -48,6 +48,10 @@
 StringExtractorGDBRemote &response,
 bool send_async);
 
+  PacketResult SendPacketHandlingOutput(llvm::StringRef payload,
+  StringExtractorGDBRemote &response,
+  std::function output_callback);
+
   bool SendvContPacket(llvm::StringRef payload,
StringExtractorGDBRemote &response);
 
Index: source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteClientB

[Lldb-commits] [PATCH] D41745: Handle O reply packets during qRcmd

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw updated this revision to Diff 128991.
owenpshaw added a comment.

Updated patch with new function name suggested by @clayborg.  Added unit test 
and changed to llvm::function_ref as suggested by @labath.

Based on Greg's comments in the other thread, I've kept the new function 
separate, rather than use a flag in SendPacketAndWaitForResponse.

Is it worth creating a function to share the locking code that's common to 
SendPacketAndWaitForResponse and SendPacketAndReceiveResponseWithOutputSupport? 
 Alternatively, should the lock-related error logging be unique to distinguish 
between the two functions?


https://reviews.llvm.org/D41745

Files:
  source/Plugins/Process/gdb-remote/GDBRemoteClientBase.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp

Index: unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
===
--- unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
+++ unittests/Process/gdb-remote/GDBRemoteClientBaseTest.cpp
@@ -339,3 +339,25 @@
   ASSERT_TRUE(async_result.get());
   ASSERT_EQ(eStateInvalid, continue_state.get());
 }
+
+TEST_F(GDBRemoteClientBaseTest, SendPacketAndReceiveResponseWithOutputSupport) {
+  StringExtractorGDBRemote response;
+  StreamString command_output;
+
+  ASSERT_EQ(PacketResult::Success, server.SendPacket("O"));
+  ASSERT_EQ(PacketResult::Success, server.SendPacket("O48656c6c6f2c"));
+  ASSERT_EQ(PacketResult::Success, server.SendPacket("O20"));
+  ASSERT_EQ(PacketResult::Success, server.SendPacket("O"));
+  ASSERT_EQ(PacketResult::Success, server.SendPacket("O776f726c64"));
+  ASSERT_EQ(PacketResult::Success, server.SendPacket("OK"));
+
+  PacketResult result = client.SendPacketAndReceiveResponseWithOutputSupport(
+"qRcmd,test", response, true,
+[&command_output](llvm::StringRef output) {
+  command_output << output;
+});
+
+  ASSERT_EQ(PacketResult::Success, result);
+  ASSERT_EQ("OK", response.GetStringRef());
+  ASSERT_EQ("Hello, world", command_output.GetString().str());
+}
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -5154,10 +5154,13 @@
 
   bool send_async = true;
   StringExtractorGDBRemote response;
-  process->GetGDBRemote().SendPacketAndWaitForResponse(
-  packet.GetString(), response, send_async);
-  result.SetStatus(eReturnStatusSuccessFinishResult);
   Stream &output_strm = result.GetOutputStream();
+  process->GetGDBRemote().SendPacketAndReceiveResponseWithOutputSupport(
+  packet.GetString(), response, send_async,
+  [&output_strm](llvm::StringRef output) {
+output_strm << output;
+  });
+  result.SetStatus(eReturnStatusSuccessFinishResult);
   output_strm.Printf("  packet: %s\n", packet.GetData());
   const std::string &response_str = response.GetStringRef();
 
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -232,6 +232,10 @@
   PacketResult ReadPacket(StringExtractorGDBRemote &response,
   Timeout timeout, bool sync_on_timeout);
 
+  PacketResult ReadPacketWithOutputSupport(StringExtractorGDBRemote &response,
+  Timeout timeout, bool sync_on_timeout,
+  std::function output_callback);
+
   // Pop a packet from the queue in a thread safe manner
   PacketResult PopPacketFromQueue(StringExtractorGDBRemote &response,
   Timeout timeout);
Index: source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
===
--- source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -274,6 +274,24 @@
   return result;
 }
 
+GDBRemoteCommunication::PacketResult
+GDBRemoteCommunication::ReadPacketWithOutputSupport(
+StringExtractorGDBRemote &response,
+Timeout timeout,
+bool sync_on_timeout,
+std::function output_callback) {
+  auto result = ReadPacket(response, timeout, sync_on_timeout);
+  while (result == PacketResult::Success && response.IsNormalResponse() &&
+ response.PeekChar() == 'O') {
+response.GetChar();
+std::string output;
+if (response.GetHexByteString(output))
+  output_callback(output);
+result = ReadPacket(response, timeout, sync_on_timeout);

[Lldb-commits] [PATCH] D41745: Handle O reply packets during qRcmd

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

Thanks!  I don't have commit access, so someone needs to commit this for me, 
right?


https://reviews.llvm.org/D41745



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw created this revision.
owenpshaw added reviewers: clayborg, labath.
Herald added subscribers: mgorny, emaste.

When writing an object file over gdb-remote, use the vFlashErase, vFlashWrite, 
and vFlashDone commands if the write address is in a flash memory region.  A 
bare metal target may have this kind of setup.

- Update ObjectFileELF to set load addresses using physical addresses.  A 
typical case may be a data section with a physical address in ROM and a virtual 
address in RAM, which should be loaded to the ROM address.
- Add support for querying the target's qXfer:memory-map, which contains 
information about flash memory regions, leveraging MemoryRegionInfo data 
structures with minor modifications
- Update ProcessGDBRemote to use vFlash commands in DoWriteMemory when the 
target address is in a flash region
- Add a new foundation for testing gdb-remote behaviors by using a mock server 
that can respond however the test requires

Original discussion at 
http://lists.llvm.org/pipermail/lldb-dev/2018-January/013093.html

---

A few questions...

1. Leveraging MemoryRegionInfo seemed like the way to go, since 
qXfer:memory-map results are similar to qMemoryRegionInfo results.  But should 
GetMemoryRegionInfo() be changed to use the qXfer results instead of having the 
separate new function I added?
2. Are the new gdb-remote python tests moving in the right direction?  I can 
add more cases, but wanted to first verify the foundation was acceptable.  It's 
similar to some code in the tools/lldb-server tests, but seemed different 
enough to justify its own base.


https://reviews.llvm.org/D42145

Files:
  include/lldb/Host/XML.h
  include/lldb/Target/MemoryRegionInfo.h
  include/lldb/Target/Process.h
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
  packages/Python/lldbsuite/test/functionalities/gdb_remote_client/a.yaml
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
  source/Host/common/XML.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  source/Symbol/ObjectFile.cpp
  unittests/Process/gdb-remote/CMakeLists.txt
  unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp

Index: unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
===
--- /dev/null
+++ unittests/Process/gdb-remote/GDBRemoteCommunicationTest.cpp
@@ -0,0 +1,54 @@
+//===-- GDBRemoteCommunicationTest.cpp --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+#include 
+
+#include "GDBRemoteTestUtils.h"
+#include "lldb/Utility/StreamString.h"
+
+using namespace lldb_private::process_gdb_remote;
+using namespace lldb_private;
+
+TEST(GDBRemoteCommunicationTest, WriteEscapedBinary) {
+  StreamString escaped;
+
+  // Nothing gets escaped
+  // Verify null and other control chars don't cause problems
+  const uint8_t data[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07};
+  GDBRemoteCommunication::WriteEscapedBinary(escaped, data, sizeof(data));
+  ASSERT_EQ(sizeof(data), escaped.GetSize());
+  ASSERT_EQ(0x00, escaped.GetString().data()[0]);
+  ASSERT_EQ(0x03, escaped.GetString().data()[3]);
+  ASSERT_EQ(0x07, escaped.GetString().data()[7]);
+
+  // 0x23 and 0x24 should be escaped
+  escaped.Clear();
+  const uint8_t data2[] = {0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27};
+  GDBRemoteCommunication::WriteEscapedBinary(escaped, data2, sizeof(data));
+  ASSERT_EQ(sizeof(data) + 2, escaped.GetSize());
+  ASSERT_EQ(0x20, escaped.GetString().data()[0]);
+  ASSERT_EQ(0x7d, escaped.GetString().data()[3]);
+  ASSERT_EQ(0x23 ^ 0x20, escaped.GetString().data()[4]);
+  ASSERT_EQ(0x7d, escaped.GetString().data()[5]);
+  ASSERT_EQ(0x24 ^ 0x20, escaped.GetString().data()[6]);
+  ASSERT_EQ(0x25, escaped.GetString().data()[7]);
+  ASSERT_EQ(0x27, escaped.GetString().data()[9]);
+
+  // 0x7d should be escaped
+  escaped.Clear();
+  const uint8_t data3[] = {0x7b, 0x74, 0x65, 0x73, 0x74, 0x7d};
+  GDBRemoteCommunication::WriteEscapedBinary(escaped, data3, sizeof(data));
+  ASSERT_EQ(sizeof(data) + 1, escaped.GetSize());
+  ASSERT_EQ(0x7b, escaped.GetString().data()[0]);
+  ASSERT_EQ(0x74, escaped.GetString().data()[1]);
+  ASSERT_EQ(0x65, e

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

Thanks for the feedback, I'm working on splitting out the testing base into 
another patch and making the requested changes.  My main unresolved question is 
about the write batching, with details below.




Comment at: include/lldb/Target/Process.h:1916
+  //--
+  virtual bool BeginWriteMemoryBatch() { return true; }
+

clayborg wrote:
> labath wrote:
> > Instead of this begin/end combo, what would you think about a WriteMemory 
> > overload that takes a list of memory regions?
> > Something like:
> > ```
> > struct WriteEntry { addr_t Dest; llvm::ArrayRef Contents; };
> > Status WriteMemory(llvm::ArrayRef);
> > ```
> > Then the default implementation can just lower this into regular 
> > `WriteMemory` sequence, and the gdb-remote class can add the extra packets 
> > around it when needed.
> Do we really need this? Can't we just take care of it inside of the standard 
> Process::WriteMemory()? This doesn't seem like something that a user should 
> have to worry about. The process plug-in should just make the write happen 
> IMHO. Let me know.
Maybe I'm misunderstanding the flash commands, but I couldn't figure a way 
around the need to somehow indicate that several writes are batched together.  
The reason has to do with how vFlashErase must erase an entire block at a time.

ObjectFile::LoadInMemory makes one Process::WriteMemory call per section.  If 
each WriteMemory call was self-contained, and two sections are in the same 
flash block, it would go something like this:

# WriteMemory (section 1)
## DoWriteMemory
### vFlashErase (block 1)
### vFlashWrite (section 1)
### vFlashDone
# WriteMemory (section 2)
## DoWriteMemory
### vFlashErase (block 1, again)
### vFlashWrite (section 2)
### vFlashDone

Wouldn't the second erase undo the first write?

I found the begin/end to be the least intrusive way around, but I'm open to 
other options.



Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4612-4613
 
+bool ProcessGDBRemote::GetMemoryMap(
+std::vector ®ion_list) {
+

clayborg wrote:
> Can we cache this value permanently? If so we should read it once and then 
> the GDBRemoteCommunicationClient::GetMemoryRegionInfo() should try the 
> qMemoryRegionInfo packet (if supported) and possibly augment the memory 
> region results with this info?
> 
> This should be pushed down into GDBRemoteCommunicationClient and it should 
> keep a member variable that caches these results.
> 
> Any reason we are using shared pointers to lldb::MemoryRegionInfo? They are 
> simple structs. No need to indirect through shared pointers IMHO.
I can merge the results in GetMemoryRegionInfo().

I put the xml parsing in ProcessGDBRemote because it seemed similar to the 
logic in ProcessGDBRemote::GetGDBServerRegisterInfo, which also uses 
ReadExtFeature.  Figured there must have been a reason for that.  Easy to move 
it.

And shared pointers were only because I was mimicking the 
Process::GetMemoryRegions api.



Comment at: source/Symbol/ObjectFile.cpp:671
+  continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)

clayborg wrote:
> labath wrote:
> > You're not actually changing this part, but it caught my eye, and you may 
> > care about this:
> > 
> > Is it true that we can ignore .bss here? Programs generally assume that 
> > .bss is zero-initialized, and if you just ignore it here, it can contain 
> > whatever garbage was in the memory before you loaded (?)
> Very important to zero out bss
For the embedded projects I've worked on, ignoring .bss here is fine and 
expected because the code always starts by zeroing out the .bss memory (and 
also copying the .data section contents from ROM to RAM, which is related to 
the physical address change).

For comparison, gdb doesn't try to load the bss section either.


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42195: [lldb] Generic base for testing gdb-remote behavior

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw created this revision.
owenpshaw added reviewers: clayborg, labath.

Adds new utilities that make it easier to write test cases for lldb acting as a 
client over a gdb-remote connection.

- A GDBRemoteTestBase class that starts a mock GDB server and provides an easy 
way to check client packets
- A MockGDBServer that, via MockGDBServerResponder, can be made to issue server 
responses that test client behavior.
- Utility functions for handling common data encoding/decoding
- Utility functions for creating dummy targets from YAML files



Split from the review at https://reviews.llvm.org/D42145, which was a new 
feature that necessitated the new testing capabilities.


https://reviews.llvm.org/D42195

Files:
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
  packages/Python/lldbsuite/test/functionalities/gdb_remote_client/a.yaml
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py

Index: packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
===
--- /dev/null
+++ packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
@@ -0,0 +1,432 @@
+import os
+import os.path
+import subprocess
+import threading
+import socket
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbtest_config
+
+
+def yaml2obj_executable():
+"""
+Get the path to the yaml2obj executable, which can be used to create test
+object files from easy to write yaml instructions.
+
+Throws an Exception if the executable cannot be found.
+"""
+# Tries to find yaml2obj at the same folder as the lldb
+path = os.path.join(os.path.dirname(lldbtest_config.lldbExec), "yaml2obj")
+if os.path.exists(path):
+return path
+raise Exception("yaml2obj executable not found")
+
+
+def yaml2elf(yaml_path, elf_path):
+"""
+Create an ELF file at the given path from a yaml file at the given path.
+
+Throws a subprocess.CalledProcessError if the ELF could not be created.
+"""
+yaml2obj = yaml2obj_executable()
+command = [yaml2obj, "-o=%s" % elf_path, yaml_path]
+system([command])
+
+
+def checksum(message):
+"""
+Calculate the GDB server protocol checksum of the message.
+
+The GDB server protocol uses a simple modulo 256 sum.
+"""
+check = 0
+for c in message:
+check += ord(c)
+return check % 256
+
+
+def frame_packet(message):
+"""
+Create a framed packet that's ready to send over the GDB connection
+channel.
+
+Framing includes surrounding the message between $ and #, and appending
+a two character hex checksum.
+"""
+return "$%s#%02x" % (message, checksum(message))
+
+
+def escape_binary(message):
+"""
+Escape the binary message using the process described in the GDB server
+protocol documentation.
+
+Most bytes are sent through as-is, but $, #, and { are escaped by writing
+a { followed by the original byte mod 0x20.
+"""
+out = ""
+for c in message:
+d = ord(c)
+if d in (0x23, 0x24, 0x7d):
+out += chr(0x7d)
+out += chr(d ^ 0x20)
+else:
+out += c
+return out
+
+
+def hex_encode_bytes(message):
+"""
+Encode the binary message by converting each byte into a two-character
+hex string.
+"""
+out = ""
+for c in message:
+out += "%02x" % ord(c)
+return out
+
+
+def hex_decode_bytes(hex_bytes):
+"""
+Decode the hex string into a binary message by converting each two-character
+hex string into a single output byte.
+"""
+out = ""
+hex_len = len(hex_bytes)
+while i < hex_len - 1:
+out += chr(int(hex_bytes[i:i + 2]), 16)
+i += 2
+return out
+
+
+class MockGDBServerResponder:
+"""
+A base class for handing client packets and issuing server responses for
+GDB tests.
+
+This handles many typical situations, while still allowing subclasses to
+completely customize their responses.
+
+Most subclasses will be interested in overriding the other() method, which
+handles any packet not recognized in the common packet handling code.
+"""
+
+registerCount = 40
+packetLog = None
+
+def __init__(self):
+self.packetLog = []
+
+def respond(self, packet):
+"""
+Return the unframed packet data that the server should issue in response
+to the given packet received from the client.
+"""
+self.packetLog.append(packet)
+if packet == "g":
+return self.readRegisters()
+if packet[0] == "G":
+return self.writeRegisters(packet[1:])
+if packet[0] == "p":
+return self.readRegister(int(packet[1:], 16))
+if packet[0] == "P":
+register, value = packet[1:].split("=")
+return sel

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw updated this revision to Diff 130258.
owenpshaw added a comment.

- Split testing base into separate review https://reviews.llvm.org/D42195
- Use StreamGDBRemote instead of duplicate new function
- Move qXfer:memory-map xml parsing into GDBRemoteCommunicationClient
- Include qXfer:memory-map data in GetMemoryRegionInfo, instead of creating a 
separate API

I've left the batch write as-is for now because I'm not quite sold on the 
solution of writing entire blocks of flash memory at time.  While I agree it 
could work, it feels like an overly complicated code solution that will also 
generate a lot of unnecessary commands between the client and server.

Can you help me understand the objection to the begin/end design?  I can't 
really think of scenario besides object file load where the begin/end batching 
calls would be used.  So perhaps instead of ObjectFile::LoadInMemory using 
Process::WriteMemory, it instead calls something like a new 
Process::WriteObjectSections(std::vector, SectionLoadList) method 
that's clearly only intended for object writing.  The GDB process could 
override and do the begin/end batching privately.


https://reviews.llvm.org/D42145

Files:
  include/lldb/Host/XML.h
  include/lldb/Target/MemoryRegionInfo.h
  include/lldb/Target/Process.h
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
  source/Host/common/XML.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  source/Symbol/ObjectFile.cpp

Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -659,22 +659,47 @@
   SectionList *section_list = GetSectionList();
   if (!section_list)
 return Status("No section in object file");
+
+  // Filter the list of loadable sections
+  std::vector loadable_sections;
   size_t section_count = section_list->GetNumSections(0);
   for (size_t i = 0; i < section_count; ++i) {
 SectionSP section_sp = section_list->GetSectionAtIndex(i);
 addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
-  DataExtractor section_data;
-  // We can skip sections like bss
-  if (section_sp->GetFileSize() == 0)
-continue;
-  section_sp->GetSectionData(section_data);
-  lldb::offset_t written = process->WriteMemory(
-  addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
-  if (written != section_data.GetByteSize())
-return error;
+if (addr == LLDB_INVALID_ADDRESS)
+  continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+  continue;
+loadable_sections.push_back(section_sp);
+  }
+
+  // Sort the sections by address because some writes, like those to flash
+  // memory, must happen in order of increasing address.
+  std::stable_sort(std::begin(loadable_sections), std::end(loadable_sections),
+  [&target](const SectionSP a, const SectionSP b){
+addr_t addr_a = target.GetSectionLoadList().GetSectionLoadAddress(a);
+addr_t addr_b = target.GetSectionLoadList().GetSectionLoadAddress(b);
+return addr_a < addr_b;
+  });
+
+  // Do a batch memory write to the process
+  if (!process->BeginWriteMemoryBatch())
+return Status("Could not start write memory batch");
+  for (auto §ion_sp : loadable_sections) {
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+lldb::offset_t written = process->WriteMemory(
+addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
+if (written != section_data.GetByteSize()) {
+  process->EndWriteMemoryBatch();
+  return error;
 }
   }
+  if (!process->EndWriteMemoryBatch())
+return Status("Could not end write memory batch");
+
   if (set_pc) {
 ThreadList &thread_list = process->GetThreadList();
 ThreadSP curr_thread(thread_list.GetSelectedThread());
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -144,6 +144,10 @@
   size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
   Status &error) override;
 
+  bool BeginWriteMemoryBatch() override;
+
+  bool EndWriteMemoryBatch() override;
+
   size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
Status &error) override;
 
@@ -302,6 +306,11 @@
  

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

I'm not envisioning that anything else needs to change to use begin/end or care 
it's there.  I guess the way I look at it, having ObjectFile::LoadInMemory do 
begin/end is basically the same as what you're saying about having it be more 
process aware.

If Process is going to introduce new concepts for ObjectFile to use either way, 
isn't a high level of abstraction (batched writes) preferable to ObjectFile 
needing to know the fine details of flash memory blocks or that flash is even 
used?  And doesn't keeping the heavy lifting in ProcessGDB make it reusable 
should another case come along?

Hope you don't mind the pushback.  I think we're looking at this from different 
angles, and I'm genuinely asking to help my understanding of your thinking so 
hopefully we can converge on the same view.




Comment at: include/lldb/Target/MemoryRegionInfo.h:109-110
   ConstString m_name;
+  OptionalBool m_flash;
+  lldb::offset_t m_blocksize;
 };

clayborg wrote:
> Could these two be combined into one entry? Could "m_blocksize" be 
> "m_flash_block_size" and if the value is zero, then it isn't flash?
Sure, I can change that.  Almost went that way initially, but thought the two 
fields made it a little clearer, and allowed for an error condition when flash 
is true, but the blocksize is invalid (0).  Collapsing to one field means 
falling back to a regular memory write rather than an error.  Not sure if it's 
a big deal either way.


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42195: [lldb] Generic base for testing gdb-remote behavior

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw updated this revision to Diff 130472.
owenpshaw added a comment.

Updated with suggestions

- Using process.Kill() to close client connection
- Refactored server._receive
- renamed elf-specific functions to be generic


https://reviews.llvm.org/D42195

Files:
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
  packages/Python/lldbsuite/test/functionalities/gdb_remote_client/a.yaml
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py

Index: packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
===
--- /dev/null
+++ packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
@@ -0,0 +1,464 @@
+import os
+import os.path
+import subprocess
+import threading
+import socket
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbtest_config
+
+
+def yaml2obj_executable():
+"""
+Get the path to the yaml2obj executable, which can be used to create test
+object files from easy to write yaml instructions.
+
+Throws an Exception if the executable cannot be found.
+"""
+# Tries to find yaml2obj at the same folder as the lldb
+path = os.path.join(os.path.dirname(lldbtest_config.lldbExec), "yaml2obj")
+if os.path.exists(path):
+return path
+raise Exception("yaml2obj executable not found")
+
+
+def yaml2obj(yaml_path, obj_path):
+"""
+Create an object file at the given path from a yaml file at the given path.
+
+Throws a subprocess.CalledProcessError if the object could not be created.
+"""
+yaml2obj = yaml2obj_executable()
+command = [yaml2obj, "-o=%s" % obj_path, yaml_path]
+system([command])
+
+
+def checksum(message):
+"""
+Calculate the GDB server protocol checksum of the message.
+
+The GDB server protocol uses a simple modulo 256 sum.
+"""
+check = 0
+for c in message:
+check += ord(c)
+return check % 256
+
+
+def frame_packet(message):
+"""
+Create a framed packet that's ready to send over the GDB connection
+channel.
+
+Framing includes surrounding the message between $ and #, and appending
+a two character hex checksum.
+"""
+return "$%s#%02x" % (message, checksum(message))
+
+
+def escape_binary(message):
+"""
+Escape the binary message using the process described in the GDB server
+protocol documentation.
+
+Most bytes are sent through as-is, but $, #, and { are escaped by writing
+a { followed by the original byte mod 0x20.
+"""
+out = ""
+for c in message:
+d = ord(c)
+if d in (0x23, 0x24, 0x7d):
+out += chr(0x7d)
+out += chr(d ^ 0x20)
+else:
+out += c
+return out
+
+
+def hex_encode_bytes(message):
+"""
+Encode the binary message by converting each byte into a two-character
+hex string.
+"""
+out = ""
+for c in message:
+out += "%02x" % ord(c)
+return out
+
+
+def hex_decode_bytes(hex_bytes):
+"""
+Decode the hex string into a binary message by converting each two-character
+hex string into a single output byte.
+"""
+out = ""
+hex_len = len(hex_bytes)
+while i < hex_len - 1:
+out += chr(int(hex_bytes[i:i + 2]), 16)
+i += 2
+return out
+
+
+class MockGDBServerResponder:
+"""
+A base class for handing client packets and issuing server responses for
+GDB tests.
+
+This handles many typical situations, while still allowing subclasses to
+completely customize their responses.
+
+Most subclasses will be interested in overriding the other() method, which
+handles any packet not recognized in the common packet handling code.
+"""
+
+registerCount = 40
+packetLog = None
+
+def __init__(self):
+self.packetLog = []
+
+def respond(self, packet):
+"""
+Return the unframed packet data that the server should issue in response
+to the given packet received from the client.
+"""
+self.packetLog.append(packet)
+if packet == "g":
+return self.readRegisters()
+if packet[0] == "G":
+return self.writeRegisters(packet[1:])
+if packet[0] == "p":
+return self.readRegister(int(packet[1:], 16))
+if packet[0] == "P":
+register, value = packet[1:].split("=")
+return self.readRegister(int(register, 16), value)
+if packet[0] == "m":
+addr, length = [int(x, 16) for x in packet[1:].split(',')]
+return self.readMemory(addr, length)
+if packet[0] == "M":
+location, encoded_data = packet[1:].split(":")
+addr, length = [int(x, 16) for x in location.split(',')]
+return self.writeMemory(addr, encoded_data)
+if packet[0:7] == "qSymbol":
+ 

[Lldb-commits] [PATCH] D42195: [lldb] Generic base for testing gdb-remote behavior

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw updated this revision to Diff 130643.
owenpshaw added a comment.
Herald added a subscriber: mgorny.

- Updated to use TestBase.process() instead of a new member
- Added yaml2obj dependency.  It it okay to be an unconditional dependency?
- I can put the socket close() back in, but had removed it because python 
should close it automatically on garbage collection, so it didn't seem to 
matter either way


https://reviews.llvm.org/D42195

Files:
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
  packages/Python/lldbsuite/test/functionalities/gdb_remote_client/a.yaml
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
  test/CMakeLists.txt

Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -13,7 +13,7 @@
 )
 endfunction()
 
-set(LLDB_TEST_DEPS lldb)
+set(LLDB_TEST_DEPS lldb yaml2obj)
 
 # darwin-debug is an hard dependency for the testsuite.
 if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
Index: packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
===
--- /dev/null
+++ packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
@@ -0,0 +1,466 @@
+import os
+import os.path
+import subprocess
+import threading
+import socket
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbtest_config
+
+
+def yaml2obj_executable():
+"""
+Get the path to the yaml2obj executable, which can be used to create test
+object files from easy to write yaml instructions.
+
+Throws an Exception if the executable cannot be found.
+"""
+# Tries to find yaml2obj at the same folder as the lldb
+path = os.path.join(os.path.dirname(lldbtest_config.lldbExec), "yaml2obj")
+if os.path.exists(path):
+return path
+raise Exception("yaml2obj executable not found")
+
+
+def yaml2obj(yaml_path, obj_path):
+"""
+Create an object file at the given path from a yaml file at the given path.
+
+Throws a subprocess.CalledProcessError if the object could not be created.
+"""
+yaml2obj = yaml2obj_executable()
+command = [yaml2obj, "-o=%s" % obj_path, yaml_path]
+system([command])
+
+
+def checksum(message):
+"""
+Calculate the GDB server protocol checksum of the message.
+
+The GDB server protocol uses a simple modulo 256 sum.
+"""
+check = 0
+for c in message:
+check += ord(c)
+return check % 256
+
+
+def frame_packet(message):
+"""
+Create a framed packet that's ready to send over the GDB connection
+channel.
+
+Framing includes surrounding the message between $ and #, and appending
+a two character hex checksum.
+"""
+return "$%s#%02x" % (message, checksum(message))
+
+
+def escape_binary(message):
+"""
+Escape the binary message using the process described in the GDB server
+protocol documentation.
+
+Most bytes are sent through as-is, but $, #, and { are escaped by writing
+a { followed by the original byte mod 0x20.
+"""
+out = ""
+for c in message:
+d = ord(c)
+if d in (0x23, 0x24, 0x7d):
+out += chr(0x7d)
+out += chr(d ^ 0x20)
+else:
+out += c
+return out
+
+
+def hex_encode_bytes(message):
+"""
+Encode the binary message by converting each byte into a two-character
+hex string.
+"""
+out = ""
+for c in message:
+out += "%02x" % ord(c)
+return out
+
+
+def hex_decode_bytes(hex_bytes):
+"""
+Decode the hex string into a binary message by converting each two-character
+hex string into a single output byte.
+"""
+out = ""
+hex_len = len(hex_bytes)
+while i < hex_len - 1:
+out += chr(int(hex_bytes[i:i + 2]), 16)
+i += 2
+return out
+
+
+class MockGDBServerResponder:
+"""
+A base class for handing client packets and issuing server responses for
+GDB tests.
+
+This handles many typical situations, while still allowing subclasses to
+completely customize their responses.
+
+Most subclasses will be interested in overriding the other() method, which
+handles any packet not recognized in the common packet handling code.
+"""
+
+registerCount = 40
+packetLog = None
+
+def __init__(self):
+self.packetLog = []
+
+def respond(self, packet):
+"""
+Return the unframed packet data that the server should issue in response
+to the given packet received from the client.
+"""
+self.packetLog.append(packet)
+if packet == "g":
+return self.readRegisters()
+if packet[0] == "G":
+return self.writeRegisters(packet[1:])
+if packet[0] == "p":
+return self.readRegister(int(packet[1:], 16))
+

[Lldb-commits] [PATCH] D42195: [lldb] Generic base for testing gdb-remote behavior

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw updated this revision to Diff 130920.
owenpshaw added a comment.

Added back socket close()

It looks like the yaml2obj target hasn't been defined yet when the check-lldb 
target is defined, so if(TARGET yaml2obj) always returns false.  Is there 
another way to do the conditional dependency?  I'm assuming we don't want to 
reorder the includes in llvm/tools/CMakeLists.txt.


https://reviews.llvm.org/D42195

Files:
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
  packages/Python/lldbsuite/test/functionalities/gdb_remote_client/a.yaml
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
  test/CMakeLists.txt

Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -13,7 +13,7 @@
 )
 endfunction()
 
-set(LLDB_TEST_DEPS lldb)
+set(LLDB_TEST_DEPS lldb yaml2obj)
 
 # darwin-debug is an hard dependency for the testsuite.
 if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
Index: packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
===
--- /dev/null
+++ packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
@@ -0,0 +1,467 @@
+import os
+import os.path
+import subprocess
+import threading
+import socket
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbtest_config
+
+
+def yaml2obj_executable():
+"""
+Get the path to the yaml2obj executable, which can be used to create test
+object files from easy to write yaml instructions.
+
+Throws an Exception if the executable cannot be found.
+"""
+# Tries to find yaml2obj at the same folder as the lldb
+path = os.path.join(os.path.dirname(lldbtest_config.lldbExec), "yaml2obj")
+if os.path.exists(path):
+return path
+raise Exception("yaml2obj executable not found")
+
+
+def yaml2obj(yaml_path, obj_path):
+"""
+Create an object file at the given path from a yaml file at the given path.
+
+Throws a subprocess.CalledProcessError if the object could not be created.
+"""
+yaml2obj = yaml2obj_executable()
+command = [yaml2obj, "-o=%s" % obj_path, yaml_path]
+system([command])
+
+
+def checksum(message):
+"""
+Calculate the GDB server protocol checksum of the message.
+
+The GDB server protocol uses a simple modulo 256 sum.
+"""
+check = 0
+for c in message:
+check += ord(c)
+return check % 256
+
+
+def frame_packet(message):
+"""
+Create a framed packet that's ready to send over the GDB connection
+channel.
+
+Framing includes surrounding the message between $ and #, and appending
+a two character hex checksum.
+"""
+return "$%s#%02x" % (message, checksum(message))
+
+
+def escape_binary(message):
+"""
+Escape the binary message using the process described in the GDB server
+protocol documentation.
+
+Most bytes are sent through as-is, but $, #, and { are escaped by writing
+a { followed by the original byte mod 0x20.
+"""
+out = ""
+for c in message:
+d = ord(c)
+if d in (0x23, 0x24, 0x7d):
+out += chr(0x7d)
+out += chr(d ^ 0x20)
+else:
+out += c
+return out
+
+
+def hex_encode_bytes(message):
+"""
+Encode the binary message by converting each byte into a two-character
+hex string.
+"""
+out = ""
+for c in message:
+out += "%02x" % ord(c)
+return out
+
+
+def hex_decode_bytes(hex_bytes):
+"""
+Decode the hex string into a binary message by converting each two-character
+hex string into a single output byte.
+"""
+out = ""
+hex_len = len(hex_bytes)
+while i < hex_len - 1:
+out += chr(int(hex_bytes[i:i + 2]), 16)
+i += 2
+return out
+
+
+class MockGDBServerResponder:
+"""
+A base class for handing client packets and issuing server responses for
+GDB tests.
+
+This handles many typical situations, while still allowing subclasses to
+completely customize their responses.
+
+Most subclasses will be interested in overriding the other() method, which
+handles any packet not recognized in the common packet handling code.
+"""
+
+registerCount = 40
+packetLog = None
+
+def __init__(self):
+self.packetLog = []
+
+def respond(self, packet):
+"""
+Return the unframed packet data that the server should issue in response
+to the given packet received from the client.
+"""
+self.packetLog.append(packet)
+if packet == "g":
+return self.readRegisters()
+if packet[0] == "G":
+return self.writeRegisters(packet[1:])
+if packet[0] == "p":
+return self.readRegister(int(packet[1:], 16))
+if packet[0] == "P":
+   

[Lldb-commits] [PATCH] D42195: [lldb] Generic base for testing gdb-remote behavior

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw updated this revision to Diff 131310.
owenpshaw added a comment.

- Updated condition for yaml2obj dependency, now based on LLDB_BUILT_STANDALONE
- Find yaml2obj in the same directory as clang
- Move yaml2obj code into lldbtest.py so it's available to all tests

Is the findBuiltClang function the right thing to use here?  I can't find 
anything else that uses it.


https://reviews.llvm.org/D42195

Files:
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteClient.py
  packages/Python/lldbsuite/test/functionalities/gdb_remote_client/a.yaml
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
  packages/Python/lldbsuite/test/lldbtest.py
  test/CMakeLists.txt

Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -34,6 +34,10 @@
   list(APPEND LLDB_TEST_DEPS lldb-mi)
 endif()
 
+if(NOT LLDB_BUILT_STANDALONE)
+  list(APPEND LLDB_TEST_DEPS yaml2obj)
+endif()
+
 # The default architecture with which to compile test executables is the default LLVM target
 # architecture, which itself defaults to the host architecture.
 string(TOLOWER "${LLVM_TARGET_ARCH}" LLDB_DEFAULT_TEST_ARCH)
Index: packages/Python/lldbsuite/test/lldbtest.py
===
--- packages/Python/lldbsuite/test/lldbtest.py
+++ packages/Python/lldbsuite/test/lldbtest.py
@@ -1604,6 +1604,31 @@
 
 return os.environ["CC"]
 
+def findYaml2obj(self):
+"""
+Get the path to the yaml2obj executable, which can be used to create
+test object files from easy to write yaml instructions.
+
+Throws an Exception if the executable cannot be found.
+"""
+# Tries to find yaml2obj at the same folder as clang
+clang_dir = os.path.dirname(self.findBuiltClang())
+path = os.path.join(clang_dir, "yaml2obj")
+if os.path.exists(path):
+return path
+raise Exception("yaml2obj executable not found")
+
+
+def yaml2obj(self, yaml_path, obj_path):
+"""
+Create an object file at the given path from a yaml file.
+
+Throws subprocess.CalledProcessError if the object could not be created.
+"""
+yaml2obj = self.findYaml2obj()
+command = [yaml2obj, "-o=%s" % obj_path, yaml_path]
+system([command])
+
 def getBuildFlags(
 self,
 use_cpp11=True,
Index: packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
===
--- /dev/null
+++ packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
@@ -0,0 +1,442 @@
+import os
+import os.path
+import subprocess
+import threading
+import socket
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbtest_config
+
+
+def checksum(message):
+"""
+Calculate the GDB server protocol checksum of the message.
+
+The GDB server protocol uses a simple modulo 256 sum.
+"""
+check = 0
+for c in message:
+check += ord(c)
+return check % 256
+
+
+def frame_packet(message):
+"""
+Create a framed packet that's ready to send over the GDB connection
+channel.
+
+Framing includes surrounding the message between $ and #, and appending
+a two character hex checksum.
+"""
+return "$%s#%02x" % (message, checksum(message))
+
+
+def escape_binary(message):
+"""
+Escape the binary message using the process described in the GDB server
+protocol documentation.
+
+Most bytes are sent through as-is, but $, #, and { are escaped by writing
+a { followed by the original byte mod 0x20.
+"""
+out = ""
+for c in message:
+d = ord(c)
+if d in (0x23, 0x24, 0x7d):
+out += chr(0x7d)
+out += chr(d ^ 0x20)
+else:
+out += c
+return out
+
+
+def hex_encode_bytes(message):
+"""
+Encode the binary message by converting each byte into a two-character
+hex string.
+"""
+out = ""
+for c in message:
+out += "%02x" % ord(c)
+return out
+
+
+def hex_decode_bytes(hex_bytes):
+"""
+Decode the hex string into a binary message by converting each two-character
+hex string into a single output byte.
+"""
+out = ""
+hex_len = len(hex_bytes)
+while i < hex_len - 1:
+out += chr(int(hex_bytes[i:i + 2]), 16)
+i += 2
+return out
+
+
+class MockGDBServerResponder:
+"""
+A base class for handing client packets and issuing server responses for
+GDB tests.
+
+This handles many typical situations, while still allowing subclasses to
+completely customize their responses.
+
+Most subclasses will be interested in overriding the other() method, which
+handles any packet not recognized in the common packet handling 

[Lldb-commits] [PATCH] D42195: [lldb] Generic base for testing gdb-remote behavior

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

Thanks for the help!  Someone with commit access will need to land it.


https://reviews.llvm.org/D42195



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

Hi Greg, I got distracted from this one for a bit.  Maybe I missed it, but do 
you have any thoughts on my previous comment/question about the batch API vs 
other options?

Thanks,
Owen


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

Thanks.  What I'm struggling to reconcile are your statements that users should 
not have to know how things must happen, but then that we should make 
ObjectFile::Load smart so it doesn't result in an unnecessary amount of 
reads/writes.  If writing to flash memory effectively requires some smarts on 
the caller's part, then how have we made things easier for the callers?  And at 
that point isn't start/end a lot simpler than requiring callers to worry about 
block sizes themselves?


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

This discussion has got me questioning if WriteMemory is even the best place to 
put the flash commands.  It seems like some of the concern here is around the 
behavior of arbitrary memory writes to flash regions, which isn't really the 
main objective of this patch (supporting object file loading into flash).

I did a little testing with gdb, and it doesn't do arbitrary memory writes to 
flash regions.  Using the set command on a flash address, for example, just 
gives an error that flash writes are not allowed.  Perhaps that's a better way 
to go than worrying about supporting one-off flash writes.



> I still like my WriteMemory(ArrayRef) proposal the 
> best.

The one difference I see between a single function and begin/end is that 
begin/end doesn't require all the bytes to be read into host memory up front, 
and can instead read the source bytes in chunks.  Perhaps that's not really a 
reason for concern, though.


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

> So the main question is: do we want WriteMemory to work anywhere and always 
> try to do the right thing, or return an error an the user would be expected 
> to know to check the memory region you are writing to and know to call 
> "Process::WriteFlash(...)". I vote to keep things simple and have 
> Process::WriteMemory() just do the right thing. I am open to differing 
> opinions, so let me know what you guys think.

What are other use cases for writing to flash memory from lldb?  Since I can't 
think of any, I'm biased towards keeping this implementation simple and 
narrowly focused on object loading, which means not worrying about preserving 
the unwritten parts of erased blocks, and giving an error for other write 
attempts to flash regions.


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

> How does the flash memory behave from the POV of the debugged process?

Just tested on a couple targets and they both silently failed to change the 
flash memory.  So no exception, but no flash write either.

Unless there are objections, I'll work on some changes along the lines you 
described.


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw updated this revision to Diff 132260.
owenpshaw added a comment.

- Remove Begin/End memory batch API
- Add Process::WriteObjectFile(llvm::ArrayRef) method.  Open to 
other names since it's not necessarily specific to object files, but wanted to 
somehow indicate that the method may do an uncommon write (like write to flash).
- Kept vFlashWrite as part of DoWriteMemory, and changed the is_batch_write 
flag to an allow_flash_writes flag.  ProcessGDB:: WriteObjectFile sets 
allow_flash_writes, and then calls the regular WriteMemory.  Didn't seem like 
there was a need to duplicate the WriteMemory logic.
- Any other memory write attempts to flash regions will now fail

I think this gets us closer to what we're talking about, but let me know if 
anything should change.


https://reviews.llvm.org/D42145

Files:
  include/lldb/Host/XML.h
  include/lldb/Target/MemoryRegionInfo.h
  include/lldb/Target/Process.h
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
  source/Host/common/XML.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  source/Symbol/ObjectFile.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -2533,6 +2533,18 @@
   return 0;
 }
 
+bool Process::WriteObjectFile(llvm::ArrayRef entries,
+  Status &error) {
+  error.Clear();
+  for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+  break;  
+  }
+  return error.Success();
+}
+
 #define USE_ALLOCATE_MEMORY_CACHE 1
 addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -659,22 +659,30 @@
   SectionList *section_list = GetSectionList();
   if (!section_list)
 return Status("No section in object file");
+
+  std::vector writeEntries;
+
+  // Create a list of write entries from loadable sections
   size_t section_count = section_list->GetNumSections(0);
   for (size_t i = 0; i < section_count; ++i) {
+Process::WriteEntry entry;
 SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
-  DataExtractor section_data;
-  // We can skip sections like bss
-  if (section_sp->GetFileSize() == 0)
-continue;
-  section_sp->GetSectionData(section_data);
-  lldb::offset_t written = process->WriteMemory(
-  addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
-  if (written != section_data.GetByteSize())
-return error;
-}
+entry.Dest = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (entry.Dest == LLDB_INVALID_ADDRESS)
+  continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+  continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+entry.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+ section_data.GetByteSize());
+writeEntries.push_back(entry);
   }
+
+  if (!process->WriteObjectFile(writeEntries, error))
+return error;
+
   if (set_pc) {
 ThreadList &thread_list = process->GetThreadList();
 ThreadSP curr_thread(thread_list.GetSelectedThread());
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -144,6 +144,9 @@
   size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
   Status &error) override;
 
+  bool WriteObjectFile(llvm::ArrayRef entries, Status &error)
+  override;
+
   size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
Status &error) override;
 
@@ -302,6 +305,11 @@
   int64_t m_breakpoint_pc_offset;
   lldb::tid_t m_initial_tid; // The initial thread ID, given by stub on attach
 
+  bool m_allow_flash_writes;
+  using FlashRangeVector = lldb_private::RangeVector;
+  using FlashRange = FlashRangeVector::Entry;
+  FlashRangeVector m_erased_flash_ranges;
+
   //--
   // Accessors
   //---

[Lldb-commits] [PATCH] D42195: [lldb] Generic base for testing gdb-remote behavior

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added inline comments.



Comment at: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py:335-337
+# However, if we aren't expecting an ack, it's likely the initial
+# ack that lldb client sends, and observations of real servers
+# suggest we're supposed to ack back.

labath wrote:
> Which servers were you observing here? I tried lldb-server and gdbserver and 
> they don't send the ack back (It seems like a bad idea anyway, as it could 
> lead to an infinite ack ping-pong).
> 
> I have removed this code as it was causing flakyness on the bots. If there is 
> a server that does this, and we care about supporting it, then we can add a 
> new targeted test which emulates this behavior, but then we need to also fix 
> the client to handle this situation better.
Just took another look and I misunderstood what was going on.  The server does 
send an opening +, but not in response to the client.

I was observing the packets from openocd, where lldb sends a + and then 
receives a + from openocd, and assumed one was a response to the other.

```
(lldb) log enable gdb-remote packets
(lldb) gdb-remote 
 <   1> send packet: +
 history[1] tid=0x0307 <   1> send packet: +
 <   1> read packet: +
 <  19> send packet: $QStartNoAckMode#b0
 <   1> read packet: +
 <   6> read packet: $OK#9a
 <   1> send packet: +

```

Can't find any docs about these opening acks, but looking at openocd code, it 
just always sends a + at the start of a connection.  Should the test server do 
the same?



Repository:
  rL LLVM

https://reviews.llvm.org/D42195



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

Thanks!  Overall it's feeling like we're getting down to mainly differences in 
personal preferences and judgements.  Not sure if you're looking for a 
discussion, which I'm happy to have, if you're just looking for changes to 
definitely be made.  If it's the latter, I think it'd be more efficient to just 
hand this off so the changes can be made without a lot of back and forth.




Comment at: include/lldb/Target/Process.h:1958
 
+  virtual bool WriteObjectFile(llvm::ArrayRef entries,
+   Status &error);

labath wrote:
> This (return bool + by-ref Status) is a bit weird of an api. Could you just 
> return Status here (but I would not be opposed to going llvm all the way and 
> returning `llvm::Error`).
Open to whatever.  I preferred how it made calling code a little simpler.

```
if (!WriteObjectFile(entries, error))
  return error;
```

rather than


```
error = WriteObjectFile(entries);
if (!error.Sucess())
  return error
```



Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2807
+  // memory, must happen in order of increasing address.
+  std::vector sortedEntries(entries);
+  std::stable_sort(std::begin(sortedEntries), std::end(sortedEntries),

labath wrote:
> Let's avoid copying the entries here. I can see two options:
> - Require that the entries are already sorted on input
> - pass them to this function as `std::vector` (and then have the 
> caller call with `std::move(entries)`).
I didn't love the copy either, but figured 1) it was pretty cheap given the 
expected values 2) it eliminates an unexpected modification of the passed 
array.  I prefer having the sort in the process file, since it's really the 
only scope that knows about the sorting requirement. 



Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2812-2821
+  m_allow_flash_writes = true;
+  if (Process::WriteObjectFile(sortedEntries, error))
+error = FlashDone();
+  else
+// Even though some of the writing failed, try to send a flash done if
+// some of the writing succeeded so the flash state is reset to normal,
+// but don't stomp on the error status that was set in the write failure

labath wrote:
> This makes the control flow quite messy. The base function is not so complex 
> that you have to reuse it at all costs. I think we should just implement the 
> loop ourselves (and call some write function while passing the 
> "allow_flash_writes" as an argument).
Guess we have different definitions of messy :)

Wouldn't any other approach pretty much require duplicating WriteMemory, 
WriteMemoryPrivate, and DoWriteMemory?

- It seemed like the breakpoint logic in WriteMemory could be important when 
writing an object to ram, so I wanted to preserve that
- The loop in WriteMemoryPrivate is fairly trivial, but why add a second one if 
not needed?
- DoWriteMemory is mostly code that is common to both ram and flash writes, 
especially if a ram-only version would still need to check if address is flash 
so it could report an error.



https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw updated this revision to Diff 132641.
owenpshaw added a comment.

- adjust WriteObjectFile signature to return Status and take an std::vector
- match project formatting style

I toyed with adding allow_flash to as an argument, but didn't really like it 
because it seemed to bring things back to basically allowing arbitrary flash 
writes via WriteMemory, and would affect all the process subclasses when only 
one (gdb) actually needs it.  I still like the member flag because it keeps the 
flash writing very private to ProcessGDB.

What can I do to get the xml dependency check in the test?  Not clear on who I 
should talk to or where to start.


https://reviews.llvm.org/D42145

Files:
  include/lldb/Host/XML.h
  include/lldb/Target/MemoryRegionInfo.h
  include/lldb/Target/Process.h
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
  source/Host/common/XML.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  source/Symbol/ObjectFile.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -2533,6 +2533,17 @@
   return 0;
 }
 
+Status Process::WriteObjectFile(std::vector entries) {
+  Status error;
+  for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+  break;  
+  }
+  return error;
+}
+
 #define USE_ALLOCATE_MEMORY_CACHE 1
 addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -659,22 +659,31 @@
   SectionList *section_list = GetSectionList();
   if (!section_list)
 return Status("No section in object file");
+
+  std::vector writeEntries;
+
+  // Create a list of write entries from loadable sections
   size_t section_count = section_list->GetNumSections(0);
   for (size_t i = 0; i < section_count; ++i) {
+Process::WriteEntry entry;
 SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
-  DataExtractor section_data;
-  // We can skip sections like bss
-  if (section_sp->GetFileSize() == 0)
-continue;
-  section_sp->GetSectionData(section_data);
-  lldb::offset_t written = process->WriteMemory(
-  addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
-  if (written != section_data.GetByteSize())
-return error;
-}
+entry.Dest = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (entry.Dest == LLDB_INVALID_ADDRESS)
+  continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+  continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+entry.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+ section_data.GetByteSize());
+writeEntries.push_back(entry);
   }
+
+  error = process->WriteObjectFile(std::move(writeEntries));
+  if (!error.Success())
+return error;
+
   if (set_pc) {
 ThreadList &thread_list = process->GetThreadList();
 ThreadSP curr_thread(thread_list.GetSelectedThread());
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -144,6 +144,8 @@
   size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
   Status &error) override;
 
+  Status WriteObjectFile(std::vector entries) override;
+
   size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
Status &error) override;
 
@@ -302,6 +304,11 @@
   int64_t m_breakpoint_pc_offset;
   lldb::tid_t m_initial_tid; // The initial thread ID, given by stub on attach
 
+  bool m_allow_flash_writes;
+  using FlashRangeVector = lldb_private::RangeVector;
+  using FlashRange = FlashRangeVector::Entry;
+  FlashRangeVector m_erased_flash_ranges;
+
   //--
   // Accessors
   //--
@@ -408,6 +415,12 @@
 
   Status UpdateAutomaticSignalFiltering() override;
 
+  Status FlashErase(lldb::addr_t 

[Lldb-commits] [PATCH] D42959: Rewrite the flaky test_restart_bug test in a more deterministic way

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

Looks good to me


https://reviews.llvm.org/D42959



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw updated this revision to Diff 135087.
owenpshaw added a comment.
Herald added a subscriber: arichardson.

Update patch to include new @skipIfXmlSupportMissing decorator on test


https://reviews.llvm.org/D42145

Files:
  include/lldb/Host/XML.h
  include/lldb/Target/MemoryRegionInfo.h
  include/lldb/Target/Process.h
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
  source/Host/common/XML.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  source/Symbol/ObjectFile.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -2533,6 +2533,17 @@
   return 0;
 }
 
+Status Process::WriteObjectFile(std::vector entries) {
+  Status error;
+  for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+  break;  
+  }
+  return error;
+}
+
 #define USE_ALLOCATE_MEMORY_CACHE 1
 addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -659,22 +659,31 @@
   SectionList *section_list = GetSectionList();
   if (!section_list)
 return Status("No section in object file");
+
+  std::vector writeEntries;
+
+  // Create a list of write entries from loadable sections
   size_t section_count = section_list->GetNumSections(0);
   for (size_t i = 0; i < section_count; ++i) {
+Process::WriteEntry entry;
 SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
-  DataExtractor section_data;
-  // We can skip sections like bss
-  if (section_sp->GetFileSize() == 0)
-continue;
-  section_sp->GetSectionData(section_data);
-  lldb::offset_t written = process->WriteMemory(
-  addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
-  if (written != section_data.GetByteSize())
-return error;
-}
+entry.Dest = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (entry.Dest == LLDB_INVALID_ADDRESS)
+  continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+  continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+entry.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+ section_data.GetByteSize());
+writeEntries.push_back(entry);
   }
+
+  error = process->WriteObjectFile(std::move(writeEntries));
+  if (!error.Success())
+return error;
+
   if (set_pc) {
 ThreadList &thread_list = process->GetThreadList();
 ThreadSP curr_thread(thread_list.GetSelectedThread());
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -144,6 +144,8 @@
   size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
   Status &error) override;
 
+  Status WriteObjectFile(std::vector entries) override;
+
   size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
Status &error) override;
 
@@ -302,6 +304,11 @@
   int64_t m_breakpoint_pc_offset;
   lldb::tid_t m_initial_tid; // The initial thread ID, given by stub on attach
 
+  bool m_allow_flash_writes;
+  using FlashRangeVector = lldb_private::RangeVector;
+  using FlashRange = FlashRangeVector::Entry;
+  FlashRangeVector m_erased_flash_ranges;
+
   //--
   // Accessors
   //--
@@ -408,6 +415,12 @@
 
   Status UpdateAutomaticSignalFiltering() override;
 
+  Status FlashErase(lldb::addr_t addr, size_t size);
+
+  Status FlashDone();
+
+  bool HasErased(FlashRange range);
+
 private:
   //--
   // For ProcessGDBRemote only
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -59,6 +59,7 @@
 #incl

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3478
+auto header = GetProgramHeaderByIndex(i);
+if (header->p_paddr != 0)
+  return true;

clayborg wrote:
> Can't p_paddr be zero and still be valid? Would a better test be 
> "header->p_memsz > 0"?
Yes, a 0 paddr can be valid.  This check will only ignore paddr if all segments 
have a 0 paddr, suggesting the linker didn't populate that field.

This decision is similar to what I did in llvm-objcopy for binary output, which 
in turn was based on GNU objcopy behavior: 
https://bugs.llvm.org/show_bug.cgi?id=35708#c4


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

@labath, any other changes you'd like to see on this one?  Skipping the test if 
there's no xml support seemed to be the final todo.


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

Thanks!  Just a reminder that I don't have commit access, so someone will need 
to land this for me.  Appreciate all the help.


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added inline comments.



Comment at: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:814
 
   value = value - header->p_vaddr;
   found_offset = true;

labath wrote:
> Ok so the issue is that here we use the virtual address to compute the load 
> bias, but at line 830 we add the bias to the physical address. This breaks as 
> soon as these two addresses start to differ.
> 
> Changing this to use the physical address as well fixes things, but as I said 
> before, I'm not sure we should be using physical addresses here.
I don't know if there's another use case besides flash load that should 
definitely use the physical address, so I can't be much help answering that 
question.  I was mainly relying on tests to catch any issue caused by using 
physical addresses.

Could the value_is_offset flag be a tell here?  Looks like that load bias is 
only calculated if value_is_offset == false.  Would it make sense to always use 
virtual address in such a case?  It wouldn't affect the "target modules load" 
case, which always sets value_is_offset to true.


Repository:
  rL LLVM

https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

> And I'm not even completely clear about your case. I understand you write the 
> module to the physical address, but what happens when you actually go around 
> to debugging it? Is it still there or does it get copied/mapped/whatever to 
> the virtual address?

In  my case it's a hardware target with ROM and RAM.  Only the data section has 
different virtual and physical addresses.  That data section should be loaded 
to its physical address in ROM so it'll be preserved across resets.  The first 
code to execute on boot copies the data section from ROM to its virtual address 
in RAM.

> So it seems to me the physical addresses should really be specific to the 
> "target modules load --load" case. I've never used that command so I don't 
> know if it can just use physical addresses unconditionally, or whether we 
> need an extra option for that "target modules load --load --physical". I 
> think I'll leave that decision up to you (or anyone else who has an opinion 
> on this). But for the other use cases, I believe we should keep using virtual 
> addresses.

The "target modules load --load" case was originally intended to accomplish 
what gdb's "load" command does (see https://reviews.llvm.org/D28804).  I know 
gdb uses the physical address when writes the sections.

> So I think we should revert this and then split out the loading part of this 
> patch from the vFlash thingy so we can focus on the virtual/physical address 
> situation and how to handle that.

It's like we need the original all-virtual SectionLoadList in 
ObjectFileELF::SetLoadAddress, but a different SectionLoadList for the 
ObjectFile::LoadInMemory case, a function which is only used by "target modules 
load --load".

Or instead instead of a second list, maybe SectionLoadList gets a second pair 
of addr<->section maps that contain physical addresses.  Then 
ObjectFile::LoadInMemory can ask for the physical instead of virtual.


Repository:
  rL LLVM

https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

> Since there is just one caller of this function maybe we don't even need to 
> that fancy. Could the LoadInMemory function do the shifting itself?
>  I'm thinking of something like where it takes the load bias as the argument 
> and then adds that to the physical address it obtains from the object file. 
> Thinking about that, I'm not even sure the function should be operating at 
> the section level. If it's going to be simulating a loader, shouldn't it be 
> based on program headers and segments completely (and not just use them for 
> obtaining the physical address)?

The original LoadInMemory thread from over a year ago discusses segments vs 
sections a little bit: 
http://lists.llvm.org/pipermail/lldb-dev/2016-December/011758.html.  Sounds 
like gdb used sections, so that was deemed ok for lldb.  It also fit with the 
pre-existing "target modules load" command that allowed section addresses to be 
specified individually.  At one point, Greg suggests:

> Since the arguments to this command are free form, we could pass these 
> arguments to ObjectFile::LoadInMemory() and let each loader (ObjectFileELF 
> and ObjectFileMach) determine what arguments are valid

But that didn't end up in the implementation.  Perhaps it should, and 
ObjectFileELF could disallow any --load that specified sections individually.  
And if we're doing a special ObjectFileELF::LoadInMemory anyway, it could go 
ahead and use segments.


Repository:
  rL LLVM

https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw updated this revision to Diff 137420.
owenpshaw added a comment.

- Revert changes to SetLoadAddress (always use virtual address there)
- Override LoadInMemory in ObjectFileELF to just load segments using physical 
address instead of using section load list

Passes tests, but I don't have access to hardware this week to double check 
that this works in the real world.

As an effect of this change, the "--load" part of "target modules load" now 
ignores the other command arguments for setting section addresses (--slide 
and/or the section/addr pairs).  --slide would be easy to add back, but would 
probably need to become an argument to LoadInMemory.  Does anyone have a clear 
sense of when someone would want to use --slide in combination with --load?  
What about section/add pairs?  Should the --load behavior instead be spun off 
into a new command?


https://reviews.llvm.org/D42145

Files:
  include/lldb/Host/XML.h
  include/lldb/Target/MemoryRegionInfo.h
  include/lldb/Target/Process.h
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
  source/Host/common/XML.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  source/Symbol/ObjectFile.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -2533,6 +2533,17 @@
   return 0;
 }
 
+Status Process::WriteObjectFile(std::vector entries) {
+  Status error;
+  for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+  break;  
+  }
+  return error;
+}
+
 #define USE_ALLOCATE_MEMORY_CACHE 1
 addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -659,22 +659,31 @@
   SectionList *section_list = GetSectionList();
   if (!section_list)
 return Status("No section in object file");
+
+  std::vector writeEntries;
+
+  // Create a list of write entries from loadable sections
   size_t section_count = section_list->GetNumSections(0);
   for (size_t i = 0; i < section_count; ++i) {
+Process::WriteEntry entry;
 SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
-  DataExtractor section_data;
-  // We can skip sections like bss
-  if (section_sp->GetFileSize() == 0)
-continue;
-  section_sp->GetSectionData(section_data);
-  lldb::offset_t written = process->WriteMemory(
-  addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
-  if (written != section_data.GetByteSize())
-return error;
-}
+entry.Dest = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (entry.Dest == LLDB_INVALID_ADDRESS)
+  continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+  continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+entry.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+ section_data.GetByteSize());
+writeEntries.push_back(entry);
   }
+
+  error = process->WriteObjectFile(std::move(writeEntries));
+  if (!error.Success())
+return error;
+
   if (set_pc) {
 ThreadList &thread_list = process->GetThreadList();
 ThreadSP curr_thread(thread_list.GetSelectedThread());
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -144,6 +144,8 @@
   size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
   Status &error) override;
 
+  Status WriteObjectFile(std::vector entries) override;
+
   size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
Status &error) override;
 
@@ -302,6 +304,11 @@
   int64_t m_breakpoint_pc_offset;
   lldb::tid_t m_initial_tid; // The initial thread ID, given by stub on attach
 
+  bool m_allow_flash_writes;
+  using FlashRangeVector = lldb_private::RangeVector;
+  using FlashRange = FlashRangeVector::Entry;
+  FlashRangeVector m_erase

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw reopened this revision.
owenpshaw added a comment.

Reopening since the previous land was reverted


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw updated this revision to Diff 137598.
owenpshaw added a comment.

- Share some common code in LoadInMemory

The DoLoadInMemory implementations call target.CalculateProcess() again, but 
passing Process or Process::WriteEntries to DoLoadInMemory would require either 
moving the Process.h include to ObjectFile.h or using forward declarations and 
pointers.  Calling CalculateProcess felt like the lest intrusive option and has 
little cost.  Open to other options.


https://reviews.llvm.org/D42145

Files:
  include/lldb/Host/XML.h
  include/lldb/Symbol/ObjectFile.h
  include/lldb/Target/MemoryRegionInfo.h
  include/lldb/Target/Process.h
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
  source/Host/common/XML.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  source/Symbol/ObjectFile.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -2533,6 +2533,17 @@
   return 0;
 }
 
+Status Process::WriteObjectFile(std::vector entries) {
+  Status error;
+  for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+  break;
+  }
+  return error;
+}
+
 #define USE_ALLOCATE_MEMORY_CACHE 1
 addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -656,25 +656,11 @@
   if (set_pc && !GetEntryPointAddress().IsValid())
 return Status("No entry address in object file");
 
-  SectionList *section_list = GetSectionList();
-  if (!section_list)
-return Status("No section in object file");
-  size_t section_count = section_list->GetNumSections(0);
-  for (size_t i = 0; i < section_count; ++i) {
-SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
-  DataExtractor section_data;
-  // We can skip sections like bss
-  if (section_sp->GetFileSize() == 0)
-continue;
-  section_sp->GetSectionData(section_data);
-  lldb::offset_t written = process->WriteMemory(
-  addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
-  if (written != section_data.GetByteSize())
-return error;
-}
-  }
+  error = DoLoadInMemory(target);
+
+  if (!error.Success())
+return error;
+
   if (set_pc) {
 ThreadList &thread_list = process->GetThreadList();
 ThreadSP curr_thread(thread_list.GetSelectedThread());
@@ -685,6 +671,32 @@
   return error;
 }
 
+Status ObjectFile::DoLoadInMemory(Target &target) {
+  SectionList *section_list = GetSectionList();
+  if (!section_list)
+return Status("No section in object file");
+
+  std::vector writeEntries;
+  // Create a list of write entries from loadable sections
+  size_t section_count = section_list->GetNumSections(0);
+  for (size_t i = 0; i < section_count; ++i) {
+Process::WriteEntry entry;
+SectionSP section_sp = section_list->GetSectionAtIndex(i);
+entry.Dest = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (entry.Dest == LLDB_INVALID_ADDRESS)
+  continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+  continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+entry.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+ section_data.GetByteSize());
+writeEntries.push_back(entry);
+  }
+  return target.CalculateProcess()->WriteObjectFile(std::move(writeEntries));
+}
+
 void ObjectFile::RelocateSection(lldb_private::Section *section)
 {
 }
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -144,6 +144,8 @@
   size_t DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
   Status &error) override;
 
+  Status WriteObjectFile(std::vector entries) override;
+
   size_t DoWriteMemory(lldb::addr_t addr, const void *buf, size_t size,
Status &error) override;
 
@@ -302,6 +304,11 @@
   int64_t m_br

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

It can be done that way.  As I mentioned, it would require moving the Process.h 
import from ObjectFile.cpp to ObjectFile.h (to get the declaration of 
Process::WriteEntry).  I'm asking how you guys feel about that.  Without a 
clear sense of project norms, I'll always go for the less intrusive change, 
which in this case meant a slightly different signature instead of moving an 
import that wasn't necessary.


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw updated this revision to Diff 137827.
owenpshaw added a comment.

Looking at LoadInMemory, it seems like the common code is actually more 
appropriate in the command handler.  It's checking inputs and doing the 
unrelated task of setting the pc.  So here's an attempt at going a little 
further to simplify ObjectFile and its dependencies

- Have the command check for a valid process, write to the process, and set the 
pc
- Replace ObjectFile::LoadInMemory with GetLoadableData
- Move/rename the Process::WriteEntry struct to ObjectFile::LoadableData

Thoughts?


https://reviews.llvm.org/D42145

Files:
  include/lldb/Core/Module.h
  include/lldb/Host/XML.h
  include/lldb/Symbol/ObjectFile.h
  include/lldb/Target/MemoryRegionInfo.h
  include/lldb/Target/Process.h
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py
  
packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py
  source/Commands/CommandObjectTarget.cpp
  source/Core/Module.cpp
  source/Host/common/XML.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
  source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.h
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
  source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
  source/Symbol/ObjectFile.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -2533,6 +2533,17 @@
   return 0;
 }
 
+Status Process::WriteObjectFile(std::vector entries) {
+  Status error;
+  for (const auto &Entry : entries) {
+WriteMemory(Entry.Dest, Entry.Contents.data(), Entry.Contents.size(),
+error);
+if (!error.Success())
+  break;
+  }
+  return error;
+}
+
 #define USE_ALLOCATE_MEMORY_CACHE 1
 addr_t Process::AllocateMemory(size_t size, uint32_t permissions,
Status &error) {
Index: source/Symbol/ObjectFile.cpp
===
--- source/Symbol/ObjectFile.cpp
+++ source/Symbol/ObjectFile.cpp
@@ -16,7 +16,6 @@
 #include "lldb/Symbol/ObjectContainer.h"
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Target/Process.h"
-#include "lldb/Target/RegisterContext.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBuffer.h"
@@ -648,41 +647,31 @@
   return ConstString(ss.GetString());
 }
 
-Status ObjectFile::LoadInMemory(Target &target, bool set_pc) {
-  Status error;
-  ProcessSP process = target.CalculateProcess();
-  if (!process)
-return Status("No Process");
-  if (set_pc && !GetEntryPointAddress().IsValid())
-return Status("No entry address in object file");
-
+std::vector
+ObjectFile::GetLoadableData(Target &target) {
+  std::vector loadables;
   SectionList *section_list = GetSectionList();
   if (!section_list)
-return Status("No section in object file");
+return loadables;
+  // Create a list of loadable data from loadable sections
   size_t section_count = section_list->GetNumSections(0);
   for (size_t i = 0; i < section_count; ++i) {
+LoadableData loadable;
 SectionSP section_sp = section_list->GetSectionAtIndex(i);
-addr_t addr = target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
-if (addr != LLDB_INVALID_ADDRESS) {
-  DataExtractor section_data;
-  // We can skip sections like bss
-  if (section_sp->GetFileSize() == 0)
-continue;
-  section_sp->GetSectionData(section_data);
-  lldb::offset_t written = process->WriteMemory(
-  addr, section_data.GetDataStart(), section_data.GetByteSize(), error);
-  if (written != section_data.GetByteSize())
-return error;
-}
-  }
-  if (set_pc) {
-ThreadList &thread_list = process->GetThreadList();
-ThreadSP curr_thread(thread_list.GetSelectedThread());
-RegisterContextSP reg_context(curr_thread->GetRegisterContext());
-Address file_entry = GetEntryPointAddress();
-reg_context->SetPC(file_entry.GetLoadAddress(&target));
+loadable.Dest =
+target.GetSectionLoadList().GetSectionLoadAddress(section_sp);
+if (loadable.Dest == LLDB_INVALID_ADDRESS)
+  continue;
+// We can skip sections like bss
+if (section_sp->GetFileSize() == 0)
+  continue;
+DataExtractor section_data;
+section_sp->GetSectionData(section_data);
+loadable.Contents = llvm::ArrayRef(section_data.GetDataStart(),
+section_data.GetByteSize());
+loadables.push_back(loadable);
   }
-  return error;
+  return loadables;
 }
 
 void ObjectFile::RelocateSection(lldb_private::Section *section)
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===
--

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

[Owen Shaw via Phabricator via lldb-commits]

owenpshaw added a comment.

In https://reviews.llvm.org/D42145#1034499, @labath wrote:

> I like this a lot. That's the kind of change I wanted to do as a follow-up 
> one day. Thank you.


Thanks!  I don't have commit access to land this.


https://reviews.llvm.org/D42145



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

[Lldb-commits] [PATCH] D44738: Add a test for setting the load address of a module with differing physical/virtual addresses

[Owen Shaw via Phabricator via lldb-commits]

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

Looks good to me.  Thanks!


https://reviews.llvm.org/D44738



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