This is an archive of the discontinued LLVM Phabricator instance.

[trace][intelpt] Support system-wide tracing [4] - Support per core tracing on lldb-server
ClosedPublic

Authored by wallace on May 3 2022, 8:40 AM.

Details

Summary

This diffs implements per-core tracing on lldb-server. It also includes tests that ensure that tracing can be initiated from the client and that the jLLDBGetState ppacket returns the list of trace buffers per core.

This doesn't include any decoder changes.

Finally, this makes some little changes here and there improving the existing code.

A specific piece of code that can't reliably be tested is when tracing
per core fails due to permissions. In this case we add a
troubleshooting message and this is the manual test:

/proc/sys/kernel/perf_event_paranoid set to 1

(lldb) process trace start --per-core-tracing                                         error: perf event syscall failed: Permission denied
 You might need that /proc/sys/kernel/perf_event_paranoid has a value of 0 or -1.
``

Diff Detail

Event Timeline

wallace created this revision.May 3 2022, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2022, 8:40 AM
Herald added a subscriber: mgorny. · View Herald Transcript
wallace requested review of this revision.May 3 2022, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2022, 8:40 AM
wallace updated this revision to Diff 426742.May 3 2022, 9:18 AM

add one test

wallace updated this revision to Diff 426745.May 3 2022, 9:24 AM
wallace edited the summary of this revision. (Show Details)

improve error message

wallace added inline comments.May 3 2022, 11:04 AM
lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
47

or use sudo

jj10306 added inline comments.May 8 2022, 3:53 PM
lldb/docs/lldb-gdb-remote.txt
372–374

Not sure what is trying to be communicated hear, so maybe my suggestion doesn't make sense, but I was confused by the wording here

465–474

Should the thread info be optional now that we have an optional cores? If not, can you explain how this output works in the case that you are doing per core tracing? Will both the tid binary data and the cores binary data section be populated?

lldb/include/lldb/Utility/TraceGDBRemotePackets.h
157–160

Similar question here as above on the GetState documentation - how do the cores and traced_threads options play together?

lldb/include/lldb/lldb-types.h
92

nit: this should be an unsigned int of some sort?

lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
228–234

Do we not need to check if m_per_thread_process_trace_up is null here (aka per core tracing is enabled)?

260

Should this be an "else if" if these two are mutually exclusive?

lldb/source/Plugins/Process/Linux/IntelPTCollector.h
63–64

Why is this named "PerThread" if it handles both the per thread and per core cases for "process trace"? Perhaps I'm missing something

152–156

So these are mutually exclusive tight?

lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp
20–22

nit: why are we using signed integers here?

lldb/source/Plugins/Process/Linux/Perf.h
111–115

Maybe add explanation of what is passed to the syscall for pid and cpu when they are None here?

wallace added inline comments.May 9 2022, 10:11 PM
lldb/docs/lldb-gdb-remote.txt
372–374

you are right!

465–474

I'll make this field optional and include more documentation below

lldb/include/lldb/lldb-types.h
92

I wasn't sure if int would be correct for all platforms, but in any case, I should go for unsigned, as you suggest, which is a more restrictive type, and if in the future someone needs a signed int, they could change this type

lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
228–234

I need to check that m_per_thread_process_trace_up exists

260

good idea

lldb/source/Plugins/Process/Linux/IntelPTCollector.h
63–64

this doesn't handle the per core case. This is handling exclusively the case of "process trace" without the "per-cpu" option. This class effectively creates a perf event per thread. Even its Start method asks for tids

152–156

yes, I'll leave a comment about that here

lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp
20–22

yes, this is bad, i'm changing to unsigned

lldb/source/Plugins/Process/Linux/Perf.h
111–115

makes sense

jj10306 added inline comments.May 10 2022, 9:18 AM
lldb/source/Plugins/Process/Linux/IntelPTCollector.cpp
228–234

Yes, I meant "is *not* null here", my bad (-:

lldb/source/Plugins/Process/Linux/IntelPTCollector.h
63–64

ahhh yes I got messed up by the diff view and thought that the fields of IntelPTCollector were being stored on this per thread structure, thanks!

jj10306 requested changes to this revision.May 10 2022, 9:19 AM
This revision now requires changes to proceed.May 10 2022, 9:19 AM
wallace updated this revision to Diff 428496.May 10 2022, 2:12 PM

address comments

wallace updated this revision to Diff 428511.May 10 2022, 3:08 PM

Now the tracedThreads field returned by the GetState request must have the list of all the threads of the process if per-core mode is enabled. This will avoid having to make that field optional.

wallace marked 4 inline comments as done.May 10 2022, 3:19 PM
wallace added inline comments.
lldb/docs/lldb-gdb-remote.txt
248

this should be "Embedded Trace Macrocells" (ETM) instead of coresight

465–474

in the end i didn't make this optional but instead forced the per-core case to return all threads

lldb/include/lldb/Utility/TraceGDBRemotePackets.h
157–160

traced_threads will contain all the threads in the per core case but without any binary data. The cores field will then be returned with per-core trace buffers

lldb/source/Plugins/Process/Linux/IntelPTSingleBufferTrace.cpp
148–149

i'm the location of this ifdef so that perf_event_attr is used only if PERF_ATTR_SIZE_VER5 is set

jj10306 accepted this revision.May 11 2022, 4:53 AM

couple minor things, but looks good overall

lldb/docs/lldb-gdb-remote.txt
465–474

so the tid field of a tracedThread object will be populated but the binaryData will not be populated in the tracedThread object but instead in the cores section?

lldb/source/Plugins/Process/Linux/Perf.h
117

nit: we could make this optional as well and just default to passing in -1 to be consistent with cpu and pid

lldb/source/Utility/TraceGDBRemotePackets.cpp
124

isn't uint64_t serialization and deserialization now supported?

This revision is now accepted and ready to land.May 11 2022, 4:53 AM
jj10306 added inline comments.May 11 2022, 5:11 AM
lldb/source/Plugins/Process/Linux/IntelPTCollector.h
152–156

this could be overkill, but potentially we could use a union here to enforce this invariant and a boolean flag/enum to determine which process tracing "handler" is being used

bool is_per_core_process_tracing_enabled; // used to determine how to interpret the union
union {
  IntelPTPerThreadProcessTraceUP m_per_thread_process_trace_up;
  IntelPTMultiCoreTraceUP m_per_core_process_trace_up;

};

imo this is much more clearly expresses the invariant of only one of the process tracing "handles" being non null at a time.
wdyt?

wallace added inline comments.May 11 2022, 9:01 AM
lldb/docs/lldb-gdb-remote.txt
465–474

yes, exactly

lldb/source/Plugins/Process/Linux/IntelPTCollector.h
152–156

I tried this, but using unions with complex types requires some non-trivial work. std::variant would be a good choice but is not available in the c++ version used by llvm. So in this case I think the current implementation is the simpler. However, I'd rather create an interface and have one single instance based on that interface. I'll try that

lldb/source/Plugins/Process/Linux/Perf.h
117

good idea

lldb/source/Utility/TraceGDBRemotePackets.cpp
124

good catch

wallace updated this revision to Diff 428687.May 11 2022, 9:08 AM

address comments

In a newer diff I'll create a base class for all "process tracing" strategies, so that we don't need to use two unique pointers for both process tracing classes in the collector. I'm not doing it here becaues rebasing would be a pain.