This is an archive of the discontinued LLVM Phabricator instance.

[lldb] [gdb-server] Implement the vFile:fstat packet
ClosedPublic

Authored by mgorny on Aug 10 2021, 9:37 AM.

Diff Detail

Event Timeline

mgorny requested review of this revision.Aug 10 2021, 9:37 AM
mgorny created this revision.
labath accepted this revision.Sep 7 2021, 5:14 AM
labath added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
776

consider:

struct GdbStat {
  llvm::support::ubig32_t st_dev;
  llvm::support::ubig32_t st_ino;
  ...
};

...

translate(gdb_stats.st_dev, file_stats.st_dev, 0); // I'm not sure that this clamping is really necessary.
...

Seems like it could be nicer, particularly as the vFile_Stat function will need to do the same thing...

This revision is now accepted and ready to land.Sep 7 2021, 5:14 AM
mgorny added inline comments.Sep 9 2021, 10:43 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
776

What's this translate() thing? I don't see anything matching in LLVM or LLDB. Or are you saying I should define a helper function?

As for clamping, I think it's better if we send 0 (a "clearly invalid value") than e.g. truncated st_dev that would be simply wrong.

mgorny updated this revision to Diff 371674.Sep 9 2021, 11:38 AM
mgorny marked an inline comment as done.

Use fancy endian-magic types ;-).

labath accepted this revision.Sep 10 2021, 12:46 AM
labath added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
763

static void

764

src <=

776

Yeah, that was a new helper function.

mgorny marked 3 inline comments as done.Sep 10 2021, 1:22 AM

Thanks!

This revision was landed with ongoing or failed builds.Sep 10 2021, 2:09 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2021, 2:09 AM
mgorny reopened this revision.Sep 10 2021, 2:48 AM
This revision is now accepted and ready to land.Sep 10 2021, 2:48 AM
mgorny updated this revision to Diff 371847.Sep 10 2021, 2:48 AM

Skip st_blocks and st_blksize on Windows.

labath added inline comments.Sep 10 2021, 2:53 AM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
791

Just make sure the fields stay initialized (to zero, I guess)

This revision was landed with ongoing or failed builds.Sep 10 2021, 2:58 AM
This revision was automatically updated to reflect the committed changes.
shafik added a subscriber: shafik.Nov 9 2021, 11:56 AM
shafik added inline comments.
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
764

I am building on a macOS and I seeing the following diagnostic for this line:

warning: comparison of integers of different signs: 'int' and 'std::numeric_limits<unsigned int>::type' (aka 'unsigned int') [-Wsign-compare]
  dest = src <= std::numeric_limits<typename T::value_type>::max() ? src
         ~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

and it is triggered by:

fill_clamp(data.gdb_st_dev, file_stats.st_dev, 0);

In this case it will do the right thing but allowing mixed types is problematic, usually I see clamp done with homogeneous types.

mgorny added inline comments.Nov 9 2021, 12:06 PM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
764

Could you suggest how to fix it? Some platforms have different signedness than the GDB struct type.

shafik added inline comments.Nov 9 2021, 1:47 PM
lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
764

Since we don't have C++20 I think you would have to roll your own cmp_less see the possible implementation on cppreference.

For reference see Safe integral comparisons proposal.

I looked and I don't see something similar in llvm anywhere but I could be missing it.