[PATCH] Avoid empty .debug_loc entries and lists

[Adrian Prantl]

> On Jun 4, 2015, at 4:09 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Mon, Jun 1, 2015 at 5:02 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
> 
> 
> > On 2015-May-29, at 11:15, Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
> >
> >
> >> On 2015-May-27, at 17:55, David Blaikie <dblaikie at gmail.com <mailto: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 <mailto: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. 
> 
> Judging by the code, does this cause us not to create a variable if it has no locations? That's probably not quite right - even if we have no locations, it's probably a good idea/correct to describe the variable so shadowing, etc, works correctly (if someone tries to print the variable the compiler still finds the right variable but just reports that it doesn't know the location)

Even worse, when it is a function argument, omitting the variable will effectively change the function signature. (Yes, there is also the subroutine type...).

-- adrian

>  
> 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.
> 
> 
> 
> 
> --
> 
> 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.
> 

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