[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 marked an inline comment as done.
Orlando added a comment.

In D82129#2107017 <https://reviews.llvm.org/D82129#2107017>, @probinson wrote:

> This patch depends on the ranges for all scopes to be (reasonably) correct,


I'd say instead that 'variable locations depend on the ranges for all scopes to be (reasonably) correct'. And that this patch just acknowledges that relationship and clears away what we cannot use/see in a debugger.

> but I think there's one modified test where that's not the case; a variable location is dropped because (AFAICT looking at the equivalent DWARF output) its containing scope isn't pointing to the right instructions.
>  If we take the stance that these cases are bugs, then this analysis is better off done in a verifier, so we can find and fix those cases, rather than papering over compiler bugs.
> 
> I might be wrong about that test, and maybe we aren't going to take that stance, but I thought it was worth bringing up here.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DbgEntityHistoryCalculator.cpp:201
+    for (auto EI = HistoryMapEntries.begin(), EE = HistoryMapEntries.end();
+         EI != EE; ++EI, ++StartIndex) {
+      // Only DBG_VALUEs can open location ranges so skip anything else.
----------------
aprantl wrote:
> Orlando wrote:
> > aprantl wrote:
> > > this looks like it could be a range-based for loop?
> > I agree that this loop is a little bit ugly; I'm doing it this way to update `StartIndex`.
> I see. I guess that is better than 
> 
> ```
> for (auto EI : HistoryMapEntries) {
>   LLVM_DEFER { StartIndex++; };
>   ...
> ```
I wasn't aware of `LLVM_DEFER` (and I can't see it with grep?). I've left it as is for now but I'm happy to change it.


================
Comment at: llvm/test/DebugInfo/X86/trim-var-locs.mir:116
+    $edi = MOV32ri 8, debug-location !15
+    ; scoope block !24 start and end range 2
+    $edi = MOV32ri 9, debug-location !26
----------------
jmorse wrote:
> nit: scooooope
I'll update this here if I need to make other changes, or when I land it otherwise, thanks!


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

https://reviews.llvm.org/D82129