[PATCH] D24014: [codeview] Add new directives to record inlined call site line info

[Reid Kleckner via llvm-commits]

rnk marked an inline comment as done.
rnk added a comment.

In https://reviews.llvm.org/D24014#529327, @amccarth wrote:

> 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!


================
Comment at: include/llvm/MC/MCCodeView.h:112
@@ +111,3 @@
+  /// function, or this is an inlined call site, and this is the parent function
+  /// id plus one.
+  unsigned ParentFuncIdPlusOne = 0;
----------------
amccarth wrote:
> 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?
Hopefully answered in the form of a comment

================
Comment at: include/llvm/MC/MCCodeView.h:162
@@ +161,3 @@
+                               unsigned IAFile, unsigned IALine,
+                               unsigned IACol);
+
----------------
amccarth wrote:
> 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?
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.


https://reviews.llvm.org/D24014