[PATCH] Make CloneFunction also clone debug info metadata

[Alon Mishne]

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/20140216/4351e83a/attachment.html>