[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:23
 #include <map>
+#include <unordered_set>
+#include <vector>
----------------
amccarth wrote:
> Why add `<unordered_set>`?  It looks like your new map is just a vector.
Good catch


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:558
+  return result;
+}
----------------
amccarth wrote:
> This is a long function (which, unfortunately, is not unusual in LLVM/LLDB), but the comments here are really good, so I'm OK with it.
I agree size-wise it's borderline. It's also a bit smelly in that it does two things (checks + initialization).

I originally had a separate "ConsistencyCheck", but there was significant duplication (the directory traversal) and the combined version won by a tiny margin :) If this grows with more checks I'd be happy to revisit and refactor it. 



================
Comment at: source/Plugins/Process/minidump/MinidumpParser.h:93
+  // Perform consistency checks and initialize internal data structures
+  Status Initialize();
+
----------------
amccarth wrote:
> I'm not a big fan of 2-step initialization, but that seems to be the way of LLDB. :-(
Agreed. I don't see how to avoid this w/o much bigger changes though (we don't have a chance to return meaningful errors during process creation).


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:172
+
+  // Do we support the minidump's architecture?
+  ArchSpec arch = GetArchitecture();
----------------
amccarth wrote:
> Should the architecture check be in the MinidumpParser::Initialize with the other checks?
> 
> I don't know the answer; I'm just asking for your thinking about this.
Good question, here's my take: the checks are for consistency and a minidump with a currently unsupported architecture is a valid minidump. 

So I think it's better to have this check external since the architecture support is not a minidump parser concern. WDYT?


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:193
   if (!pid) {
-    error.SetErrorString("failed to parse PID");
+    error.SetErrorString("Failed to parse PID");
     return error;
----------------
amccarth wrote:
> I realize nothing is perfectly consistent, but I think LLVM tends to start error strings with a lowercase letter (except for proper nouns).  Can you revert this case change and make sure your new error strings follow that pattern?
Sigh, sure. I was hoping it's the other way around.


================
Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52
     ASSERT_GT(parser->GetData().size(), 0UL);
+    auto result = parser->Initialize();
+    if (expect_failure)
----------------
amccarth wrote:
> I would rather see this function return the result of the Initialize and let the individual tests check the expectation explicitly.
> 
> I know that will lead to a little bit of duplication in the tests, but it will make the individual tests easier to understand in isolation.  It also makes it possible for each test to decide whether to ASSERT or EXPECT.  And it eliminates the need for the bool parameter which is hard to decipher at the call sites.
Good idea, although gunit doesn't let us ASSERT in non-void returning functions.

I agree that the bool parameter is ugly, so I created a separate TruncateMinidump() helper (which cleans up the SetUpData since the load_size was only used for truncation)


https://reviews.llvm.org/D49202