[Lldb-commits] [PATCH] D62502: Implement xfer:libraries-svr4:read packet

[António Afonso via Phabricator via lldb-commits]

aadsm marked 3 inline comments as done.
aadsm added inline comments.


================
Comment at: lldb/packages/Python/lldbsuite/test/tools/lldb-server/TestGdbRemoteLibrariesSvr4Support.py:51-52
+
+        OFFSET = 0
+        LENGTH = 0xFFFF
+
----------------
labath wrote:
> Why are these all caps? TBH, you probably don't need to make these variables at all. You can just place them directly into the packet as strings.
tbh I mostly copy pasted the auxv test :D but yeah it can be made simpler.


================
Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:2197
+
+  info.name = std::string(name_buffer);
+  info.link_map = link_map_addr;
----------------
labath wrote:
> What if `name_buffer` is not null-terminated?
Nicely spotted. I should add the size but this made me realize I'm not ensuring a `\0` on my next diff where I implement ReadCStringFromMemory.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2778
+    for (auto const &library : library_list) {
+      response.Printf("<library name=\"%s\" ", library.name.c_str());
+      response.Printf("lm=\"0x%" PRIx64 "\" ", library.link_map);
----------------
labath wrote:
> You're completely ignoring escaping here, which is a pretty complex topic, and one of the reasons why constructing this via the DOM api is so complicated. There are a couple of considerations here:
> - `"`, `&`, and other special xml characters: these will definitely need to be xml-escaped properly
> - non-7-bit characters: how these will come out depends on whether the client&server agree on the encoding used. I'm not sure what's the best thing to do here
> - low ascii (nonprintable) characters: it looks like these cannot be represented in xml (1.0) at all (?)
> 
> It might be good to check what gdb does in these situations
Oh yeah, this is a path so of course that will happen. (we probably should update ds2 for that as well).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62502