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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
