This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Add serial:// protocol for connecting to serial port
ClosedPublic

Authored by mgorny on Oct 7 2021, 2:18 PM.

Details

Summary

Add a new serial:// protocol along with SerialPort that provides a new
API to open serial ports. The URL consists of serial device path
followed by URL-style options, e.g.:

serial:///dev/ttyS0?baud=115200&parity=even

If no options are provided, the serial port is only set to raw mode
and the other attributes remain unchanged. Attributes provided via
options are modified to the specified values. Upon closing the serial
port, its original attributes are restored.

Diff Detail

Event Timeline

mgorny requested review of this revision.Oct 7 2021, 2:18 PM
mgorny created this revision.

This looks reasonable to me. In case the url does not specify port parameters, it might be better to set them to known sane defaults instead of leaving them unchanged. That way lldb and lldb-server could "just connect". However, I don't know much about serial ports so I'm leaving that up to you.

I don't really have a strong opinion about this. I think we should leave the option of leaving port settings unchanged. I'll check what gdb does, as I suppose interoperability with gdbserver is also useful. I guess people are more likely to connect to other servers than lldb over serial port after all.

mgorny updated this revision to Diff 380731.Oct 19 2021, 10:50 AM
mgorny retitled this revision from [lldb] Add serial:// protocol for connecting to serial port [WIP/PoC] to [lldb] Add serial:// protocol for connecting to serial port.

Move serial:// URL parsing to SerialPort class. Improve error handling.

labath added inline comments.Oct 20 2021, 4:06 AM
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
790–792

It would be better to hide this by making the constructor private and have a static factory function returning Expected<unique_ptr<SerialPort>>. Bonus points if you can move all the fallible operations into the factory function so that the (now private) constructor does not need the Error argument at all

mgorny added inline comments.Oct 20 2021, 4:15 AM
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
790–792

I know but I couldn't make this work — the compiler just won't find a way to convert the new SerialPort instance into Expected<SerialPort>. I suspect I would have to add move ctors and assignment operators all the way up the class hierarchy.

labath added inline comments.Oct 20 2021, 4:17 AM
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
790–792

That's where the unique_ptr part comes in. :)

mgorny added inline comments.Oct 20 2021, 7:24 AM
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
790–792

I presume you mean shared_ptr here, or know some dirty trick.

mgorny updated this revision to Diff 380954.Oct 20 2021, 7:48 AM

Use a fallible ctor approach.

labath added inline comments.Oct 20 2021, 8:50 AM
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
790–792

a unique_ptr is convertible to a shared_ptr, but not the other way around, so it's better to return the former

mgorny updated this revision to Diff 380985.Oct 20 2021, 9:11 AM

Use unique_ptr.

mgorny marked 3 inline comments as done.Oct 20 2021, 9:11 AM
mgorny added inline comments.
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
790–792

Ah, indeed. I was missing std::move.

labath accepted this revision.Oct 21 2021, 12:30 AM
labath added inline comments.
lldb/source/Host/common/File.cpp
847

Empty line here

This revision is now accepted and ready to land.Oct 21 2021, 12:30 AM
mgorny marked an inline comment as done.Oct 21 2021, 1:17 AM
mgorny marked an inline comment as done.Oct 21 2021, 1:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2021, 1:47 AM