Page MenuHomePhabricator

Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format
ClosedPublic

Authored by clayborg on Thu, Aug 22, 9:38 AM.

Diff Detail

Event Timeline

clayborg created this revision.Thu, Aug 22, 9:38 AM
clayborg retitled this revision from Add encode and decode methods to InlineInfo and document encoding format. to Add encode and decode methods to InlineInfo and document encoding format to the GSYM file format.
aprantl added inline comments.Thu, Aug 22, 10:21 AM
lib/DebugInfo/GSYM/InlineInfo.cpp
63

This returns a bool.. what does that mean?

84

and here the return value is ignored?

clayborg marked 2 inline comments as done.Thu, Aug 22, 10:44 AM
clayborg added inline comments.
lib/DebugInfo/GSYM/InlineInfo.cpp
63

returns true if a InlineInfo is successfully decoded. This is a static function that is called recursively from within this function. The while statement on line 74 uses the return value.

84

I will clear the InlineInfo if this returns false.

clayborg updated this revision to Diff 216668.Thu, Aug 22, 10:48 AM
  • document arguments to "static bool decodeAll(...)" in InlineInfo.cpp
  • If decodeAll() returns false, clear the InlineInfo
aprantl added inline comments.Thu, Aug 22, 11:00 AM
lib/DebugInfo/GSYM/InlineInfo.cpp
101

This also seems weird. Why wouldn't decode() guarantee that it leaves the object in a consistent state? Why silently swallow the error?

Perhaps it would be cleaner if decode returned an expected<InlineInfo> object? Then you don't need a clear() method in the first place.

clayborg marked an inline comment as done.Thu, Aug 22, 2:27 PM
clayborg added inline comments.
lib/DebugInfo/GSYM/InlineInfo.cpp
98–101

We could leave it out as it was before. The decodeAll just returning a bool was for the recursion. It would actually be nice to have this fail with the data it was able to decode. Not sure what error we can really return besides "not enough data"?

clayborg updated this revision to Diff 216725.Thu, Aug 22, 3:26 PM

Don't clear the inline info when decodeAll returns false so that we can see what was actually decoded. Not sure how great of an error we can return other than "not enough data". I am open to suggestions?

MaskRay added inline comments.Sun, Aug 25, 11:50 PM
lib/DebugInfo/GSYM/InlineInfo.cpp
103

Doesn't it need a few bytes to represent an empty Ranges? Or is it just unexpected to call encode on an empty Ranges (in this case an assert or llvm_unreachable might be better)

clayborg marked an inline comment as done.Mon, Aug 26, 11:26 AM
clayborg added inline comments.
lib/DebugInfo/GSYM/InlineInfo.cpp
102–103

I think you are correct in that we should avoid calling this function in this case and have the user verify the data is good since this data will eventually be in a blob in the GSYM file that won't make sense to encode it at all of the data is not valid as it will just waste file space.

clayborg updated this revision to Diff 217217.Mon, Aug 26, 11:37 AM
  • use llvm_unreachable when encoding InlineInfo objects. We don't want them emitted into the GSYM file and taking up space if they aren't valid.
  • improve comment about terminating child InlineInfo object siblings.
JDevlieghere added inline comments.Tue, Aug 27, 10:45 AM
lib/DebugInfo/GSYM/InlineInfo.cpp
63

I think Adrian means that an llvm:Error might be a better fit. That way the error can propagate up.

86

This could be an early return.

113

This could be an early return.

clayborg updated this revision to Diff 218136.Fri, Aug 30, 11:27 AM

Added error handling to InlineInfo::encode and InlineInfo::decode with full testing.

For encoding return an error when we try to encode an invalid InlineInfo and when a child InlineInfo has an address range that isn't contained in the parent's address range. This will help detect clients that use bad debug info data to create GSYM InlineInfo objects ensure that they correctly construct valid InlineInfo hierarchies. Added the ability to check if an address range is contained within an AddressRanges object to support this feature.

For decoding detect when data is missing from the steam and report an error with the exact offset with an appropriate error message.

aprantl accepted this revision.Tue, Sep 3, 8:57 AM

Thanks for implementing the error handling! There is one more comment inline.

lib/DebugInfo/GSYM/InlineInfo.cpp
127

What's the advantage of having invalid InlineInfo objects? Could we just have whoever creates them return a a valid object or an error otherwise?

This revision is now accepted and ready to land.Tue, Sep 3, 8:57 AM
MaskRay added inline comments.Tue, Sep 3, 9:25 AM
lib/DebugInfo/GSYM/Range.cpp
53

Did you intend to use It[-1].End twice?

I am not certain what this function intends to do, but
should it be return Range.End <= It[-1].End;?

MaskRay added inline comments.Tue, Sep 3, 9:28 AM
lib/DebugInfo/GSYM/InlineInfo.cpp
149
if (llvm::Error E = ...)
  return E

(Avoid Error which is also a type name)

clayborg marked 3 inline comments as done.Wed, Sep 4, 10:29 AM
clayborg added inline comments.
lib/DebugInfo/GSYM/InlineInfo.cpp
127

InlineInfo objects will be created by converting debug info into these objects. So they start out default constructed and then get populated as we parse the attributes. Clients should ensure these objects are valid prior to adding them to a GSYM file or into another InlineInfo object.

$ svn commit
Sending include/llvm/DebugInfo/GSYM/InlineInfo.h
Sending include/llvm/DebugInfo/GSYM/Range.h
Sending lib/DebugInfo/GSYM/InlineInfo.cpp
Sending lib/DebugInfo/GSYM/Range.cpp
Sending unittests/DebugInfo/GSYM/GSYMTest.cpp
Transmitting file data .....done
Committing transaction...
Committed revision 370936.

clayborg closed this revision.Wed, Sep 4, 10:33 AM
MaskRay added inline comments.Wed, Sep 4, 10:55 PM
lib/DebugInfo/GSYM/Range.cpp
46

Do you know why it returns false if Range.size() == 0?

If I remove Range.size() == 0, the following test will break

EXPECT_FALSE(Ranges.contains(AddressRange(0x1000, 0x1000)));
clayborg marked an inline comment as done.Fri, Sep 6, 7:03 AM
clayborg added inline comments.
lib/DebugInfo/GSYM/Range.cpp
46

This is looking forward to the time when we convert debug info into GSYM. Many compilers will make the low PC and high PC the same for debug info that has been optimized out. Also, an address range with no size is not a valid address range, so it can't be contained inside of another.