<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Sat, Feb 1, 2014 at 11:40 PM, Alon Mishne <span dir="ltr"><<a href="mailto:alonmishne@gmail.com" target="_blank">alonmishne@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">As I have commit permissions to neither LLVM nor Clang, will appreciate if you could commit for me :-)<br>
</div></blockquote><div><br></div><div>Committed as <span style="font-family:monospace">200721 and 200722. Thanks! (apologies - but I forgot to credit you in those commits)</span></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div>Regarding (3) - is there anything else you think should be changed?<br></div></div></blockquote><div><br></div><div>I've still not given them enough of a look to say yes/no - I might, if someone else doesn't get to it first.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div></div><div><br>
</div><div>Eric - I don't see where I've indented a namespace...</div></div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On 30 January 2014 21:02, 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Agreed, 1 & 2 look fine. 3 will need a bit of a deeper look. One nit:<br>
don't indent the namespace :)<br>
<span><font color="#888888"><br>
-eric<br>
</font></span><div><div><br>
On Thu, Jan 30, 2014 at 9:31 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> 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 expect<br>
> there's a bunch of LLVM tests that all have these {i32 0} in them that I'll<br>
> clean up when I remove the bits of code that silently handle this 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" target="_blank">alonmishne@gmail.com</a>> 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 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" target="_blank">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 separate<br>
>>> patch? It'd be good to review/commit that (and maybe remove a bunch of those<br>
>>> conditionals I was mentioning) separately. Including updating the relevant<br>
>>> Clang test.<br>
>>><br>
>>> To answer your question, yes, I think it's fine to change the [[EMPTY]]<br>
>>> thing to {} rather than {i32 0}. I believe it was some kind of legacy (I'd<br>
>>> /love/ to know what the original reason was just to satisfy myself that it<br>
>>> no longer (or never) holds, but I'm not sure I'll ever know for sure).<br>
>>><br>
>>><br>
>>> On Wed, Jan 29, 2014 at 12:48 AM, Alon Mishne <<a href="mailto:alonmishne@gmail.com" target="_blank">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 "metadata !{i32<br>
>>>> 0}" where "metadata !{}" now appears. Changing this check allows the test to<br>
>>>> pass. It's not quite clear to me whether that's okay, though - 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" target="_blank">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" target="_blank">dblaikie@gmail.com</a> [mailto:<a href="mailto:dblaikie@gmail.com" target="_blank">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 your<br>
>>>>>> change to DIBuilder? DIBuilder is, unfortunately, not tested by LLVM's tests<br>
>>>>>> - but is indirectly tested by Clang's debug info IRGen level 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 are<br>
>>>>>> emitted as arrays with a single zero. I assumed there was an IR limitation<br>
>>>>>> (that we couldn't have zero-length metadata values) that created this<br>
>>>>>> situation, but I never went to look/verify that that was actually the case.<br>
>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>> (we can remove a bunch of conditionals that can just become 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 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 lists<br>
>>>>>> could probably just bail out once it finds a single copy - that list<br>
>>>>>> shouldn't contain duplicates.<br>
>>>>><br>
>>>>><br>
>>>>> That's exactly what I thought, but then Michael Kuperstein showed me<br>
>>>>> that it actually can happen: if you have two compilation units that were<br>
>>>>> linked together and each contained the same function definition (possible<br>
>>>>> with "linkonce_odr", for example), then you'll indeed get two subprogram<br>
>>>>> entries pointing to the *same* function, linked from the two different CUs.<br>
>>>>><br>
>>>>>> I'd have to think more about whether what you've done is sufficient<br>
>>>>>> for the duplication. I have a sneaking suspicion that it isn't - what<br>
>>>>>> happens to the dbg_declare/dbg_value intrinsics in the cloned function? What<br>
>>>>>> variables do they point to? what context do those variables point to? (I<br>
>>>>>> suspect they point back to the original, not the cloned, 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 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 <<a href="mailto:alon.mishne@intel.com" target="_blank">alon.mishne@intel.com</a>><br>
>>>>>> wrote:<br>
>>>>>> Right now CloneFunction ignores the debug info metadata, which can<br>
>>>>>> lead to debug info loss when the cloned function is used. This patch<br>
>>>>>> modifies that so that if the 'ModuleLevelChanges' parameter to CloneFunction<br>
>>>>>> is true, any debug info metadata for the old function is cloned 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 DIArray,<br>
>>>>>> but apparently DIBuilder doesn't make those - if asked to create an empty<br>
>>>>>> array, it creates an array with a single "null" value instead. I've changed<br>
>>>>>> DIBuilder to no longer do that because I don't understand why it's done and<br>
>>>>>> all the tests still pass after the change.<br>
>>>><br>
>>>><br>
>>><br>
>><br>
><br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>