[PATCH] D58044: [DwarfDebug] Dump call site debug info into DWARF

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 2 inline comments as done.
djtodoro added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1214
+      addDIEEntry(CSDie, dwarf::DW_AT_abstract_origin, *CalledDIE);
+    // TODO: DW_AT_call_origin is DWARF5 replacement of abstract_origin
+  } else {
----------------
probinson wrote:
> vsk wrote:
> > probinson wrote:
> > > vsk wrote:
> > > > Any chance to share code with DwarfCompileUnit::constructCallSiteEntryDIE?
> > > I don't understand this comment.  DW_AT_call_origin is for the call-site stuff, DW_AT_abstract_origin is for inlined instances.  call_origin is not a "replacement" for abstract_origin.
> > Sorry, didn't mean to conflate abstract_origin vs. call_origin here. What I'm getting at is that DwarfCompileUnit::constructCallSiteDIE and the pre-existing constructCallSiteEntryDIE both do some work at each call site. I'm just wondering if it's feasible to have one entry point for that sort of work.
> Hi Vedant, actually I was asking about the TODO comment in the source, not commenting on your comment.
>Any chance to share code with DwarfCompileUnit::constructCallSiteEntryDIE?

@vsk Thanks for your comment!

All of this, for now, works under an experimental option. We generate GNU DWARF extensions, and we plan to add also DWARF 5 support as well.
There is still discussion on RFC how we should proceed with the final implementation.

We saw that you implemented a portion of dumping DWARF 5 symbols for call sites, but in order to support debug call site parameters, we would need to introduce new meta instructions etc. (as you can see in other patches) and place the production of that info somewhere earlier in backend pipeline.  Also, there will be support for indirect calls through the meta instruction that holds the register.

So, the point is that we will include the part of this in 'DwarfCompileUnit::constructCallSiteEntryDIE' , but also in order to avoid redundant functionality, we will need to change 'DwarfDebug::constructCallSiteEntryDIEs’.

Let me know what you think. :)


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1214
+      addDIEEntry(CSDie, dwarf::DW_AT_abstract_origin, *CalledDIE);
+    // TODO: DW_AT_call_origin is DWARF5 replacement of abstract_origin
+  } else {
----------------
djtodoro wrote:
> probinson wrote:
> > vsk wrote:
> > > probinson wrote:
> > > > vsk wrote:
> > > > > Any chance to share code with DwarfCompileUnit::constructCallSiteEntryDIE?
> > > > I don't understand this comment.  DW_AT_call_origin is for the call-site stuff, DW_AT_abstract_origin is for inlined instances.  call_origin is not a "replacement" for abstract_origin.
> > > Sorry, didn't mean to conflate abstract_origin vs. call_origin here. What I'm getting at is that DwarfCompileUnit::constructCallSiteDIE and the pre-existing constructCallSiteEntryDIE both do some work at each call site. I'm just wondering if it's feasible to have one entry point for that sort of work.
> > Hi Vedant, actually I was asking about the TODO comment in the source, not commenting on your comment.
> >Any chance to share code with DwarfCompileUnit::constructCallSiteEntryDIE?
> 
> @vsk Thanks for your comment!
> 
> All of this, for now, works under an experimental option. We generate GNU DWARF extensions, and we plan to add also DWARF 5 support as well.
> There is still discussion on RFC how we should proceed with the final implementation.
> 
> We saw that you implemented a portion of dumping DWARF 5 symbols for call sites, but in order to support debug call site parameters, we would need to introduce new meta instructions etc. (as you can see in other patches) and place the production of that info somewhere earlier in backend pipeline.  Also, there will be support for indirect calls through the meta instruction that holds the register.
> 
> So, the point is that we will include the part of this in 'DwarfCompileUnit::constructCallSiteEntryDIE' , but also in order to avoid redundant functionality, we will need to change 'DwarfDebug::constructCallSiteEntryDIEs’.
> 
> Let me know what you think. :)
>I don't understand this comment. DW_AT_call_origin is for the call-site stuff, DW_AT_abstract_origin is for inlined instances. call_origin is >not a "replacement" for abstract_origin.

@probinson Thanks for your comment!

As I wrote in the previous comment, we use GNU DWARF extensions for describing debug info about call sites and call site parameters in DWARF sections (for now).

Since 'call_origin' is used in DWARF 5 proposal, we leave it on the side, but we plan to implement that as well.

For DWARF 4 support, even in GCC, there were no GNU extensions for every symbol needed for the feature. For example, instead of 'call_pc' for marking address after a call instruction 'low_pc' DWARF attribute was used for that. Also, instead of 'call_origin', that is actually reference of called subprogram of direct calls, 'abstract_origin' was used. In addition, GNU GDB debugger is aware how to consume it in right way.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58044/new/

https://reviews.llvm.org/D58044