[PATCH] Make CloneFunction also clone debug info metadata

[Alon Mishne]

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/d3fb79bd/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clone-debug-info2.patch
Type: application/octet-stream
Size: 13002 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140129/d3fb79bd/attachment.obj>