<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Feb 4, 2015, at 10:18 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Wed, Feb 4, 2015 at 10:13 AM, Frederic Riss <span dir="ltr" class=""><<a href="mailto:friss@apple.com" target="_blank" class="">friss@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks. I'm testing an update addressing your feedback. Some comments bellow:<br class="">
<span class=""><br class="">
<br class="">
================<br class="">
Comment at: include/llvm/DebugInfo/DWARFExpression.h:109<br class="">
@@ +108,3 @@<br class="">
+<br class="">
+ bool operator==(const iterator &I) const {<br class="">
+ return Expression == I.Expression && Offset == I.Offset;<br class="">
----------------<br class="">
</span><span class="">dblaikie wrote:<br class="">
> operator overloads that can be non-members generally should be (to allow symmetric conversions on LHS and RHS) - could define it inline as a friend if that's useful.<br class="">
</span>Yeah, but the iterator_facade that I'm using requires a member function to deriver != from ==. I added a couple of non-members operators that forward to the member ones.<br class=""></blockquote><div class=""><br class="">Reckon we can fix that to allow non-member versions?<br class=""></div></div></div></div></div></blockquote><div><br class=""></div><div>This would certainly be possible, although I suspect it wouldn’t gain us much. The iterator_facade uses the == to derive a != that will be a member function. So if we want non-member function, we’d still need to add that code by ourselves. I’ll just implement both == and != as out of lines and this should make all users happy.</div><br class=""><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br class="">
================<br class="">
Comment at: lib/DebugInfo/DWARFDebugLoc.cpp:34<br class="">
@@ +33,3 @@<br class="">
+ Unit->getAddressByteSize());<br class="">
+ DWARFExpression(Data, Unit).Print(OS, false);<br class="">
+ }<br class="">
----------------<br class="">
</span><span class="">dblaikie wrote:<br class="">
> what's the boolean? ('false')<br class="">
><br class="">
> (could use a comment, maybe, or an enum, etc)<br class="">
</span>It decides wether to use the standard or the EH register mapping. We never use the EH mapping for now, but I thought I'd thread it through anyway. I've changed that to a enum with a default value.<br class=""></blockquote><div class=""><br class="">Generally I wouldn't bother adding extra code for a use case we don't have yet, since it'll be dead/untested code.<br class=""></div></div></div></div></blockquote><div><br class=""></div><div>Yeah, right. I’ll drop the parameter altogether.</div><div><br class=""></div><div>Fred</div><div><br class=""></div><blockquote type="cite" class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br class="">
================<br class="">
Comment at: test/CodeGen/X86/2011-01-24-DbgValue-Before-Use.ll:12<br class="">
@@ -11,2 +11,3 @@<br class="">
; CHECK-NEXT: DW_AT_location<br class="">
-; CHECK-NEXT: DW_AT_name {{.*}} "z_s"<br class="">
+; CHECK-NOT: DW_TAG<br class="">
+; CHECK: DW_AT_name {{.*}} "z_s"<br class="">
----------------<br class="">
</span><span class="">dblaikie wrote:<br class="">
> Maybe CHECK-NOT: DW_{{TAG|AT}} ? I assume this was here so we didn't skip any extra attributes on this TAG.<br class="">
</span>I changed that to match the old test, although I'm not sure their is any value in checking the ordering of the attributes within a TAG.<br class=""></blockquote><div class=""><br class="">IIRC/I suspect it's not so much testing for the ordering of the attributes, but testing for the existence of certain attributes and ensuring no other attributes are expected. But maybe not - this test predates my work on debug info by a few years.<br class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br class="">
================<br class="">
Comment at: test/DebugInfo/X86/debug-loc-offset.ll:50<br class="">
@@ -49,1 +49,3 @@<br class="">
+; do not check the location stored here, as it will be offseted by the<br class="">
+; compile unit's low_pc. The real check is below in the debug_loc section.<br class="">
; CHECK-NOT: DW_TAG<br class="">
----------------<br class="">
</span><span class="">dblaikie wrote:<br class="">
> Not sure I follow why the low_pc offset would be problematic - it'll always be zero in a single-CU file, right?<br class="">
</span>This is the whole point of this test. It's a multi-cu test that verifies that the location entries are stored relatively to the low_pc of the unit. The test is that the location's base address should be 0. When it is displayed inline the offset is applied - which is a good thing - but it's not what we want to test. So I defer the test to the raw dump of the debug_lc section bellow.<br class="">
The comment shouldn't read 'stored here' though, 'deployed here' would be more accurate.<br class=""></blockquote><div class=""><br class="">I think I'm roughly seeing that.<br class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br class="">
<a href="http://reviews.llvm.org/D6771" target="_blank" class="">http://reviews.llvm.org/D6771</a><br class="">
<br class="">
EMAIL PREFERENCES<br class="">
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank" class="">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br class="">
<br class="">
<br class="">
</div></div></blockquote></div><br class=""></div></div>
</blockquote></div><br class=""></body></html>