[PATCH] Make CloneFunction also clone debug info metadata

[David Blaikie]

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/20140203/5b085025/attachment.html>