<div dir="ltr">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 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)</div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jan 30, 2014 at 1:58 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">OK, so I've prepared 3 patches:<div><br></div><div>1. LLVM patch changing DIBuilder</div><div>2. Clang patch changing the test</div>
<div>3. LLVM patch changing the clone functionality and its tests</div>
<div>
<br></div><div>Patches (1) and (2) should be submitted together, and (3) later.</div><div><br></div><div>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 :-)</div>
<span class="HOEnZb"><font color="#888888">
<div><br></div><div>- Alon</div></font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On 29 January 2014 19:41, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Thanks Alon for following up.<br><br>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.<br>
<br>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).</div>
<div><div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jan 29, 2014 at 12:48 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Update to address David's comments:<div><br></div><div>1. Added tests to verify dbg.declare and dbg.value are cloned as expected.</div>
<div>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?</div>
<div><br></div><div>Also CC to Eric (Nadav suggested you might be interested)</div><span><font color="#888888"><div><br></div><div>- Alon</div></font></span></div><div><div><div class="gmail_extra">
<br><br><div class="gmail_quote">On 2 January 2014 21:28, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Thanks for the quick reply - see responses below.</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">
<b><span style="font-size:11pt;font-family:Calibri,sans-serif">From:</span></b><span style="font-size:11pt;font-family:Calibri,sans-serif"> <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>
<b>Sent:</b> Thursday, January 02, 2014 20:08</span><span style="font-size:11pt;font-family:Calibri,sans-serif"><br></span> <br>Thanks for working on this.<br></blockquote><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">
</blockquote><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">
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.<br></blockquote><div> </div><div>Didn't run them, thanks for pointing this out, will do so.</div><div><br></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">
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.<br></blockquote><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">
</blockquote><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">
(we can remove a bunch of conditionals that can just become asserts if we can
remove that zero/one/i32 0 array encoding weirdness)<br></blockquote><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">
</blockquote><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">
If it passes Clang's tests, it's probably worth committing that change
separately.<br></blockquote><div><br></div><div>You mean that the DIBuilder modification should be committed first separately, before the rest of this patch? </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">
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.<br></blockquote><div> </div><div>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.</div>
<div><br></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">
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)<br>
<br></blockquote><div> </div><div>It's a good point, I'll add unit tests to check it - and if they fail will also fix the code.</div><div><br></div><div>Thanks again for the thorough review!</div><div><br></div>
<div>- Alon</div><div><br></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">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>>
wrote:<br>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.<br> <br>Also expanded the cloning unit tests to test for this new functionality.<br> <br><span lang="EN">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.</span></blockquote>
<p class="MsoNormal"></p>
<p class="MsoNormal"></p>
<p></p>
<p></p>
<p></p>
<p></p>
<p></p></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>