This is an archive of the discontinued LLVM Phabricator instance.

remove File::SetStream(), make new files instead.
ClosedPublic

Authored by lawrence_danna on Sep 22 2019, 10:58 AM.

Details

Summary

This patch removes File::SetStream() and File::SetDescriptor(),
and replaces most direct uses of File with pointers to File.
Instead of calling SetStream() on a file, we make a new file and
replace it.

My ultimate goal here is to introduce a new API class SBFile, which
has full support for python io.IOStream file objects. These can
redirect read() and write() to python code, so lldb::Files will
need a way to dispatch those methods. Additionally it will need some
form of sharing and assigning files, as a SBFile will be passed in and
assigned to the main IO streams of the debugger.

In my prototype patch queue, I make File itself copyable and add a
secondary class FileOps to manage the sharing and dispatch. In that
case SBFile was a unique_ptr<File>.
(here: https://github.com/smoofra/llvm-project/tree/files)

However in review, Pavel Labath suggested that it be shared_ptr instead.
(here: https://reviews.llvm.org/D67793)

In order for SBFile to use shared_ptr<File>, everything else should
as well.

If this patch is accepted, I will make SBFile use a shared_ptr
I will remove FileOps from future patches and use subclasses of File
instead.

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2019, 10:58 AM
lawrence_danna edited the summary of this revision. (Show Details)

updated description

While I am fully in favour of removing the SetXXX methods, and it does seem like some amount of shared_ptrs will be needed at the (though I don't fully understand your vission yet), I don't really like this proliferation of shared_ptrs everywhere (there's way too many of shared_ptrs in lldb already). I'd like it to be possible to use these APIs in the simplest cases without any shared_ptrs involved. For instance, instead of Status FileSystem::Open(FileSP &, FileSpec, uint32_t, uint32_t, bool), we could have Expected<File> FileSystem::Open(FileSpec, uint32_t, uint32_t, bool) or Expected<std::unique_ptr<File>> FileSystem::Open(FileSpec, uint32_t, uint32_t, bool). The first option would require making the File class movable (so you could write file = File(...) instead of file.SetXXX(...), while the second option would enable fully immutable File objects. Given that we already have File::Close (and ergo, a natural representation for the moved-from state), I'd try to go for the first option.

Could we make changes like that first, and later decide how much shared_ptring needs to be done? One can always convert a non-shared pointer into a shared one, but the other direction is more tricky. In the mean time, I'll respond to the lldb-dev thread you started (thanks for doing that!) to get a better idea of what you're trying to do here.

lldb/include/lldb/Core/Debugger.h
123–131

The lldb_private qualification is superfluous here.

....The first option would require making the File class movable....

I don't think making File movable will work. I'll be wanting a PythonFile subclass, which overrides virtual methods in File. It won't be possible to move one of those into a File variable. I do think the second option, with unique_ptr does make sense.

lawrence_danna edited the summary of this revision. (Show Details)

converted many uses of shared_ptr<File> into unique_ptr

@labath How's it look now? I made FileSystem::Open return a unique_ptr, and only convert that over to a shared_ptr where necessary.

Yeah, after writing the previous comment, I realized that you'll probably want to subclass File to implement the python stuff (which I agree is a good idea). So, going for unique_ptr<File> is probably the best way forward here. Nonetheless, I still believe the return type of the FileSystem method should be Expected<unique_ptr<File>>, as that is the direction both llvm and lldb is trying to move in (with llvm being much more successful at that, but lldb is making progress too). If it turns out that a lot of the callers are unable to do anything useful with the returned error (with llvm::Expected, you are forced to handle it somehow), we can make a special overload like OpenIngoringErrors which avoids the boilerplate needed to explicitly ignore an error (but I hope that won't be needed).

The other thing is that the interface changes in the Debugger class are causing a lot of churn, which makes it hard to figure out what exactly is changing. Would it be possible to split these out into a separate patch, so that the functional changes are more obvious here?

lldb/source/Host/common/FileSystem.cpp
439–440

once this returns Expected<unique_ptr<File>>, you can do a return llvm::errorCodeToError(std::error_code(errno, std::generic_category())) here.

444–445

I don't think this is really needed as the else branch is guaranteed to return a valid File object, right? (i.e., you could just do return std::make_unique(...) there...)

lldb/source/Interpreter/CommandInterpreter.cpp
2319–2320

the lldb convention would be to name these xxx_up

lawrence_danna edited the summary of this revision. (Show Details)

converted to Expected<FileUP>, and split into two separate patches

Thanks for splitting these up. It makes it much more obvious what is going on.

I see now that you're making use of this SetFile method which I wanted to remove in the previous patch. I kind of like that, because it pushes the Set one level up (so it now happens on the Stream instead of the File), but I do wonder if we shouldn't remove it completely. It seems like we could have Debugger::SetXXXFileHandle just create a fresh stream instead of twiddling with the existing one. This has the potential to change behavior as this change will not propagate to anybody who has fetched the stream_sp previously. However, I'm not sure if this is a change we care about because if anybody was writing to a stream while we were in the process of changing it, then he would likely trigger a race condition anyway. This way, we risk some code not immediately noticing the stream change, but at least we're thread safe. @jingham, what do you think about that?

Other than that, I am pretty happy with how this looks, and I think you've convinced me that the shared pointer inside the StreamFile object is really needed.

@labath how's this one looking? need any changes?

labath accepted this revision.Sep 27 2019, 1:15 AM

looks good to me

lldb/source/Commands/CommandObjectGUI.cpp
32–33

This looks like it would fit on one line. Did you run clang-format?

This revision is now accepted and ready to land.Sep 27 2019, 1:15 AM
lawrence_danna marked 3 inline comments as done.

style fixes.

fixed style issues

updated commit message

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2019, 7:34 AM