aleksandr.urakov marked 4 inline comments as done.
aleksandr.urakov added a comment.
Hi Jason, thanks for the review!
Initially, the reason of these changes was to reuse the code that works with
`eh_frame`, because on Windows we have a very similar compiler generated info
designed to help with unwinding stacks during exception handling. That's why I
have chosen `DWARFCallFrameInfo` and have extracted its interface (`I` in
`ICallFrameInfo` stands for `Interface`, but I've renamed it to `CallFrameInfo`
in the last patch, because this notation doesn't look common in LLVM).
But there is one more reason that makes it very difficult to just create
`PECallFrameInfo` in `UnwindTable` like we do with `DWARFCallFrameInfo`:
`PECallFrameInfo` is coupled very tight with `ObjectFilePECOFF` and can't work
over plain `ObjectFile`. First of all, it uses several functions for work with
relative virtual addresses (RVA), and they are implemented with the use of
`ObjectFilePECOFF`'s private variable `m_image_base` (however, may be it is
possible to implement these methods over the `ObjectFile` interface, I'm not
sure about that). The second, it requires information about exception directory
location, but it is an inner mechanics of `ObjectFilePECOFF`. So its relations
with `ObjectFilePECOFF` are not the same as between `DWARFCallFrameInfo` and
ELF or Mach-O.
To resolve the situation we can:
- use something like a `dynamic_cast` to `ObjectFilePECOFF` in `UnwindTable`
and create `PECallFrameInfo` with it. But in this case `lldbSymbol` becomes
dependent on `lldbPluginObjectFilePECOFF` (and we get a circular dependency);
- extend `ObjectFile`'s interface to make it possible for `PECallFrameInfo` to
work with required PE abstractions. In this case we can just create
`PECallFrameInfo` in `UnwindTable` like we do with `DWARFCallFrameInfo`, but it
adds weird operations to `ObjectFile`'s interface, which are not related to
other object file formats;
- allow `ObjectFile` to create its own unwind infos by himself.
I've found the last idea good, and decided to redo things in this way (because
`DWARFCallFrameInfo` etc. are working over object files too). But you are
right, it also has a negative impact on `ObjectFile`: it becomes knowing of the
things it shouldn't. So in the last patch I have left only a most abstract
method, which only allows to get some unwind info from an object file, and
introduced corresponding entities in `UnwindTable` and `FuncUnwinders`. I think
it can be a good start for moving in the direction that Greg suggests.
What do you think of this?
================
Comment at: lldb/include/lldb/Symbol/DWARFCallFrameInfo.h:74
- void ForEachFDEEntries(
- const std::function<bool(lldb::addr_t, uint32_t, dw_offset_t)>
&callback);
+ void ForEachEntries(const std::function<bool(lldb::addr_t, uint32_t)>
&callback) override;
----------------
amccarth wrote:
> I find the name `ForEachEntries` confusing. I know this is a leftover from
> the original code that you're modifying, but I wonder if it can get a better
> name. In particular, I don't know why it's plural, so I'd expect
> `ForEachEntry`, but even that is pretty vague. I realize `FDE` is
> DWARF-specific, but at least it gives a clue as to what type of entries are
> involved.
I just have removed it from the new version.
================
Comment at: lldb/source/Plugins/ObjectFile/PECOFF/PECallFrameInfo.cpp:541
+const RuntimeFunction *PECallFrameInfo::FindRuntimeFunctionItersectsWithRange(
+ const AddressRange &range) const {
+ uint32_t rva = m_object_file.GetRVA(range.GetBaseAddress());
----------------
amccarth wrote:
> Isn't it possible for more than one RuntimeFunction to intersect with an
> address range? In normal use, it might not happen because it's being called
> with constrained ranges, so maybe it's nothing to worry about. I suppose if
> the range were too large and it encompassed several functions, returning any
> one of them is acceptable.
Yes, it is possible. The problem here is that `FuncUnwinders` requests the plan
with an `AddressRange`, not with an `Address`, and the information about the
original requested address is lost to that moment. The required `AddressRange`
is calculated in `UnwindTable::GetAddressRange`, that's why the order of
querying unwind infos is important in that function. I think that this logic
requires some rework, but it seems that it is not a good idea to do it in this
patch.
================
Comment at: lldb/source/Symbol/ObjectFile.cpp:687
+ return std::make_unique<DWARFCallFrameInfo>(*this, sect,
+ DWARFCallFrameInfo::EH);
+}
----------------
amccarth wrote:
> It seems a bit weird for DWARF-specific code to be here, when there are
> ObjectFile plugins for PECOFF, ELF, and Mach-O. Obviously the
> PECOFFCallFrameInfo is instantiated in the PECOFF plugin. The ELF and Mach-O
> versions instantiate DWARFCallFrameInfos. Does the generic ObjectFile need
> to do the same?
I just have made a default implementation to avoid duplication of the code in
ELF and Mach-O plugins, but I agree, it looks weird here. It was removed in the
new implementation.
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67347/new/
https://reviews.llvm.org/D67347
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
