[PATCH] D27550: Fix LLVM's use of DW_OP_bit_piece in DWARF expressions.

[Adrian Prantl via llvm-commits]

To verify this you would need to replicate all the buildLocationList() logic in the Verifier. I'm also not sure if this isn't something that could happen legitimately because of optimizations. I can't think of a way this would happen today, but I wouldn't be surprised if it were possible due to, e.g, repeated SROA. The backend could certainly become smarter about which fragment to choose to get optimal coverage though.

-- adrian

> On Dec 9, 2016, at 11:13 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> Should we assert/fail verification on this? (probably a separate question from this patch)
> 
> 
> On Fri, Dec 9, 2016 at 11:08 AM Adrian Prantl <aprantl at apple.com> wrote:
>> On Dec 9, 2016, at 11:01 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>> 
>> 
>> On Fri, Dec 9, 2016 at 10:43 AM Adrian Prantl via Phabricator <reviews at reviews.llvm.org> wrote:
>> aprantl marked 4 inline comments as done.
>> aprantl added a comment.
>> 
>> In https://reviews.llvm.org/D27550#618335, @dblaikie wrote:
>> 
>> > Not quite following all the changes here, but they look roughly plausible.
>> >
>> > Is there some code I couldn't find that sorts all the fragments before emitting them? (the old scheme wouldn't've needed this, since each bit_piece could specify where it appears - but maybe we were already sorting them anyway so pieces were linearly increasing in position? - the new scheme would require such sorting because the positions are implicit relative to prior pieces)
>> 
>> 
>> Yes, DebugLocEntry::sortUniqueValues() (and possibly others locations) is sorting the fragments by offset.
>> 
>> Ah, thanks - and what happens if there's overlap? Is there some error handling?
> 
> DwarfDebug::buildLocationList() collects looks at the fragments and and then chops up their ranges into the smallest continuous region that is covered by a set of non-overlapping fragments. So what will likely happen is that it will produce a zero-sized region for the first overlapping fragment that will then be silently dropped.
> 
> -- adrian
> 
>>  
>> 
>> 
>> 
>> ================
>> Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:505
>>    }
>> +  Expr.AddExpression(ArrayRef<uint64_t>(DIExpr));
>> +  Expr.finalize();
>> ----------------
>> dblaikie wrote:
>> > makeArrayRef?
>> Nice. Didn't know we had that.
>> 
>> 
>> ================
>> Comment at: test/DebugInfo/AArch64/frameindices.ll:9
>> +; CHECK: DW_AT_location [DW_FORM_block1]    (<0x0c> 93 01 91 51 93 0f 93 01 91 4a 93 07 )
>> +;  -- piece 0x00000001, fbreg -47, piece 0x0000000f, piece 0x00000001, fbreg -54, piece 0x00000007 ------^
>>  ; CHECK: DW_AT_abstract_origin {{.*}} "p1"
>> ----------------
>> dblaikie wrote:
>> > Took me a bit to go through both these examples (keeping both the DW_OP_bit_piece/piece interpretations in my head, etc) - but yes, great to see this, as it looks good/right/etc :)
>> Thanks, from past experience,  I really appreciate someone double-checking this!
>> 
>> 
>> ================
>> Comment at: test/DebugInfo/MIR/X86/bit-piece-dh.mir:1
>> +# RUN: llc -filetype=obj -o - %s | llvm-dwarfdump - | FileCheck %s
>> +# CHECK: .debug_info contents:
>> ----------------
>> dblaikie wrote:
>> > what's 'dh' stand for in the title of this test?
>> >
>> > What codepath does this test? (looks like the expression isn't in DIExpression - it's created in some MI/CodeGen layer?)
>> The 8-bit x86 register "dh" hi-part of (r)dx.
>> 
>> ```
>> 0    8   16    32       64
>> +-----------------------+
>> |              rdx      |
>> |        edx    |       |
>> |    dx   |     |       |
>> | dl | dh |     |       |
>> +----+----+-----+-------+
>> ```
>> 
>> This is testing DwarfExpression.cpp:111 (Walk up the super-register chain until we find a valid number and emit a DW_OP_bit_piece for it).
>> 
>> 
>> ================
>> Comment at: test/DebugInfo/X86/dw_op_minus_direct.ll:9-12
>> +; CHECK: Beginning address offset: 0x0000000000000000
>> +; CHECK:    Ending address offset: 0x0000000000000004
>> +; CHECK:     Location description: 50 10 01 1c 93 04
>> +;                                  rax, constu 0x00000001, minus, piece 0x00000004
>> ----------------
>> dblaikie wrote:
>> > This is test coverage for DwarfExpression.cpp:244-ish? Probably better as a separate commit?
>> It tests a bugfix that fell out of this rewrite/factoring. The code in trunk would have crashed when it encountered a DW_OP_minus without a DW_OP_deref. I would leave it in here.
>> 
>> 
>> https://reviews.llvm.org/D27550
>> 
>> 
>>