[PATCH] This phabricator revision is the merge of 4 patches that aim to provide resolving of AT_abstract_origin and AT_specification attributes.
David Blaikie
dblaikie at gmail.com
Fri Sep 5 12:52:02 PDT 2014
================
Comment at: lib/DebugInfo/DWARFContext.cpp:392-411
@@ -391,18 +391,22 @@
namespace {
- struct OffsetComparator {
-
- bool operator()(const std::unique_ptr<DWARFCompileUnit> &LHS,
- const std::unique_ptr<DWARFCompileUnit> &RHS) const {
- return LHS->getOffset() < RHS->getOffset();
- }
- bool operator()(const std::unique_ptr<DWARFCompileUnit> &LHS,
- uint32_t RHS) const {
- return LHS->getOffset() < RHS;
- }
- bool operator()(uint32_t LHS,
- const std::unique_ptr<DWARFCompileUnit> &RHS) const {
- return LHS < RHS->getOffset();
- }
- };
+template<typename UnitType>
+struct OffsetComparator {
+ // The beginning offset of a CU is the NextUnitOffset of the
+ // preceding one. When searching for this particular offset, the
+ // comparator has to return true so that lower_bound skips the
+ // CU. Thus the <= comparison.
+ bool operator()(const std::unique_ptr<UnitType> &LHS,
+ const std::unique_ptr<UnitType> &RHS) const {
+ return LHS->getNextUnitOffset() <= RHS->getNextUnitOffset();
+ }
+ bool operator()(const std::unique_ptr<UnitType> &LHS,
+ uint32_t RHS) const {
+ return LHS->getNextUnitOffset() <= RHS;
+ }
+ bool operator()(uint32_t LHS,
+ const std::unique_ptr<UnitType> &RHS) const {
+ return LHS <= RHS->getNextUnitOffset();
+ }
+};
}
----------------
friss wrote:
> This patch was meant to only fix the logic of the comparator, but by rebasing/reworking my patch queue, it got templatized along the way. It's not apparent in this patch series, but I have followups that introduce a similar lookup function for TypeUnits which will need the template form.
Might be nice to have that templating go where it's needed rather than here.
For what it's worth, when I do end up in the unfortunate situation of a lot of code that needs to be submitted in smaller patches (something worth avoiding when possible - especially when the incremental patches need precommit review (though it applies similarly to post-commit review) - as in that case flooding a reviewer is a bit tricky compared to having them time slice in a small review here or there as you're making progress) I tend to just copy/paste/rewrite portions of the large patch, into a separate branch, and send that for review - less chance of things getting too jumbled up. But once or twice I've managed to juggle my own incremental patch work into something reviewable.
================
Comment at: lib/DebugInfo/DWARFContext.cpp:401
@@ +400,3 @@
+ const std::unique_ptr<UnitType> &RHS) const {
+ return LHS->getNextUnitOffset() <= RHS->getNextUnitOffset();
+ }
----------------
I have a sneaking suspicion that <= ordering would violate the requirements of lower_bound...
I /suspect/ the right answer is:
Off < RHS->getOffset()
LHS->getNextUnitOffset() <= Off
LHS->getNextUnitOffset() <= RHS->getOffset()
That should be a proper "less than" for a half open range [offset, next unit offset), I think... maybe?
================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:105
@@ +104,3 @@
+ // with something more descriptive.
+ if (attr == DW_AT_specification ||
+ attr == DW_AT_abstract_origin) {
----------------
This needs some test coverage (the one test update seems like it wouldn't start failing if this code was all reverted/broken tomorrow & doesn't cover all the cases here - testing specification and abstract origin, testing cases where the name isn't available, etc)
I'm rather surprised more tests didn't need updating - we should have a bunch that test abstract_origin and specification values...
================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:110
@@ +109,3 @@
+ if (DIE.extractFast(u, &Ref)) {
+ const char *Name = DIE.getSubroutineName(u, FunctionNameKind::LinkageName);
+ if (Name)
----------------
Roll the variable declaration into the 'if' here (& then you can drop the braces on the outer "if (DIE.extractFast" if you like)
================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:319
@@ +318,3 @@
+ Offset >= U->getNextUnitOffset()) {
+ U = U->getContext().getCompileUnitForOffset(Offset);
+ assert(U && "Invalid DIE referenced.");
----------------
Is "getCompileUnitForOffset" cheap enough that we should just always call it? (& maybe pass in the context to this function, rather than the DWARFUnit, at that point)
http://reviews.llvm.org/D5208
More information about the llvm-commits
mailing list