This is an archive of the discontinued LLVM Phabricator instance.

Enhance the Pipe interface for better portability
ClosedPublic

Authored by zturner on Dec 16 2014, 11:40 AM.

Details

Summary

This patch makes a number of improvements to the Pipe interface.

  1. An interface (PipeBase) is provided which exposes pure virtual methods for any implementation of Pipe to override. While not strictly necessary, this helps catch errors where the interfaces are out of sync. In fact, this would have caught a subtle bug on Windows where the signature of the Open() method had a compatible but different signature to the Posix version due to the use of default arguments. This guarantees that the interfaces remain in sync.
  1. All methods return lldb_private::Error instead of bools, or in some cases nothing. This allows richer error information to be propagated up to LLDB.
  1. A new ReadWithTimeout() method is exposed in the base class and implemented on Windows.
  1. Support for both named and anonymous pipes is exposed through the base interface and implemented on Windows. For creating a new pipe, both named and anonymous pipes are supported, and for opening an existing pipe, only named pipes are supported.

The new interface will need to be implemented in PipePosix before this can go live. I was hoping Oleksiy could do that at the same time he puts the logic back for reading the port from debugserver's pipe with timeout. This new interface should provide sufficient flexibility that this will be portable on all platforms.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 17346.Dec 16 2014, 11:40 AM
zturner retitled this revision from to Enhance the Pipe interface for better portability.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: clayborg, ovyalov.
zturner added a subscriber: Unknown Object (MLST).
zturner updated this revision to Diff 17358.Dec 16 2014, 1:06 PM

I went ahead and updated the Posix side to conform to the base interface. I compiled on MacOSX and there were no errors. There should be no functionality change here, so this patch is actually probably ok to go in as-is. However, a few methods are still unimplemented. These are all new methods that didn't exist before, so having them unimplemented shouldn't cause anything to break that was previously working. They are in PipePosix.cpp, and should be obvious by inspecting the patch.

Hopefully Oleksiy can fill out these stubs in a followup patch, but I think this patch is ready to go in after review.

clayborg accepted this revision.Dec 16 2014, 1:40 PM
clayborg edited edge metadata.

Looks fine.

This revision is now accepted and ready to land.Dec 16 2014, 1:40 PM
ovyalov edited edge metadata.Dec 16 2014, 2:10 PM

Please see my comments.

include/lldb/Host/PipeBase.h
27 ↗(On Diff #17358)

Minor thing - you may have name passed as a first parameter so a signature will be in accord with open sys call syntax - open(name, flags)

source/Host/posix/PipePosix.cpp
66 ↗(On Diff #17358)

Please add error.SetErrorToErrno();

76 ↗(On Diff #17358)

Could you call error.SetErrorToErrno(); before Close() to make sure that Close doesn't overwrite errno?

source/Host/windows/PipeWindows.cpp
116 ↗(On Diff #17358)

Just out of curiosity - why are you using std::string::data here, not c_str?

zturner added inline comments.Dec 16 2014, 2:17 PM
include/lldb/Host/PipeBase.h
27 ↗(On Diff #17358)

Sounds reasonable.

source/Host/posix/PipePosix.cpp
76 ↗(On Diff #17358)

Yea, good point. I'll fix this.

source/Host/windows/PipeWindows.cpp
116 ↗(On Diff #17358)

This isn't std::string::data, it's llvm::StringRef::data. llvm::StringRef is a wrapper around a char *, but it's immutable and doesn't own the string data. It just provides convenience methods like length(), equals_lower(), etc.

However, it doesn't provide a c_str() method. The reason for that is because StringRef can be used to refer to non-null terminated string data. So it stores the length internally. In theory they could provide a c_str() method that's the same as the data() method, but they argue that people will be confused and assume that the result of c_str() is null terminated. If you create the StringRef yourself, and you know it refers to a null terminated string, then it's safe to treat data() as a null terminated string.

zturner updated this revision to Diff 17371.Dec 16 2014, 3:14 PM
zturner edited edge metadata.

Updated with changes suggested by ovyalov

Updated with changes suggested by ovyalov

I missed one of your requested changes (adding the call to error.SetErrorToErrno()). I fixed that after I uploaded the last patch, but it's probably not worth uploading a new patch just for that one line. Let me know if there's anything else though.

ovyalov accepted this revision.Dec 16 2014, 4:32 PM
ovyalov edited edge metadata.

Looks good - I ran your patch on Ubuntu, no regression.

Just minor things.

include/lldb/Host/posix/PipePosix.h
34 ↗(On Diff #17371)

You may follow the same style here as you did in PipeWindows - no need to have virtual prefix.

55 ↗(On Diff #17371)

It seems an indentation issue - could you fix it?

source/Host/posix/ConnectionFileDescriptorPosix.cpp
297 ↗(On Diff #17371)

return (result.Success() && bytes_written == 1) ?

337 ↗(On Diff #17371)

Do we need to log result variable instead of bytes_written?

zturner added inline comments.Dec 16 2014, 4:40 PM
source/Host/posix/ConnectionFileDescriptorPosix.cpp
297 ↗(On Diff #17371)

It seems redundant to check for bytes_written == 1. A postcondition of Pipe::Write should be that result.Success() implies bytes_written == bytes_requested.

337 ↗(On Diff #17371)

Seems reasonable. After fixing these issues i'll go ahead and commit.

ovyalov added inline comments.Dec 16 2014, 4:48 PM
source/Host/posix/ConnectionFileDescriptorPosix.cpp
297 ↗(On Diff #17371)

Quote from PipePosix::Write:

int result = write(fd, buf, num_bytes);
        if (result >= 0)
            bytes_written = result;

So, if write returns 0 Write still will be successful.

zturner added inline comments.Dec 16 2014, 5:00 PM
source/Host/posix/ConnectionFileDescriptorPosix.cpp
297 ↗(On Diff #17371)

Is it possible to re-write it in such a way that Read() blocks until either success, failure, or EOF, and Write() blocks until either success or failure?

I'm not familiar with all the posix intricacies, but a quick look at the man page suggests that the only case this might happen is when the pipe's output buffer is full. But shouldn't we require that Pipe::Write() blocks until the data is written or an error occurs? Of course, the function might need to be modified in order to guarantee that (similarly for read).

If we can agree that the functions should behave that way (even if they currently aren't), then maybe you could do that at the same time you implement the fifo and ReadWithTimeout logic? It might be easier for someone who is more familiar with linux than I am.

ovyalov added inline comments.Dec 16 2014, 5:51 PM
source/Host/posix/ConnectionFileDescriptorPosix.cpp
297 ↗(On Diff #17371)

Yes, I think it makes to implement Read/Write in the way that you suggested and such change shouldn't affect the code that potentially anticipates possible partial read/writes.
It's definitely should be done in another CL - I can take on this later.

This revision was automatically updated to reflect the committed changes.