[lldb] [gdb-remote] Sync vFile:open mode constants with GDB

[mgorny]

Summary

Sync the mode constants used to drive vFile:open requests with these
used by GDB and defined for the gdb remote protocol. This makes it
possible to use 'platform file open' after connecting to gdbremote
server (and to some degree to operate on the open file modulo other
incompatibilities).

[mgorny]

Add a test for correct flag encoding.

[jasonmolenda]

Thanks for fixing this, it's long overdue. For a long time I thought we'd need to operate in a compatibility mode to accept either set of constants, but I finally realized I was wrong and we should just align ourselves with the correct gdb behavior, but hadn't made the change myself yet.

[mgorny]

Removed obsolete FIXME from server as well.

[mgorny]

@jasonmolenda, thanks for the review. Could you also look at D106984 which is a prerequisite for this?

lldb/docs/lldb-platform-packets.txt

 //     2. flags passed to the open call, base 16.
 //        Note that these are not the oflags that open(2) takes, but
 //        are the constant values in enum OpenOptions from lldb's File.h
 //     3. mode bits, base 16
 //  
 //  response is F followed by the opened file descriptor in base 10.
 //  "F-1,errno" with the errno if an error occurs.
 //
-//  COMPATIBILITY
-//    The gdb-remote serial protocol documentatio defines a vFile:open:
-//    packet which uses incompatible flag values, e.g. 1 means O_WRONLY
-//    in gdb's vFile:open:, but it means eOpenOptionReadOnly to lldb's
-//    implementation.
-
 //----------------------------------------------------------------------
 // vFile:close:
 //
 // BRIEF
 //   Close a previously opened file descriptor.
 //
 // EXAMPLE
 // 

lldb/include/lldb/Host/File.h

 class File : public IOObject {
 public:
   static int kInvalidDescriptor;
   static FILE *kInvalidStream;
 
   // NB this enum is used in the lldb platform gdb-remote packet
   // vFile:open: and existing values cannot be modified.
   //
-  // FIXME
-  // These values do not match the values used by GDB
+  // The first set of values is defined by gdb headers and can be found
+  // in the documentation at:
   // * https://sourceware.org/gdb/onlinedocs/gdb/Open-Flags.html#Open-Flags
-  // * rdar://problem/46788934
+  //
+  // The second half are LLDB extensions and use the highest uint32_t bits
+  // to avoid risk of collisions with future gdb remote protocol changes.
   enum OpenOptions : uint32_t {
-    eOpenOptionReadOnly = (1u << 0),  // Open file for reading (only)
-    eOpenOptionWriteOnly = (1u << 1), // Open file for writing (only)
-    eOpenOptionReadWrite =
-        eOpenOptionReadOnly |
-        eOpenOptionWriteOnly, // Open file for both reading and writing
+    eOpenOptionReadOnly = 0x0,  // Open file for reading (only)
+    eOpenOptionWriteOnly = 0x1, // Open file for writing (only)
+    eOpenOptionReadWrite = 0x2, // Open file for both reading and writing
     eOpenOptionAppend =
-        (1u << 2), // Don't truncate file when opening, append to end of file
-    eOpenOptionTruncate = (1u << 3),    // Truncate file when opening
-    eOpenOptionNonBlocking = (1u << 4), // File reads
-    eOpenOptionCanCreate = (1u << 5),   // Create file if doesn't already exist
+        0x8, // Don't truncate file when opening, append to end of file
+    eOpenOptionCanCreate = 0x200, // Create file if doesn't already exist
+    eOpenOptionTruncate = 0x400,  // Truncate file when opening
     eOpenOptionCanCreateNewOnly =
-        (1u << 6), // Can create file only if it doesn't already exist
-    eOpenOptionDontFollowSymlinks = (1u << 7),
+        0x800, // Can create file only if it doesn't already exist
+
+    eOpenOptionNonBlocking = (1u << 28), // File reads
+    eOpenOptionDontFollowSymlinks = (1u << 29),
     eOpenOptionCloseOnExec =
-        (1u << 8), // Close the file when executing a new process
-    LLVM_MARK_AS_BITMASK_ENUM(/* largest_value= */ eOpenOptionCloseOnExec)
+        (1u << 30), // Close the file when executing a new process
+    eOpenOptionInvalid = (1u << 31), // Used as invalid value
+    LLVM_MARK_AS_BITMASK_ENUM(/* largest_value= */ eOpenOptionInvalid)
   };
 
   static mode_t ConvertOpenOptionsForPOSIXOpen(OpenOptions open_options);
   static llvm::Expected<OpenOptions> GetOptionsFromMode(llvm::StringRef mode);
   static bool DescriptorIsValid(int descriptor) { return descriptor >= 0; };
   static llvm::Expected<const char *>
   GetStreamOpenModeFromOptions(OpenOptions options);
 

lldb/source/Host/common/File.cpp

OpenOptions opts =
                  eOpenOptionCanCreate)
           .Cases("r+", "rb+", "r+b", eOpenOptionReadWrite)
           .Cases("w+", "wb+", "w+b",
                  eOpenOptionReadWrite | eOpenOptionCanCreate |
                  eOpenOptionTruncate)
           .Cases("a+", "ab+", "a+b",
                  eOpenOptionReadWrite | eOpenOptionAppend |
                      eOpenOptionCanCreate)
-          .Default(OpenOptions());
-  if (opts)
+          .Default(eOpenOptionInvalid);
+  if (opts != eOpenOptionInvalid)
     return opts;
   return llvm::createStringError(
       llvm::inconvertibleErrorCode(),
       "invalid mode, cannot convert to File::OpenOptions");
 }
 
 int File::kInvalidDescriptor = -1;
 FILE *File::kInvalidStream = nullptr;

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp

 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerCommon::Handle_vFile_Open(
     StringExtractorGDBRemote &packet) {
   packet.SetFilePos(::strlen("vFile:open:"));
   std::string path;
   packet.GetHexByteStringTerminatedBy(path, ',');
   if (!path.empty()) {
     if (packet.GetChar() == ',') {
-      // FIXME
-      // The flag values for OpenOptions do not match the values used by GDB
-      // * https://sourceware.org/gdb/onlinedocs/gdb/Open-Flags.html#Open-Flags
-      // * rdar://problem/46788934
       auto flags = File::OpenOptions(packet.GetHexMaxU32(false, 0));
       if (packet.GetChar() == ',') {
         mode_t mode = packet.GetHexMaxU32(false, 0600);
         FileSpec path_spec(path);
         FileSystem::Instance().Resolve(path_spec);
         // Do not close fd.
         auto file = FileSystem::Instance().Open(path_spec, flags, mode, false);
 

lldb/test/API/functionalities/gdb_remote_client/TestGDBRemotePlatformFile.py

+from gdbclientutils import *
+
+class TestGDBRemotePlatformFile(GDBRemoteTestBase):
+
+    def test_file_open(self):
+        """Test mock-opening a remote file"""
+
+        class Responder(MockGDBServerResponder):
+            def vFile(self, packet):
+                return "F10"
+
+        self.server.responder = Responder()
+
+        try:
+            self.runCmd("platform select remote-gdb-server")
+            self.runCmd("platform connect connect://" +
+                        self.server.get_connect_address())
+            self.assertTrue(self.dbg.GetSelectedPlatform().IsConnected())
+
+            self.runCmd("platform file open /some/file.txt -v 0755")
+            self.assertPacketLogContains([
+                "vFile:open:2f736f6d652f66696c652e747874,0000020a,000001ed"
+                ])
+        finally:
+            self.dbg.GetSelectedPlatform().DisconnectRemote()

lldb/test/API/functionalities/gdb_remote_client/gdbclientutils.py

def respond(self, packet):
         if packet == "qOffsets":
             return self.qOffsets();
         if packet == "qsProcessInfo":
             return self.qsProcessInfo()
         if packet.startswith("qfProcessInfo"):
             return self.qfProcessInfo(packet)
         if packet.startswith("qPathComplete:"):
             return self.qPathComplete()
+        if packet.startswith("vFile:"):
+            return self.vFile(packet)
 
         return self.other(packet)
 
     def qsProcessInfo(self):
         return "E04"
 
     def qfProcessInfo(self, packet):
         return "E04"
def QListThreadsInStopReply(self):
         return ""
 
     def qMemoryRegionInfo(self, addr):
         return ""
 
     def qPathComplete(self):
         return ""
 
+    def vFile(self, packet):
+        return ""
+
     """
     Raised when we receive a packet for which there is no default action.
     Override the responder class to implement behavior suitable for the test at
     hand.
     """
     class UnexpectedPacketException(Exception):
         pass