[PATCH] D82129: [DebugInfo] Drop location ranges for variables which exist entirely outside the variable's scope

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

@djtodoro, @dblaikie, thank you both for your comments.

In D82129#2102344 <https://reviews.llvm.org/D82129#2102344>, @dblaikie wrote:

> Thanks for working this up/sending it out.
>
> The textual description doesn't /sound/ like it handles partial overlap ("If the entry opens a location range which does not intersect any of the variable's scope's ranges then we mark it for removal.") how are they handled?


Partial overlaps are ignored for now. i.e. if the location range partially overlaps a scope range then we do not drop it.

> Are locations trimmed down to match the scope range too?

Not in this patch - I have a TODO comment in there for exactly that though. The change required would be somewhat invasive so I thought it best to leave it for now.

> I'm guessing they're actually covered and tested - overlap that extends beyond the end or start of the scope, etc.

They've been considered and handled in the code, but my test has room for improvement here.

> Personally: I'm marginally in favor. Though I wouldn't mind seeing more data about cases where this turns up (which should be easier to find with this prototype) to see if they're readily fixable bugs.

I don't think these numbers are what you had in mind, but I had them to hand, and may be interesting?

  clang-3.4 RelWithDebInfo -trim-var-locs=true build
  variables analysed                3693356
  variables with dropped locations  512874   (13.89% of variables)  
  locations analysed                8798121
  locations dropped                 665043   (7.56% of locations)

> Oh, also: did you try running llvm-dwarfdump statistics before/after? I believe it tracks number of bytes of variable location relative to the enclosing scope. Assuming it doesn't count bytes of variable location extending beyond the enclosing scope (if it does, that bug should be fixed - maybe flagging those "extra bytes" in a separate penalty bucket - and could have a separate penalty bucket for cases where a single location could've been used but a location list was used instead (either because of these extended scopes - or otherwise)) - the number of covered bytes should remain the same before/after this patch? Does it?

Good idea. I tried this just now and was alarmed to see a difference in scope bytes covered! But then I had a look at the statistics code and IIUC it looks like no distinction is made between which scope the bytes cover:

  in llvm/tools/llvm-dwarfdump/Statistics.cpp
  
   ...
   @ https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-dwarfdump/Statistics.cpp#L280
      for (auto Entry : *Loc) {
        uint64_t BytesEntryCovered = Entry.Range->HighPC - Entry.Range->LowPC;
        BytesCovered += BytesEntryCovered;
        if (IsEntryValue(Entry.Expr))
          BytesEntryValuesCovered += BytesEntryCovered;
      }
  ...
  @ https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-dwarfdump/Statistics.cpp#L317
  // Turns out we have a lot of ranges that extend past the lexical scope.
  GlobalStats.ScopeBytesCovered += std::min(BytesInScope, BytesCovered);


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82129/new/

https://reviews.llvm.org/D82129