This is an archive of the discontinued LLVM Phabricator instance.

[codeview] Add new directives to record inlined call site line info
ClosedPublic

Authored by rnk on Aug 29 2016, 4:42 PM.

Details

Summary

Previously we were trying to represent this with the "contains" list of
the .cv_inline_linetable directive, which was not enough information.
Now we directly represent the chain of inlined call sites, so we know
what location to emit when we encounter a .cv_loc directive of an inner
inlined call site while emitting the line table of an outer function or
inlined call site. Fixes PR29146.

Also fixes PR29147, where we would crash when .cv_loc directives crossed
sections. Now we write down the section of the first .cv_loc directive,
and emit an error if any other .cv_loc directive for that function is in
a different section.

Also fixes issues with discontiguous inlined source locations, like in
this example:

volatile int unlikely_cond = 0;
extern void __declspec(noreturn) abort();
__forceinline void f() {
  if (!unlikely_cond) abort();
}
int main() {
  unlikely_cond = 0;
  f();
  unlikely_cond = 0;
}

Previously our tables gave bad location information for the 'abort'
call, and the debugger wouldn't snow the inlined stack frame for 'f'.
It is important to emit good line tables for this code pattern, because
it comes up whenever an asan bug occurs in an inlined function. The
__asan_report* stubs are generally placed after the normal function
epilogue, leading to discontiguous regions of inlined code.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 69636.Aug 29 2016, 4:42 PM
rnk retitled this revision from to [codeview] Add new directives to record inlined call site line info.
rnk updated this object.
rnk added reviewers: majnemer, amccarth.
rnk added a subscriber: llvm-commits.
amccarth edited edge metadata.Aug 30 2016, 11:55 AM

I'm curious if you tried the example in your cv-inline-linetable-unlikely test to see how the VS debugger does with the improved inline information.

Overall, this looks good.

include/llvm/MC/MCCodeView.h
112 ↗(On Diff #69636)

This comment is a bit muddled and hard to understand. How about starting with the typical case:

/// ParentFuncIdPlusOne is the parent function ID plus one.  If this is a top-level function and
/// there is no parent, then ParentFuncIdPlusOne is FunctionSentinel.

I'm still not sure what zero means. What specifically is unallocated?

133 ↗(On Diff #69636)

It would be nice to put this closer to ParentFuncIdPlusOne.

162 ↗(On Diff #69636)

What does the IA- prefix mean in this context?

I'm concerned about a method with five parameters of the same type--seems very easy to call with arguments in the wrong order.

Is the return a simple true-mean-success/false-means-failure?

rnk marked an inline comment as done.Aug 31 2016, 10:18 AM

I'm curious if you tried the example in your cv-inline-linetable-unlikely test to see how the VS debugger does with the improved inline information.

It works in windbg, but VS doesn't seem to support inline line table information. =/

Overall, this looks good.

Thanks for the review!

include/llvm/MC/MCCodeView.h
112 ↗(On Diff #69636)

Hopefully answered in the form of a comment

162 ↗(On Diff #69636)

It's supposed to mean "inlined at", since it's trying to mirror the concept of DILocation::getInlinedAt(). I beefed up the comments here, hopefully it makes more sense now.

As for the parameter order confusion, I'm not too worried about argument reordering bugs because it directly corresponds to the argument order of the .cv_inline_site_id directive, which has prepositions to make it more readable:

.cv_inline_site_id <funcid> within <parent_funcid> inlined_at <iafile> <ialine> <iacol>

Triples of file, line, col are also pretty common in other directives.

rnk updated this revision to Diff 69875.Aug 31 2016, 10:20 AM
rnk edited edge metadata.
  • Improve comments and fix bug in MCCVFunctionInfo
amccarth accepted this revision.Aug 31 2016, 10:39 AM
amccarth edited edge metadata.

LGTM, but you might want to wait for David's feedback.

This revision is now accepted and ready to land.Aug 31 2016, 10:39 AM
majnemer added inline comments.Aug 31 2016, 10:51 AM
lib/MC/MCParser/AsmParser.cpp
3257–3258 ↗(On Diff #69875)

Should we also check that it is < UNSIGNED_MAX ?

3288 ↗(On Diff #69875)

Ditto.

3311 ↗(On Diff #69875)

Dow we want to validate using isValidFileNumber?

rnk added a comment.Aug 31 2016, 5:18 PM

David, any thoughts on the directive naming? I want to make sure the bikeshed is the right color before I let the paint dry. :)

rnk marked 2 inline comments as done.Sep 6 2016, 10:50 AM
rnk added inline comments.
lib/MC/MCParser/AsmParser.cpp
3311 ↗(On Diff #69875)

Yep, done.

3311 ↗(On Diff #69875)

I added a bunch of diagnostics tests for this in cv-errors.s. It's not as good as clang -verify, but such is life.

majnemer accepted this revision.Sep 6 2016, 10:51 AM
majnemer edited edge metadata.

LGTM

rnk updated this revision to Diff 70431.Sep 6 2016, 10:51 AM
rnk edited edge metadata.
  • Beef up asm parsing diagnostics
This revision was automatically updated to reflect the committed changes.