Make File option flags consistent for Python API

[fjricci]

Summary

Fixes SBCommandReturnObject::SetImmediateOutputFile() and
SBCommandReturnObject::SetImmediateOutputFile() for files opened
with "a" or "a+" by resolving inconsistencies between File and
our Python parsing of file objects.

[clayborg]

source/Host/common/File.cpp

48 ↗

(On Diff #50876)

This change isn't needed if "File::eOpenOptionWrite" is specified in the "a+" spec below...

source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

1258

Docs for this say:

``a+''  Open for reading and writing.  The file is created if it does not exist.  The stream is posi-tioned positioned
        tioned at the end of the file.  Subsequent writes to the file will always end up at the then
        current end of file, irrespective of any intervening fseek(3) or similar.

Why is the File:: eOpenOptionWrite not included for "a+"?

[fjricci]

So this was definitely a decision that I was debating. But I assume that the Append flag must imply the Write flag, since you can't open a file for Append without also opening it for write ("a" and "a+" both include write privileges). So I figured that having both "Append" and "Write" was redundant. And since "a" was previously using "Append" while "a+" was previously using "Write", I think that one of them needed to be changed to be consistent with the other. I can do it the other way around though if that's preferable.

[clayborg]

Just to be clear in the code we should set the write flag and not make any assumptions. So then the first diff isn't needed and only PythonDataObjects.cpp needs to change.

[fjricci]

That makes sense. Will do.

[fjricci]

Always use write flag, even in append mode

[clayborg]

Looks great, thanks for catching and fixing this. Can we add a test for this so we don't regress?

[fjricci]

I want to add this testing to functionalities/command_script_immediate_output/TestCommandScriptImmediateOutput.py, since that test is already very basic coverage of the same functionality.

However, it appears that TestCommandScriptImmediateOutput is an expected fail on FreeBSD, Linux, and Windows. And when I run it on the master branch on OSX, fails as ERROR. So it might be that the test doesn't actually pass on any platform.

When I run locally on Linux, the test succeeds, so I extended the test and made sure it passes Linux locally. But I'm not sure how useful that is, since the test is currently disabled on Linux.

I'll upload the test I wrote and we can go from there.

[fjricci]

Added unit test for python file api

[fjricci]

Friendly ping.

[sas]

Committed as r264351.

packages/Python/lldbsuite/test/functionalities/command_script_immediate_output/TestCommandScriptImmediateOutput.py

def setUp(self):
         # Call super's setUp().
         PExpectTest.setUp(self)
 
     @skipIfRemote # test not remote-ready llvm.org/pr24813
     @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr22274: need a pexpect replacement for windows")
     @expectedFailureAll(oslist=["freebsd","linux"], bugnumber="llvm.org/pr26139")
     def test_command_script_immediate_output (self):
         """Test that LLDB correctly allows scripted commands to set an immediate output file."""
-        self.launch(timeout=5)
+        self.launch(timeout=60)
 
         script = os.path.join(os.getcwd(), 'custom_command.py')
         prompt = "(lldb)"
         
         self.sendline('command script import %s' % script, patterns=[prompt])
         self.sendline('command script add -f custom_command.command_function mycommand', patterns=[prompt])
         self.sendline('mycommand', patterns='this is a test string, just a test string')
         self.sendline('command script delete mycommand', patterns=[prompt])
+
+        test_files = {os.path.join(os.getcwd(), 'read.txt')        :'r',
+                      os.path.join(os.getcwd(), 'write.txt')       :'w',
+                      os.path.join(os.getcwd(), 'append.txt')      :'a',
+                      os.path.join(os.getcwd(), 'write_plus.txt')  :'w+',
+                      os.path.join(os.getcwd(), 'read_plus.txt')   :'r+',
+                      os.path.join(os.getcwd(), 'append_plus.txt') :'a+'}
+
+        starter_string = 'Starter Garbage\n'
+        write_string   = 'writing to file with mode: '
+
+        for path, mode in test_files.iteritems():
+            with open(path, 'w+') as init:
+                init.write(starter_string)
+
+        self.sendline('command script add -f custom_command.write_file mywrite', patterns=[prompt])
+        for path, mode in test_files.iteritems():
+            command = 'mywrite ' + path + ' ' + mode
+
+            self.sendline(command, patterns=[prompt])
+
+        self.sendline('command script delete mywrite', patterns=[prompt])
+
         self.quit(gracefully=False)
+
+        for path, mode in test_files.iteritems():
+            with open(path, 'r') as result:
+                if mode in ['r', 'a', 'a+']:
+                    self.assertEquals(result.readline(), starter_string)
+                if mode in ['w', 'w+', 'r+', 'a', 'a+']:
+                    self.assertEquals(result.readline(), write_string + mode + '\n')
+
+            self.assertTrue(os.path.isfile(path))
+            os.remove(path)
+

packages/Python/lldbsuite/test/functionalities/command_script_immediate_output/custom_command.py

 from __future__ import print_function
 
 import sys
 
 def command_function(debugger, command, exe_ctx, result, internal_dict):
         result.SetImmediateOutputFile(sys.__stdout__)
         print('this is a test string, just a test string', file=result)
 
+def write_file(debugger, command, exe_ctx, result, internal_dict):
+        args = command.split(' ')
+        path = args[0]
+        mode = args[1]
+
+        with open(path, mode) as f:
+            result.SetImmediateOutputFile(f)
+            if not mode in ['r']:
+                print('writing to file with mode: ' + mode, file=result)

source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

 PythonFile::GetOptionsFromMode(llvm::StringRef mode)
 {
     if (mode.empty())
         return 0;
 
     return llvm::StringSwitch<uint32_t>(mode.str().c_str())
     .Case("r",   File::eOpenOptionRead)
     .Case("w",   File::eOpenOptionWrite)
-    .Case("a",   File::eOpenOptionAppend|File::eOpenOptionCanCreate)
+    .Case("a",   File::eOpenOptionWrite|File::eOpenOptionAppend|File::eOpenOptionCanCreate)
     .Case("r+",  File::eOpenOptionRead|File::eOpenOptionWrite)
     .Case("w+",  File::eOpenOptionRead|File::eOpenOptionWrite|File::eOpenOptionCanCreate|File::eOpenOptionTruncate)
-    .Case("a+",  File::eOpenOptionRead|File::eOpenOptionWrite|File::eOpenOptionCanCreate)
+    .Case("a+",  File::eOpenOptionRead|File::eOpenOptionWrite|File::eOpenOptionAppend|File::eOpenOptionCanCreate)
     .Default(0);
 }
 
 bool
 PythonFile::GetUnderlyingFile(File &file) const
 {
     if (!IsValid())
         return false;