<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Dec 9, 2016 at 11:34 AM Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.</blockquote><div><br></div><div>Right - but (much like that infamous assertion I added) knowing why/what the right behavior rather than allowing it implicitly might be worthwhile.<br><br>Adding a check until we discover a reasonable case where it should occur and decide what should happen there may be worthwhile.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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.</blockquote><div><br></div><div>Yeah, that's somewhat my concern - if it's valid to have multiple simultaneous locations (which it is, and DWARF supports - but it supports it with location lists, I believe, not with a direct location description? - not sure whether the location list applies to global variables, or how we'd describe the address range (since it might cover comdats, other functions not in this CU, etc)... )<br><br>Just a thought.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The backend could certainly become smarter about which fragment to choose to get optimal coverage though.<br class="gmail_msg">
<br class="gmail_msg">
-- adrian<br class="gmail_msg">
<br class="gmail_msg">
> On Dec 9, 2016, at 11:13 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:<br class="gmail_msg">
><br class="gmail_msg">
> Should we assert/fail verification on this? (probably a separate question from this patch)<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> On Fri, Dec 9, 2016 at 11:08 AM Adrian Prantl <<a href="mailto:aprantl@apple.com" class="gmail_msg" target="_blank">aprantl@apple.com</a>> wrote:<br class="gmail_msg">
>> On Dec 9, 2016, at 11:01 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>> wrote:<br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>> On Fri, Dec 9, 2016 at 10:43 AM Adrian Prantl via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="gmail_msg" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br class="gmail_msg">
>> aprantl marked 4 inline comments as done.<br class="gmail_msg">
>> aprantl added a comment.<br class="gmail_msg">
>><br class="gmail_msg">
>> In <a href="https://reviews.llvm.org/D27550#618335" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D27550#618335</a>, @dblaikie wrote:<br class="gmail_msg">
>><br class="gmail_msg">
>> > Not quite following all the changes here, but they look roughly plausible.<br class="gmail_msg">
>> ><br class="gmail_msg">
>> > 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)<br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>> Yes, DebugLocEntry::sortUniqueValues() (and possibly others locations) is sorting the fragments by offset.<br class="gmail_msg">
>><br class="gmail_msg">
>> Ah, thanks - and what happens if there's overlap? Is there some error handling?<br class="gmail_msg">
><br class="gmail_msg">
> 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.<br class="gmail_msg">
><br class="gmail_msg">
> -- adrian<br class="gmail_msg">
><br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>> ================<br class="gmail_msg">
>> Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:505<br class="gmail_msg">
>> }<br class="gmail_msg">
>> + Expr.AddExpression(ArrayRef<uint64_t>(DIExpr));<br class="gmail_msg">
>> + Expr.finalize();<br class="gmail_msg">
>> ----------------<br class="gmail_msg">
>> dblaikie wrote:<br class="gmail_msg">
>> > makeArrayRef?<br class="gmail_msg">
>> Nice. Didn't know we had that.<br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>> ================<br class="gmail_msg">
>> Comment at: test/DebugInfo/AArch64/frameindices.ll:9<br class="gmail_msg">
>> +; CHECK: DW_AT_location [DW_FORM_block1] (<0x0c> 93 01 91 51 93 0f 93 01 91 4a 93 07 )<br class="gmail_msg">
>> +; -- piece 0x00000001, fbreg -47, piece 0x0000000f, piece 0x00000001, fbreg -54, piece 0x00000007 ------^<br class="gmail_msg">
>> ; CHECK: DW_AT_abstract_origin {{.*}} "p1"<br class="gmail_msg">
>> ----------------<br class="gmail_msg">
>> dblaikie wrote:<br class="gmail_msg">
>> > 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 :)<br class="gmail_msg">
>> Thanks, from past experience, I really appreciate someone double-checking this!<br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>> ================<br class="gmail_msg">
>> Comment at: test/DebugInfo/MIR/X86/bit-piece-dh.mir:1<br class="gmail_msg">
>> +# RUN: llc -filetype=obj -o - %s | llvm-dwarfdump - | FileCheck %s<br class="gmail_msg">
>> +# CHECK: .debug_info contents:<br class="gmail_msg">
>> ----------------<br class="gmail_msg">
>> dblaikie wrote:<br class="gmail_msg">
>> > what's 'dh' stand for in the title of this test?<br class="gmail_msg">
>> ><br class="gmail_msg">
>> > What codepath does this test? (looks like the expression isn't in DIExpression - it's created in some MI/CodeGen layer?)<br class="gmail_msg">
>> The 8-bit x86 register "dh" hi-part of (r)dx.<br class="gmail_msg">
>><br class="gmail_msg">
>> ```<br class="gmail_msg">
>> 0 8 16 32 64<br class="gmail_msg">
>> +-----------------------+<br class="gmail_msg">
>> | rdx |<br class="gmail_msg">
>> | edx | |<br class="gmail_msg">
>> | dx | | |<br class="gmail_msg">
>> | dl | dh | | |<br class="gmail_msg">
>> +----+----+-----+-------+<br class="gmail_msg">
>> ```<br class="gmail_msg">
>><br class="gmail_msg">
>> 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).<br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>> ================<br class="gmail_msg">
>> Comment at: test/DebugInfo/X86/dw_op_minus_direct.ll:9-12<br class="gmail_msg">
>> +; CHECK: Beginning address offset: 0x0000000000000000<br class="gmail_msg">
>> +; CHECK: Ending address offset: 0x0000000000000004<br class="gmail_msg">
>> +; CHECK: Location description: 50 10 01 1c 93 04<br class="gmail_msg">
>> +; rax, constu 0x00000001, minus, piece 0x00000004<br class="gmail_msg">
>> ----------------<br class="gmail_msg">
>> dblaikie wrote:<br class="gmail_msg">
>> > This is test coverage for DwarfExpression.cpp:244-ish? Probably better as a separate commit?<br class="gmail_msg">
>> 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.<br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>> <a href="https://reviews.llvm.org/D27550" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D27550</a><br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
>><br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>