This is an archive of the discontinued LLVM Phabricator instance.

[Reproducers] Serialize process arguments in ProcessInfo
ClosedPublic

Authored by JDevlieghere on May 8 2020, 12:58 PM.

Details

Summary

While debugging why TestProcessList.py failed during passive replay, I remembered that we don't serialize the arguments for ProcessInfo. This is necessary to make the test pass and to make platform process list -v behave the same during capture and replay.

$ ./bin/lldb --capture
(lldb) platform process list -v
303 matching processes were found on "host"
PID    PARENT USER       GROUP      EFF USER   EFF GROUP  TRIPLE                         ARGUMENTS
====== ====== ========== ========== ========== ========== ============================== ============================
40118  40116  jonas      staff      jonas      staff      x86_64-apple-macosx             -fish
40116  2502   jonas      staff      jonas      staff      x86_64-apple-macosx             fish -c reattach-to-user-namespace -l fish
...
$./bin/lldb --replay /var/folders/n4/g9s84l4125s3x9fqjjmncxzh0000gn/T/reproducer-71d635
(lldb) platform process list -v
303 matching processes were found on "host"
PID    PARENT USER       GROUP      EFF USER   EFF GROUP  TRIPLE                         ARGUMENTS
====== ====== ========== ========== ========== ========== ============================== ============================
40118  40116  jonas      staff      jonas      staff      x86_64-apple-macosx
40116  2502   jonas      staff      jonas      staff      x86_64-apple-macosx

Diff Detail

Event Timeline

JDevlieghere created this revision.May 8 2020, 12:58 PM
labath added inline comments.May 11 2020, 2:44 AM
lldb/source/Utility/Args.cpp
691

The GetCommandString -> SetCommandString roundtrip is very lossy (mainly because of the Get part). That may be enough for platform process list, but I don't think it's a good general representation. I think it would be better to represent this as a vector of (string, quote_char) pairs.

lldb/test/Shell/Reproducer/TestProcessList.test
2–22

This isn't too bad, though for a change like this, it may be enough to write a (c++) unit test for the ProcessInfo<->yaml conversion -- it's going to run everywhere, and you can make proper assertions about the expected output.

JDevlieghere marked an inline comment as done.
  • Serialize Args::ArgEntry
  • Add unit test
labath accepted this revision.May 12 2020, 4:07 AM
This revision is now accepted and ready to land.May 12 2020, 4:07 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2020, 11:17 AM