[Lldb-commits] [PATCH] D105741: [trace] Add `thread trace dump ctf -f <filename>` command

Mon Jul 12 14:52:14 PDT 2021

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.

================
Comment at: lldb/include/lldb/Target/TraceHTR.h:1-2
+//===-- TraceHTR.h -------------------------------------------------*- C++
+//-*-===//
+//
----------------
Make this into one line of the right length

================
Comment at: lldb/include/lldb/Target/TraceHTR.h:3
+//-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
Maybe add a definition here of what "HTR" stands for and a bit of background on what classes are in this file here or below.

================
Comment at: lldb/include/lldb/Target/TraceHTR.h:27
+/// \class HTRBlockMetadata TraceHTR.h "lldb/Target/TraceHTR.h"
+/// Metadata associated with an HTR block
+/// Metadata is initially populated in Layer 1 and merged as blocks are merged
----------------
What is HTR? A definition here or in the initial comment block as the top of the file with a URL or full explanation would be great.

================
Comment at: lldb/include/lldb/Target/TraceHTR.h:41
+  size_t m_num_instructions;
+  std::unordered_map<std::string, size_t> m_func_calls;
+  friend llvm::json::Value toJSON(const HTRBlockMetadata &metadata);
----------------
Use ConstString here instead of std::string. std::string will make a copy of the string. ConstString objects should always be used for storing function and type names as a constant string pool will unique any strings that are placed into a ConstString. This will avoid consuming more memory for these traces and ConstString also is much more efficient for comparing strings for equality if that is needed anywhere.

================
Comment at: lldb/include/lldb/Target/TraceHTR.h:48-50
+/// Sequences of blocks are merged to create a new, single block
+/// Each block indirectly corresponds to a sequence of "unit" blocks (ie
+/// instructions)
----------------
Maybe some more complete documentation here would be great to fully explain what this class does and tracks.

================
Comment at: lldb/include/lldb/Target/TraceHTR.h:67-68
+  template <class T> friend class HTRLayer;
+  template <class M> friend llvm::json::Value toJSON(const TraceHTR<M> &layer);
+  template <class B> friend llvm::json::Value toJSON(const HTRBlock<B> &block);
+};
----------------
These should be public on the classes (TraceHTR and HTRBlock), so this should not be needed? By that I mean just add a "toJSON" method to TraceHTR and HTRBlock classes.

================
Comment at: lldb/include/lldb/Target/TraceHTR.h:73
+/// Maps the unique Block IDs to their respective Blocks
+template <class TMetadata> class HTRBlockDefs {
+public:
----------------
Does this need to be templatized? Are we using this same class with multiple different TMetadata classes or are we always using it with HTRBlockMetadata?

================
Comment at: lldb/include/lldb/Target/TraceHTR.h:81
+/// Layer of the HTR representing a sequence of blocks
+template <class TMetadata> class HTRLayer {
+public:
----------------
Same comment as above. Are we intantiating this only with HTRBlockMetadata?

================
Comment at: lldb/include/lldb/Target/TraceHTR.h:143-144
+  std::vector<size_t> block_id_trace;
+  template <class T> friend class TraceHTR;
+  template <class M> friend llvm::json::Value toJSON(const TraceHTR<M> &layer);
+};
----------------
using the "friend" keyword usually means some methods that are private should actually be public. Can you see if you can make some methods public to avoid having to "friend" other classes?

================
Comment at: lldb/source/Commands/CommandObjectThread.cpp:2172
+
+  CommandObjectTraceDumpChromeTraceFormat(CommandInterpreter &interpreter)
+      : CommandObjectIterateOverThreads(
----------------
Instead of making a new custom command, I would say we should just add a "--export" option to the "thread trace dump" command that can take a export enumeration as its value. Possible values would be "chrome-trace-format" or "ctf". If we architect it correctly, we would create a Trace export plug-in within LLDB to allow other people to create new trace export plug-ins in the future. 

================
Comment at: lldb/source/Target/TraceHTR.cpp:1-2
+//===-- TraceHTR.cpp
+//---------------------------------------------------------===//
+//
----------------
Fix this to be one line the same length as the last comment separator on line 8

================
Comment at: lldb/source/Target/TraceHTR.cpp:56
+  int block_id = 0;
+  std::unordered_map<lldb::addr_t, size_t> load_address_to_block_id;
+  // This flag becomes true when cursor->Next() returns false
----------------
So each unique address gets added to this map? if we have many instructions that are in the same block like 0x1000, 0x1004 and 0x1008, we will have all three addresses in the map pointing to the same block ID? If we used an ordered_map, we could use lower_bound to find the lowest address that points to the block, and the block could have an address range inside of it that could be updated as we went along. It might end up saving memory if we can do it this way. 

So if we have:
```
0x1000: nop
0x1004: nop
0x1008: nop
0x100c: call 0x2000
0x2000: nop 
0x2004: nop 
0x2008: nop 
```
This would end up with two blocks, one whose range is [0x1000-0x1010) (4 bytes after 0x100c), and one from [0x2000-0x200c). If we used a map the map could simply end up containing:
```
0x1000 -> 1 (Block [0x1000-0x1010)
0x2000 -> 2 (Block [0x2000-0x200c)
```
Using an ordered_map would take a bit more logic to always update a Block's address range on the fly and as load addresses come in, like 0x1004, 0x1008, 0x100c, we would need to locate the 0x1000 entry using "load_address_to_block_id.lower_bound(addr)" and seeing if the block's address range has been finalized, and if not, update it, and if it was, just make sure the address is in the block.

Where as right now the load_address_to_block_id would end up containing:
```
0x1000 -> 1
0x1004 -> 1
0x1008 -> 1
0x100c -> 1
0x2000 -> 2
0x2004 -> 2
0x2008 -> 2
```
I worry about the size of the load_address_to_block_id data structure if it isn't minimized.

================
Comment at: lldb/source/Target/TraceHTR.cpp:64
+    // If no `instruction_count` was specified, traverse the entire trace
+    bool valid_trace_index = instruction_count ? i < *instruction_count : true;
+    return valid_cursor && valid_trace_index;
----------------
Calculate "const size_t max_instruction_count = instruction_count ? *instruction_count : SIZET_MAX;" outside of this lambda and let this lambda use "max_instruction_count" to avoid having to evaluate the if/then for the ?: operator inside this lambda each time through

================
Comment at: lldb/source/Target/TraceHTR.cpp:70
+  // 2. unique block id: block
+  while (continue_traversal()) {
+    // TODO: how should we handle cursor errors in this loop?
----------------
You can problably get rid of the above lambda if you calculate "max_instruction_count" as detailed above

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105741/new/

https://reviews.llvm.org/D105741