llvm dev - Mar 2018 - [RFC] Generate Debug Information for Labels in Function

> On Mar 30, 2018, at 9:25 AM, Adrian Prantl via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> 
> 
>> On Mar 29, 2018, at 11:29 PM, Hsiangkai Wang <hsiangkai at
gmail.com> wrote:
>> 
>> I agree with you. Attach debug metadata to basic block will be a
>> better solution. I will change my design to convey debug metadata
>> through basic block instead of intrinsic.
>> 
>> https://reviews.llvm.org/D45078
> 
> In this revised design it is now possible to attach a DILabel to a
BasicBlock. When the basic block is inlined it will be ambiguous to which
function the DILabel belongs. For instructions, we store the inline information
in the inlinedAt: field of its DILocation. In order to handle inlining for
DILabels we have two options:
> 
>  1. Also attach a DILocation to be associated with the label to carry the
inline information, and teach the inliner to correctly update the DILocation on
basic blocks during inlining. This would also solve the issue of hypothetical
scoped labels that Paul brought up. We'll also need to figure out what to do
when two labels are being merged by a transformation.
> 
>  2. Teach the inliner to drop all metadata attachments on basic blocks.
> 
> Option (2) is obviously going to be easier to implement and might be a good
as a first step.
I'm really sorry for not realizing this yesterday, but the problems
pertaining to inlining made me realize that your original design with the
dbg.label intrinsic might actually be a better approach especially when
considering optimized code. We will get inlining support for free because it is
just another instruction and it can deal with more than one label at the same
address. It looks a bit more complicated in unoptimized code, but that seems
like a small price to pay.

We just need to make sure that the backend doesn't get confused when loop
unrolling duplicates a dbg.label but that should be doable.

-- adrian
> 
>> 
>>> That said, perhaps this isn't even necessary. The only
information that is stored in DILabel is the name of the label (which is
redundant with the actual name of the label) and its source location, which is
also stored in the DILocation (!11). I'm wondering if the DILocation of a
label is even useful. When a debugger user sets a breakpoint of a label, we
might as well use the location of the first instruction in the basic block
described by the label, since that is where execution will continue.
>>> 
>>> Based on that I think it might be sufficient to have a flag on an
IR label that marks a user-originated label and triggers the backend to create a
DW_TAG_label for it. If we do need source location information for the
DW_TAG_label, we could grab it from the first instruction.
>> I still think that we should collect debug information from source
>> code level instead of infer from instructions in the basic block. As
>> Paul said, "the top instructions in a block do not necessarily
have a
>> valid source location." So, I will keep DILabel metadata and
remove
>> llvm.dbg.label intrinsic.
> 
> I'm still not convinced that this information will be useful to a
debugger, but if you have a compelling use-case please let me know.
> 
> -- adrian
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On Fri, Mar 30, 2018 at 9:39 AM Adrian Prantl <aprantl at apple.com>
wrote:
> I'm really sorry for not realizing this yesterday, but the problems
> pertaining to inlining made me realize that your original design with the
> dbg.label intrinsic might actually be a better approach especially when
> considering optimized code. We will get inlining support for free because
> it is just another instruction and it can deal with more than one label at
> the same address. It looks a bit more complicated in unoptimized code, but
> that seems like a small price to pay.
>
> We just need to make sure that the backend doesn't get confused when
loop
> unrolling duplicates a dbg.label but that should be doable.
>
+10, it should be an intrinsic.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180330/b9251ab4/attachment.html>

Hi all,

I am sorry that I didn’t carefully think about how to handle labels in
inlined function.

Today, I did some simple experiments and found that some basic block
may be removed in CFGSimplifyPass and the attached metadata will be
eliminated. So, if the optimization is turned on, label metadata will
disappear. However, intrinsic will keep existing in the function if we
handle it correctly. So, if we take care about optimization and
inlining, to keep label metadata through intrinsic could be right for
free.

I learned a lot from discussions. Thanks for your comments and
suggestions. If there is no other concern, I will keep my original
implementation and send patches to Phabricator again. Sorry have
bothered you.

Any comments?

On Sat, Mar 31, 2018 at 5:38 AM, Reid Kleckner <rnk at google.com>
wrote:
> On Fri, Mar 30, 2018 at 9:39 AM Adrian Prantl <aprantl at apple.com>
wrote:
>>
>> I'm really sorry for not realizing this yesterday, but the problems
>> pertaining to inlining made me realize that your original design with
the
>> dbg.label intrinsic might actually be a better approach especially when
>> considering optimized code. We will get inlining support for free
because it
>> is just another instruction and it can deal with more than one label at
the
>> same address. It looks a bit more complicated in unoptimized code, but
that
>> seems like a small price to pay.
>>
>> We just need to make sure that the backend doesn't get confused
when loop
>> unrolling duplicates a dbg.label but that should be doable.
>
>
> +10, it should be an intrinsic.

As far as I can see we now get DW_TAG_label on trunk. Nice!

I also see that there has been some talk about loop unrolling etc. when
discussing this RFC. Was there a consensus on what should happen if we
end up with multiple dbg.label intrinsics for the same label due to
code duplication?

Here is an example where we get two "#DEBUG_LABEL: main:foo" but only
one
DW_TAG_label for "foo": https://godbolt.org/z/oTpHQp

Maybe that is as expected, or is it allowed to have multiple DW_TAG_label
for the same label name?

/Björn
> -----Original Message-----
> From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of
Adrian
> Prantl via llvm-dev
> Sent: den 30 mars 2018 18:40
> To: Adrian Prantl <aprantl at apple.com>
> Cc: llvm-dev <llvm-dev at lists.llvm.org>
> Subject: Re: [llvm-dev] [RFC] Generate Debug Information for Labels in
> Function
> 
> 
> 
> > On Mar 30, 2018, at 9:25 AM, Adrian Prantl via llvm-dev <llvm-
> dev at lists.llvm.org> wrote:
> >
> >
> >
> >> On Mar 29, 2018, at 11:29 PM, Hsiangkai Wang <hsiangkai at
gmail.com>
> wrote:
> >>
> >> I agree with you. Attach debug metadata to basic block will be a
> >> better solution. I will change my design to convey debug metadata
> >> through basic block instead of intrinsic.
> >>
> >> https://reviews.llvm.org/D45078
> >
> > In this revised design it is now possible to attach a DILabel to a
> BasicBlock. When the basic block is inlined it will be ambiguous to
> which function the DILabel belongs. For instructions, we store the
> inline information in the inlinedAt: field of its DILocation. In order
> to handle inlining for DILabels we have two options:
> >
> >  1. Also attach a DILocation to be associated with the label to carry
> the inline information, and teach the inliner to correctly update the
> DILocation on basic blocks during inlining. This would also solve the
> issue of hypothetical scoped labels that Paul brought up. We'll also
> need to figure out what to do when two labels are being merged by a
> transformation.
> >
> >  2. Teach the inliner to drop all metadata attachments on basic
> blocks.
> >
> > Option (2) is obviously going to be easier to implement and might be
> a good as a first step.
> 
> I'm really sorry for not realizing this yesterday, but the problems
> pertaining to inlining made me realize that your original design with
> the dbg.label intrinsic might actually be a better approach especially
> when considering optimized code. We will get inlining support for free
> because it is just another instruction and it can deal with more than
> one label at the same address. It looks a bit more complicated in
> unoptimized code, but that seems like a small price to pay.
> 
> We just need to make sure that the backend doesn't get confused when
> loop unrolling duplicates a dbg.label but that should be doable.
> 
> -- adrian
> 
> >
> >>
> >>> That said, perhaps this isn't even necessary. The only
information
> that is stored in DILabel is the name of the label (which is redundant
> with the actual name of the label) and its source location, which is
> also stored in the DILocation (!11). I'm wondering if the DILocation of
> a label is even useful. When a debugger user sets a breakpoint of a
> label, we might as well use the location of the first instruction in
> the basic block described by the label, since that is where execution
> will continue.
> >>>
> >>> Based on that I think it might be sufficient to have a flag on
an
> IR label that marks a user-originated label and triggers the backend to
> create a DW_TAG_label for it. If we do need source location information
> for the DW_TAG_label, we could grab it from the first instruction.
> >> I still think that we should collect debug information from source
> >> code level instead of infer from instructions in the basic block.
As
> >> Paul said, "the top instructions in a block do not
necessarily have
> a
> >> valid source location." So, I will keep DILabel metadata and
remove
> >> llvm.dbg.label intrinsic.
> >
> > I'm still not convinced that this information will be useful to a
> debugger, but if you have a compelling use-case please let me know.
> >
> > -- adrian
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

What does GCC do in situations like this?

I suspect the rightiest thing to do is probably multiple DW_TAG_labels with
the same name, if that's what's happened.

I guess another question/possibility - what happens if some inline asm
references the label? I guess it probably constrains the optimizer not to
unroll? Maybe? If the label can be duplicated and inline asm can still
correctly reference the label, maybe that's the answer - describe the label
the same way inline asm "sees" it.

On Wed, Jan 30, 2019 at 6:04 AM Björn Pettersson A via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> As far as I can see we now get DW_TAG_label on trunk. Nice!
>
> I also see that there has been some talk about loop unrolling etc. when
> discussing this RFC. Was there a consensus on what should happen if we
> end up with multiple dbg.label intrinsics for the same label due to
> code duplication?
>
> Here is an example where we get two "#DEBUG_LABEL: main:foo" but
only one
> DW_TAG_label for "foo": https://godbolt.org/z/oTpHQp
>
> Maybe that is as expected, or is it allowed to have multiple DW_TAG_label
> for the same label name?
>
> /Björn
>
> > -----Original Message-----
> > From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of
Adrian
> > Prantl via llvm-dev
> > Sent: den 30 mars 2018 18:40
> > To: Adrian Prantl <aprantl at apple.com>
> > Cc: llvm-dev <llvm-dev at lists.llvm.org>
> > Subject: Re: [llvm-dev] [RFC] Generate Debug Information for Labels in
> > Function
> >
> >
> >
> > > On Mar 30, 2018, at 9:25 AM, Adrian Prantl via llvm-dev <llvm-
> > dev at lists.llvm.org> wrote:
> > >
> > >
> > >
> > >> On Mar 29, 2018, at 11:29 PM, Hsiangkai Wang <hsiangkai at
gmail.com>
> > wrote:
> > >>
> > >> I agree with you. Attach debug metadata to basic block will
be a
> > >> better solution. I will change my design to convey debug
metadata
> > >> through basic block instead of intrinsic.
> > >>
> > >> https://reviews.llvm.org/D45078
> > >
> > > In this revised design it is now possible to attach a DILabel to
a
> > BasicBlock. When the basic block is inlined it will be ambiguous to
> > which function the DILabel belongs. For instructions, we store the
> > inline information in the inlinedAt: field of its DILocation. In order
> > to handle inlining for DILabels we have two options:
> > >
> > >  1. Also attach a DILocation to be associated with the label to
carry
> > the inline information, and teach the inliner to correctly update the
> > DILocation on basic blocks during inlining. This would also solve the
> > issue of hypothetical scoped labels that Paul brought up. We'll
also
> > need to figure out what to do when two labels are being merged by a
> > transformation.
> > >
> > >  2. Teach the inliner to drop all metadata attachments on basic
> > blocks.
> > >
> > > Option (2) is obviously going to be easier to implement and might
be
> > a good as a first step.
> >
> > I'm really sorry for not realizing this yesterday, but the
problems
> > pertaining to inlining made me realize that your original design with
> > the dbg.label intrinsic might actually be a better approach especially
> > when considering optimized code. We will get inlining support for free
> > because it is just another instruction and it can deal with more than
> > one label at the same address. It looks a bit more complicated in
> > unoptimized code, but that seems like a small price to pay.
> >
> > We just need to make sure that the backend doesn't get confused
when
> > loop unrolling duplicates a dbg.label but that should be doable.
> >
> > -- adrian
> >
> > >
> > >>
> > >>> That said, perhaps this isn't even necessary. The
only information
> > that is stored in DILabel is the name of the label (which is redundant
> > with the actual name of the label) and its source location, which is
> > also stored in the DILocation (!11). I'm wondering if the
DILocation of
> > a label is even useful. When a debugger user sets a breakpoint of a
> > label, we might as well use the location of the first instruction in
> > the basic block described by the label, since that is where execution
> > will continue.
> > >>>
> > >>> Based on that I think it might be sufficient to have a
flag on an
> > IR label that marks a user-originated label and triggers the backend
to
> > create a DW_TAG_label for it. If we do need source location
information
> > for the DW_TAG_label, we could grab it from the first instruction.
> > >> I still think that we should collect debug information from
source
> > >> code level instead of infer from instructions in the basic
block. As
> > >> Paul said, "the top instructions in a block do not
necessarily have
> > a
> > >> valid source location." So, I will keep DILabel metadata
and remove
> > >> llvm.dbg.label intrinsic.
> > >
> > > I'm still not convinced that this information will be useful
to a
> > debugger, but if you have a compelling use-case please let me know.
> > >
> > > -- adrian
> > > _______________________________________________
> > > LLVM Developers mailing list
> > > llvm-dev at lists.llvm.org
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190130/2cf60540/attachment.html>