[lldb] [gdb-remote] Correct st_dev values in vFile:fstat packet

[mgorny]

Summary

Correct the st_dev values used by vFile:fstat packet to conform to
the GDB protocol. Thanks to Raphael Isemann for noticing.

[teemperor]

LGTM. Could you maybe add a FIXME: at the end of that comment to point out that the whole 'console' thing isn't implemented/supported.

Probably wait and see if Pavel has any objections against this, but I think this is more correct than the old implementation so I think this can land.

[mgorny]

@labath has pointed out that this seems not to apply to the vFile:fstat that we're using — gdbserver seems to pass (truncated) st_dev there.

[labath]

So I guess all that's left to do is to add some cast to placate compilers ?

[mgorny]

In D111052#3042181, @labath wrote:

So I guess all that's left to do is to add some cast to placate compilers ?

Nah, my original logic checks for overflow and replaces the value with 0 if one occurs (which IMO is more correct than truncating the value).

[labath]

In D111052#3042184, @mgorny wrote:

In D111052#3042181, @labath wrote:

So I guess all that's left to do is to add some cast to placate compilers ?

Nah, my original logic checks for overflow and replaces the value with 0 if one occurs (which IMO is more correct than truncating the value).

I was talking about the warning Raphael ran into.

We already discussed truncation vs 0 on the initial patch. I don't think we need to strictly copy gdb behavior here, though I would also be fine with changing it.

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

GDBRemoteCommunicationServerCommon::Handle_vFile_FStat(
   struct stat file_stats;
   if (::fstat(fd, &file_stats) == -1) {
     const int save_errno = errno;
     response.Printf("F-1,%x", system_errno_to_gdb(save_errno));
     return SendPacketNoLock(response.GetString());
   }
 
   GDBRemoteFStatData data;
-  fill_clamp(data.gdb_st_dev, file_stats.st_dev, 0);
+  // st_dev is used specially here: 0 means a file, 1 a console
+  // (i.e. a fd used as program's stdin/stdout/stderr)
+  data.gdb_st_dev = 0;
   fill_clamp(data.gdb_st_ino, file_stats.st_ino, 0);
   data.gdb_st_mode = file_stats.st_mode;
   fill_clamp(data.gdb_st_nlink, file_stats.st_nlink, UINT32_MAX);
   fill_clamp(data.gdb_st_uid, file_stats.st_uid, 0);
   fill_clamp(data.gdb_st_gid, file_stats.st_gid, 0);
   fill_clamp(data.gdb_st_rdev, file_stats.st_rdev, 0);
   data.gdb_st_size = file_stats.st_size;
 #if !defined(_WIN32)

lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py

def test_platform_file_fstat(self):
             # NB: we're using .encode() as a hack because the test suite
             # is wrongly using (unicode) str instead of bytes
             gdb_stat = GDBStat(
                 *struct.unpack(">IIIIIIIQQQIII",
                                self.decode_gdbremote_binary(context["data"])
                                 .encode("iso-8859-1")))
             sys_stat = os.fstat(temp_file.fileno())
 
-            self.assertEqual(gdb_stat.st_dev, uint32_or_zero(sys_stat.st_dev))
+            self.assertEqual(gdb_stat.st_dev, 0)
             self.assertEqual(gdb_stat.st_ino, uint32_or_zero(sys_stat.st_ino))
             self.assertEqual(gdb_stat.st_mode, uint32_trunc(sys_stat.st_mode))
             self.assertEqual(gdb_stat.st_nlink, uint32_or_max(sys_stat.st_nlink))
             self.assertEqual(gdb_stat.st_uid, uint32_or_zero(sys_stat.st_uid))
             self.assertEqual(gdb_stat.st_gid, uint32_or_zero(sys_stat.st_gid))
             self.assertEqual(gdb_stat.st_rdev, uint32_or_zero(sys_stat.st_rdev))
             self.assertEqual(gdb_stat.st_size, sys_stat.st_size)
             self.assertEqual(gdb_stat.st_blksize, sys_stat.st_blksize)