[PATCH] D85018: [WIP][POC][DebugInfo] Support for DW_AT_start_scope for scoped variables

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1817-1818
+        ScopeBeginSym = getLabelBeforeInsn(R.first);
+      // FIXME: This causes loclist emission even at `-O0` is there way
+      // we can avoid this ?
+      RegVar = cast<DbgVariable>(
----------------
dblaikie wrote:
> djtodoro wrote:
> > dblaikie wrote:
> > > SouraVX wrote:
> > > > djtodoro wrote:
> > > > > we can avoid creating `.debug_loc` list by introducing new `DbgEntity`, e.g. called `DbgScopedVariable` which is handled in a special way (by always creating a single location for it)? Or to set `isSafeForSingleLocation`  (from `DwarfDebug::buildLocationList`) to true if `ScopeBeginSym != nullptr`?
> > > > Thank you for your inputs!, I think I've been to this route(or not). Let me give it one more shot.
> > > Not sure introducing a new kind of DbgEntity is going to be the right general solution - I'd think the right general solution could only be achieved by introducing a new kind of scope - and having every variable in its own DIVariableScope (like DILexicalScope/DiLexicalScopeFile) - with the slight problem that DIVariableScopes can overlap... which would be problematic, since then a location could be in more than one scope.
> > > 
> > > I guess they could all nest, because C++/scoped languages do create explicitly nested scopes, almost... I think there are some quirks around variables declared in conditionals, but might work out OK?
> > I agree that this would address this issue locally and may be just a mask/workaround for this implementation.
> > That is neat idea, and I think we should try to generalize it that way. A "disadvantage" may be the fact we'd need to carry that metadata throughout pipeline (IR && MIR).
> > I agree that this would address this issue locally and may be just a mask/workaround for this implementation.
> 
> The first "this" here is referring to your DbgEntity suggestion?
> 
> > That is neat idea, and I think we should try to generalize it that way. A "disadvantage" may be the fact we'd need to carry that metadata throughout pipeline (IR && MIR).
> 
> Yeah, I don't think it's cheap (Either to implement, or in terms of metadata overhead) to implement - which is one of the reasons I'm fairly hesitant about the feature in general. But I do think if it's going to be implemented, I'm not sure there's really an "incremental" approach that improves the fidelity of scope_start in steps starting from this patch - I think we do have to look at what it'd take to have fairly full fidelity - and I know of no other way than having DI scopes for each variable - and I think even then I'm worried that there are some cases where they're not strictly nested inside each other, which would be extra difficult to represent.
> > I agree that this would address this issue locally and may be just a mask/workaround for this implementation.
> 
> The first "this" here is referring to your DbgEntity suggestion?
> 
Yes, sorry for the confusion.
> > That is neat idea, and I think we should try to generalize it that way. A "disadvantage" may be the fact we'd need to carry that metadata throughout pipeline (IR && MIR).
> 
> Yeah, I don't think it's cheap (Either to implement, or in terms of metadata overhead) to implement - which is one of the reasons I'm fairly hesitant about the feature in general. But I do think if it's going to be implemented, I'm not sure there's really an "incremental" approach that improves the fidelity of scope_start in steps starting from this patch - I think we do have to look at what it'd take to have fairly full fidelity - and I know of no other way than having DI scopes for each variable - and I think even then I'm worried that there are some cases where they're not strictly nested inside each other, which would be extra difficult to represent.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85018