This is an archive of the discontinued LLVM Phabricator instance.

[trace][intelpt] Support system-wide tracing [17] - Some improvements
ClosedPublic

Authored by wallace on Jun 9 2022, 4:42 PM.

Details

Summary

This improves several things and addresses comments up to the diff [11] in this stack.

  • Simplify many functions to receive less parameters that they can identify easily
  • Create Storage classes for Trace and TraceIntelPT that can make it easier to reason about what can change with live process refreshes and what cannot.
  • Don't cache the perf zero conversion numbers in lldb-server to make sure we get the most up-to-date numbers.
  • Move the thread identifaction from context switches to the bundle parser, to leave TraceIntelPT simpler. This also makes sure that the constructor of TraceIntelPT is invoked when the entire data has been checked to be correct.
  • Normalize all bundle paths before the Processes, Threads and Modules are created, so that they can assume that all paths are correct and absolute
  • Fix some issues in the tests. Now they all pass.
  • return the specific instance when constructing PerThread and MultiCore processor tracers.
  • Properly implement IntelPTMultiCoreTrace::TraceStart.
  • Improve some comments.
  • Use the typedef ContextSwitchTrace more often for clarity.
  • Move CreateContextSwitchTracePerfEvent to Perf.h as a utility function.
  • Synchronize better the state of the context switch and the intel pt

perf events.

  • Use a booblean instead of an enum for the PerfEvent state.

Diff Detail

Event Timeline

wallace created this revision.Jun 9 2022, 4:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 4:42 PM
wallace requested review of this revision.Jun 9 2022, 4:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 4:42 PM
jj10306 requested changes to this revision.Jun 15 2022, 9:32 AM

Looks great overall, thanks for making these improvements - just a couple minor things

lldb/include/lldb/Target/Trace.h
524

nice

lldb/source/Plugins/Process/Linux/Perf.cpp
188

why this intermediate was_enabled variable?

248

same question as above

347–349

it may not make a difference since this is a context switch event, but should these values be set based on the parent's perf event?

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
319

this is much cleaner now (:

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
205

use llvm::DenseMap instead?

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h
33–38

pid is really a int32_t in the kernel. should these be int32_t or pid_t/tid_t instead?

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.h
65

why are we storing a pointer now if the constructor is still passing in a ref?

This revision now requires changes to proceed.Jun 15 2022, 9:32 AM
wallace added inline comments.Jun 15 2022, 9:55 AM
lldb/source/Plugins/Process/Linux/Perf.cpp
188

it stores the original state of the perf event, so that after pausing it and reading, the perf event is returned to that state. We need to pause the perf event before reading so that the CPU flushed out its internal buffer. We also want to prevent the cpu from writing to the buffer while we read, which would give us wrong data

248

same response

347–349

not necessarily. Maybe you want context switches that include kernel but you want your intel pt trace to be user only. That could give you an interesting visualization

lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
205

I've fixed that in a later diff, but good catch!

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTJSONStructs.h
33–38

On OSX it's 64 bit IIRC, so I'm just adhering to that extreme case. lldb::pid_t and lldb::tid_t are 64 bits to be able to support all systems

lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.h
65

We have two options: store a pointer or a a weak pointer. We can't use a reference variable because the new version of the code requires the multicore decoder to have a copy constructor (Optional is the one asking for that).
So I'm changing this now to use weak_pointers to make it more c++ idiomatic.

wallace requested review of this revision.Jun 15 2022, 10:38 AM
This comment was removed by wallace.
lldb/source/Plugins/Trace/intel-pt/TraceIntelPTMultiCoreDecoder.h
65
jj10306 accepted this revision.Jun 15 2022, 10:44 AM
jj10306 added inline comments.
lldb/source/Plugins/Process/Linux/Perf.cpp
188

ahhh I see, I forgot that the Disable and Enable methods were potentially modifying m_enabled internally - thanks!

This revision is now accepted and ready to land.Jun 15 2022, 10:44 AM
This revision was landed with ongoing or failed builds.Jun 16 2022, 11:23 AM
This revision was automatically updated to reflect the committed changes.
max-kudr added inline comments.
lldb/source/Plugins/Process/Linux/IntelPTPerThreadProcessTrace.cpp
55–59

@wallace, @jj10306

We are getting build errors here on CentOS/GNU 7.3.1:

/llvm-project/lldb/source/Plugins/Process/Linux/IntelPTPerThreadProcessTrace.cpp: In static member function 'static llvm::Expected<std::unique_ptr<lldb_private::process_linux::IntelPTPerThreadProcessTrace> > lldb_private::process_linux::IntelPTPerThreadProcessTrace::Start(const lldb_private::TraceIntelPTStartRequest&, llvm::ArrayRef<long unsigned int>)':
/llvm-project/lldb/source/Plugins/Process/Linux/IntelPTPerThreadProcessTrace.cpp:65:10: error: could not convert 'trace' from 'std::unique_ptr<lldb_private::process_linux::IntelPTPerThreadProcessTrace>' to 'llvm::Expected<std::unique_ptr<lldb_private::process_linux::IntelPTPerThreadProcessTrace> >'
   return trace;
          ^~~~~

Can you please fix this?

wallace added inline comments.Jun 20 2022, 1:31 PM
lldb/source/Plugins/Process/Linux/IntelPTPerThreadProcessTrace.cpp
55–59

Hi Max, I'm aware of this issue because it was reported by someone else. Sadly I wasn't able to install that toolchain on my centos machine (I don't manage it) so I can't reproduce the issue to make sure I can fix it. This issue doesn't happen with other toolchains.

So, I would like to ask for your help to try to fix it. I think the fix might be something as simple as doing

return std::move(trace)
or perhaps something like

return Expected<std::unique_ptr<IntelPTPerThreadProcessTrace>>(std::move(trace));
At this point I can only rely on you to make sure this is properly fixed

max-kudr added inline comments.Jun 20 2022, 2:43 PM
lldb/source/Plugins/Process/Linux/IntelPTPerThreadProcessTrace.cpp
55–59

Hi Walter (@wallace),

Thank you for following up and proposing fix. I checked and confirm that the code below fixes the issue

return std::move(trace)

Would you please push this fix to the repository? Thank you!

wallace added inline comments.Jun 20 2022, 2:44 PM
lldb/source/Plugins/Process/Linux/IntelPTPerThreadProcessTrace.cpp
55–59

nice! I will do so now