Page MenuHomePhabricator

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
622

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

649

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
622

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?

649

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
618

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

629

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

648

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
618

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

629

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

648

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