[Lldb-commits] [PATCH] D24385: MinidumpParsing: pid, modules, exceptions

[Zachary Turner via lldb-commits]

zturner added inline comments.

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:88-89
@@ +87,4 @@
+MinidumpParser::GetMinidumpString(uint32_t rva) {
+  llvm::ArrayRef<uint8_t> arr_ref(m_data_sp->GetBytes() + rva,
+                                  m_data_sp->GetByteSize() - rva);
+  return MinidumpString::Parse(arr_ref);
----------------
This is fine, but I prefer to avoid the math wherever possible since it makes code slightly more difficult to read.  I would write this as:

```
auto arr_ref = m_data_sp->GetData().drop_front(rva);
```

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:171
@@ +170,3 @@
+llvm::Optional<LinuxProcStatus> MinidumpParser::GetLinuxProcStatus() {
+  llvm::Optional<llvm::ArrayRef<uint8_t>> data =
+      GetStream(MinidumpStreamType::LinuxProcStatus);
----------------
Same as before, an `Optional<ArrayRef>` seems a bit redundant.  I would just return an empty `ArrayRef` to represent it not being there.  I dont' think there's a valid case for a strema to actually be present, but be 0 bytes in length.

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:202
@@ +201,3 @@
+
+  return MinidumpModule::ParseModuleList(data.getValue());
+}
----------------
It will do this parsing every time you ask for the module list.  How about storing a `std::vector<const MinidumpModule *>` in the `MinidumpParser` class and then you can return an `ArrayRef<const MinidumpModule*>` to it.  So it's lazy evaluation, only done once, with the side benefit of allowing you to return an `ArrayRef<T>` instead of a more complicated `Optional<vector<T>>`

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:47
@@ -45,1 +46,3 @@
 
+  llvm::Optional<const MinidumpString> GetMinidumpString(uint32_t rva);
+
----------------
There's a lot of `Optional`'s here.  Is it really possible for any subset of these to be optional?  Or is it more like if you find one, you will find them all?

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:61
@@ +60,3 @@
+
+  llvm::Optional<std::vector<const MinidumpModule *>> GetModuleList();
+
----------------
I would drop the `Optional` here and return `ArrayRef<const MinidumpModule*>`.  Returning an entire vector by value is wasteful on the stack.  Using an `ArrayRef` makes it very lightweight, while still providing value-like semantics in that you won't be able to modify the underlyign container.  Also, if its size is 0, you can treat that the same as if the `Optional` did not have a value.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:21
@@ +20,3 @@
+llvm::StringRef
+lldb_private::minidump::consumeString(llvm::ArrayRef<uint8_t> &Buffer) {
+  return llvm::StringRef(reinterpret_cast<const char *>(Buffer.data()),
----------------
labath wrote:
> This is not consistent with the consumeObject function, which also drops the consumed bytes from the buffer.
Is this logic correct?  A buffer may be arbitrarily large and have more data in it besides the string.  Perhaps you need to search forward for a null terminator, then only return that portion of the string, then drop that many bytes (plus the null terminator) from the input buffer?

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:223
@@ +222,3 @@
+
+  static llvm::Optional<const MinidumpString>
+  Parse(llvm::ArrayRef<uint8_t> &data);
----------------
labath wrote:
> MinidumpString is just a wrapper around the std::string. Why not return the string directly? (Also the "const" there is unnecessary).
Agree, the `MinidumpString` class seems unnecessary to me.  Just make `parseMinidumpString` be a file static global function and return an `std::string`.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:299
@@ -282,1 +298,3 @@
+  llvm::support::ulittle32_t
+      flags1; // represent what info in the struct is valid
   llvm::support::ulittle32_t process_id;
----------------
labath wrote:
> this is oddly formatted now.
Probably because the comment passed 80 columns.  I would put the comment on a new line above the field to fix this.


https://reviews.llvm.org/D24385