[Lldb-commits] [PATCH] D60268: Breakpad: Parse Stack CFI records

[Pavel Labath via Phabricator via lldb-commits]

labath marked 3 inline comments as done.
labath added inline comments.


================
Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:173
+  llvm::DenseMap<llvm::StringRef, llvm::StringRef> UnwindRules;
+};
+
----------------
amccarth wrote:
> I'm not a fan of deep class hierarchies, but given that StackCFIRecord is a subset of StackCFIInitRecord and given that the naming suggests StackCFIInitRecord is a concrete type of StackCFIRecord, should StackCFIInitRecord derive from StackCFIRecord?  (Or perhaps contain one?)
> 
> If not, perhaps a comment to explain why not.
Good question. I was doing the same thing as I did with FUNC and PUBLIC records, which are also very similar, but I wouln't be comfortable saying one is a special case of the other. However, the case here is not as clear, as one could consider the "init" record to be a special kind of a "stack cfi" record.  OTOH, I also don't like deep hierarchies and particularly having a concrete class (init record) inherit from another concrete class (stack cfi record).

Since this code is likely to have only a single consumer (the thing which converts these into UnwindPlans), adding an AbstractCFIRecord class would seem like an overkill. Instead, how about we just collapse things even more and use just a single class to represent both records (with an Optional<addr_t> for the size)?

I don't think this should make it the job of the consumer much harder, but it will allow us to:
- avoid needing special code to group these two records together
- give us an excuse to call this group (section) "STACK CFI". (I have previously called this group "STACK CFI INIT" for consistency with how the FUNC+LINE groups is handled, but that didn't feel quite right here).
- speed up parsing as we don't have to parse the "STACK CFI" part twice just to find out we're not parsing the correct record type.


================
Comment at: unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp:123
+                            "STACK CFI 47 .cfa: $esp 4 + $eip: .cfa 4 - ^"));
+}
+
----------------
amccarth wrote:
> All of the negative parsing tests seem to be incomplete (e.g., truncated or a missing `INIT`).  Should there be others, like having the wrong token type?  Or an invalid token?  Or one that has extra tokens at the end of an otherwise valid record?
> 
> I see you're following the pattern for the other types here, but is that enough?
Following the pattern isn't much of an excuse for me, as I was the one who established that pattern. :)

The reason the pattern is like it is was that I was trying to cover all of the paths through the parsing code. I believe I have succeeded at that, but I haven't used a profiler or anything to verify that. Knowing the implementation, it's obvious that a test with an invalid record type is not going to be interesting, as we will bail out straight after the first token. However, I agree that from a black-box testing perspective, these are interesting cases to check, so I've added a couple more that I could think of.

This is going to be the most extensively tested piece of code in lldb :P.


================
Comment at: unittests/ObjectFile/Breakpad/BreakpadRecordsTest.cpp:132
+            StackCFIRecord::parse("STACK CFI INIT 47 8 .cfa: $esp 4 +"));
+}
----------------
amccarth wrote:
> This test relies on the parsing test for the StackCFIInitRecord having covered most of the cases.  That works because the parsing implementation is shared, so I'm OK with it.  Will others be concerned that this test isn't as independent as it could be?
Another advantage of merging the two record types is that it makes this comment void. :)


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

https://reviews.llvm.org/D60268