labath added inline comments.
================
Comment at: lldb/test/Shell/SymbolFile/DWARF/x86/debug_loc.s:28-29
# CHECK: Variable{{.*}}, name = "x0", {{.*}}, scope = parameter, location =
# CHECK-NEXT: [0x0000000000000000, 0x0000000000000001): DW_OP_reg5 RDI
# CHECK-NEXT: [0x0000000000000001, 0x0000000000000006): DW_OP_reg0 RAX
# CHECK: Variable{{.*}}, name = "x1", {{.*}}, scope = parameter
----------------
zequanwu wrote:
> labath wrote:
> > zequanwu wrote:
> > > labath wrote:
> > > > zequanwu wrote:
> > > > > `image dump symfile` already prints valid ranges for variables along
> > > > > with where the value is at each range.
> > > > Are you sure it does?
> > > >
> > > > I was under the impression that there are two distinct range concepts
> > > > being combined here. One is the range list member of the Variable
> > > > object (as given by `GetScopeRange` -- that's the one you're printing
> > > > now), and the other is the list of ranges hidden in the DWARFExpression
> > > > object, which come from the debug_loc(lists) section (that's the one
> > > > we've been printing so far). And that the root cause of the confusion
> > > > is the very existence of these two concepts.
> > > >
> > > > If I got it wrong, then do let me know, cause it would make things a
> > > > lot simpler if there is only one validity concept to think about.
> > > Dwarf plugin is supposed to construct the `m_scope_range` member of an
> > > Variable, but it doesn't. `scope_ranges` is empty at
> > > https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L3468.
> > >
> > > `image dump symfile` dumps the dwarf location list in `m_location` in
> > > `Variable`.
> > > The dwarf location list has more information than `m_scope_range` as it
> > > contains info about where the value is during each range. (e.g. which
> > > register the variable lives in).
> > >
> > > So, I think we need to use similar logic to construct `m_scope_range`
> > > when creating `Variable` in dwarf plugin like this
> > > https://github.com/llvm/llvm-project/blob/main/lldb/source/Expression/DWARFExpression.cpp#L145.
> > Ok, I see where you're coming from. You're essentially saying that the fact
> > that the dwarf plugin does not fill this out is a bug.
> >
> > I don't think that's the case. My interpretation was (and [[
> > https://github.com/llvm/llvm-project/blob/main/lldb/source/Symbol/Variable.cpp#L313
> > | this comment]] confirms it) that an empty range here means the entire
> > enclosing block. (Also, DWARF was for a long time the only symbol file
> > plugin, so what it does is kinda "correct by definition").
> >
> > I don't think we want to change that interpretation, as forcing a copy of
> > the range in the location list would be wasteful (it would be different if
> > this was an interface that one could query, and that the dwarf plugin could
> > implement by consulting the location list). However, since the dwarf class
> > does not actually make use of this functionality (it was [[
> > https://reviews.llvm.org/D17449 | added ]] to support DW_AT_start_scope,
> > then broken at some point, and eventually [[
> > https://reviews.llvm.org/D62302 | removed ]]), we do have some freedom in
> > defining the interactions of the two fields (if you still want to pursue
> > this, that is).
> >
> > So how about this: if the user passes the extra flag, then we print both
> > the range field (if it exists), and the *full* location list (in that
> > order, ideally). That way the output will be either `range = [a, b), [c,
> > d), location = DW_OP_reg47` or `location = [a,b) -> DW_OP_reg4, [c,d) ->
> > DW_OP_reg7`. If the dwarf plugin starts using the range field again then
> > the output will contain both fields, which will be slightly confusing, but
> > at least not misleading (and we can also change the format then).
> Oh, I think I misunderstood `m_scope_range`. It's the range list where the
> variable is valid regardless whether its value is accessible or not (valid
> range). As for `m_location` in `Variable`, it's describing the ranges where
> the value is (value range). They are not the same.
>
> So, currently how NativePDB creates local Variable's range is not correct.
> That only works when it's not optimized build such that the valid range is
> the same as the value range. It still need to create dwarf location lists to
> correctly represent the value range, but as mentioned [[
> https://reviews.llvm.org/D119508#3319113 | here ]], we need to choose a
> generic "variable location provider" interface for that.
>
> Oh, I think I misunderstood m_scope_range. It's the range list where the
> variable is valid regardless whether its value is accessible or not (valid
> range). As for m_location in Variable, it's describing the ranges where the
> value is (value range).
Yes, that was my initial assumption as well, and I think that is the only
interpretation in which it makes sense to have two sources of range information
for a variable. However, I've done some research since then, and I haven't
found any compiler or debugger which would model the program sufficiently
precisely to be able to make that distinction.
There are definite limits as to how far you can go with pdb using these
abstractions, but given they (the m_scope_range) exist, I think you could make
use of them (as you've done now), if they are sufficient for your current use
case. That said, I would definitely encourage you to create a better
abstraction for providing the location information for a variable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119963/new/
https://reviews.llvm.org/D119963
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
