<div dir="ltr">Rebased patch attached (including latest movement of headers into IR/ folder)</div><div class="gmail_extra"><br><br><div class="gmail_quote">On 25 February 2014 02:49, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Alon,<br>
<br>
Sorry for the delay, can post/repost a rebased patch please?<br>
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Sun, Feb 16, 2014 at 1:22 AM, Alon Mishne <<a href="mailto:alonmishne@gmail.com">alonmishne@gmail.com</a>> wrote:<br>
> Thanks David! Meanwhile I got my own commit access so next time it should be<br>
> simpler.<br>
><br>
> Any thoughts about patch (3)?<br>
><br>
><br>
> On 4 February 2014 02:04, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>><br>
>><br>
>><br>
>> On Sat, Feb 1, 2014 at 11:40 PM, Alon Mishne <<a href="mailto:alonmishne@gmail.com">alonmishne@gmail.com</a>> wrote:<br>
>>><br>
>>> As I have commit permissions to neither LLVM nor Clang, will appreciate<br>
>>> if you could commit for me :-)<br>
>><br>
>><br>
>> Committed as 200721 and 200722. Thanks! (apologies - but I forgot to<br>
>> credit you in those commits)<br>
>><br>
>>><br>
>>> Regarding (3) - is there anything else you think should be changed?<br>
>><br>
>><br>
>> I've still not given them enough of a look to say yes/no - I might, if<br>
>> someone else doesn't get to it first.<br>
>><br>
>>><br>
>>><br>
>>> Eric - I don't see where I've indented a namespace...<br>
>>><br>
>>><br>
>>> On 30 January 2014 21:02, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br>
>>>><br>
>>>> Agreed, 1 & 2 look fine. 3 will need a bit of a deeper look. One nit:<br>
>>>> don't indent the namespace :)<br>
>>>><br>
>>>> -eric<br>
>>>><br>
>>>> On Thu, Jan 30, 2014 at 9:31 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>>>> wrote:<br>
>>>> > Patches 1 & 2 look fine - can you commit them, or do you need me to?<br>
>>>> ><br>
>>>> > (I'm rather surprised only one test had trouble with this (though I<br>
>>>> > expect<br>
>>>> > there's a bunch of LLVM tests that all have these {i32 0} in them that<br>
>>>> > I'll<br>
>>>> > clean up when I remove the bits of code that silently handle this<br>
>>>> > case) -<br>
>>>> > but awesome... shows how cargo culty some things can be I guess)<br>
>>>> ><br>
>>>> ><br>
>>>> > On Thu, Jan 30, 2014 at 1:58 AM, Alon Mishne <<a href="mailto:alonmishne@gmail.com">alonmishne@gmail.com</a>><br>
>>>> > wrote:<br>
>>>> >><br>
>>>> >> OK, so I've prepared 3 patches:<br>
>>>> >><br>
>>>> >> 1. LLVM patch changing DIBuilder<br>
>>>> >> 2. Clang patch changing the test<br>
>>>> >> 3. LLVM patch changing the clone functionality and its tests<br>
>>>> >><br>
>>>> >> Patches (1) and (2) should be submitted together, and (3) later.<br>
>>>> >><br>
>>>> >> If you think the other conditionals you mentioned should be fixed in<br>
>>>> >> patches (1) or (2) above, I'd appreciate some help - either you<br>
>>>> >> change them<br>
>>>> >> or at least give me pointers on where to look :-)<br>
>>>> >><br>
>>>> >> - Alon<br>
>>>> >><br>
>>>> >><br>
>>>> >> On 29 January 2014 19:41, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>>> >>><br>
>>>> >>> Thanks Alon for following up.<br>
>>>> >>><br>
>>>> >>> Could you pull out the change to make those {i32 0} -> {} into a<br>
>>>> >>> separate<br>
>>>> >>> patch? It'd be good to review/commit that (and maybe remove a bunch<br>
>>>> >>> of those<br>
>>>> >>> conditionals I was mentioning) separately. Including updating the<br>
>>>> >>> relevant<br>
>>>> >>> Clang test.<br>
>>>> >>><br>
>>>> >>> To answer your question, yes, I think it's fine to change the<br>
>>>> >>> [[EMPTY]]<br>
>>>> >>> thing to {} rather than {i32 0}. I believe it was some kind of<br>
>>>> >>> legacy (I'd<br>
>>>> >>> /love/ to know what the original reason was just to satisfy myself<br>
>>>> >>> that it<br>
>>>> >>> no longer (or never) holds, but I'm not sure I'll ever know for<br>
>>>> >>> sure).<br>
>>>> >>><br>
>>>> >>><br>
>>>> >>> On Wed, Jan 29, 2014 at 12:48 AM, Alon Mishne <<a href="mailto:alonmishne@gmail.com">alonmishne@gmail.com</a>><br>
>>>> >>> wrote:<br>
>>>> >>>><br>
>>>> >>>> Update to address David's comments:<br>
>>>> >>>><br>
>>>> >>>> 1. Added tests to verify dbg.declare and dbg.value are cloned as<br>
>>>> >>>> expected.<br>
>>>> >>>> 2. Ran the Clang tests... and indeed, one test failed:<br>
>>>> >>>> debug-info-template.cpp. The test has a check that expects<br>
>>>> >>>> "metadata !{i32<br>
>>>> >>>> 0}" where "metadata !{}" now appears. Changing this check allows<br>
>>>> >>>> the test to<br>
>>>> >>>> pass. It's not quite clear to me whether that's okay, though -<br>
>>>> >>>> David, I saw<br>
>>>> >>>> you're the one who added that check - is it okay to modify it?<br>
>>>> >>>><br>
>>>> >>>> Also CC to Eric (Nadav suggested you might be interested)<br>
>>>> >>>><br>
>>>> >>>> - Alon<br>
>>>> >>>><br>
>>>> >>>><br>
>>>> >>>> On 2 January 2014 21:28, Alon Mishne <<a href="mailto:alonmishne@gmail.com">alonmishne@gmail.com</a>> wrote:<br>
>>>> >>>>><br>
>>>> >>>>> Thanks for the quick reply - see responses below.<br>
>>>> >>>>><br>
>>>> >>>>>><br>
>>>> >>>>>> From: <a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a> [mailto:<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>]<br>
>>>> >>>>>> Sent: Thursday, January 02, 2014 20:08<br>
>>>> >>>>>><br>
>>>> >>>>>> Thanks for working on this.<br>
>>>> >>>>>><br>
>>>> >>>>>><br>
>>>> >>>>>><br>
>>>> >>>>>> I hate to do this to you - but did you run the Clang tests after<br>
>>>> >>>>>> your<br>
>>>> >>>>>> change to DIBuilder? DIBuilder is, unfortunately, not tested by<br>
>>>> >>>>>> LLVM's tests<br>
>>>> >>>>>> - but is indirectly tested by Clang's debug info IRGen level<br>
>>>> >>>>>> tests.<br>
>>>> >>>>><br>
>>>> >>>>><br>
>>>> >>>>> Didn't run them, thanks for pointing this out, will do so.<br>
>>>> >>>>><br>
>>>> >>>>>> I'd be more than happy to remove that weird zero-length arrays<br>
>>>> >>>>>> are<br>
>>>> >>>>>> emitted as arrays with a single zero. I assumed there was an IR<br>
>>>> >>>>>> limitation<br>
>>>> >>>>>> (that we couldn't have zero-length metadata values) that created<br>
>>>> >>>>>> this<br>
>>>> >>>>>> situation, but I never went to look/verify that that was actually<br>
>>>> >>>>>> the case.<br>
>>>> >>>>>><br>
>>>> >>>>>><br>
>>>> >>>>>><br>
>>>> >>>>>> (we can remove a bunch of conditionals that can just become<br>
>>>> >>>>>> asserts if<br>
>>>> >>>>>> we can remove that zero/one/i32 0 array encoding weirdness)<br>
>>>> >>>>>><br>
>>>> >>>>>><br>
>>>> >>>>>><br>
>>>> >>>>>> If it passes Clang's tests, it's probably worth committing that<br>
>>>> >>>>>> change<br>
>>>> >>>>>> separately.<br>
>>>> >>>>><br>
>>>> >>>>><br>
>>>> >>>>> You mean that the DIBuilder modification should be committed first<br>
>>>> >>>>> separately, before the rest of this patch?<br>
>>>> >>>>><br>
>>>> >>>>>><br>
>>>> >>>>>> The loop that adds the new subprogram to the CUs' subprogram<br>
>>>> >>>>>> lists<br>
>>>> >>>>>> could probably just bail out once it finds a single copy - that<br>
>>>> >>>>>> list<br>
>>>> >>>>>> shouldn't contain duplicates.<br>
>>>> >>>>><br>
>>>> >>>>><br>
>>>> >>>>> That's exactly what I thought, but then Michael Kuperstein showed<br>
>>>> >>>>> me<br>
>>>> >>>>> that it actually can happen: if you have two compilation units<br>
>>>> >>>>> that were<br>
>>>> >>>>> linked together and each contained the same function definition<br>
>>>> >>>>> (possible<br>
>>>> >>>>> with "linkonce_odr", for example), then you'll indeed get two<br>
>>>> >>>>> subprogram<br>
>>>> >>>>> entries pointing to the *same* function, linked from the two<br>
>>>> >>>>> different CUs.<br>
>>>> >>>>><br>
>>>> >>>>>> I'd have to think more about whether what you've done is<br>
>>>> >>>>>> sufficient<br>
>>>> >>>>>> for the duplication. I have a sneaking suspicion that it isn't -<br>
>>>> >>>>>> what<br>
>>>> >>>>>> happens to the dbg_declare/dbg_value intrinsics in the cloned<br>
>>>> >>>>>> function? What<br>
>>>> >>>>>> variables do they point to? what context do those variables point<br>
>>>> >>>>>> to? (I<br>
>>>> >>>>>> suspect they point back to the original, not the cloned,<br>
>>>> >>>>>> function, which<br>
>>>> >>>>>> could be problematic)<br>
>>>> >>>>>><br>
>>>> >>>>><br>
>>>> >>>>><br>
>>>> >>>>> It's a good point, I'll add unit tests to check it - and if they<br>
>>>> >>>>> fail<br>
>>>> >>>>> will also fix the code.<br>
>>>> >>>>><br>
>>>> >>>>> Thanks again for the thorough review!<br>
>>>> >>>>><br>
>>>> >>>>> - Alon<br>
>>>> >>>>><br>
>>>> >>>>>> On Thu Jan 02 2014 at 7:47:28 AM, Mishne, Alon<br>
>>>> >>>>>> <<a href="mailto:alon.mishne@intel.com">alon.mishne@intel.com</a>><br>
>>>> >>>>>> wrote:<br>
>>>> >>>>>> Right now CloneFunction ignores the debug info metadata, which<br>
>>>> >>>>>> can<br>
>>>> >>>>>> lead to debug info loss when the cloned function is used. This<br>
>>>> >>>>>> patch<br>
>>>> >>>>>> modifies that so that if the 'ModuleLevelChanges' parameter to<br>
>>>> >>>>>> CloneFunction<br>
>>>> >>>>>> is true, any debug info metadata for the old function is cloned<br>
>>>> >>>>>> for the new<br>
>>>> >>>>>> function.<br>
>>>> >>>>>><br>
>>>> >>>>>> Also expanded the cloning unit tests to test for this new<br>
>>>> >>>>>> functionality.<br>
>>>> >>>>>><br>
>>>> >>>>>> Additionally, one of my tests required checking for an empty<br>
>>>> >>>>>> DIArray,<br>
>>>> >>>>>> but apparently DIBuilder doesn't make those - if asked to create<br>
>>>> >>>>>> an empty<br>
>>>> >>>>>> array, it creates an array with a single "null" value instead.<br>
>>>> >>>>>> I've changed<br>
>>>> >>>>>> DIBuilder to no longer do that because I don't understand why<br>
>>>> >>>>>> it's done and<br>
>>>> >>>>>> all the tests still pass after the change.<br>
>>>> >>>><br>
>>>> >>>><br>
>>>> >>><br>
>>>> >><br>
>>>> ><br>
>>><br>
>>><br>
>><br>
><br>
</div></div></blockquote></div><br></div>