[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Host/posix/ProcessLauncherPosixFork.cpp:293
+    r = llvm::sys::RetryAfterSignal(-1, read, pipe.GetReadFileDescriptor(), pos,
+                                    buf.end() - pos);
+  } while (r > 0);
----------------
rupprecht wrote:
> IIUC, this will overrun the buffer if there are >1000 bytes to read; whereas previously we just wouldn't have read everything.
> 
> Should each loop iteration grow the buffer by a certain amount? Otherwise I think we need to remove the loop.
It won't overrun because I am passing the remaining part of the buffer as the size argument. However, I am not totally sure what would happen if we actually filled the buffer (and size becomes zero).  That won't happen right now because the error string is coming from `ExitWithError` (line 50) and its length is limited by the longest errno message. That said, the dynamical resize is quite simple in this case, so I might as well do it.


================
Comment at: lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py:18
+        args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"]
+        hex_args = [seven.hexlify(x) for x in args]
+
----------------
mgorny wrote:
> labath wrote:
> > mgorny wrote:
> > > Since we no longer support Python 2, I'd rather prefer to work towards removing `seven` rather than making more use of it.
> > Is the problem with the name/location of the function or the functionality (string/byte conversion) itself?
> > Because, if it's the first, then that could easily be solved by renaming the module (now or later), but in order to avoid elaborate casts we'd have to make all code be byte/string correct. This is not a problem here (because of the fixed strings), but it becomes one once you start working with things that aren't necessarily valid utf8 strings.
> Ok, I suppose this makes sense given how LLDB's Python API is screwed up :-(.
Just to clarify, I would like if we used the byte/string separation more (and I started that when I refactored the gdb server test harness), but it's fairly tricky because we're often working with things (e.g. memory contents, or stdout) that can represent arbitrary data in general, but which usually (for our own convenience and sanity) contain simple ASCII strings. So we have a problem where the API (and here I just mean the gdbserver test API, which we can change) really wants us to use bytes, but the the most natural way to write the test is with strings. I haven't come up with a solution to that yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133352