This is an archive of the discontinued LLVM Phabricator instance.

[trace][intel pt] Create a common accessor for live and postmortem data
ClosedPublic

Authored by wallace on Apr 6 2022, 10:31 PM.

Details

Summary

Some parts of the code have to distinguish between live and postmortem threads
to figure out how to get some data, e.g. thread trace buffers. This makes the
code less generic and more error prone. An example of that is that we have
two different decoders: LiveThreadDecoder and PostMortemThreadDecoder. They
exist because getting the trace bufer is different for each case.

The problem doesn't stop there. Soon we'll have even more kinds of data, like
the context switch trace, whose fetching will be different for live and post-
mortem processes.

As a way to fix this, I'm creating a common API for accessing thread data,
which is able to figure out how to handle the postmortem and live cases on
behalf of the caller. As a result of that, I was able to eliminate the two
decoders and unify them into a simpler one. Not only that, our TraceSave
functionality only worked for live threads, but now it can also work for
postmortem processes, which might be useful now, but it might in the future.

This common API is OnThreadBinaryDataRead. More information in the inline
documentation.

Diff Detail

Event Timeline

wallace created this revision.Apr 6 2022, 10:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2022, 10:31 PM
wallace requested review of this revision.Apr 6 2022, 10:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2022, 10:31 PM
jj10306 added inline comments.Apr 7 2022, 8:23 AM
lldb/include/lldb/Target/Trace.h
265

typedef the callback to be cleaner and make the intention more clear?

385

Can you explain what "kind" represents and why we need the nested map? Also, I see that for live tracing we have a map for both processes and threads, why is this not the case for post mortem?

lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp
272–273

The couple minor changes to Libiptdecoder aren't related to this diff, maybe move these to D123106.

lldb/source/Target/Trace.cpp
265

is guaranteed that the UP will be non-null?

wallace added inline comments.Apr 7 2022, 10:34 AM
lldb/include/lldb/Target/Trace.h
265

good idea

385

those "kinds" are just arbitrarily tagged data. If you see the jLLDBTraceGetState, there's a mention to those data kinds. This is the client side of those data kinds. The difference in usage is that lldb-server will return it as a data blob in memory, and in the post-mortem case the data will be in files.

I should make those maps DenseMaps, now that I think of.

We still don't have a map for post mortem process because we haven't needed it yet, but we might soon.

lldb/source/Plugins/Trace/intel-pt/LibiptDecoder.cpp
272–273

these are needed because ArrayRef and MutableArrayRef have different const modifiers for their pointers. I like the fact that we are now passing an ArrayRef here instead of a mutable one for safety

lldb/source/Target/Trace.cpp
265

yes, otherwise there would have been an error in the previous if

wallace updated this revision to Diff 421291.Apr 7 2022, 11:11 AM

address comments

jj10306 accepted this revision.Apr 7 2022, 3:46 PM
jj10306 added inline comments.
lldb/include/lldb/Target/Trace.h
380–387

Why not change all the maps to DenseMap while we're at it?

This revision is now accepted and ready to land.Apr 7 2022, 3:46 PM
wallace added inline comments.Apr 7 2022, 3:57 PM
lldb/include/lldb/Target/Trace.h
380–387

interestingly DenseMap doesn't know how to work with std::strings as key, only with const char *. Maybe they want to force a comparison by pointer and not by string content.

This revision was landed with ongoing or failed builds.Apr 7 2022, 4:06 PM
This revision was automatically updated to reflect the committed changes.