[PATCH] Make CloneFunction also clone debug info metadata

[Alon Mishne]

As I have commit permissions to neither LLVM nor Clang, will appreciate if
you could commit for me :-)

Regarding (3) - is there anything else you think should be changed?

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/20140202/35006ea9/attachment.html>