[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace export` command for Intel PT trace visualization
walter erquinigo via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Fri Jul 23 00:33:08 PDT 2021
wallace requested changes to this revision.
wallace added a comment.
This revision now requires changes to proceed.
So much better. There's an optimization that you can make when merging maps, and besides that you need a more comprehensive test.
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:66
+ /// None if no function is called from this block.
+ llvm::Optional<std::string> GetMostFrequentlyCalledFunction() const;
+
----------------
just return a ConstString
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:210
+
+ ~HTRInstructionLayer() override = default;
+
----------------
you dont need this btw, as the parent's destructor is already virtual
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:272
+ /// The map of block IDs to \a HTRBlock.
+ std::unordered_map<size_t, HTRBlock> const &GetBlockDefs() const;
+
----------------
As this is const, the only way to use this unordered_map is to do look ups, then let's better hide the implementation detail that is an unordered map and create this method instead
const HTRBlock &GetBlockById(size_t id);
that way we can later change unordered_map for anything else without affecting any callers
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:323
+ /// The trace cursor that gives access to the trace's contents.
+ TraceHTR(Thread &thread, Stream &s, TraceCursor &cursor);
+
----------------
the constructor shouldn't print anything and it it doesn't look right for a TraceHTR object to own a stream It's not part of its state.
================
Comment at: lldb/include/lldb/Target/TraceHTR.h:339
+ /// The file that the exported HTR data will be written to.
+ void Export(Stream &s, TraceExportFormat export_format, std::string outfile);
+
----------------
better return an llvm::Error that will contain the error message, if any. The CommandObject that invokes this method can then get the error and dump it if necessary. So, don't pass any streams here
================
Comment at: lldb/include/lldb/lldb-enumerations.h:1147
};
-
} // namespace lldb
----------------
revert this change to diminish the noise
================
Comment at: lldb/include/lldb/lldb-forward.h:330-331
typedef std::shared_ptr<lldb_private::FuncUnwinders> FuncUnwindersSP;
+typedef std::unique_ptr<lldb_private::HTRBlockLayer> HTRBlockLayerUP;
+typedef std::unique_ptr<lldb_private::HTRInstructionLayer>
+ HTRInstructionLayerUP;
----------------
move this to the same file as HTR as we'll end up moving HTR to its own plugin folder
================
Comment at: lldb/source/Target/TraceHTR.cpp:75
+}
+llvm::ArrayRef<size_t> HTRInstructionLayer::GetInstructionTrace() const {
+ return m_instruction_trace;
----------------
add an empty life before this
================
Comment at: lldb/source/Target/TraceHTR.cpp:124
+ : m_instruction_layer_up(std::make_unique<HTRInstructionLayer>(0)) {
+ // m_instruction_layer_up = std::make_unique(HTRInstructionLayer(0));
+ // Move cursor to the first instruction in the trace
----------------
remove this
================
Comment at: lldb/source/Target/TraceHTR.cpp:126
+ // Move cursor to the first instruction in the trace
+ cursor.SeekToBegin();
+
----------------
once you rebase with the current code, you might need to invoke
cursor.SetForwards(true)
cursor.SeekToBegin(TraceCursor::SeekType::Set, 0)
================
Comment at: lldb/source/Target/TraceHTR.cpp:131
+ lldb_private::Address pc_addr;
+ Target &target = thread.GetProcess()->GetTarget();
+ SymbolContext sc;
----------------
calculate this outside of this function and reuse the variable
================
Comment at: lldb/source/Target/TraceHTR.cpp:152
+ valid_cursor = cursor.Next();
+ s.Printf("%s\n", toString(std::move(err)).c_str());
+ } else {
----------------
this Printf is too noisy. You don't really need that. Just remove it and instead of invoking cursor.GetError(), invoke cursor.IsError() after rebasing the code
================
Comment at: lldb/source/Target/TraceHTR.cpp:160
+ current_instruction_load_address);
+ valid_cursor = cursor.Next();
+ if (current_instruction_type &
----------------
valid_cursor is not a nice name, just call it no_more_data, end_of_trace, or something like that
================
Comment at: lldb/source/Target/TraceHTR.cpp:164
+ if (valid_cursor) {
+ if (llvm::Error err = cursor.GetError()) {
+ m_instruction_layer_up->AddCallInstructionMetadata(
----------------
just call IsError as you don't need the error message
================
Comment at: lldb/source/Target/TraceHTR.cpp:164
+ if (valid_cursor) {
+ if (llvm::Error err = cursor.GetError()) {
+ m_instruction_layer_up->AddCallInstructionMetadata(
----------------
wallace wrote:
> just call IsError as you don't need the error message
i'm a little bit confused here, lines 165 -> 167 will only be execute if the current instruction is an error, whose load address is going to be 0, so you'll get an invalid function name. This should instead be
if(!end_of_trace && !cursor.IsError()) {
m_instruction_layer_up->AddCallInstructionMetadata(
current_instruction_load_address,
function_name_from_load_address(cursor.GetLoadAddress()));
} else {
m_instruction_layer_up->AddCallInstructionMetadata(
current_instruction_load_address, llvm::None);
}
================
Comment at: lldb/source/Target/TraceHTR.cpp:168
+ function_name_from_load_address(cursor.GetLoadAddress()));
+ s.Printf("%s\n", toString(std::move(err)).c_str());
+ }
----------------
remove this
================
Comment at: lldb/source/Target/TraceHTR.cpp:184-185
+ size_t num_instructions = m1.m_num_instructions + m2.m_num_instructions;
+ llvm::DenseMap<ConstString, size_t> func_calls;
+ func_calls.insert(m1.m_func_calls.begin(), m1.m_func_calls.end());
+ for (const auto &[name, num_calls] : m2.m_func_calls) {
----------------
you could also just do
llvm::DenseMap<ConstString, size_t> func_calls = m1.m_func_calls;
if the object doesn't allow it, then it's fine
================
Comment at: lldb/source/Target/TraceHTR.cpp:187-190
+ if (func_calls.find(name) == func_calls.end())
+ func_calls[name] = num_calls;
+ else
+ func_calls[name] += num_calls;
----------------
did you try just doing
func_calls[name] += num_calls;
std::map will create a zeroed value for a new key when using the [] operator, so I imagine DenseMap work the same way. Nevermind if that doesn't work.
================
Comment at: lldb/source/Target/TraceHTR.cpp:197
+HTRBlock IHTRLayer::MergeUnits(size_t start_unit_index, size_t num_units) {
+ assert(num_units > 0);
+ llvm::Optional<HTRBlockMetadata> merged_metadata = llvm::None;
----------------
remove this assert, the code wouldn't break if num_units is 0
================
Comment at: lldb/source/Target/TraceHTR.cpp:204-205
+ } else {
+ merged_metadata =
+ HTRBlockMetadata::MergeMetadata(*merged_metadata, current_metadata);
+ }
----------------
this is very expensive. Let's suppose that num_units is N, then you'll invoke MergeMetadata N - 1 times, and each element of the first map will be copied N - 1 times, each one from the second map will be copied N - 2 times, etc. So you'll have N^2 copies, which is just too slow.
Instead, you have two options:
- when you merge A and B, you don't create a new map, you just copy the elements from B to A, effectively modifying A's map. That way after N merges, each element from each map will need to be copied only once. This might intuitively makes sense from an API standpoint, because when you merge blocks, you might expect the original blocks to go away
- remove the merge method that accepts only two blocks and do the merging here coping everything to the same single map
up to you
================
Comment at: lldb/source/Target/TraceHTR.cpp:212-214
+ lldb::HTRBlockLayerUP current_block_layer_up = BasicSuperBlockMerge(
+ *m_instruction_layer_up); // Does a copy occur here for
+ // *instruction_layer_up?
----------------
remove this comment
================
Comment at: lldb/source/Target/TraceHTR.cpp:216
+ HTRBlockLayer ¤t_block_layer = *current_block_layer_up;
+ if (m_instruction_layer_up->GetNumUnits() ==
+ current_block_layer.GetNumUnits())
----------------
better create a new method ArePassesDone
================
Comment at: lldb/source/Target/TraceHTR.cpp:223
+ BasicSuperBlockMerge(current_block_layer);
+ if (current_block_layer.GetNumUnits() == new_block_layer_up->GetNumUnits())
+ break;
----------------
also call the ArePassesDone method here
================
Comment at: lldb/source/Target/TraceHTR.cpp:225-227
+ // How is this valid? I thought you couldn't reassign references?
+ // Is this really copying the layer under the hood? Would using a pointer
+ // instead of a reference make more sense to prevent the copying?
----------------
remove this
================
Comment at: lldb/source/Target/TraceHTR.cpp:233
+
+void TraceHTR::Export(Stream &s, TraceExportFormat export_format,
+ std::string outfile) {
----------------
wallace wrote:
> remove the TraceExportFormat enum. You don't really need it because there's one single case for now
return an llvm::Error instead of receiving a Stream
================
Comment at: lldb/source/Target/TraceHTR.cpp:233-234
+
+void TraceHTR::Export(Stream &s, TraceExportFormat export_format,
+ std::string outfile) {
+ if (export_format == eTraceExportChromeTraceFormat) {
----------------
remove the TraceExportFormat enum. You don't really need it because there's one single case for now
================
Comment at: lldb/source/Target/TraceHTR.cpp:240-241
+ if (os.has_error()) {
+ s.Printf("error opening %s to export trace representation\n",
+ outfile_cstr);
+ } else {
----------------
include the error code in the error message. See how other parts of the code emit a meaningful error message for this kind of calls
================
Comment at: lldb/source/Target/TraceHTR.cpp:245
+ os.flush();
+ if (os.has_error()) {
+ s.Printf("error exporting trace representation to %s\n", outfile_cstr);
----------------
can you also show the actual error message from os?
================
Comment at: lldb/source/Target/TraceHTR.cpp:253-257
+ } else {
+ // this shouldn't be reachable since the export format's validity is
+ // enforced in CommandObjectThread.cpp
+ s.Printf("unknown export format");
+ }
----------------
remove
================
Comment at: lldb/source/Target/TraceHTR.cpp:260
+
+namespace lldb_private {
+
----------------
you don't need this
================
Comment at: lldb/source/Target/TraceHTR.cpp:412
+ {"ph", "B"},
+ {"ts", (ssize_t)i}, // TODO: is there a way to serialize size_t to JSON?
+ // if not, how should we handle overflow here?
----------------
wallace wrote:
> use int64_t just in case, as that's how it's defined
remove this comment
================
Comment at: lldb/source/Target/TraceHTR.cpp:412-422
+ {"ts", (ssize_t)i}, // TODO: is there a way to serialize size_t to JSON?
+ // if not, how should we handle overflow here?
+ {"pid", (ssize_t)layer_id},
+ {"tid", (ssize_t)layer_id},
+ });
+
+ layers_as_json.emplace_back(llvm::json::Object{
----------------
use int64_t just in case, as that's how it's defined
================
Comment at: lldb/source/Target/TraceHTR.cpp:439-440
+
+ // Can't use ConstString here because no toJSON implementation exists for
+ // ConstString
+ llvm::Optional<std::string> most_freq_func =
----------------
this comment doens't make sense, as you are instead passing display_name to the json object
================
Comment at: lldb/test/API/commands/trace/TestTraceExport.py:60
+ error=True)
+
----------------
Add a new test in which you run this algorithm on a program with a few levels of function calls, and check that the output trace makes sense. Don't check for individual load addresses, but instead check for the appearance of the methods and blocks that you expect.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105741/new/
https://reviews.llvm.org/D105741
More information about the lldb-commits
mailing list