[Lldb-commits] [PATCH] D34613: Add debug_frame section support

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Symbol/DWARFCallFrameInfo.h:38
   DWARFCallFrameInfo(ObjectFile &objfile, lldb::SectionSP &section,
                      lldb::RegisterKind reg_kind, bool is_eh_frame);
 
----------------
clayborg wrote:
> labath wrote:
> > clayborg wrote:
> > > Remove "reg_kind" and "is_eh_frame" and replace with DWARFCallFrameInfo::Type. See inlined comment below by the CFIVersion enum.
> > I think the enum could be used more prominently internally (I haven't checked that more closely yet). However, I think that using it here is wrong. The reason is that the decision which debug_frame version we are parsing does not happen at this level. This is a per-CIE property -- the same debug_frame section can contain CIE's with different version numbers (e.g. if they were compiled with different compilers or flags -- in fact, that's how I built my test module). At this level, all we need to know is whether we are parsing an eh_frame or debug_frame section, which is a still boolean value.
> > 
> > Theoretically, we could have a separate two-valued (eh, dwarf) enum here, and then, internally, when speaking about a specific CIE, use the 4-valued enum you proposed.
> > 
> > The register kind argument could probably be removed though.
> Ah, interesting. Might be nice to make a DWARF/EHFrame enum later to make the code more readable when DWARFCallFrameInfo are created. No need to do that now if you need to get this in. 
Thanks for the quick review. I'm in a moderate hurry, as an android compiler change caused us to fail to unwind on x86 without debug_frame, but I'll spin up a separate patch for that.


https://reviews.llvm.org/D34613