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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

First, I'd like to thank you for taking the time to create a method for testing patches like these. I think this will be very valuable for all out-of-tree stub support patches we will get in the future. Could I ask that you split out the generic test-suite-stuff part of this into a separate patch. This will make it easier to review and make sure it runs on the various buildbots that we have.

My first batch of comments is below, but I'll need to take a closer look at this one more time.



================
Comment at: include/lldb/Target/Process.h:1916
+  //------------------------------------------------------------------
+  virtual bool BeginWriteMemoryBatch() { return true; }
+
----------------
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<uint8_t> Contents; };
Status WriteMemory(llvm::ArrayRef<WriteEntry>);
```
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.


================
Comment at: packages/Python/lldbsuite/test/functionalities/gdb_remote_client/TestGDBRemoteLoad.py:43
+
+            MEMORY_MAP = """<?xml version="1.0"?>
+<memory-map>
----------------
We will need a way to skip this test when lldb is compiled without libxml support. This will be a bit tricky, as right now we don't have a mechanism for passing build-configuration from cmake into dotest. I guess we will need some equivalent of `lit.site.cfg.in`.

The thing that makes this tricky is that this will need to work from xcode as well. As a transitional measure we could probably assume "true" if we don't have that file, as I think the Xcode project is always configured to use libxml.


================
Comment at: packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py:13
+    # 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):
----------------
We should also add yaml2obj as a dependency of the check-lldb target in cmake.


================
Comment at: packages/Python/lldbsuite/test/functionalities/gdb_remote_client/gdbclientutils.py:301
+
+    mydir = TestBase.compute_mydir(__file__)
+    server = None
----------------
You can set `NO_DEBUG_INFO_TESTCASE = True` here, and avoid the decorators on every test case.


================
Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2962
+    packet.Printf("vFlashWrite:%" PRIx64 ":", addr);
+    GDBRemoteCommunication::WriteEscapedBinary(packet, buf, size);
+  } else {
----------------
This function is unnecessary. You should use StreamGDBRemote::PutEscapedBytes.


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


https://reviews.llvm.org/D42145