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> ®ions) {
}
// 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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
