clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Great start to this. Many comments inlined.
When you are dumping instructions we are only showing one hex value. Is this
the instruction address or the opcode itself?
================
Comment at: lldb/include/lldb/Target/Trace.h:122-123
+
+ /// Run the provided callback on the instructions of the trace of the given
+ /// thread.
+ ///
----------------
I am going to comment here on what this function should look like and how it
will be used by all of the APIs. This function can probably be used to
implement the forward and reverse stepping/continue commands eventually. So I
would propose that this function should be able to start from a given index and
be able to go forward or backward in the instructions (for forward/reverse
step/continue).
So with this in mind how about:
```
void TraverseInstructions(const Thread &thread, size_t position, bool forward,
std::function<...> callback) = 0;
```
================
Comment at: lldb/include/lldb/Target/Trace.h:146
+ const Thread &thread,
+ std::function<bool(size_t index, llvm::Expected<lldb::addr_t>
&load_addr)>
+ callback,
----------------
This should probably be passed by value as it could contain an error. If there
is an error the error must be consumed. If we pass by reference then it is
unclear who must consume the error.
================
Comment at: lldb/include/lldb/Target/Trace.h:157
+ /// The total number of instructions of the trace.
+ virtual size_t GetInstructionCount(const Thread &thread) = 0;
+
----------------
Will we always know instruction count? Could this to very expensive to
calculate? Can we add this to the generic trace API and expect all trace
formats to implement this?
================
Comment at: lldb/include/lldb/Target/Trace.h:171
+ /// or an actual Error in case of failures.
+ virtual llvm::Error GetTraceErrorStatus(const Thread &thread) = 0;
};
----------------
Can we just use the ForEachInstruction and get the error during that call? Is
this call redundant? If there is an error, it might be better to get it via the
ForEachInstruction function and know where the problem is. If there is no data,
you will get the error on the first access to the first instruction. Knowing
where the error happened might help the user have more context.
================
Comment at: lldb/source/Target/ProcessTrace.cpp:151-152
size_t ProcessTrace::DoReadMemory(addr_t addr, void *buf, size_t size,
Status &error) {
+ const std::map<MemoryRegionInfo::RangeType, SectionSP> §ion_map =
----------------
You should be able to just call:
```
size_t Target::ReadMemoryFromFileCache(const Address &addr, void *dst, size_t
dst_len, Status &error);
```
It already does what you are doing here if all that is happening here is
reading from loaded object file section data.
================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:41-61
self.expect("thread trace dump instructions",
- substrs=['thread #1: tid = 3842849, total instructions = 1000',
- 'would print 20 instructions from position 0'])
+ substrs=['''thread #1: tid = 3842849, total instructions = 21
+ [ 0] 0x40052d
+ [ 1] 0x400529
+ [ 2] 0x400525
+ [ 3] 0x400521
+ [ 4] 0x40052d
----------------
What is the default count here? 19? Seems like an odd number to choose as a
default?
================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:137
+ [16] 0x40052d''', result.GetOutput())
+
+ def testWrongImage(self):
----------------
Do we have a test for when the offset is invalid? Another test for the count
being too large and the output would get truncated?
================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:142-143
+ substrs=['''thread #1: tid = 3842849, total instructions = 2
+ [0] no memory mapped at this address
+ [1] no memory mapped at this address'''])
+
----------------
We should be showing addresses here. It doesn't matter if they are mapped or
not. This will happen for JIT'ed code.
================
Comment at: lldb/test/API/commands/trace/TestTraceDumpInstructions.py:163
+ [ 3] 0x400654
+ [ 4] no memory mapped at this address
+ [ 5] 0x400516
----------------
Why aren't we showing the address here? We will run into cases, for possibly
JIT'ed code where we won't have a section for an address, so we should still
show the address
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89283/new/
https://reviews.llvm.org/D89283
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
