COFF: Create a separate "section" for the file header

[labath]

Summary

In an attempt to ensure that every part of the module's memory image is
accounted for, D56537 created a special "container section" spanning the
entire image. While that seemed reasonable at the time (and it still
mostly does), it did create a problem of what to put as the "file size"
of the section, because the image is not continuous on disk, as we
generally assume (which is why I put zero there). Additionally, this
arrangement makes it unclear what kind of permissions should be assigned
to that section (which is what my next patch does).

To get around these, this patch partially reverts D56537, and goes back
to top-level sections. Instead, what I do is create a new "section" for
the object file header, which is also being loaded into memory, though
its not considered to be a section in the strictest sense. This makes it
possible to correctly assign file size section, and we can later assign
permissions to it as well.

[jankratochvil]

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

797

dropped /*file_size*/

[clayborg]

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

794

maybe name this "COFF header" or "PECOFF header"?

[labath]

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

794

good idea.

797

somehow I thought that a non-trivial argument value does not need a comment, but you're right that this is still ambiguous. Adding it back.

[labath]

Address review comments

[mstorsjo]

Looks and sounds good to me, although I'm not familiar with this code and its context yet - so I'm not sure if I'm qualified to formally approve it.

[mstorsjo]

I've happened to poke around a bit more in this piece of code now recently, and it does indeed seem reasonable.

[amccarth]

I'm OK with this. I'm just wondering whether it would be a good idea to make it clear that these header sections are "not considered to be a section in the strictest sense." Is the distinction going to be important to future code readers? Do we already have "sections" that aren't truly "sections"?

Perhaps just a comment where the header sections are created?

[labath]

In D69100#1717251, @amccarth wrote:

I'm OK with this. I'm just wondering whether it would be a good idea to make it clear that these header sections are "not considered to be a section in the strictest sense." Is the distinction going to be important to future code readers? Do we already have "sections" that aren't truly "sections"?

Perhaps just a comment where the header sections are created?

Both ELF and MachO have the concept of a "segment", which is the thing which describes how objects are loaded into memory. These are also represented as "sections" in lldb (and real sections are their "subsections"). However, this is the only case where we're inventing a "section" out of thin air, but that seems reasonable as neither elf nor mach-o have a concept of a piece of file which is loaded into memory, but is not a part of any section/segment.

So yeah, sure, adding a comment sounds like a good idea.

[labath]

Committed as 73a7a55.

source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

void ObjectFilePECOFF::CreateSections(SectionList &unified_section_list) {
   if (m_sections_up)
     return;
   m_sections_up.reset(new SectionList());
 
   ModuleSP module_sp(GetModule());
   if (module_sp) {
     std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
 
-    SectionSP image_sp = std::make_shared<Section>(
-        module_sp, this, ~user_id_t(0), ConstString(), eSectionTypeContainer,
-        m_coff_header_opt.image_base, m_coff_header_opt.image_size,
-        /*file_offset*/ 0, /*file_size*/ 0, m_coff_header_opt.sect_alignment,
+    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);
-    m_sections_up->AddSection(image_sp);
-    unified_section_list.AddSection(image_sp);
+    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]));
       static ConstString g_code_sect_name(".code");
       static ConstString g_CODE_sect_name("CODE");
       static ConstString g_data_sect_name(".data");
for (uint32_t idx = 0; idx < nsects; ++idx) {
                  llvm::COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA) {
         if (m_sect_headers[idx].size == 0)
           section_type = eSectionTypeZeroFill;
         else
           section_type = eSectionTypeData;
       }
 
       SectionSP section_sp(new Section(
-          image_sp,        // Parent 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,
+          m_coff_header_opt.image_base +
               m_sect_headers[idx].vmaddr, // File VM address == addresses as
                                           // they are found in the object file
-          m_sect_headers[idx].vmsize, // VM size in bytes of this section
+          m_sect_headers[idx].vmsize,     // VM size in bytes of this section
           m_sect_headers[idx]
               .offset, // Offset to the data for this section in the file
           m_sect_headers[idx]
               .size, // Size in bytes of this section as found in the file
           m_coff_header_opt.sect_alignment, // Section alignment
           m_sect_headers[idx].flags));      // Flags for this section
 
-      image_sp->GetChildren().AddSection(std::move(section_sp));
+      m_sections_up->AddSection(section_sp);
+      unified_section_list.AddSection(section_sp);
     }
   }
 }
 
 UUID ObjectFilePECOFF::GetUUID() {
   if (m_uuid.IsValid())
     return m_uuid;
 

test/Shell/ObjectFile/PECOFF/export-dllfunc.yaml

 # REQUIRES: lld
 # RUN: yaml2obj < %s > %t.obj
 #
 # RUN: lld-link /machine:x64 /out:%t.dll /noentry /nodefaultlib /debug /dll %t.obj /export:DllFunc
 #
 # RUN: lldb-test object-file %t.dll | FileCheck -check-prefix=BASIC-CHECK %s
 # RUN: lldb-test object-file -dep-modules %t.dll | FileCheck -check-prefix=DEPS %s
 
 # BASIC-CHECK: Plugin name: pe-coff
 
 # UUID should not be empty if the module is built with debug info.
 # BASIC-CHECK-DAG: UUID: {{[0-9A-F]{7,}[0-9A-F]}}-{{.*}}
 
-# BASIC-CHECK: Showing 3 subsections
+# BASIC-CHECK: Showing 4 sections
+#
 # BASIC-CHECK:  Index: 0
+# BASIC-CHECK:  Name: PECOFF header
+#
+# BASIC-CHECK:  Index: 1
 # BASIC-CHECK:  Name: .text
 # BASIC-CHECK:  Type: code
 # BASIC-CHECK:  VM size: 22
 # BASIC-CHECK:  File size: 512
 #
-# BASIC-CHECK:  Index: 1
+# BASIC-CHECK:  Index: 2
 # BASIC-CHECK:  Name: .rdata
 # BASIC-CHECK:  Type: data
 # BASIC-CHECK:  VM size: {{.}}
 # BASIC-CHECK:  File size: 512
 #
-# BASIC-CHECK:  Index: 2
+# BASIC-CHECK:  Index: 3
 # BASIC-CHECK:  Name: .pdata
 # BASIC-CHECK:  Type: data
 # BASIC-CHECK:  VM size: 12
 # BASIC-CHECK:  File size: 512
 #
 # DEPS: Showing 0 dependent module(s)
 
 

test/Shell/ObjectFile/PECOFF/sections.yaml

 # RUN: yaml2obj %s > %t
 # RUN: lldb-test object-file %t | FileCheck %s
 
 
-# CHECK:      Showing 1 sections
+# CHECK:      Showing 3 sections
 # CHECK-NEXT:   Index: 0
 # CHECK-NEXT:   ID: 0xffffffffffffffff
-# CHECK-NEXT:   Name:
-# CHECK-NEXT:   Type: container
+# CHECK-NEXT:   Name: PECOFF header
+# CHECK-NEXT:   Type: regular
 # CHECK-NEXT:   Permissions: ---
 # CHECK-NEXT:   Thread specific: no
 # CHECK-NEXT:   VM address: 0x40000000
-# CHECK-NEXT:   VM size: 12288
-# CHECK-NEXT:   File size: 0
-# CHECK-NEXT:   Showing 2 subsections
-# CHECK-NEXT:     Index: 0
-# CHECK-NEXT:     ID: 0x1
-# CHECK-NEXT:     Name: .text
-# CHECK-NEXT:     Type: code
-# CHECK-NEXT:     Permissions: ---
-# CHECK-NEXT:     Thread specific: no
-# CHECK-NEXT:     VM address: 0x40001000
-# CHECK-NEXT:     VM size: 64
-# CHECK-NEXT:     File size: 512
-# CHECK-EMPTY: 
-# CHECK-NEXT:     Index: 1
-# CHECK-NEXT:     ID: 0x2
-# CHECK-NEXT:     Name: .data
-# CHECK-NEXT:     Type: data
-# CHECK-NEXT:     Permissions: ---
-# CHECK-NEXT:     Thread specific: no
-# CHECK-NEXT:     VM address: 0x40002000
-# CHECK-NEXT:     VM size: 64
-# CHECK-NEXT:     File size: 512
+# CHECK-NEXT:   VM size: 512
+# CHECK-NEXT:   File size: 512
+# CHECK-EMPTY: 
+# CHECK-NEXT:   Index: 1
+# CHECK-NEXT:   ID: 0x1
+# CHECK-NEXT:   Name: .text
+# CHECK-NEXT:   Type: code
+# CHECK-NEXT:   Permissions: ---
+# CHECK-NEXT:   Thread specific: no
+# CHECK-NEXT:   VM address: 0x40001000
+# CHECK-NEXT:   VM size: 64
+# CHECK-NEXT:   File size: 512
+# CHECK-EMPTY:
+# CHECK-NEXT:   Index: 2
+# CHECK-NEXT:   ID: 0x2
+# CHECK-NEXT:   Name: .data
+# CHECK-NEXT:   Type: data
+# CHECK-NEXT:   Permissions: ---
+# CHECK-NEXT:   Thread specific: no
+# CHECK-NEXT:   VM address: 0x40002000
+# CHECK-NEXT:   VM size: 64
+# CHECK-NEXT:   File size: 512
 
 
 --- !COFF
 OptionalHeader:
   AddressOfEntryPoint: 4616
   ImageBase:       1073741824
   SectionAlignment: 4096
   FileAlignment:   512