zturner added inline comments.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:738-742
+ StartProcessorTracing(tid, m_pt_process_config, traceMonitor);
+ if (traceMonitor.get() != nullptr) {
+ traceMonitor->setUserID(m_pt_process_uid);
+ m_pt_traced_thread_group.insert(tid);
+ }
----------------
This function returns a `Status`. Can't we assume that `traceMonitor` will be
valid if and only if the returned `Status` is a success condition? And if it's
not a success condition, shouldn't you log the error?
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2517-2522
+ auto iter = m_processor_trace_monitor.begin();
+ for (; iter != m_processor_trace_monitor.end(); iter++) {
+ if (uid == iter->second->getUID() &&
+ (thread == iter->first || thread == LLDB_INVALID_THREAD_ID))
+ return iter->second;
+ }
----------------
Please use a range based for here.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2626
+ lldb::user_id_t uid = LLDB_INVALID_UID;
+ if (config.getType() == lldb::TraceType::eTraceTypeProcessorTrace) {
+ if (m_pt_process_uid == LLDB_INVALID_UID) {
----------------
Can you do an early return here if the condition is not true?
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2627
+ if (config.getType() == lldb::TraceType::eTraceTypeProcessorTrace) {
+ if (m_pt_process_uid == LLDB_INVALID_UID) {
+ m_pt_process_config = config;
----------------
And here
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2638-2642
+ llvm::StringRef intel_custom_params_key("intel-pt");
+ llvm::StringRef cpu_family_key("cpu_family");
+ llvm::StringRef cpu_model_key("cpu_model");
+ llvm::StringRef cpu_stepping_key("cpu_stepping");
+ llvm::StringRef cpu_vendor_intel_key("cpu_vendor_intel");
----------------
Nothing specifically wrong with this, but it's implicitly convertible, so if
you like it you can simply just pass these strings into the `AddIntegerItem`
function as string literals.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2717
+ m_processor_trace_monitor.insert(
+ std::pair<lldb::tid_t, ProcessorTraceMonitorSP>(thread, traceMonitor));
+
----------------
`std::make_pair(thread, traceMonitor)` might allow this to fit on one line.
`m_processor_trace_monitor.emplace(thread, traceMonitor);` almost definitely
would.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2744
+ error = getCPUType(cpu_family, model, stepping, vendor_id);
+ if (error.Success()) {
+ StructuredData::Dictionary *intel_params = new
StructuredData::Dictionary();
----------------
Early return here.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2745
+ if (error.Success()) {
+ StructuredData::Dictionary *intel_params = new
StructuredData::Dictionary();
+
----------------
Who is responsible for freeing this memory?
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2794-2795
+
+ switch (trace_type) {
+ case lldb::TraceType::eTraceTypeProcessorTrace:
+ error =
----------------
Seems like this would be more straightforward to just say:
```
if (trace_type != eTraceTypeProcessorTrace)
return NativeProcessProtocol::StartTrace(config, error);
```
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2870-2871
+ lldb::tid_t thread) {
+ Status error;
+ Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PTRACE));
+
----------------
Bunch of opportunities in this function for early returning on error conditions.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2876-2877
+ bool uid_found = false;
+ for (; iter != m_processor_trace_monitor.end(); iter++) {
+ if (iter->second->getUID() == uid) {
+ // Stopping a trace instance for an individual thread
----------------
Range based for with an inverted conditional and early continue inside the loop.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2931-2938
+static uint64_t ComputeFloorLog2(uint64_t input) {
+ uint64_t prev = input;
+ while ((input & (input - 1)) != 0) {
+ input &= (input - 1);
+ prev = input;
+ }
+ return prev;
----------------
Delete and replace callsites with `llvm::Log2_64`
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2940-2943
+Status NativeProcessLinux::ProcessorTraceMonitor::StartTrace(
+ lldb::pid_t pid, lldb::tid_t tid, TraceOptions &config) {
+ Status error;
+ Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PTRACE));
----------------
What happens if you call this function twice in a row? Or from different
threads? Is that something you care about?
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3009-3010
+ errno = 0;
+ m_mmap_base =
+ mmap(NULL, (metabufsize + page_size), PROT_WRITE, MAP_SHARED, m_fd, 0);
+ if (m_mmap_base == MAP_FAILED) {
----------------
Perhaps you can use `llvm::MemoryBuffer` here? It mmaps internally
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3046
+ uint64_t &cpu_family, uint64_t &model, uint64_t &stepping,
+ std::string &vendor_id) {
+ Status error;
----------------
It looks to me like `vendor_id` param could be a `StringRef&`.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:156
+ // ---------------------------------------------------------------------
+ static size_t ReadCyclicBuffer(void *buf, size_t buf_size, void *cyc_buf,
+ size_t cyc_buf_size, size_t cyc_start,
----------------
labath wrote:
> How about ` void ReadCyclicBuffer(ArrayRef<uint8_t> buffer, size_t position,
> MutableArrayRef<uint8_t> &dest)`
Better yet, drop the `position` argument entirely. `ReadCyclicBuffer(src, N,
dest)` is equivalent to `ReadCyclicBuffer(src.drop_front(N), dest);`
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:319-320
+ int m_fd;
+ void *m_mmap_data;
+ void *m_mmap_aux;
+ void *m_mmap_base;
----------------
Instead of having
```
void *m_mmap_data;
void *m_mmap_aux;
void *m_mmap_base;
```
and then doing some ugly casting every time someone calls `getAuxBufferSize` or
`getDataBufferSize`, instead just write:
```
MutableArrayRef<uint8_t> m_mmap_data;
MutableArrayRef<uint8_t> m_mmap_aux;
```
and initializing these array refs up front using the size from the header.
This way you don't have to worry about anyone using the buffers incorrectly,
and the `ReadPerfTraceAux(size_t offset)` function no longer needs to return a
`Status`, but instead it can just return `MutableArrayRef<uint8_t>` since
nothing can go wrong.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:397-398
+
+ std::unordered_map<lldb::tid_t, ProcessorTraceMonitorSP>
+ m_processor_trace_monitor;
+
----------------
Can you use an `llvm::DenseMap` here?
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:400-402
+ // Set for tracking threads being traced under
+ // same process user id.
+ std::set<lldb::tid_t> m_pt_traced_thread_group;
----------------
And an `llvm::DenseSet` here?
================
Comment at:
source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:1319-1320
- for (size_t i = 0; i < buf.size(); ++i)
+ for (uint64_t i = 0; i < buf.size(); ++i)
response.PutHex8(buf[i]);
----------------
```
for (const auto &I : buf)
response.PutHex8(I);
```
https://reviews.llvm.org/D33674
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
