jj10306 requested changes to this revision.
jj10306 added a comment.
This revision now requires changes to proceed.
feedback-v1
================
Comment at: lldb/include/lldb/Target/Trace.h:520
+ /// core id -> data kind -> size
+ llvm::DenseMap<lldb::core_id_t, std::unordered_map<std::string, size_t>>
+ m_live_core_data;
----------------
Would this work instead? I noticed that the several other maps around this code
also use unordered_map and string, maybe we could update those too at some
point.
================
Comment at: lldb/include/lldb/Target/Trace.h:538
+ /// core id -> data kind -> file
+ llvm::DenseMap<lldb::core_id_t, std::unordered_map<std::string, FileSpec>>
+ m_postmortem_core_data;
----------------
same as comment above related to using LLVM types rather than std types
wherever possible.
================
Comment at: lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.cpp:193
-Expected<std::vector<uint8_t>>
-IntelPTMultiCoreTrace::GetBinaryData(const TraceGetBinaryDataRequest &request)
{
- return createStringError(inconvertibleErrorCode(), "Unimplemented");
+Expected<Optional<std::vector<uint8_t>>>
+IntelPTMultiCoreTrace::TryGetBinaryData(
----------------
`Expected<Optional<...>>` feels weirddddd. Are both of these "layers" needed?
I noticed this in a couple places but I'm just leaving a single comment here.
================
Comment at: lldb/source/Plugins/Process/Linux/Perf.cpp:191
+ * with respect to their definition of head pointer. In the case
+ * of Aux data buffer the head always wraps around the aux buffer
+ * and we don't need to care about it, whereas the data_head keeps
----------------
Be careful with the "strong" language here. The AUX head only wraps around when
the AUX buffer is mmaped with read-only, if it is mmapped with write perms,
then the AUX head behaves like the data head (it doesn't auto wrap)
See this snipped of kernel code, the overwrite flag is set based on the mmap
PROT and that overwrite flag is passed to the intelpt code.
https://github.com/torvalds/linux/blob/df0cc57e/kernel/events/ring_buffer.c#L670-L733
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126015/new/
https://reviews.llvm.org/D126015
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
