[Lldb-commits] [PATCH] D70745: [LLDB] [PECOFF] Look for the truncated ".eh_fram" section name

Wed Nov 27 01:10:17 PST 2019

mstorsjo added a comment.

In D70745#1760917 <https://reviews.llvm.org/D70745#1760917>, @amccarth wrote:

> Is `.eh_frame` the only one that matters?  Should this be more general and compare `const_sect_name` to the full name and the truncated name for any known section names?

Out of the sections name I see in executables, it's only `.eh_frame` where this is relevant. LLD produces another similarly truncated section name, `.gcc_except_table` truncated to `.gcc_exc`, but LLDB doesn't look for that one.

As for doing it in general for any known section name; I think that could end up ambiguous for some of the `.debug_*` section types, like `.debug_line` and `.debug_loc` which both would end up as `.debug_l` in truncated form. Although, as those section names wouldn't be truncated in the executables, I don't think a generic check for a truncated form would hurt either.

> Although not directly relevant to this patch, it seems like a lot this cascade could be made more readable and maintainable by replacing the simple if-name-set-section_type cases were replaced with a lookup into a table (and that lookup could be the one place to check both the full name and the truncated name).

In D70745#1761259 <https://reviews.llvm.org/D70745#1761259>, @labath wrote:

> I think you should be able to write a test with a yaml2obj + `lldb-test object-file`. That's how the equivalent elf functionality is tested (see `test/Shell/ObjectFile/ELF/section-types.yaml`). It won't check that the section is actually parsed properly, but I don't think that's needed for the kind of fix you're making here.

Ok, thanks! Yes, that's exactly the kind of test I was thinking of.

> As for the long if-else cascade, the equivalent elf code was recently refactored to use llvm::StringSwitch. Doing the same here would be nice.

A StringSwitch sounds nice. Some of the cases come with extra conditions (header flags and checking section sizes) though - is it best to keep that as is, if the StringSwitch didn't match anything?

As for @amccarth's suggestion on generically checking for truncated forms, I guess that's not doable with a StringSwitch, but would require e.g. creating more of an ad-hoc table, that we could iterate over, checking for both full and truncated matches? Does that sound sensible to you?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70745