[PATCH] Make CloneFunction also clone debug info metadata

Thu Mar 6 00:41:20 PST 2014

Rebased patch attached (including latest movement of headers into IR/
folder)

On 25 February 2014 02:49, Eric Christopher <echristo at gmail.com> wrote:

> Hi Alon,
>
> Sorry for the delay, can post/repost a rebased patch please?
>
> -eric
>
> On Sun, Feb 16, 2014 at 1:22 AM, Alon Mishne <alonmishne at gmail.com> wrote:
> > Thanks David! Meanwhile I got my own commit access so next time it
> should be
> > simpler.
> >
> > Any thoughts about patch (3)?
> >
> >
> > On 4 February 2014 02:04, David Blaikie <dblaikie at gmail.com> wrote:
> >>
> >>
> >>
> >>
> >> On Sat, Feb 1, 2014 at 11:40 PM, Alon Mishne <alonmishne at gmail.com>
> wrote:
> >>>
> >>> As I have commit permissions to neither LLVM nor Clang, will appreciate
> >>> if you could commit for me :-)
> >>
> >>
> >> Committed as 200721 and 200722. Thanks! (apologies - but I forgot to
> >> credit you in those commits)
> >>
> >>>
> >>> Regarding (3) - is there anything else you think should be changed?
> >>
> >>
> >> I've still not given them enough of a look to say yes/no - I might, if
> >> someone else doesn't get to it first.
> >>
> >>>
> >>>
> >>> Eric - I don't see where I've indented a namespace...
> >>>
> >>>
> >>> On 30 January 2014 21:02, Eric Christopher <echristo at gmail.com> wrote:
> >>>>
> >>>> Agreed, 1 & 2 look fine. 3 will need a bit of a deeper look. One nit:
> >>>> don't indent the namespace :)
> >>>>
> >>>> -eric
> >>>>
> >>>> On Thu, Jan 30, 2014 at 9:31 AM, David Blaikie <dblaikie at gmail.com>
> >>>> wrote:
> >>>> > Patches 1 & 2 look fine - can you commit them, or do you need me to?
> >>>> >
> >>>> > (I'm rather surprised only one test had trouble with this (though I
> >>>> > expect
> >>>> > there's a bunch of LLVM tests that all have these {i32 0} in them
> that
> >>>> > I'll
> >>>> > clean up when I remove the bits of code that silently handle this
> >>>> > case) -
> >>>> > but awesome... shows how cargo culty some things can be I guess)
> >>>> >
> >>>> >
> >>>> > On Thu, Jan 30, 2014 at 1:58 AM, Alon Mishne <alonmishne at gmail.com>
> >>>> > wrote:
> >>>> >>
> >>>> >> OK, so I've prepared 3 patches:
> >>>> >>
> >>>> >> 1. LLVM patch changing DIBuilder
> >>>> >> 2. Clang patch changing the test
> >>>> >> 3. LLVM patch changing the clone functionality and its tests
> >>>> >>
> >>>> >> Patches (1) and (2) should be submitted together, and (3) later.
> >>>> >>
> >>>> >> If you think the other conditionals you mentioned should be fixed
> in
> >>>> >> patches (1) or (2) above, I'd appreciate some help - either you
> >>>> >> change them
> >>>> >> or at least give me pointers on where to look :-)
> >>>> >>
> >>>> >> - Alon
> >>>> >>
> >>>> >>
> >>>> >> On 29 January 2014 19:41, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>>> >>>
> >>>> >>> Thanks Alon for following up.
> >>>> >>>
> >>>> >>> Could you pull out the change to make those {i32 0} -> {} into a
> >>>> >>> separate
> >>>> >>> patch? It'd be good to review/commit that (and maybe remove a
> bunch
> >>>> >>> of those
> >>>> >>> conditionals I was mentioning) separately. Including updating the
> >>>> >>> relevant
> >>>> >>> Clang test.
> >>>> >>>
> >>>> >>> To answer your question, yes, I think it's fine to change the
> >>>> >>> [[EMPTY]]
> >>>> >>> thing to {} rather than {i32 0}. I believe it was some kind of
> >>>> >>> legacy (I'd
> >>>> >>> /love/ to know what the original reason was just to satisfy myself
> >>>> >>> that it
> >>>> >>> no longer (or never) holds, but I'm not sure I'll ever know for
> >>>> >>> sure).
> >>>> >>>
> >>>> >>>
> >>>> >>> On Wed, Jan 29, 2014 at 12:48 AM, Alon Mishne <
> alonmishne at gmail.com>
> >>>> >>> wrote:
> >>>> >>>>
> >>>> >>>> Update to address David's comments:
> >>>> >>>>
> >>>> >>>> 1. Added tests to verify dbg.declare and dbg.value are cloned as
> >>>> >>>> expected.
> >>>> >>>> 2. Ran the Clang tests... and indeed, one test failed:
> >>>> >>>> debug-info-template.cpp. The test has a check that expects
> >>>> >>>> "metadata !{i32
> >>>> >>>> 0}" where "metadata !{}" now appears. Changing this check allows
> >>>> >>>> the test to
> >>>> >>>> pass. It's not quite clear to me whether that's okay, though -
> >>>> >>>> David, I saw
> >>>> >>>> you're the one who added that check - is it okay to modify it?
> >>>> >>>>
> >>>> >>>> Also CC to Eric (Nadav suggested you might be interested)
> >>>> >>>>
> >>>> >>>> - Alon
> >>>> >>>>
> >>>> >>>>
> >>>> >>>> On 2 January 2014 21:28, Alon Mishne <alonmishne at gmail.com>
> wrote:
> >>>> >>>>>
> >>>> >>>>> Thanks for the quick reply - see responses below.
> >>>> >>>>>
> >>>> >>>>>>
> >>>> >>>>>> From: dblaikie at gmail.com [mailto:dblaikie at gmail.com]
> >>>> >>>>>> Sent: Thursday, January 02, 2014 20:08
> >>>> >>>>>>
> >>>> >>>>>> Thanks for working on this.
> >>>> >>>>>>
> >>>> >>>>>>
> >>>> >>>>>>
> >>>> >>>>>> I hate to do this to you - but did you run the Clang tests
> after
> >>>> >>>>>> your
> >>>> >>>>>> change to DIBuilder? DIBuilder is, unfortunately, not tested by
> >>>> >>>>>> LLVM's tests
> >>>> >>>>>> - but is indirectly tested by Clang's debug info IRGen level
> >>>> >>>>>> tests.
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> Didn't run them, thanks for pointing this out, will do so.
> >>>> >>>>>
> >>>> >>>>>> I'd be more than happy to remove that weird zero-length arrays
> >>>> >>>>>> are
> >>>> >>>>>> emitted as arrays with a single zero. I assumed there was an IR
> >>>> >>>>>> limitation
> >>>> >>>>>> (that we couldn't have zero-length metadata values) that
> created
> >>>> >>>>>> this
> >>>> >>>>>> situation, but I never went to look/verify that that was
> actually
> >>>> >>>>>> the case.
> >>>> >>>>>>
> >>>> >>>>>>
> >>>> >>>>>>
> >>>> >>>>>> (we can remove a bunch of conditionals that can just become
> >>>> >>>>>> asserts if
> >>>> >>>>>> we can remove that zero/one/i32 0 array encoding weirdness)
> >>>> >>>>>>
> >>>> >>>>>>
> >>>> >>>>>>
> >>>> >>>>>> If it passes Clang's tests, it's probably worth committing that
> >>>> >>>>>> change
> >>>> >>>>>> separately.
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> You mean that the DIBuilder modification should be committed
> first
> >>>> >>>>> separately, before the rest of this patch?
> >>>> >>>>>
> >>>> >>>>>>
> >>>> >>>>>> The loop that adds the new subprogram to the CUs' subprogram
> >>>> >>>>>> lists
> >>>> >>>>>> could probably just bail out once it finds a single copy - that
> >>>> >>>>>> list
> >>>> >>>>>> shouldn't contain duplicates.
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> That's exactly what I thought, but then Michael Kuperstein
> showed
> >>>> >>>>> me
> >>>> >>>>> that it actually can happen: if you have two compilation units
> >>>> >>>>> that were
> >>>> >>>>> linked together and each contained the same function definition
> >>>> >>>>> (possible
> >>>> >>>>> with "linkonce_odr", for example), then you'll indeed get two
> >>>> >>>>> subprogram
> >>>> >>>>> entries pointing to the *same* function, linked from the two
> >>>> >>>>> different CUs.
> >>>> >>>>>
> >>>> >>>>>> I'd have to think more about whether what you've done is
> >>>> >>>>>> sufficient
> >>>> >>>>>> for the duplication. I have a sneaking suspicion that it isn't
> -
> >>>> >>>>>> what
> >>>> >>>>>> happens to the dbg_declare/dbg_value intrinsics in the cloned
> >>>> >>>>>> function? What
> >>>> >>>>>> variables do they point to? what context do those variables
> point
> >>>> >>>>>> to? (I
> >>>> >>>>>> suspect they point back to the original, not the cloned,
> >>>> >>>>>> function, which
> >>>> >>>>>> could be problematic)
> >>>> >>>>>>
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> It's a good point, I'll add unit tests to check it - and if they
> >>>> >>>>> fail
> >>>> >>>>> will also fix the code.
> >>>> >>>>>
> >>>> >>>>> Thanks again for the thorough review!
> >>>> >>>>>
> >>>> >>>>> - Alon
> >>>> >>>>>
> >>>> >>>>>> On Thu Jan 02 2014 at 7:47:28 AM, Mishne, Alon
> >>>> >>>>>> <alon.mishne at intel.com>
> >>>> >>>>>> wrote:
> >>>> >>>>>> Right now CloneFunction ignores the debug info metadata, which
> >>>> >>>>>> can
> >>>> >>>>>> lead to debug info loss when the cloned function is used. This
> >>>> >>>>>> patch
> >>>> >>>>>> modifies that so that if the 'ModuleLevelChanges' parameter to
> >>>> >>>>>> CloneFunction
> >>>> >>>>>> is true, any debug info metadata for the old function is cloned
> >>>> >>>>>> for the new
> >>>> >>>>>> function.
> >>>> >>>>>>
> >>>> >>>>>> Also expanded the cloning unit tests to test for this new
> >>>> >>>>>> functionality.
> >>>> >>>>>>
> >>>> >>>>>> Additionally, one of my tests required checking for an empty
> >>>> >>>>>> DIArray,
> >>>> >>>>>> but apparently DIBuilder doesn't make those - if asked to
> create
> >>>> >>>>>> an empty
> >>>> >>>>>> array, it creates an array with a single "null" value instead.
> >>>> >>>>>> I've changed
> >>>> >>>>>> DIBuilder to no longer do that because I don't understand why
> >>>> >>>>>> it's done and
> >>>> >>>>>> all the tests still pass after the change.
> >>>> >>>>
> >>>> >>>>
> >>>> >>>
> >>>> >>
> >>>> >
> >>>
> >>>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140306/24dc70f7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clone-debug-info7.patch
Type: application/octet-stream
Size: 12229 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140306/24dc70f7/attachment.obj>