<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><div><br></div><div>- Alon</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" style>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>