This is an archive of the discontinued LLVM Phabricator instance.

First step towards PR21189 -- Teach llvm-readobj to dump bits of COFF symbol subsections required to debug using VS2012+
ClosedPublic

Authored by timurrrr on Oct 13 2014, 11:25 AM.

Details

Summary

Thanks to Andrey Guskov for his help investigating this!

Diff Detail

Event Timeline

timurrrr updated this revision to Diff 14808.Oct 13 2014, 11:25 AM
timurrrr retitled this revision from to First step towards PR21189 -- Teach llvm-readobj to dump bits of COFF symbol subsections required to debug using VS2012+.
timurrrr updated this object.
timurrrr edited the test plan for this revision. (Show Details)
timurrrr added reviewers: echristo, rnk.
timurrrr added a subscriber: Unknown Object (MLST).
timurrrr updated this object.Oct 14 2014, 4:05 AM
timurrrr added a reviewer: majnemer.
nrieck added a subscriber: nrieck.Oct 17 2014, 10:43 AM
nrieck added inline comments.
tools/llvm-readobj/COFFDumper.cpp
621

Minor nit. This should be DictScope because the content is not a list.

648

Ditto here. Does this subsection have any content not added yet? If not, I'd just print "ProcEnd".

timurrrr added inline comments.Oct 17 2014, 11:56 AM
tools/llvm-readobj/COFFDumper.cpp
621

Done!

Interesting, I kinda think I should actually use DictScope in a few other places of printCodeViewSymbolsSubsection()...
Can you please suggest me a good criteria of choosing ListScope vs DictScope?

648

Oh, good point!

I've actually went one step further and added an extra Size sanity check.

timurrrr updated this revision to Diff 15092.Oct 17 2014, 11:57 AM

Addressed Nico's comments

As a rule of thumb: Use DictScope for key-value pairs. Usually there's only a fixed amount of these per scope, like a single symbol info. Use ListScope if you want to delimit a list of things. Usually there's a loop printing the contents, and the amount is not fixed, like a list of symbols or sections. HTH!

timurrrr updated this revision to Diff 15341.Oct 23 2014, 12:39 PM

Added offset checks.

majnemer added inline comments.Oct 23 2014, 2:25 PM
tools/llvm-readobj/COFFDumper.cpp
617

You don't have a check to ensure that we can succeed in this getU16 call.

628

36 makes me think we didn't take into account uint32_t CodeSize = DE.getU32(&Offset);

647

Is it not valid for the DataExtractor to be at the end of the file at this point?

timurrrr added inline comments.Oct 23 2014, 2:44 PM
tools/llvm-readobj/COFFDumper.cpp
617

Per offline discussion: the check at line 620 is enough.

628

Per offline discussion: 12+4+12+4+3+1 seems to be 36

647

It isn't, see line 673.

majnemer accepted this revision.Oct 23 2014, 2:45 PM
majnemer edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 23 2014, 2:45 PM
timurrrr accepted this revision.Oct 23 2014, 3:36 PM
timurrrr added a reviewer: timurrrr.

r220526

timurrrr closed this revision.Oct 23 2014, 3:36 PM