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

[lawrence_danna]

Summary

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

[clayborg]

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]

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]

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]

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]

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.

[clayborg]

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

lldb/include/lldb/API/SBDebugger.h

public:
   FILE *GetErrorFileHandle();
 
   SBError SetInputFile(SBFile file);
 
   SBError SetOutputFile(SBFile file);
 
   SBError SetErrorFile(SBFile file);
 
+  SBError SetInputFile(FileSP file);
+
+  SBError SetOutputFile(FileSP file);
+
+  SBError SetErrorFile(FileSP file);
+
   SBFile GetInputFile();
 
   SBFile GetOutputFile();
 
   SBFile GetErrorFile();
 
   void SaveInputTerminalState();
 

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

def test_sbfile_write_string(self):
         self.assertTrue(e.Success())
         self.assertEqual(n, 3)
         sbf.Close()
         self.assertTrue(f.closed)
 
 
     @add_test_categories(['pyapi'])
     @skipIf(py_version=['<', (3,)])
-    @expectedFailureAll() # fixme multiple problems with this
     def test_string_out(self):
         f = io.StringIO()
         status = self.debugger.SetOutputFile(f)
         self.assertTrue(status.Success())
         self.handleCmd("script 'foobar'")
         self.assertEqual(f.getvalue().strip(), "'foobar'")
 
 
     @add_test_categories(['pyapi'])
-    @expectedFailureAll() # FIXME need FileSP version of SBDebugger::SetErrorFile
     @skipIf(py_version=['<', (3,)])
     def test_string_error(self):
         f = io.StringIO()
         debugger = self.debugger
         status = debugger.SetErrorFile(f)
         self.assertTrue(status.Success())
         self.handleCmd('lolwut', check=False, collect_result=False)
         errors = f.getvalue()
def test_sbfile_out(self):
             status = self.debugger.SetOutputFile(sbf)
             self.assertTrue(status.Success())
             self.handleCmd('script 2+2')
         with open(self.out_filename, 'r') as f:
             self.assertEqual(f.read().strip(), '4')
 
 
     @add_test_categories(['pyapi'])
-    @expectedFailureAll() # FIXME need FileSP version of SBDebugger::SetErrorFile
     @skipIf(py_version=['<', (3,)])
     def test_file_out(self):
         with open(self.out_filename, 'w') as f:
             status = self.debugger.SetOutputFile(f)
             self.assertTrue(status.Success())
             self.handleCmd('script 2+2')
         with open(self.out_filename, 'r') as f:
             self.assertEqual(f.read().strip(), '4')
 
 
     @add_test_categories(['pyapi'])
     def test_sbfile_error(self):
         with open(self.out_filename, 'w') as f:
             sbf = lldb.SBFile(f)
             status = self.debugger.SetErrorFile(sbf)
             self.assertTrue(status.Success())
             self.handleCmd('lolwut', check=False, collect_result=False)
         with open(self.out_filename, 'r') as f:
             errors = f.read()
             self.assertTrue(re.search(r'error:.*lolwut', errors))
 
 
     @add_test_categories(['pyapi'])
-    @expectedFailureAll() # FIXME need FileSP version of SBDebugger::SetErrorFile
     def test_file_error(self):
         with open(self.out_filename, 'w') as f:
             status = self.debugger.SetErrorFile(f)
             self.assertTrue(status.Success())
             self.handleCmd('lolwut', check=False, collect_result=False)
         with open(self.out_filename, 'r') as f:
             errors = f.read()
             self.assertTrue(re.search(r'error:.*lolwut', errors))
def test_fileno_flush(self):
             sbf.Write(b'bar')
             sbf = None
             self.assertFalse(f.closed)
             f.seek(0)
             self.assertEqual(f.read(), 'foobar')
 
 
     @add_test_categories(['pyapi'])
-    @expectedFailureAll() # FIXME need FileSP version of SBDebugger::SetOutputFile
     def test_close(self):
         debugger = self.debugger
         with open(self.out_filename, 'w') as f:
             status = debugger.SetOutputFile(f)
             self.assertTrue(status.Success())
             self.handleCmd("help help", check=False, collect_result=False)
             # make sure the file wasn't closed early.
             f.write("\nZAP\n")
             lldb.SBDebugger.Destroy(debugger)
             # check that output file was closed when debugger was destroyed.
             with self.assertRaises(ValueError):
                 f.write("\nQUUX\n")
         with open(self.out_filename, 'r') as f:
             output = f.read()
             self.assertTrue(re.search(r'Show a list of all debugger commands', output))
             self.assertTrue(re.search(r'ZAP', output))
 
 
     @add_test_categories(['pyapi'])
     @skipIf(py_version=['<', (3,)])
-    @expectedFailureAll() # FIXME need FileSP version of SBDebugger::SetOutputFile
     def test_stdout(self):
         f = io.StringIO()
         status = self.debugger.SetOutputFile(f)
         self.assertTrue(status.Success())
         self.handleCmd(r"script sys.stdout.write('foobar\n')")
         self.assertEqual(f.getvalue().strip().split(), ["foobar", "7"])
 
 

lldb/scripts/interface/SBDebugger.i

public:
     SetAsync (bool b);
 
     bool
     GetAsync ();
 
     void
     SkipLLDBInitFiles (bool b);
 
+    %feature("autodoc", "DEPRECATED, use SetInputFile");
     void
     SetInputFileHandle (FILE *f, bool transfer_ownership);
 
+    %feature("autodoc", "DEPRECATED, use SetOutputFile");
     void
     SetOutputFileHandle (FILE *f, bool transfer_ownership);
 
+    %feature("autodoc", "DEPRECATED, use SetErrorFile");
     void
     SetErrorFileHandle (FILE *f, bool transfer_ownership);
 
+    %feature("autodoc", "DEPRECATED, use GetInputFile");
     FILE *
     GetInputFileHandle ();
 
+    %feature("autodoc", "DEPRECATED, use GetOutputFile");
     FILE *
     GetOutputFileHandle ();
 
+    %feature("autodoc", "DEPRECATED, use GetErrorFile");
     FILE *
     GetErrorFileHandle ();
 
     SBError
     SetInputFile (SBFile file);
 
     SBError
     SetOutputFile (SBFile file);
 
     SBError
     SetErrorFile (SBFile file);
 
+    SBError
+    SetInputFile (FileSP file);
+
+    SBError
+    SetOutputFile (FileSP file);
+
+    SBError
+    SetErrorFile (FileSP file);
+
     SBFile
     GetInputFile ();
 
     SBFile
     GetOutputFile ();
 
     SBFile
     GetErrorFile ();

lldb/source/API/SBDebugger.cpp

 }
 
 void SBDebugger::SetInputFileHandle(FILE *fh, bool transfer_ownership) {
   LLDB_RECORD_METHOD(void, SBDebugger, SetInputFileHandle, (FILE *, bool), fh,
                      transfer_ownership);
   SetInputFile((FileSP)std::make_shared<NativeFile>(fh, transfer_ownership));
 }
 
+SBError SBDebugger::SetInputFile(FileSP file_sp) {
+  LLDB_RECORD_METHOD(SBError, SBDebugger, SetInputFile, (FileSP), file_sp);
+  return SetInputFile(SBFile(file_sp));
+}
+
 // Shouldn't really be settable after initialization as this could cause lots
 // of problems; don't want users trying to switch modes in the middle of a
 // debugging session.
 SBError SBDebugger::SetInputFile(SBFile file) {
   LLDB_RECORD_METHOD(SBError, SBDebugger, SetInputFile, (SBFile), file);
 
   SBError error;
   if (!m_opaque_sp) {
if (!file_sp || !file_sp->IsValid()) {
     error.ref().SetErrorString("invalid file");
     return error;
   }
 
   m_opaque_sp->SetInputFile(file_sp, recorder);
   return error;
 }
 
+SBError SBDebugger::SetOutputFile(FileSP file_sp) {
+  LLDB_RECORD_METHOD(SBError, SBDebugger, SetOutputFile, (FileSP), file_sp);
+  return SetOutputFile(SBFile(file_sp));
+}
+
 void SBDebugger::SetOutputFileHandle(FILE *fh, bool transfer_ownership) {
   LLDB_RECORD_METHOD(void, SBDebugger, SetOutputFileHandle, (FILE *, bool), fh,
                      transfer_ownership);
   SetOutputFile((FileSP)std::make_shared<NativeFile>(fh, transfer_ownership));
 }
 
 SBError SBDebugger::SetOutputFile(SBFile file) {
   LLDB_RECORD_METHOD(SBError, SBDebugger, SetOutputFile, (SBFile file), file);

 }
 
 void SBDebugger::SetErrorFileHandle(FILE *fh, bool transfer_ownership) {
   LLDB_RECORD_METHOD(void, SBDebugger, SetErrorFileHandle, (FILE *, bool), fh,
                      transfer_ownership);
   SetErrorFile((FileSP)std::make_shared<NativeFile>(fh, transfer_ownership));
 }
 
+SBError SBDebugger::SetErrorFile(FileSP file_sp) {
+  LLDB_RECORD_METHOD(SBError, SBDebugger, SetErrorFile, (FileSP), file_sp);
+  return SetErrorFile(SBFile(file_sp));
+}
+
 SBError SBDebugger::SetErrorFile(SBFile file) {
   LLDB_RECORD_METHOD(SBError, SBDebugger, SetErrorFile, (SBFile file), file);
   SBError error;
   if (!m_opaque_sp) {
     error.ref().SetErrorString("invalid debugger");
     return error;
   }
   if (!file) {

 }
 
 static void SetFileHandleRedirect(SBDebugger *, FILE *, bool) {
   // Do nothing.
 }
 
 static SBError SetFileRedirect(SBDebugger *, SBFile file) { return SBError(); }
 
+static SBError SetFileRedirect(SBDebugger *, FileSP file) { return SBError(); }
+
 static bool GetDefaultArchitectureRedirect(char *arch_name,
                                            size_t arch_name_len) {
   // The function is writing to its argument. Without the redirect it would
   // write into the replay buffer.
   char buffer[1024];
   return SBDebugger::GetDefaultArchitecture(buffer, arch_name_len);
 }
 
R.Register(&invoke<SBError (SBDebugger::*)(
              &SetFileRedirect);
   R.Register(&invoke<SBError (SBDebugger::*)(
                  SBFile)>::method<&SBDebugger::SetOutputFile>::doit,
              &SetFileRedirect);
   R.Register(&invoke<SBError (SBDebugger::*)(
                  SBFile)>::method<&SBDebugger::SetErrorFile>::doit,
              &SetFileRedirect);
 
+  R.Register(&invoke<SBError (SBDebugger::*)(
+                 FileSP)>::method<&SBDebugger::SetInputFile>::doit,
+             &SetFileRedirect);
+  R.Register(&invoke<SBError (SBDebugger::*)(
+                 FileSP)>::method<&SBDebugger::SetOutputFile>::doit,
+             &SetFileRedirect);
+  R.Register(&invoke<SBError (SBDebugger::*)(
+                 FileSP)>::method<&SBDebugger::SetErrorFile>::doit,
+             &SetFileRedirect);
+
   LLDB_REGISTER_CONSTRUCTOR(SBDebugger, ());
   LLDB_REGISTER_CONSTRUCTOR(SBDebugger, (const lldb::DebuggerSP &));
   LLDB_REGISTER_CONSTRUCTOR(SBDebugger, (const lldb::SBDebugger &));
   LLDB_REGISTER_METHOD(lldb::SBDebugger &,
                        SBDebugger, operator=,(const lldb::SBDebugger &));
   LLDB_REGISTER_STATIC_METHOD(void, SBDebugger, Initialize, ());
   LLDB_REGISTER_STATIC_METHOD(lldb::SBError, SBDebugger,
                               InitializeWithErrorHandling, ());