[PATCH] Make CloneFunction also clone debug info metadata

[Eric Christopher]

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.
>>>>
>>>>
>>>
>>
>