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

[Pavel Labath via lldb-commits]

labath added a comment.

A collection of small remarks, mostly to do with style. Mainly, I'd like to make the tests more strict about what they accept as reasonable values.

A higher level question: Given that we are already in the `minidump` namespace, do we want all (most) of these symbols to be prefixed with `Minidump` ? Thoughts?


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:65
@@ -68,1 +64,3 @@
+    : m_data_sp(data_buf_sp), m_header(header),
+      m_directory_map(std::move(directory_map)) {}
 
----------------
this will still invoke the copy-constructor of the map. For that to work, your constructor needs to take a rvalue reference to the map.

================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:154
@@ -142,1 +153,3 @@
 
+  triple.setOS(llvm::Triple::OSType::Linux);
+  arch_spec.SetTriple(triple);
----------------
How hard would it be to actually detect the os properly here?

Since you've already started processing windows minidumps, it would be great if we could have a GetArchitecture() test for both OSs.

Update: So I see you already have that test, but it does not check the OS part of the triple. If it's not too hard for some reason, let's set that as a part of this CL.

================
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()),
----------------
This is not consistent with the consumeObject function, which also drops the consumed bytes from the buffer.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:127
@@ +126,3 @@
+
+llvm::Optional<pid_t>
+MinidumpMiscInfo::GetPid(const MinidumpMiscInfo &misc_info) {
----------------
`pid_t` is a host-specific type. Use `lldb::pid_t`.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.cpp:148
@@ +147,3 @@
+  llvm::SmallVector<llvm::StringRef, 42> lines;
+  proc_status.proc_status.split(lines, '\n', 42);
+  for (auto line : lines) {
----------------
This can end up adding 43 elements to the vector. It will still work, but you'll be doubling your memory usage for no reason. If you really want it, make the vector larger, but as this is not performance critical, maybe you could just use `SmallVector<StringRef, 0>` ?

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:223
@@ +222,3 @@
+
+  static llvm::Optional<const MinidumpString>
+  Parse(llvm::ArrayRef<uint8_t> &data);
----------------
MinidumpString is just a wrapper around the std::string. Why not return the string directly? (Also the "const" there is unnecessary).

================
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;
----------------
this is oddly formatted now.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:307
@@ -288,1 +306,3 @@
+
+  static llvm::Optional<pid_t> GetPid(const MinidumpMiscInfo &misc_info);
 };
----------------
It looks like this should be a normal method instead of a static one.

================
Comment at: source/Plugins/Process/minidump/MinidumpTypes.h:318
@@ +317,3 @@
+
+  static llvm::Optional<pid_t> GetPid(const LinuxProcStatus &misc_info);
+};
----------------
It looks like this should be a normal method instead of a static one.

================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:95
@@ +94,3 @@
+  llvm::Optional<LinuxProcStatus> proc_status = parser->GetLinuxProcStatus();
+  ASSERT_TRUE(proc_status.hasValue());
+}
----------------
Also check whether the returned ProcStatus object has the contents you expect?

================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:111
@@ +110,3 @@
+  ASSERT_EQ(8UL, modules->size());
+  // TODO check for specific modules here
+}
----------------
Fix the todo.

================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:118
@@ +117,3 @@
+      parser->GetStream(MinidumpStreamType::Exception);
+  ASSERT_TRUE(data.hasValue());
+}
----------------
Check the contents of the returned object.

================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:138
@@ +137,3 @@
+  const MinidumpMiscInfo *misc_info = parser->GetMiscInfo();
+  ASSERT_TRUE(misc_info != nullptr);
+}
----------------
Check MiscInfo contents.


https://reviews.llvm.org/D24385