This is an archive of the discontinued LLVM Phabricator instance.

update SBDebugger::SetInputFile() etc to work on native Files
ClosedPublic

Authored by lawrence_danna on Oct 9 2019, 2:44 PM.

Details

Summary

This patch adds FileSP versions of SetInputFile(),
SetOutputFile, and SetErrorFile(). SWIG will convert native
python file objects into FileSP.

Diff Detail

Event Timeline

lawrence_danna created this revision.Oct 9 2019, 2:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2019, 2:44 PM
clayborg added inline comments.
lldb/include/lldb/API/SBDebugger.h
97–102

Are these really for public consumption? I would hope not, and might add then as protected and friend in any code that needs to use them? At the very least a comment specifying these are for internal use. Does SBFile not have a way to construct with a FileSP? Maybe these are not needed if that is the case?

lawrence_danna marked an inline comment as done.Oct 9 2019, 3:12 PM
lawrence_danna added inline comments.
lldb/include/lldb/API/SBDebugger.h
97–102

They are kind of for public consumption. They're impossible to use from a C++ client, because they can't see the definition of lldb_private::File.

But the SWIG wrappers can bind against them. They will convert a native python file
into a FileSP.

That way SetInputFile works just like SetInputFileHandle works, except it doesn't have the bugs and limitations that SetInputFileHandle has. Normally you can just use the FileSP version, and not create a SBFile. But in some cases creating a SBFile manually is required, such as if you want to control how the file object is translated from python into C++.

lawrence_danna marked an inline comment as done.Oct 9 2019, 3:12 PM
lawrence_danna marked an inline comment as done.Oct 9 2019, 3:17 PM
lawrence_danna added inline comments.
lldb/include/lldb/API/SBDebugger.h
97–102

If I mad them protected, with a friend declaration, the friend would have to be some name that is auto-generated by SWIG.

lawrence_danna marked an inline comment as done.Oct 9 2019, 3:57 PM
lawrence_danna added inline comments.
lldb/include/lldb/API/SBDebugger.h
97–102

@clayborg, you can see the same thing happening with FileSP already in SBFile.i, and SBCommandReturnObject.i. My plan is to convert all the FILE* interfaces in the swig files into FileSP, and ultimately delete the FILE* typemap. From python's perspective then new bindings will work the same way as the old, and the FILE* methods will still be there for C++ clients.

labath accepted this revision.Oct 10 2019, 1:35 AM

Looks fine to me. The public-consumptionness of the FileSP overloads was also discussed in D68546, and I currently do not see a better solution than this one. The way I've resolved this issue for myself is to look at the FileSP argument as a placeholder for some hypothetical future feature which would allow C++ users to pass in custom file objects into lldb (i.e. do the same thing that python can do now). If/when we have that, we can replace FileSP with the new thing, and have the python objects typemap to that.

This revision is now accepted and ready to land.Oct 10 2019, 1:35 AM

Sounds good. LGTM then. Thanks for the explanation. I was out on vacation for a week and catching up on email.

This revision was automatically updated to reflect the committed changes.