[llvm] [DebugInfo] getMergedLocation: match scopes based on their location (PR #132286)

[David Blaikie via llvm-commits]

dwblaikie wrote:

> > Thanks for all the details - so maybe to paraphrase/check my understanding, this boils down to: Without this patch, we chose the nearest common scope. With this patch, we choose the first scope in LHS that has the same file/line/column as any scope in RHS? (& use the shared Scope if there happens to be one, otherwise picks an arbitrary one)
> > >
> 
> Yes.
> 
> > > In the given example, the output location will be `!DILocation(line: 300, column: 4, scope: !9)`.
> > 
> > 
> > That seems like a problem/not acceptable to me, if I'm following correctly.
> > That would place the merged instruction inside the scope that belongs to the "if" branch of this block. This would be inconsistent with the general debug info location principles we have so far (that we should never create "false" coverage - ie: it would appear as if the "if" condition was true (because code "inside" it is executing) even if the condition was false). Is that the case/am I understanding this correctly?
> 
> Yes. Is it a problem considering that scope !9 refers to y.c, which is included from both branches in main.c, and it is "equivalent" to !17 in the sense of file/line/column? If we had
> 
> ```c
> if (...) {
> #include "z1.c"
> } else {
> #include "z2.c"
> }
> ```
> 
> in our example, the algorithm would produce `!DILocation(scope: !13, line: 3, column: 7)` (scope of `main()`), which does not belong to any of the branches. I thought intermediate scopes (between DILocation's scope and DISubprogram) did not matter from the debugger's prospective.

At least they matter for name lookup. For instance if inside each side of the if/else a local variable `x` was declared, then with the proposed location merging, being at that location and printing `x` would refer to the `x` in the if, not the else, with no hint that it's ambiguous. Current merging/the direction I'd consider, would ensure `x` was not valid at all, which is about the best we can do since DWARF doesn't provide a way to describe the ambiguity.

> > I /think/ it'd be OK to place this in scope !13 with perhaps a newly created/artificial DILexicalBlockFile with the shared source file/line/column on it, maybe? Though I'm not sure exactly how we could/should pick that shared one - "do any collide" seems a bit too vague to me. Perhaps a strict suffix/tail in the source location chain could be preserved? Or just the final location? Not sure...
> 
> We could try the following:
> 
> 1. Find the nearest common scope (the current mainline implementation). `Scope = GetNearestCommonScope(...)`.
> 2. If any of `L1`, `L2` or `Scope` files are different:
>    
>    1. Use the algorithm from this PR to determine `Scope2` (the nearest scope from `L2`'s chain with the common file/line/column as some scope in `L1`'s chain).
>    2. `NewScope = Scope2.clone()` (create an artificial scope), put it inside `Scope` (as we are sure it does not belong to divergent branches with respect to `L1`, `L2`).
>    3. If any of `L1`, `L2` or `NewScope` files are different, return the location of `NewScope` (a location from which `L1`, `L2` were included).
>    4. `Scope = NewScope;`
> 3. Match L1, L2 as it is done currently in the mainline, `return DILocation(Scope, Line, Col)`.

Yeah, something like that ^ seems plausible to me at the moment.

About the only thing I'm currently concerned about in that direction is the "nearest scope from `L2` with a common file/line/column as a scope in `L1`" - that algorithm doesn't obviously sound symmetric, but I guess it probably is. Is it symmetric? (like is "this_thing(L1, L2)" the same as "this_thing(L2, L1)") that'd be a nice property to have/preserve & reduce that chance this some nuanced/fussy algorithm isn't even fussier.

@SLTozer Guess you've got some context from the previous reviews - thoughts on all this?

https://github.com/llvm/llvm-project/pull/132286