[PATCH] Avoid empty .debug_loc entries and lists

[Duncan P. N. Exon Smith]

> On 2015-May-29, at 11:15, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> 
>> On 2015-May-27, at 17:55, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>> Can't say I know of any particular situations that cause this (I guess it's trickier than just a variable that's optimized away entirely) - I'd just start reducing an existing test case (probably add the assertion and try a non-LTO build, hope that it reproduces there, then just creduce any specific file where it reproduces)
>> 
>>> On Wed, May 27, 2015 at 5:51 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>> I've noticed there are debug loc lists with no entries sometimes
>>> in my heap profiling workload (a linker dump of `-flto -g`).  It
>>> seems to happen when *all* the locations correspond to `%noreg`.
>>> 
>>> This patch adds an assertion (which fires on my big workload,
>>> but not in any of our tests), and adds a likely fix that's
>>> commented out.
>>> 
>>> Does anyone know how to generate this kind of thing, or should I
>>> just start compiling things to see if I can make it happen on my
>>> own?  (I've tried reducing my workload, but I'm having trouble
>>> getting very far with it.)

Thanks Adrian and David for your pointers.  In person, Adrian walked
me through how my original fix was insufficient: there are lots of
code paths that can produce empty .debug_loc entries.  While the one
I avoided originally might be worth doing as a compile-time
optimization, the correct fix is to:

 1. Finalize the entries that do exist.  If an entry turns out to be
    empty, delete it (pop_back()).
 2. Once all entries have been finalized, check if the list has any
    entries.  If not, delete the list (pop_back()).
 3. Delay creating a symbol and referring to the list until after
    all its entries have been finalized.

The attached patches accomplish this:

  - 0001: Refactor.
  - 0002: Avoid empty .debug_loc lists.  The testcase here is
    actually fairly large, and I think the CHECK line is brittle.  I
    wonder if it would be better to combine this with 0003 and rely
    only on the latter's testcase?
  - 0003: Avoid empty .debug_loc list entries.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AsmPrinter-Extract-DwarfDebug-createConcreteVariable.patch
Type: application/octet-stream
Size: 4063 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150601/ea2361a4/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-AsmPrinter-Don-t-emit-empty-.debug_loc-lists.patch
Type: application/octet-stream
Size: 16120 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150601/ea2361a4/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-AsmPrinter-Don-t-emit-empty-.debug_loc-entries.patch
Type: application/octet-stream
Size: 6916 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150601/ea2361a4/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: all.patch
Type: application/octet-stream
Size: 23733 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150601/ea2361a4/attachment-0003.obj>
-------------- next part --------------


--

Besides these patches (but related):

David, you pointed out in some other thread that the objects to do
with DebugLocStream aren't particularly obvious anymore, and I
figure this makes it worse.  Once some version of the attached
patches are in, I plan to (as a follow up) clean up the API here.

In particular, I think DebugLocStream should have iterators to walk
through lightweight ListProxy objects, which have iterators to walk
through lightweight EntryProxy objects.  These would provide a nice
interface to DebugLocStream that made it look as if we had a simple
`vector<List:(entries: vector<Entry:(bytes: vector<char>)>)>` kind
of storage.

Furthermore, I suspect a Builder interface would clarify these
{start,finalize}{List,Entry}() functions, where the builders are
constructed by the "start" functions, and their destructors take
care of calling "finalize".  For exposition, something like this:

    // In collectVariableInfo():
    DebugLocStream::ListBuilder LB = DebugLocs.startList(Asm, *this);
    for (auto &E : Entries)
      E.finalize(LB, ...);

    // In DebugLocEntry::finalize():
    DebugLocStream::EntryBuilder EB = LB.startEntry();
    BufferByteStreamer Streamer = EB.getStreamer();

(0003 already does something like this for building the entries.)

Let me know what you guys think of this as well.