[PATCH] Make CloneFunction also clone debug info metadata

[Eric Christopher]

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