[Lldb-commits] [PATCH] D55522: Cache memory regions in ProcessMinidump and use the linux maps as the source of the information if available.

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:409
+    return false;
+  llvm::StringRef text((const char *)data.data(), data.size());
+  llvm::StringRef line;
----------------
You can write this as `auto text = llvm::toStringRef(data);`


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:411-412
+  llvm::StringRef line;
+  constexpr const auto yes = MemoryRegionInfo::eYes;
+  constexpr const auto no = MemoryRegionInfo::eNo;
+  while (!text.empty()) {
----------------
No reason to have both `constexpr` and `const`, since the former implies the latter.

As an aside, I find this quite strange.  I'm not sure why we don't just use `llvm::Optional<bool>` and delete `MemoryRegionInfo::OptionalBool`.  The former is an `enum`, which defaults to `int`, while `sizeof(Optional<bool>) == 2`, so it seems strictly better, as well as being more intuitive.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:426
+    line = line.ltrim();
+    llvm::StringRef permissions = line.substr(0, 4);
+    line = line.drop_front(4);
----------------
`line.take_front(4);` is a little more idiomatic, but this is fine anyway.  If we need to handle these strings being an invalid format we should also check for length before any `take`, `drop`, or `substr` operation


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:447-449
+    region.SetReadable(permissions[0] == 'r' ? yes : no);
+    region.SetWritable(permissions[1] == 'w' ? yes : no);
+    region.SetExecutable(permissions[2] == 'x' ? yes : no);
----------------
This is a good example where `Optional<bool>` would be nice.  You could just eliminate the ternary and pass the result of the equality comparison and it would work fine.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:467-474
       const uint32_t PageNoAccess =
           static_cast<uint32_t>(MinidumpMemoryProtectionContants::PageNoAccess);
-      info.SetReadable((entry->protect & PageNoAccess) == 0 ? yes : no);
-
       const uint32_t PageWritable =
           static_cast<uint32_t>(MinidumpMemoryProtectionContants::PageWritable);
-      info.SetWritable((entry->protect & PageWritable) != 0 ? yes : no);
-
-      const uint32_t PageExecutable = static_cast<uint32_t>(
-          MinidumpMemoryProtectionContants::PageExecutable);
-      info.SetExecutable((entry->protect & PageExecutable) != 0 ? yes : no);
-
+  const uint32_t PageExecutable =
+      static_cast<uint32_t>(MinidumpMemoryProtectionContants::PageExecutable);
       const uint32_t MemFree =
----------------
Maybe we could add some member functions to `MinidumpMemoryInfo` such as `isExecutable()`, `isReadable()`, etc


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:541
+MinidumpParser::FindMemoryRegion(lldb::addr_t load_addr) const {
+  if (!m_regions.empty()) {
+    auto begin = m_regions.begin();
----------------
Can we early out here if `m_regions.empty()` is true?


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:547
+      return *pos;
+    } else if (pos != begin) {
+      --pos;
----------------
Since the previous line is a return statement, we don't need the else.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:555-556
+    if (pos == end) {
+      if (pos == begin)
+        return llvm::None;
+      auto prev = pos - 1;
----------------
This condition doesn't seem possible based on the code, because it would imply the list of regions is empty, but then we would either not be in this branch (current code), or have already returned (based on suggestion above).

I think I see what it's trying to do though.  I think we need to move this check up in between the previous `if/else` block.  So it should be something like:

```
if (pos != end && pos->GetRange().Contains(load_addr))
  return *pos;

if (pos == begin)
  return llvm::None;

--pos;
if (pos->GetRange().Contains(load_addr))
  return pos;
```

However, at this point, haven't we handled every possible case?  Either the user passed the exact address of offset 0 of a memory region (case 0), they passed an address that is not in any region (case 2), or they passed an address that is in the middle of a region (case 3).

Can't we just write `return llvm::None` at this point?   What is the rest of the code for?


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:582-583
+  // See if we have cached our memory regions yet?
+  if (!m_regions.empty())
+    return FindMemoryRegion(load_addr);
 
----------------
Actually, we're already checking if it's empty here before we call the function.   Then inside the function we check if it's empty again.  Since it's part of the function's private interface, we have full control over the inputs, we should assert inside of the implementation of `FindMemoryRegion` that it's not empty.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:97-100
+  bool CreateRegionsCacheFromLinuxMaps();
+  bool CreateRegionsCacheFromMemoryInfoList();
+  bool CreateRegionsCacheFromMemoryList();
+  bool CreateRegionsCacheFromMemory64List();
----------------
Minor nit, but wherever possible, it's nice if we can eliminate private functions from a class's definition and move them to static internal-linkage functions in the implementation file.  For example, I'm imagining you could re-write these as:

```
static bool CreateRegionsCacheFromLinuxMaps(ArrayRef<uint8_t> linux_maps, std::vector<MemoryRegionInfo> &regions) {
}

// etc
```

in the implementation file.  It makes the header file smaller which is helpful when trying to read it and understand the functionality of a class, and also improves link time.


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

https://reviews.llvm.org/D55522