[Lldb-commits] [PATCH] D111052: [lldb] [gdb-remote] Correct st_dev values in vFile:fstat packet

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111052/new/

https://reviews.llvm.org/D111052

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D111052: [lldb] [gdb-remote] Correct st_dev values in vFile:fstat packet

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

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).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111052/new/

https://reviews.llvm.org/D111052

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D111052: [lldb] [gdb-remote] Correct st_dev values in vFile:fstat packet

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111052/new/

https://reviews.llvm.org/D111052

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D111052: [lldb] [gdb-remote] Correct st_dev values in vFile:fstat packet

[Michał Górny via Phabricator via lldb-commits]

mgorny added a comment.

@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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111052/new/

https://reviews.llvm.org/D111052

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D111052: [lldb] [gdb-remote] Correct st_dev values in vFile:fstat packet

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111052/new/

https://reviews.llvm.org/D111052

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D111052: [lldb] [gdb-remote] Correct st_dev values in vFile:fstat packet

[Michał Górny via Phabricator via lldb-commits]

mgorny created this revision.
mgorny added reviewers: teemperor, labath, krytarowski, emaste.
mgorny requested review of this revision.

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


https://reviews.llvm.org/D111052

Files:
  lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
  lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py


Index: lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
===
--- lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
+++ lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
@@ -266,7 +266,7 @@
 .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))
Index: 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
===
--- 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
+++ 
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
@@ -790,7 +790,9 @@
   }
 
   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);


Index: lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
===
--- lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
+++ lldb/test/API/tools/lldb-server/TestGdbRemotePlatformFile.py
@@ -266,7 +266,7 @@
 .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))
Index: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
===
--- lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
+++ lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
@@ -790,7 +790,9 @@
   }
 
   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);
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits