[PATCH] [dwarfdump] Pretty print location expressions and location lists

[Frédéric Riss]

> On Feb 4, 2015, at 10:18 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Wed, Feb 4, 2015 at 10:13 AM, Frederic Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
> Thanks. I'm testing an update addressing your feedback. Some comments bellow:
> 
> 
> ================
> Comment at: include/llvm/DebugInfo/DWARFExpression.h:109
> @@ +108,3 @@
> +
> +    bool operator==(const iterator &I) const {
> +      return Expression == I.Expression && Offset == I.Offset;
> ----------------
> dblaikie wrote:
> > 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.
> 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.
> 
> Reckon we can fix that to allow non-member versions?

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.

> 
> ================
> Comment at: lib/DebugInfo/DWARFDebugLoc.cpp:34
> @@ +33,3 @@
> +                       Unit->getAddressByteSize());
> +    DWARFExpression(Data, Unit).Print(OS, false);
> +  }
> ----------------
> dblaikie wrote:
> > what's the boolean? ('false')
> >
> > (could use a comment, maybe, or an enum, etc)
> 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.
> 
> Generally I wouldn't bother adding extra code for a use case we don't have yet, since it'll be dead/untested code.

Yeah, right. I’ll drop the parameter altogether.

Fred

> 
> ================
> Comment at: test/CodeGen/X86/2011-01-24-DbgValue-Before-Use.ll:12
> @@ -11,2 +11,3 @@
>  ; CHECK-NEXT:   DW_AT_location
> -; CHECK-NEXT:   DW_AT_name {{.*}} "z_s"
> +; CHECK-NOT: DW_TAG
> +; CHECK:   DW_AT_name {{.*}} "z_s"
> ----------------
> dblaikie wrote:
> > Maybe CHECK-NOT: DW_{{TAG|AT}} ? I assume this was here so we didn't skip any extra attributes on this TAG.
> 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.
> 
> 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.
>  
> 
> ================
> Comment at: test/DebugInfo/X86/debug-loc-offset.ll:50
> @@ -49,1 +49,3 @@
> +; do not check the location stored here, as it will be offseted by the
> +; compile unit's low_pc. The real check is below in the debug_loc section.
>  ; CHECK-NOT: DW_TAG
> ----------------
> dblaikie wrote:
> > Not sure I follow why the low_pc offset would be problematic - it'll always be zero in a single-CU file, right?
> 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.
> The comment shouldn't read 'stored here' though, 'deployed here' would be more accurate.
> 
> I think I'm roughly seeing that.
>  
> 
> http://reviews.llvm.org/D6771 <http://reviews.llvm.org/D6771>
> 
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/ <http://reviews.llvm.org/settings/panel/emailpreferences/>
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150204/851fcf89/attachment.html>