[LLD] [COFF] Don't write long section names for sections that will be mapped at runtime

[mstorsjo]

Summary

Sections that will be mapped at runtime will only have the short section name available, since the string table it points into isn't mapped. Therefore prefer truncating those names over writing a long name that is unavailable at runtime.

This allows libunwind to find the .eh_frame section at runtime even if the module was built with debug info enabled.

This relates to the issue discussed in D39918.

[rnk]

I wonder if we should limit the long section name extension to sections that aren't mapped at runtime. This makes sense because if some tool cares about unmapped section contents, they're going to need to look at the file on disk, not the image in memory, and that will have a full symbol table.

Does that make sense, or should we just go with this?

[smeenai]

In D40025#924844, @rnk wrote:

I wonder if we should limit the long section name extension to sections that aren't mapped at runtime. This makes sense because if some tool cares about unmapped section contents, they're going to need to look at the file on disk, not the image in memory, and that will have a full symbol table.

Does that make sense, or should we just go with this?

I'd be in favor of that. It'd be more consistent with link's behavior as well (I believe it always truncates).

8 characters ought to be enough for anybody :)

[ruiu]

COFF/Writer.cpp

544 ↗

(On Diff #122832)

I'd prefer doing this at the very early stage instead of this late. Can you modify InputFiles.cpp so that a section with name ".eh_frame" is read as if ".eh_fram"?

I also wonder if there's a problem if we do not limit it to MinGW. Generally I'd try reducing "if (MinGW)" code, so if we can do this unconditionally, we should do that unconditionally.

[mstorsjo]

COFF/Writer.cpp

544 ↗

(On Diff #122832)

I guess that's doable as well.

What do you think about @rnk 's suggestion about always truncating it for any section that will be mapped at runtime (only write long names for sections with the flag IMAGE_SCN_MEM_DISCARDABLE, or what's the right way to find out about that)?

I don't mind removing the "if (MinGW)" if what the patch does is considered favourable in general and not just a tradeoff/workaround for this case.

[ruiu]

What do you think about @rnk 's suggestion about always truncating it for any section that will be mapped at runtime (only write long names for sections with the flag IMAGE_SCN_MEM_DISCARDABLE, or what's the right way to find out about that)?

I think that is a great idea.

[mstorsjo]

Always truncate the names of sections that will be mapped at runtime, as suggested by @rnk.

[mstorsjo]

@ruiu Any comments on this version of the patch?

[ruiu]

LGTM

COFF/Writer.cpp

548 ↗

(On Diff #122985)

Please also mention that this is different from MSVC link.exe but we found it is useful to have a section name like ".eh_fram" instead of something like "/4".

[ruiu]

With that change, LGTM.

lld/trunk/COFF/Writer.cpp

 // Write the section header to a given buffer.
 void OutputSection::writeHeaderTo(uint8_t *Buf) {
   auto *Hdr = reinterpret_cast<coff_section *>(Buf);
   *Hdr = Header;
   if (StringTableOff) {
     // If name is too long, write offset into the string table as a name.
     sprintf(Hdr->Name, "/%d", StringTableOff);
   } else {
-    assert(!Config->Debug || Name.size() <= COFF::NameSize);
+    assert(!Config->Debug || Name.size() <= COFF::NameSize ||
+           (Hdr->Characteristics & IMAGE_SCN_MEM_DISCARDABLE) == 0);
     strncpy(Hdr->Name, Name.data(),
             std::min(Name.size(), (size_t)COFF::NameSize));
   }
 }
 
 } // namespace coff
 } // namespace lld
 
if (!Config->Debug || !Config->WriteSymtab)
     return;
 
   // Name field in the section table is 8 byte long. Longer names need
   // to be written to the string table. First, construct string table.
   for (OutputSection *Sec : OutputSections) {
     StringRef Name = Sec->getName();
     if (Name.size() <= COFF::NameSize)
       continue;
+    // If a section isn't discardable (i.e. will be mapped at runtime),
+    // prefer a truncated section name over a long section name in
+    // the string table that is unavailable at runtime. This is different from
+    // what link.exe does, but finding ".eh_fram" instead of "/4" is useful
+    // to libunwind.
+    if ((Sec->getPermissions() & IMAGE_SCN_MEM_DISCARDABLE) == 0)
+      continue;
     Sec->setStringTableOff(addEntryToStringTable(Name));
   }
 
   for (ObjFile *File : ObjFile::Instances) {
     for (Symbol *B : File->getSymbols()) {
       auto *D = dyn_cast<Defined>(B);
       if (!D || D->WrittenToSymtab)
         continue;

lld/trunk/test/COFF/long-section-name.test

 # RUN: yaml2obj < %s > %t.obj
 # RUN: lld-link /debug /out:%t.exe /entry:main %t.obj
 # RUN: llvm-readobj -sections %t.exe | FileCheck %s
 
+# CHECK: Name: .eh_fram (
 # CHECK: Name: .data_long_section_name
 # CHECK: Name: .text_long_section_name
 
 --- !COFF
 header:
   Machine:         IMAGE_FILE_MACHINE_AMD64
   Characteristics: [  ]
 sections:
   - Name:            .text_long_section_name
-    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_DISCARDABLE ]
     Alignment:       4
     SectionData:     B82A000000C3
   - Name:            .data_long_section_name
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE, IMAGE_SCN_MEM_DISCARDABLE ]
+    Alignment:       4
+    SectionData:     "00"
+  - Name:            .eh_frame
     Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
     Alignment:       4
     SectionData:     "00"
 symbols:
   - Name:            "@comp.id"
     Value:           10394907
     SectionNumber:   65535
     SimpleType:      IMAGE_SYM_TYPE_NULL
- Name:            .data_long_section_name
     ComplexType:     IMAGE_SYM_DTYPE_NULL
     StorageClass:    IMAGE_SYM_CLASS_STATIC
     SectionDefinition:
       Length:          0
       NumberOfRelocations: 0
       NumberOfLinenumbers: 0
       CheckSum:        0
       Number:          0
+  - Name:            .eh_frame
+    Value:           0
+    SectionNumber:   3
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          0
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          0
   - Name:            main
     Value:           0
     SectionNumber:   1
     SimpleType:      IMAGE_SYM_TYPE_NULL
     ComplexType:     IMAGE_SYM_DTYPE_NULL
     StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
 ...