This is an archive of the discontinued LLVM Phabricator instance.

[lldb-server] Improve support on Windows
ClosedPublic

Authored by asmith on Jan 2 2019, 4:16 PM.

Details

Summary

This commit contains the following changes:

  • Rewrite vfile close/read/write packet handlers with portable routines from lldb. This removes #if(s) and allows the handlers to work on Windows.
  • Fix a bug in File::Write. This is intended to write data at an offset to a file but actually writes at the current position of the file.
  • Add a default boolean argument 'should_close_fd' to FileSystem::Open to let the user decide whether to close the fd or not.

Diff Detail

Event Timeline

asmith created this revision.Jan 2 2019, 4:16 PM
labath added a subscriber: labath.

Nice to see some action on the lldb-server for windows front. It looks like it should be easy to add a unit test for the File::Write bug you fixed. Can you add something like that?

Hui added a subscriber: Hui.Jan 4 2019, 1:05 PM

With all the aaron's pending reviews on lldb-server, I could try the patch with the following platform apis.

Seems to me is working on Windows and no regression on Linux side. Some difference (performance) might be

(1) Previous, the vfile:write packet has a maximum as 1024 bytes, now it turns to 16384 byte from observations.
(2) FileSystem::Instance().Open will introduce some performance bumps however I think it is minor along with the ctor/dtor of the File

I think the python tests already cover the changes in this commit, especially for Linux.

Not applicable for Windows I think, since we need the availability of lldb-server.exe.

Remote:
./lldb-server.exe p --listen *:2000 --log-channels="lldb all:gdb-remote all:windows all"

LLDB:
platform select remote-windows
platform connect <URL>
file a.exe
r

source/Host/common/File.cpp
608

I think this line makes Windows equivalence since posix part is using 'pwrite' which means to write from a set offset in the file.

zturner added inline comments.Jan 7 2019, 10:14 AM
source/Host/common/File.cpp
607–609

Be careful here. pwrite on posix is atomic, which means that if multiple threads both use pwrite at different offsets, there is no race condition. The only way to do a similar atomic write-at-offset on Windows is with overlapped i/o., and it's a completely different API / interface.

I mention this because in a previous review Pavel suggested to change lldb-server to spawn 1 thread / connection instead of 1 process / connection, so this can be a real problem in such a scenario if they are racing for the same file. Probably they won't be, since each connection will use different output files, but we should at least be aware of (and perhaps document) the different in atomicity semantics here.

Alternatively, we could just err on the side of safety and put this in a mutex.

asmith updated this revision to Diff 180992.Jan 9 2019, 7:43 PM

Added lock suggested in review

asmith marked 2 inline comments as done.Jan 9 2019, 7:44 PM

Any other comments on this one?

zturner accepted this revision.Feb 7 2019, 10:29 AM
This revision is now accepted and ready to land.Feb 7 2019, 10:29 AM
asmith updated this revision to Diff 185817.Feb 7 2019, 10:44 AM
asmith closed this revision.Feb 7 2019, 10:46 AM