[PATCH] Make CloneFunction also clone debug info metadata

[Eric Christopher]

LGTM.

Sorry for the delay.

-eric

On Thu, Mar 6, 2014 at 12:41 AM, Alon Mishne <alonmishne at gmail.com> wrote:
> 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.
>> >>>> >>>>
>> >>>> >>>>
>> >>>> >>>
>> >>>> >>
>> >>>> >
>> >>>
>> >>>
>> >>
>> >
>
>