This is an archive of the discontinued LLVM Phabricator instance.

[tests] [trace] Add a more comprehensive test for `thread trace export ctf` command
ClosedPublic

Authored by jj10306 on Aug 6 2021, 3:41 PM.

Details

Summary

Follow up on https://reviews.llvm.org/D105741

  • Add new test that exhaustively checks the output file's content
  • Fix typos in documentation and other minor fixes

Diff Detail

Event Timeline

jj10306 requested review of this revision.Aug 6 2021, 3:41 PM
jj10306 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2021, 3:42 PM

Thank you, this is much more understandable

lldb/docs/htr.rst
1

I'd be good to put these documentation files next in source/Plugins/TraceExporter/docs so that the documentation can live next to the source code. It'll give more visibility to this doc

lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
393–394

a little comment explaining what "ph", "X", "dur" mean would be good. Also that in the absence of timestamps "ts" is just an index. You can repeat the comments you have in the test, btw

450

explain why the duration is the num of instructions in the case of no timestamps

467

in your devserver you can run
"arc lint"
and it will fix all these formatting issues for you

wallace requested changes to this revision.Aug 9 2021, 6:45 PM
This revision now requires changes to proceed.Aug 9 2021, 6:45 PM
wallace retitled this revision from [tests] [trace] Add more comprehensive for `thread trace export ctf` command to [tests] [trace] Add a more comprehensive test for `thread trace export ctf` command.Aug 9 2021, 7:01 PM
wallace added inline comments.
lldb/test/API/commands/trace/TestTraceExport.py
79–80

you don't need this. Whenever the test runs, all previous data will be wiped out

jj10306 updated this revision to Diff 365341.Aug 9 2021, 8:45 PM
  • move documentation to plugin directory
  • remove unnecessary checks in tests and remove test that wasn't testing anything different than the others
  • nits
wallace accepted this revision.Aug 9 2021, 8:52 PM
wallace added inline comments.
lldb/source/Plugins/TraceExporter/common/TraceHTR.cpp
394–419

use // style comments, as mentioned here https://llvm.org/docs/CodingStandards.html#comment-formatting

450–457

same here

This revision is now accepted and ready to land.Aug 9 2021, 8:52 PM
jj10306 updated this revision to Diff 365346.Aug 9 2021, 8:56 PM
  • fix comment style
jj10306 updated this revision to Diff 365477.Aug 10 2021, 7:56 AM

lint + comments