[PATCH] Make CloneFunction also clone debug info metadata

[David Blaikie]

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/20140129/462d3303/attachment.html>