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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
