[PATCH] Make CloneFunction also clone debug info metadata

[Alon Mishne]

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/20140102/270aaa3d/attachment.html>