wallace requested changes to this revision. wallace added a comment. This revision now requires changes to proceed.
Much better! We are getting closer. There's documentation missing. Besides, you should use unique_ptrs as mentioned in the comments so that you prevent copying massively large objects. I also left some indications on how to deal with errors. ================ Comment at: lldb/docs/htr.rst:1 +Hierarchical Trace Representation (HTR) +====================================== ---------------- jj10306 wrote: > @clayborg @wallace Is there a way to view the "rendered" rst to ensure the > the format is as I intended and that the image links are working? TL;DR: just do the small fix I mention below you can use http://rst.ninjs.org/ to check the rendering. According to it you need to make the small fix I mention below. Regarding the image, well, probably you could build the documentation locally and see that everything is working. If you look at docs/CMakeLists.txt, you'll see that there's a target called lldb-cpp-doc, but you need to enable doxygen first. However, some parts of the documentation are kept within the tree and not exposed in the lldb website. For example, if you look at docs/lldb-gdb-remote.txt, it's not shown anywhere in https://lldb.llvm.org/index.html. So if we keep this documentation internal, this file is okay as it is, as the reader can open the image manually if needed. Once all of tracing becomes more stable and with more features, we can start making documentation like this more visible in the website, but that won't happen soon. ================ Comment at: lldb/docs/htr.rst:42-45 +- For each block, compute the number of distinct predecessor and successor blocks. + - **Predecessor** - the block that occurs directly before (to the left of) the current block + - **Successor** - the block that occurs directly after (to the right of) the current block +- A block with more than one distinct successor is always the start of a super block, the super block will continue until the next block with more than one distinct predecessor or successor. ---------------- you need some blank lines ================ Comment at: lldb/include/lldb/Target/Trace.h:66-68 + /// \param[in] count + /// The number of trace instructions to include in the CTF dump + /// ---------------- remove this and add an entry for export_format ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:1-9 +//===-- TraceHTR.h --------------------------------------------------------===// +// +// Hierarchical Trace Representation (HTR) - see lldb/docs/htr.rst for +// comprehensive HTR documentation// Part of the LLVM Project, under the Apache +// License v2.0 with LLVM Exceptions.// See https://llvm.org/LICENSE.txt for +// license information.// SPDX-License-Identifier: Apache-2.0 WITH +// LLVM-exception ---------------- ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:26-36 + HTRBlockMetadata(lldb::addr_t first_instruction_load_address, + size_t num_instructions, + llvm::DenseMap<ConstString, size_t> &func_calls) + : m_first_instruction_load_address(first_instruction_load_address), + m_num_instructions(num_instructions), m_func_calls(func_calls) {} + static HTRBlockMetadata MergeMetadata(HTRBlockMetadata const &m1, + HTRBlockMetadata const &m2); ---------------- leave spaces between functions for readability and add documentation for all the methods ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:45 +/// \class HTRBlock TraceHTR.h "lldb/Target/TraceHTR.h" +/// Block structure representing a sequence of trace "units" (ie instructions) +/// Sequences of blocks are merged to create a new, single block ---------------- add a period . after the parenthesis ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:50-54 + HTRBlock(size_t offset, size_t size, HTRBlockMetadata metadata) + : m_offset(offset), m_size(size), m_metadata(metadata) {} + size_t GetOffset() const; + size_t GetSize() const; + HTRBlockMetadata const &GetMetadata() const; ---------------- spaces between methods and documentation ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:57-63 + /// Offset in the previous layer + size_t m_offset; + /// Number of blocks/instructions that make up this block in the previous + /// layer + size_t m_size; + /// General metadata for this block + HTRBlockMetadata m_metadata; ---------------- leave this comments here, but generally we try to make the documentation of the API methods more verbose than the private members, which are rarely seen ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:72 + size_t GetLayerId() const; + void SetLayerId(size_t id); + /// Get the metadata associated with the unit (instruction or block) at ---------------- better have a constructor here that expects an ID, so that at any point in the life of the object, there's always a valid ID ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:99 + +/// See lldb/docs/htr.rst for comprehensive HTR documentation +class HTRInstructionLayer : public IHTRLayer { ---------------- Add a small line in the documentation summarizing what this is. The htr.rst file should be used for digging deeper into this structure ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:104-106 + /// This allows passes to operate on an IHTRLayer without knowing whether it + /// is an HTRInstructionLayer + // or HTRBlockLayer ---------------- remove this, instead mention in the original function declaration that the implementation class can decide how to store or generate this data, as for instructions we want to do it lazily ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:107-113 + HTRBlockMetadata GetMetadataByIndex(size_t index) const override; + size_t GetNumUnits() const override; + + std::vector<lldb::addr_t> const &GetInstructionTrace() const; + void AddCallInstructionMetadata(lldb::addr_t load_addr, + llvm::Optional<ConstString> func_name); + void AppendInstruction(lldb::addr_t load_addr); ---------------- documentation of non-override methods ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:116 +private: + std::vector<lldb::addr_t> m_instruction_trace; + // Only store metadata for instructions of interest (call instructions) ---------------- mention here about the order of the trace. Are the instructions stored backwards or forwards chronologically? ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:124 + // name can't be determined) + std::unordered_map<lldb::addr_t, llvm::Optional<ConstString>> m_call_isns; +}; ---------------- pretty nice! ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:134 + + std::unordered_map<size_t, HTRBlock> const &GetBlockDefs() const; + std::vector<size_t> const &GetBlockIdTrace() const; ---------------- documentation ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:136-140 + /// Updates block_id: block mapping and append `block_id` to the block id + /// trace + void AddNewBlock(size_t block_id, HTRBlock &block); + /// Append `block_id` to the block id trace + void AppendBlockId(size_t block_id); ---------------- why do you need both? ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:170 + // There is a single instruction layer per HTR + HTRInstructionLayer m_instruction_layer; + // There are one or more block layers per HTR ---------------- Make this a std::unique_ptr<HTRInstructionLayer> so that you avoid making copies of it. You can use std::make_unique<HTRInstructionLayer>(constructor args...) to create a new instance. Otherwise you are at the risk of creating copies, which would be super memory inefficient ================ Comment at: lldb/include/lldb/Target/TraceHTR.h:172 + // There are one or more block layers per HTR + std::vector<HTRBlockLayer> m_block_layers; +}; ---------------- Same here, you should use unique pointers. You could create typedef/using declarations to have shorter names HTRInstructionLayerUP and HTRBlockLayerUP. Also your variable should have the _up suffix, e.g. m_instruction_layer_up ================ Comment at: lldb/include/lldb/lldb-enumerations.h:1148-1151 +enum TraceExportFormat { + eTraceExportUnknownFormat = 0, + eTraceExportChromeTraceFormat = 1, +}; ---------------- move this enum to Trace.h. to avoid exposing this too much. Also remove the "unknown" format, as it's useless I'm working on a way to create Trace Plugins, so that you could add "exporter" plugins without having to modify anything from lldb proper. That would be similar to TraceIntelPT, which is self-contained in its own folder ================ Comment at: lldb/source/Commands/CommandObjectThread.cpp:2145-2151 + m_trace_export_format = + (TraceExportFormat)OptionArgParser::ToOptionEnum( + option_arg, GetDefinitions()[option_idx].enum_values, + eTraceExportUnknownFormat, error); + if (m_trace_export_format == eTraceExportUnknownFormat) + error.SetErrorStringWithFormat("unknown format '%s' specified.", + option_arg.str().c_str()); ---------------- as we are going to have exporter plugins, to simplify this code, just expect the "ctf" string here explicitly. Later I'll do the smart lookup ================ Comment at: lldb/source/Target/TraceHTR.cpp:26 +llvm::Optional<std::string> +HTRBlockMetadata::GetMostFrequentylyCalledFunction() const { + size_t max_ncalls = 0; ---------------- s/GetMostFrequentyly/GetMostFrequently ================ Comment at: lldb/source/Target/TraceHTR.cpp:28-34 + llvm::Optional<std::string> max_name = llvm::None; + for (const auto &[name, ncalls] : m_func_calls) { + if (ncalls > max_ncalls) { + max_ncalls = ncalls; + max_name = name.AsCString(); + } + } ---------------- a possible future optimization could be to calculate this maximum number on the fly as the map is being generated ================ Comment at: lldb/source/Target/TraceHTR.cpp:39-41 +HTRBlockMetadata::GetFunctionCalls() const { + return m_func_calls; +} ---------------- you have to explain somewhere how you identify functions: do you find the corresponding function of each instruction, or do you do it just for CALL instructions? ================ Comment at: lldb/source/Target/TraceHTR.cpp:53 + +std::vector<HTRBlockLayer> const &TraceHTR::GetBlockLayers() const { + return m_block_layers; ---------------- wallace wrote: > as these layers could change because of merges and what not, better remove > the consts out of these methods remove a llvm::ArrayRef<HTRBlockLayerUP> ================ Comment at: lldb/source/Target/TraceHTR.cpp:53-57 +std::vector<HTRBlockLayer> const &TraceHTR::GetBlockLayers() const { + return m_block_layers; +} + +HTRInstructionLayer const &TraceHTR::GetInstructionLayer() const { ---------------- as these layers could change because of merges and what not, better remove the consts out of these methods ================ Comment at: lldb/source/Target/TraceHTR.cpp:57 + +HTRInstructionLayer const &TraceHTR::GetInstructionLayer() const { + return m_instruction_layer; ---------------- return HTRInstructionLayer & ================ Comment at: lldb/source/Target/TraceHTR.cpp:61-62 + +void TraceHTR::AddNewBlockLayer(HTRBlockLayer &block_layer) { + m_block_layers.emplace_back(block_layer); +} ---------------- it might be better to use move semantics here, to avoid creating a copy inside m_block_layers. Something like this should be fine ================ Comment at: lldb/source/Target/TraceHTR.cpp:67 + +void IHTRLayer::SetLayerId(size_t id) { layer_id = id; } + ---------------- it would be better if the layer id is set at construction time ================ Comment at: lldb/source/Target/TraceHTR.cpp:71 + AppendBlockId(block_id); + m_block_defs.emplace(block_id, block); +} ---------------- consider moving the HTRBlock instead of copying ================ Comment at: lldb/source/Target/TraceHTR.cpp:78 + +std::vector<lldb::addr_t> const & +HTRInstructionLayer::GetInstructionTrace() const { ---------------- Use an ArrayRef, and remove the const from the return type ================ Comment at: lldb/source/Target/TraceHTR.cpp:126 + +TraceHTR::TraceHTR(Thread &thread, lldb::TraceCursorUP &&cursor) { + // Layer 0 of HTR ---------------- probably you don't want to get the ownership of the cursor, just request a lldb::TraceCursor & reference instead of the unique_ptr. You can use *cursor_up to get that reference ================ Comment at: lldb/source/Target/TraceHTR.cpp:151 + while (valid_cursor) { + // TODO: how should we handle cursor errors in this loop? + lldb::addr_t current_instruction_load_address = cursor->GetLoadAddress(); ---------------- you need to enhance the Instruction object, to support errors. You can store the error string in it using a char * pointer from a ConstString. Errors are not frequent, so that should be fine. You need to display the errors in the export data as well, as hiding them could be misleading to the user ================ Comment at: lldb/source/Target/TraceHTR.cpp:159 + if (current_instruction_type & lldb::eTraceInstructionControlFlowTypeCall) { + if (valid_cursor) { + instruction_layer.AddCallInstructionMetadata( ---------------- check here as well that the current position of the trace is not an error If you need a trace with an error, try doing trace load test/API/commands/trace/intelpt-trace-multi-file/multi-file-no-ld.json e.g. (lldb) trace load /data/users/wallace/llvm-sand/external/llvm-project/lldb/test/API/commands/trace/intelpt-trace-multi-file/multi-file-no-ld.json (lldb) thread trace dump instructions -f -c 100 thread #1: tid = 815455 a.out`main + 15 at main.cpp:10 [ 0] 0x000000000040066f callq 0x400540 ; symbol stub for: foo() a.out`symbol stub for: foo() [ 1] 0x0000000000400540 jmpq *0x200ae2(%rip) ; _GLOBAL_OFFSET_TABLE_ + 40 [ 2] 0x0000000000400546 pushq $0x2 [ 3] 0x000000000040054b jmp 0x400510 a.out`(none) [ 4] 0x0000000000400510 pushq 0x200af2(%rip) ; _GLOBAL_OFFSET_TABLE_ + 8 [ 5] 0x0000000000400516 jmpq *0x200af4(%rip) ; _GLOBAL_OFFSET_TABLE_ + 16 [ 6] 0x00007ffff7df1950 error: no memory mapped at this address ...missing instructions a.out`main + 20 at main.cpp:10 ================ Comment at: lldb/source/Target/TraceHTR.cpp:206 + // Why does using `IHTRLayer ¤t_layer = m_instruction_layer;` not work? + HTRInstructionLayer instruction_layer = m_instruction_layer; + HTRBlockLayer current_layer = BasicSuperBlockMerge(instruction_layer); ---------------- remove this assignment, just use m_instruction_layer directly ================ Comment at: lldb/source/Target/TraceHTR.cpp:207 + HTRInstructionLayer instruction_layer = m_instruction_layer; + HTRBlockLayer current_layer = BasicSuperBlockMerge(instruction_layer); + if (instruction_layer.GetNumUnits() == current_layer.GetNumUnits()) ---------------- you might need to cast *m_instruction_layer_up to ILayer & CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105741/new/ https://reviews.llvm.org/D105741 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
