[LLDB] [PECOFF] Factorize mapping section names to types using StringSwitch. NFCI.

[mstorsjo]

Summary

Keep the existing special cases based on combinations of section name, flags and sizes/offsets.

Some of those special cases have been added intentionally, with test cases, rather recently, so I don't want to try to generalize them at the moment.

[labath]

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

864–866

This makes pretty weird control flow. I think it would be way clearer if all of this code were moved into a function like GetSectionType (there's a function like that in ObjectFileELF). Then you can use return statements to shortcut control flow, like so:

if (m_sect_headers[idx].flags & llvm::COFF::IMAGE_SCN_CNT_CODE &&
          ((const_sect_name == g_code_sect_name) ||
           (const_sect_name == g_CODE_sect_name)))
  return eSectionTypeCode;
if (...)
  return eSectionTypeZeroFill;
SectionType type = StringSwitch<SectionType>(name)...;
if (type != Invalid)
  return type;
...

[amccarth]

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

864–866

+1. Factoring the decision tree into a separate function is part of what I was trying to ask for in the patch. That it simplifies things with early returns is an added bonus.

[mstorsjo]

Split the code into a separate function as suggested.

[labath]

Thanks for taking the time to do this. Just get rid of the elses and this is good to go.

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

803–804

Now that this is a return, you don't need the else as per http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return.

[mstorsjo]

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

803–804

Ok. What about the individual if/return/else/return within the if statements? Is it preferred to keep them as is here, e.g.

if (condition) {
  if (othercondition)
    return eSectionTypeZeroFill;
  else
    return eSectionTypeData;
}

Or to skip the inner else in the same way?

if (condition) {
  if (othercondition)
    return eSectionTypeZeroFill;
  return eSectionTypeData;
}

(Ternary operators for keeping just a single return here would be bad for readability IMO, as some conditions aren't entirely trivial.)

[labath]

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

803–804

Strictly speaking, those should not use else either, but I don't think that's super important here, so you can leave it as they are now if you want. I'd be fine with a ternary operator too, as those conditions are not _that_ complicated...

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

void ObjectFilePECOFF::CreateSections(SectionList &unified_section_list) {
   ModuleSP module_sp(GetModule());
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
 
     SectionSP header_sp = std::make_shared<Section>(
         module_sp, this, ~user_id_t(0), ConstString("PECOFF header"),
         eSectionTypeOther, m_coff_header_opt.image_base,
         m_coff_header_opt.header_size,
         /*file_offset*/ 0, m_coff_header_opt.header_size,
         m_coff_header_opt.sect_alignment,
         /*flags*/ 0);
     header_sp->SetPermissions(ePermissionsReadable);
     m_sections_up->AddSection(header_sp);
     unified_section_list.AddSection(header_sp);
 
     const uint32_t nsects = m_sect_headers.size();
     ModuleSP module_sp(GetModule());
     for (uint32_t idx = 0; idx < nsects; ++idx) {
-      ConstString const_sect_name(GetSectionName(m_sect_headers[idx]));
+      llvm::StringRef sect_name = GetSectionName(m_sect_headers[idx]);
+      ConstString const_sect_name(sect_name);
       static ConstString g_code_sect_name(".code");
       static ConstString g_CODE_sect_name("CODE");
       static ConstString g_data_sect_name(".data");
       static ConstString g_DATA_sect_name("DATA");
       static ConstString g_bss_sect_name(".bss");
       static ConstString g_BSS_sect_name("BSS");
-      static ConstString g_debug_sect_name(".debug");
-      static ConstString g_reloc_sect_name(".reloc");
-      static ConstString g_stab_sect_name(".stab");
-      static ConstString g_stabstr_sect_name(".stabstr");
-      static ConstString g_sect_name_dwarf_debug_abbrev(".debug_abbrev");
-      static ConstString g_sect_name_dwarf_debug_aranges(".debug_aranges");
-      static ConstString g_sect_name_dwarf_debug_frame(".debug_frame");
-      static ConstString g_sect_name_dwarf_debug_info(".debug_info");
-      static ConstString g_sect_name_dwarf_debug_line(".debug_line");
-      static ConstString g_sect_name_dwarf_debug_loc(".debug_loc");
-      static ConstString g_sect_name_dwarf_debug_loclists(".debug_loclists");
-      static ConstString g_sect_name_dwarf_debug_macinfo(".debug_macinfo");
-      static ConstString g_sect_name_dwarf_debug_names(".debug_names");
-      static ConstString g_sect_name_dwarf_debug_pubnames(".debug_pubnames");
-      static ConstString g_sect_name_dwarf_debug_pubtypes(".debug_pubtypes");
-      static ConstString g_sect_name_dwarf_debug_ranges(".debug_ranges");
-      static ConstString g_sect_name_dwarf_debug_str(".debug_str");
-      static ConstString g_sect_name_dwarf_debug_types(".debug_types");
-      static ConstString g_sect_name_eh_frame(".eh_frame");
-      static ConstString g_sect_name_go_symtab(".gosymtab");
-      SectionType section_type = eSectionTypeOther;
+      SectionType section_type =
+          llvm::StringSwitch<SectionType>(sect_name)
+              .Case(".debug", eSectionTypeDebug)
+              .Case(".stabstr", eSectionTypeDataCString)
+              .Case(".reloc", eSectionTypeOther)
+              .Case(".debug_abbrev", eSectionTypeDWARFDebugAbbrev)
+              .Case(".debug_aranges", eSectionTypeDWARFDebugAranges)
+              .Case(".debug_frame", eSectionTypeDWARFDebugFrame)
+              .Case(".debug_info", eSectionTypeDWARFDebugInfo)
+              .Case(".debug_line", eSectionTypeDWARFDebugLine)
+              .Case(".debug_loc", eSectionTypeDWARFDebugLoc)
+              .Case(".debug_loclists", eSectionTypeDWARFDebugLocLists)
+              .Case(".debug_macinfo", eSectionTypeDWARFDebugMacInfo)
+              .Case(".debug_names", eSectionTypeDWARFDebugNames)
+              .Case(".debug_pubnames", eSectionTypeDWARFDebugPubNames)
+              .Case(".debug_pubtypes", eSectionTypeDWARFDebugPubTypes)
+              .Case(".debug_ranges", eSectionTypeDWARFDebugRanges)
+              .Case(".debug_str", eSectionTypeDWARFDebugStr)
+              .Case(".debug_types", eSectionTypeDWARFDebugTypes)
+              .Case(".eh_frame", eSectionTypeEHFrame)
+              .Case(".gosymtab", eSectionTypeGoSymtab)
+              .Default(eSectionTypeInvalid);
       if (m_sect_headers[idx].flags & llvm::COFF::IMAGE_SCN_CNT_CODE &&
           ((const_sect_name == g_code_sect_name) ||
            (const_sect_name == g_CODE_sect_name))) {
         section_type = eSectionTypeCode;
       } else if (m_sect_headers[idx].flags &
                      llvm::COFF::IMAGE_SCN_CNT_INITIALIZED_DATA &&
                  ((const_sect_name == g_data_sect_name) ||
                   (const_sect_name == g_DATA_sect_name))) {
         if (m_sect_headers[idx].size == 0 && m_sect_headers[idx].offset == 0)
           section_type = eSectionTypeZeroFill;
         else
           section_type = eSectionTypeData;
       } else if (m_sect_headers[idx].flags &
                      llvm::COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA &&
                  ((const_sect_name == g_bss_sect_name) ||
                   (const_sect_name == g_BSS_sect_name))) {
         if (m_sect_headers[idx].size == 0)
           section_type = eSectionTypeZeroFill;
         else
           section_type = eSectionTypeData;
-      } else if (const_sect_name == g_debug_sect_name) {
-        section_type = eSectionTypeDebug;
-      } else if (const_sect_name == g_stabstr_sect_name) {
-        section_type = eSectionTypeDataCString;
-      } else if (const_sect_name == g_reloc_sect_name) {
-        section_type = eSectionTypeOther;
-      } else if (const_sect_name == g_sect_name_dwarf_debug_abbrev)
-        section_type = eSectionTypeDWARFDebugAbbrev;
-      else if (const_sect_name == g_sect_name_dwarf_debug_aranges)
-        section_type = eSectionTypeDWARFDebugAranges;
-      else if (const_sect_name == g_sect_name_dwarf_debug_frame)
-        section_type = eSectionTypeDWARFDebugFrame;
-      else if (const_sect_name == g_sect_name_dwarf_debug_info)
-        section_type = eSectionTypeDWARFDebugInfo;
-      else if (const_sect_name == g_sect_name_dwarf_debug_line)
-        section_type = eSectionTypeDWARFDebugLine;
-      else if (const_sect_name == g_sect_name_dwarf_debug_loc)
-        section_type = eSectionTypeDWARFDebugLoc;
-      else if (const_sect_name == g_sect_name_dwarf_debug_loclists)
-        section_type = eSectionTypeDWARFDebugLocLists;
-      else if (const_sect_name == g_sect_name_dwarf_debug_macinfo)
-        section_type = eSectionTypeDWARFDebugMacInfo;
-      else if (const_sect_name == g_sect_name_dwarf_debug_names)
-        section_type = eSectionTypeDWARFDebugNames;
-      else if (const_sect_name == g_sect_name_dwarf_debug_pubnames)
-        section_type = eSectionTypeDWARFDebugPubNames;
-      else if (const_sect_name == g_sect_name_dwarf_debug_pubtypes)
-        section_type = eSectionTypeDWARFDebugPubTypes;
-      else if (const_sect_name == g_sect_name_dwarf_debug_ranges)
-        section_type = eSectionTypeDWARFDebugRanges;
-      else if (const_sect_name == g_sect_name_dwarf_debug_str)
-        section_type = eSectionTypeDWARFDebugStr;
-      else if (const_sect_name == g_sect_name_dwarf_debug_types)
-        section_type = eSectionTypeDWARFDebugTypes;
-      else if (const_sect_name == g_sect_name_eh_frame)
-        section_type = eSectionTypeEHFrame;
-      else if (const_sect_name == g_sect_name_go_symtab)
-        section_type = eSectionTypeGoSymtab;
-      else if (m_sect_headers[idx].flags & llvm::COFF::IMAGE_SCN_CNT_CODE) {
+      } else if (section_type != eSectionTypeInvalid) {
+        // If the StringSwitch above picked any type, including
+        // eSectionTypeOther, accept that instead of the generic mappings
+        // based on flags below.
+      } else if (m_sect_headers[idx].flags & llvm::COFF::IMAGE_SCN_CNT_CODE) {
         section_type = eSectionTypeCode;
       } else if (m_sect_headers[idx].flags &
                  llvm::COFF::IMAGE_SCN_CNT_INITIALIZED_DATA) {
         section_type = eSectionTypeData;
       } else if (m_sect_headers[idx].flags &
                  llvm::COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA) {
         if (m_sect_headers[idx].size == 0)
           section_type = eSectionTypeZeroFill;
         else
           section_type = eSectionTypeData;
+      } else {
+        // The StringSwitch didn't pick anything, and the custom cases above
+        // didn't trigger either - set eSectionTypeOther as default.
+        section_type = eSectionTypeOther;
       }
 
       SectionSP section_sp(new Section(
           module_sp,       // Module to which this section belongs
           this,            // Object file to which this section belongs
           idx + 1,         // Section ID is the 1 based section index.
           const_sect_name, // Name of this section
           section_type,