new api class: SBFile

[lawrence_danna]

Summary

SBFile is a scripting API wrapper for lldb_private::File

This is the first step in a project to enable arbitrary python
io.IOBase file objects -- including those that override the read()
and write() methods -- to be used as the main debugger IOStreams.

Currently this is impossible because python file objects must first
be converted into FILE* streams by SWIG in order to be passed into
the debugger.

full prototype: https://github.com/smoofra/llvm-project/tree/files

[labath]

This is an interesting undertaking. There's a lot of confusion about FILE* in lldb, and it would be great to clean some of that stuff up.

The api seems reasonably straight-forward. The one thing that struck me is that you're using unique_ptrs for the pimpled objects. That would mean the SBFile objects are non-copyable, which is generally not a good thing, as python really likes to copy things. Looking ahead at your patchset, it looks like I am onto something, as you're then trying to implement shared semantics inside lldb_private::File. That doesn't sound like the most fortunate solution to me, as most of the lldb_private::File users don't need that functionality. I am wondering if it wouldn't be better to use a shared_ptr in SBFile, and implement any extra sharing semantics you need at the SB level. Also, this needs a bunch of tests.

From a higher level perspective, it would be good to have a couple of paragraphs saying what exactly is the end goal here (what new apis will you introduce, how will they work, etc.). I can sort of guess some of that from looking at the patchset, but it would be nice to have that written down, so that we know we're on the same page. This looks like it is a sufficiently big of a change that it would deserve a quick RFC email to lldb-dev.

lldb/source/API/SBFile.cpp

20–26

I think it would be better if these were constructors instead of member functions. That way you might be able to get rid of the all the if (!m_opaque_up) { checks as the File member will always be initialized.

21

It looks like you need to reformat this file.

[lawrence_danna]

In D67793#1676592, @labath wrote:

The api seems reasonably straight-forward. The one thing that struck me is that you're using unique_ptrs for the pimpled objects. That would mean the SBFile objects are non-copyable, which is generally not a good thing, as python really likes to copy things. Looking ahead at your patchset, it looks like I am onto something, as you're then trying to implement shared semantics inside lldb_private::File. That doesn't sound like the most fortunate solution to me, as most of the lldb_private::File users don't need that functionality. I am wondering if it wouldn't be better to use a shared_ptr in SBFile, and implement any extra sharing semantics you need at the SB level. Also, this needs a bunch of tests.

That was my initial thought as well, to use a shared_ptr here, and not have any sharing at the lldb_private::File level. However, I found I couldn't do it that way. lldb_private::File doesn't really have the semantics of a file object, it has the semantics of a variable, which can point to a file object. Users of lldb_private::File expect to be able to Close() a file and re-open it as something else. They expect to be able to embed File immediately in other objects. They expect to be able to make an empty File and check it with IsValid(). I thought it would be best to let them keep using File in that way, so later in the queue I make File copyable and add FileOps class to manage the sharing between copied files. The other way to do it would be to track down every use of lldb_private::File that isn't through a pointer and convert it to shared_ptr<File> instead. Then SBFile could just be another shared_ptr<File>. Should I do it that way?

As far as python copying things goes, that's not a problem. SWIG will always allocate a SBFile on the heap and copy the pointers to it, it doesn't need an actual C++ copy constructor.

[lawrence_danna]

added some tests, rebased

[lawrence_danna]

@labath So what do you think?
Update all the lldb_private::File clients to use shared_ptr<File> instead?
Or do it this way, with sharing handled inside of File?

[lawrence_danna]

@labath

I wrote a patch for the shared_ptr approach. It's simple, but it touches a lot of lines.

https://reviews.llvm.org/D67891

Now that I've gone and done it, I kind of like it better that way. If you approve the other patch, I'll update this one accordingly.

[labath]

In D67793#1678259, @lawrence_danna wrote:

@labath

I wrote a patch for the shared_ptr approach. It's simple, but it touches a lot of lines.

https://reviews.llvm.org/D67891

Now that I've gone and done it, I kind of like it better that way. If you approve the other patch, I'll update this one accordingly.

Thanks for the quick turnaround. /I think/ the approach in the other patch looks better (I am super happy about the removal of the SetXXX methods), but I'd still like to understand whether we need to introduce that many shared_ptrs, so I'll need to think about this some more (I just got back from vacation and I am still working through my backlog).

[lawrence_danna]

changed unique_ptr to shared_ptr

[lawrence_danna]

lldb/source/API/SBFile.cpp

20–26

Unfortunately, SWIG does not allow return types that don't have default constructors.

https://github.com/swig/swig/issues/1062

[labath]

lldb/source/API/SBFile.cpp

20–26

Hmm.. that is unfortunate. I guess that means we still need the validity checks, but you should still be able to replace these with constructors, right? (you've guessed by now that I am not a fan of Set methods :) ).

[lawrence_danna]

rebased, and converted Set* methods into constructors.

[lawrence_danna]

@labath, OK sets are now constructors.

[lawrence_danna]

rebased, removed duplicate test

[labath]

Thanks. This looks good to me, but given that this is going to become a stable api that well need to maintain for a looong time, I'd like to get signoff from someone else as well. @jingham maybe ?

lldb/include/lldb/API/SBDefines.h

95

Sort this list ?

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py

27 ↗

(On Diff #222264)

Please add a comment to explain why is this needed (I guess that has something to do with needing more control over the command output stream), because superficially it looks like you're reimplementing TestBase.runCmd here.

37–38 ↗

(On Diff #222264)

Maybe instead of raise_on_fail this should be called check (in analogy to runCmd function), and instead of raising an exception you could assert that ret.Succeeded() is true?

52 ↗

(On Diff #222264)

I believe all of these will end up writing to the source tree. Could you try replacing that with self.getBuildArtifact("output") ?

127–132 ↗

(On Diff #222264)

delete

lldb/scripts/interface/SBFile.i

37

add operator bool here. I'm not sure if swig handles operator!, but if it does, then we should add that too.

lldb/source/API/SBFile.cpp

2

I think you'll need to add the reproducer boilerplate to this file. I think it should be possible to do that automatically via lldb-instr, but I've never done that. @JDevlieghere should be able to help here.

lldb/source/Host/common/File.cpp

72–74 ↗

(On Diff #222264)

I know you're just copying, but could you remove the .str() call the `.empty() check, as neither of them are really needed here.

[jingham]

This looks fine to me, it doesn't look like you are leaving out anything useful.

Note, I think just to enforce the "default constructor" discipline, there's a test in the test suite (python_api/default-constructor/TestDefaultConstructorForAPIObjects.py) that has a file per class that just makes a default constructed object and calls all of its methods to make sure they don't crash. According to that pattern your test_sbfile_invalid test should actually go there.

[lawrence_danna]

fixed according to reviewer comments

[lawrence_danna]

addressed comments

[lawrence_danna]

flush fix for python2

[lawrence_danna]

add reproducer instrumentation.

[lawrence_danna]

lldb/scripts/Python/python-typemaps.swig

464 ↗

(On Diff #222528)

swig/LICENSE says the following, so it should be fine to copy and paste these.

The SWIG library and examples, under the Lib and Examples top level 
directories, are distributed under the following terms:

  You may copy, modify, distribute, and make derivative works based on
  this software, in source code or object code form, without
  restriction. If you distribute the software to others, you may do
  so according to the terms of your choice. This software is offered as
  is, without warranty of any kind.

[lawrence_danna]

no inlines in the API

[lawrence_danna]

@labath any more comments on this one?

[labath]

Nah, this looks fine to me now.

[labath]

BTW, I've had to add (or rather, extend) a fairly ugly hack in r373573 in order to get the new tests running on non-darwin platforms. The reason for that is that the FILE* out typemap needs to get the "mode" of the object in order to construct the python wrapper.

My hope is that we can be improved once you're finished your work here. For instance by reimplementing GetOutputFileHandle on top of GetOutputFile in swig, as the latter will (hopefully) be able to get the mode of a file correctly. Does that sound reasonable, or am I totally off-track here?

[lawrence_danna]

In D67793#1692544, @labath wrote:

BTW, I've had to add (or rather, extend) a fairly ugly hack in r373573 in order to get the new tests running on non-darwin platforms. The reason for that is that the FILE* out typemap needs to get the "mode" of the object in order to construct the python wrapper.

My hope is that we can be improved once you're finished your work here. For instance by reimplementing GetOutputFileHandle on top of GetOutputFile in swig, as the latter will (hopefully) be able to get the mode of a file correctly. Does that sound reasonable, or am I totally off-track here?

oops, sorry I missed this. Are you happy with how it all turned out?

[labath]

In D67793#1713981, @lawrence_danna wrote:

In D67793#1692544, @labath wrote:

BTW, I've had to add (or rather, extend) a fairly ugly hack in r373573 in order to get the new tests running on non-darwin platforms. The reason for that is that the FILE* out typemap needs to get the "mode" of the object in order to construct the python wrapper.

My hope is that we can be improved once you're finished your work here. For instance by reimplementing GetOutputFileHandle on top of GetOutputFile in swig, as the latter will (hopefully) be able to get the mode of a file correctly. Does that sound reasonable, or am I totally off-track here?

oops, sorry I missed this. Are you happy with how it all turned out?

Yes, very much. Thank you for cleaning that up.

[mgorny]

While looking at the other bug, I noticed something suspicious here.

lldb/trunk/source/Host/common/File.cpp

74 ↗

(On Diff #222960)

Shouldn't this also include File::eOpenOptionCanCreate | File::eOpenOptionTruncate?

[labath]

lldb/trunk/source/Host/common/File.cpp

74 ↗

(On Diff #222960)

Yeah, I guess it should, though it probably does not matter in practice right now as we're only using this on already opened FILE* objects.

[vadimcn]

lldb/trunk/scripts/Python/python-typemaps.swig

481 ↗

(On Diff #222960)

Sorry for being late to the party, but I just stumbled upon this code...

It seems to return a pointer from a view that had just been released. Isn't this kind of risky? While most of the time buffer views point into object's internal memory, buffer protocol does not prohibit allocating memory just to fulfill the buffer request. In which case PyBuffer_Release would be expected to release that memory, leaving the caller with a dangling pointer.

[labath]

lldb/trunk/scripts/Python/python-typemaps.swig

481 ↗

(On Diff #222960)

Yes, that does seem worrying, though the swig typemap also does the same thing (but it could have the same bug too). @lawrence_danna, do you know anything about this? Should the buffer be released after the function returns (in the "out" typemap, I guess)?

[lawrence_danna]

lldb/trunk/scripts/Python/python-typemaps.swig

481 ↗

(On Diff #222960)

@vadimcn is correct. If GetBuffer allocates to fulfill the request, rather than returning a pre-existing buffer, then this will access a dangling pointer.

@labath "in" and "out" type-maps aren't paired up like that.

I believe there is a way you can instruct SWIG to create a C++ helper variable for a typemap. The solution in this case is to use such a temporary to carry the PyBuffer reference in RAII-style.

lldb/include/lldb/API/LLDB.h

 #include "lldb/API/SBDebugger.h"
 #include "lldb/API/SBDeclaration.h"
 #include "lldb/API/SBDefines.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBExecutionContext.h"
 #include "lldb/API/SBExpressionOptions.h"
 #include "lldb/API/SBFileSpec.h"
+#include "lldb/API/SBFile.h"
 #include "lldb/API/SBFileSpecList.h"
 #include "lldb/API/SBFrame.h"
 #include "lldb/API/SBFunction.h"
 #include "lldb/API/SBHostOS.h"
 #include "lldb/API/SBInstruction.h"
 #include "lldb/API/SBInstructionList.h"
 #include "lldb/API/SBLanguageRuntime.h"
 #include "lldb/API/SBLaunchInfo.h"

lldb/include/lldb/API/SBDefines.h

 class LLDB_API SBTypeSummaryOptions;
 class LLDB_API SBTypeSynthetic;
 class LLDB_API SBTypeList;
 class LLDB_API SBValue;
 class LLDB_API SBValueList;
 class LLDB_API SBVariablesOptions;
 class LLDB_API SBWatchpoint;
 class LLDB_API SBUnixSignals;
+class LLDB_API SBFile;
 
 typedef bool (*SBBreakpointHitCallback)(void *baton, SBProcess &process,
                                         SBThread &thread,
                                         lldb::SBBreakpointLocation &location);
 }
 
 #endif // LLDB_SBDefines_h_

lldb/include/lldb/API/SBError.h

protected:
   friend class SBProcess;
   friend class SBReproducer;
   friend class SBStructuredData;
   friend class SBTarget;
   friend class SBThread;
   friend class SBTrace;
   friend class SBValue;
   friend class SBWatchpoint;
+  friend class SBFile;
 
   lldb_private::Status *get();
 
   lldb_private::Status *operator->();
 
   const lldb_private::Status &operator*() const;
 
   lldb_private::Status &ref();

lldb/include/lldb/API/SBFile.h

+//===-- SBFile.h --------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_SBFile_h_
+#define LLDB_SBFile_h_
+
+#include "lldb/API/SBDefines.h"
+
+namespace lldb {
+
+class LLDB_API SBFile {
+public:
+
+  SBFile();
+  ~SBFile();
+
+
+  void SetStream(FILE *file, bool transfer_ownership);
+  void SetDescriptor(int fd, bool transfer_ownership);
+
+  SBError Read(uint8_t *buf, size_t num_bytes, size_t *bytes_read);
+  SBError Write(const uint8_t *buf, size_t num_bytes, size_t *bytes_written);
+  SBError Flush();
+  bool IsValid() const;
+  SBError Close();
+
+  operator bool() const { return IsValid(); }
+  bool operator !() const { return !IsValid(); }
+
+private:
+  DISALLOW_COPY_AND_ASSIGN(SBFile);
+  FileUP m_opaque_up;
+};
+
+} // namespace lldb
+
+#endif // LLDB_SBFile_h_

lldb/include/lldb/lldb-forward.h

 typedef std::shared_ptr<lldb_private::Event> EventSP;
 typedef std::shared_ptr<lldb_private::EventData> EventDataSP;
 typedef std::shared_ptr<lldb_private::EventDataStructuredData>
     EventDataStructuredDataSP;
 typedef std::shared_ptr<lldb_private::ExecutionContextRef>
     ExecutionContextRefSP;
 typedef std::shared_ptr<lldb_private::ExpressionVariable> ExpressionVariableSP;
 typedef std::shared_ptr<lldb_private::File> FileSP;
+typedef std::unique_ptr<lldb_private::File> FileUP;
 typedef std::shared_ptr<lldb_private::Function> FunctionSP;
 typedef std::shared_ptr<lldb_private::FunctionCaller> FunctionCallerSP;
 typedef std::shared_ptr<lldb_private::FuncUnwinders> FuncUnwindersSP;
 typedef std::shared_ptr<lldb_private::InlineFunctionInfo> InlineFunctionInfoSP;
 typedef std::shared_ptr<lldb_private::Instruction> InstructionSP;
 typedef std::shared_ptr<lldb_private::InstrumentationRuntime>
     InstrumentationRuntimeSP;
 typedef std::shared_ptr<lldb_private::IOHandler> IOHandlerSP;

lldb/scripts/interface/SBFile.i

+//===-- SWIG Interface for SBFile -----------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+%include <pybuffer.i>
+
+%pybuffer_binary(const uint8_t *buf, size_t num_bytes);
+%pybuffer_mutable_binary(uint8_t *buf, size_t num_bytes);
+
+namespace lldb {
+
+%feature("docstring",
+"Represents a file."
+) SBFile;
+
+class SBFile
+{
+public:
+
+    SBFile();
+    ~SBFile ();
+
+    void SetStream (FILE *file, bool transfer_ownership);
+    void SetDescriptor (int fd, bool transfer_ownership);
+
+    %feature("autodoc", "Read(buffer) -> SBError, bytes_read") Read;
+    SBError Read(uint8_t *buf, size_t num_bytes, size_t *OUTPUT);
+
+    %feature("autodoc", "Write(buffer) -> SBError, written_read") Write;
+    SBError Write(const uint8_t *buf, size_t num_bytes, size_t *OUTPUT);
+
+    void Flush();
+
+    bool IsValid() const;
+
+    SBError Close();
+};
+
+} // namespace lldb

lldb/scripts/lldb.swig

 #include "lldb/API/SBData.h"
 #include "lldb/API/SBDebugger.h"
 #include "lldb/API/SBDeclaration.h"
 #include "lldb/API/SBError.h"
 #include "lldb/API/SBEvent.h"
 #include "lldb/API/SBExecutionContext.h"
 #include "lldb/API/SBExpressionOptions.h"
 #include "lldb/API/SBFileSpec.h"
+#include "lldb/API/SBFile.h"
 #include "lldb/API/SBFileSpecList.h"
 #include "lldb/API/SBFrame.h"
 #include "lldb/API/SBFunction.h"
 #include "lldb/API/SBHostOS.h"
 #include "lldb/API/SBInstruction.h"
 #include "lldb/API/SBInstructionList.h"
 #include "lldb/API/SBLanguageRuntime.h"
 #include "lldb/API/SBLaunchInfo.h"

 %include "./interface/SBData.i"
 %include "./interface/SBDebugger.i"
 %include "./interface/SBDeclaration.i"
 %include "./interface/SBError.i"
 %include "./interface/SBEvent.i"
 %include "./interface/SBExecutionContext.i"
 %include "./interface/SBExpressionOptions.i"
 %include "./interface/SBFileSpec.i"
+%include "./interface/SBFile.i"
 %include "./interface/SBFileSpecList.i"
 %include "./interface/SBFrame.i"
 %include "./interface/SBFunction.i"
 %include "./interface/SBHostOS.i"
 %include "./interface/SBInstruction.i"
 %include "./interface/SBInstructionList.i"
 %include "./interface/SBLanguageRuntime.i"
 %include "./interface/SBLaunchInfo.i"

lldb/source/API/CMakeLists.txt

add_lldb_library(liblldb SHARED
   SBData.cpp
   SBDebugger.cpp
   SBDeclaration.cpp
   SBError.cpp
   SBEvent.cpp
   SBExecutionContext.cpp
   SBExpressionOptions.cpp
   SBFileSpec.cpp
+  SBFile.cpp
   SBFileSpecList.cpp
   SBFrame.cpp
   SBFunction.cpp
   SBHostOS.cpp
   SBInstruction.cpp
   SBInstructionList.cpp
   SBLanguageRuntime.cpp
   SBLaunchInfo.cpp

lldb/source/API/SBFile.cpp

+//===-- SBFile.cpp ------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/API/SBFile.h"
+#include "lldb/API/SBError.h"
+#include "lldb/Host/File.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+SBFile::~SBFile() {}
+
+SBFile::SBFile () {}
+
+void SBFile::SetStream(FILE *file, bool transfer_ownership) {
+    m_opaque_up = std::make_unique<File>(file, transfer_ownership);
+}
+
+void SBFile::SetDescriptor(int fd, bool transfer_owndership) {
+    m_opaque_up = std::make_unique<File>(fd, transfer_owndership);
+}
+
+SBError SBFile::Read(uint8_t *buf, size_t num_bytes, size_t *bytes_read) {
+    SBError error;
+    if (!m_opaque_up) {
+        error.SetErrorString("invalid SBFile");
+        *bytes_read = 0;
+    } else {
+        Status status = m_opaque_up->Read(buf, num_bytes);
+        error.SetError(status);
+        *bytes_read = num_bytes;
+    }
+    return error;
+}
+
+SBError SBFile::Write(const uint8_t *buf, size_t num_bytes, size_t *bytes_written) {
+    SBError error;
+    if (!m_opaque_up) {
+        error.SetErrorString("invalid SBFile");
+        *bytes_written = 0;
+    } else {
+        Status status = m_opaque_up->Write(buf, num_bytes);
+        error.SetError(status);
+        *bytes_written = num_bytes;
+    }
+    return error;
+}
+
+SBError SBFile::Flush() {
+    SBError error;
+    if (!m_opaque_up) {
+        error.SetErrorString("invalid SBFile");
+    } else {
+        Status status = m_opaque_up->Flush();
+        error.SetError(status);
+    }
+    return error;
+}
+
+bool SBFile::IsValid() const {
+    return m_opaque_up && m_opaque_up->IsValid();
+}
+
+SBError SBFile::Close() {
+    SBError error;
+    if (m_opaque_up) {
+        Status status = m_opaque_up->Close();
+        error.SetError(status);
+    }
+    return error;
+}
+
+