Differential D70745
[LLDB] [PECOFF] Look for the truncated ".eh_fram" section name Authored by mstorsjo on Nov 26 2019, 2:23 PM.
Details COFF section names can either be stored truncated to 8 chars, in the section header, or as a longer section name, stored separately in the string table. libunwind locates the .eh_frame section by runtime introspection, which only works for section names stored in the section header (as the string table isn't mapped at runtime). To support this behaviour, lld always truncates the section names for sections that will be mapped, like .eh_frame.
Diff Detail
Event TimelineComment Actions 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? If the giant cascade of else-if were factored into a separate function, then a trivial unit test could verify that it returns the right section_type for a variety of full and truncated names (as well as other combinations of section headers). 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). Comment Actions 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. 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. Comment Actions 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.
Ok, thanks! Yes, that's exactly the kind of test I was thinking of.
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? Comment Actions I don't really know what are the conventions in the COFF world (you probably know that better), but I would tend to trust the section flags *more* than any name-based deductions. So I'd try to factor this such (and this is what I've done in the elf case) to do the section flag checks first, and only then fall back to the section names. The checks for the size 0 in data vs. bss sections looks weird to me, and I don't know if its really needed, but if you think it is, then it's probably best to pull that those cases out separately.
If you think it's good to check the truncated name on all sections, then yes, some kind of a table would probably be nice. If it's just one or two cases, then you can just spell out both forms in the StringSwitch (.Cases(".eh_frame", ".eh_fram", eSectionTypeEHFrame) Comment Actions Some of those special cases were added rather recently (1 year ago), so I'd rather not touch them. Checking the flags alone doesn't say much for e.g. "data" sections (that can be anything between actual data and debug info), so I didn't find much I dared to touch here, but it can at least be made some amount more readable, in D70778.
Ok, going with just a single Cases here. A table and generic logic for checking truncated forms might be nice, but I don't see a direct need right now. |