Improve error message for bad SHF_MERGE sections

[ruiu]

Summary

This patch adds a section name to error messages.

[MaskRay]

lld/ELF/InputFiles.cpp

520

Make it a bit more detailed, e.g.

section size (1337) must be a multiple of sh_entsize (4)

[ruiu]

• address review comments
• merged with a existing test file

[MaskRay]

LGTM.

lld/test/ELF/invalid/merge-writable.s

1

git mv writable-merge.s merge-writable.s

[ruiu]

lld/test/ELF/invalid/merge-writable.s

1

This file is actually already merge-writable.s. Am I missing something?

[MaskRay]

lld/test/ELF/invalid/merge-writable.s

1

There is an existing file writable-merge.s

I think we can reuse the test, though probably rename it to merge-writable.s

[grimar]

lld/test/ELF/invalid/merge-invalid-size.s

4

You should probably be able to do something like the following I think:

// ... | FileCheck %s - DFILE=%t.o
// CHECK: [[FILE]]:(.foo): SHF_MERGE section size (2) must be a multiple of sh_entsize (4)

[MaskRay]

This patch adds a section name to error messages.

The summary needs an update.

[ruiu]

lld/test/ELF/invalid/merge-invalid-size.s

4

I'm not sure if that use of -D makes things easier, so I'd like to stick with this.

[grimar]

lld/test/ELF/invalid/merge-invalid-size.s

4

Hardcoding something is indeed easier. Problem is that here you depend on the naming rules.
I.e. what if ".tmp." suffix changes to something else? Such tests will fail.
FWIW in llvm tools test cases -D is used heavily.

[ruiu]

lld/test/ELF/invalid/merge-invalid-size.s

4

Yeah, you are right, though I'm not too worried about that. If writing a full filename here is too much, I'd change this to .o:(.foo) so that it is not too picky about filenames.

[grimar]

lld/test/ELF/invalid/merge-invalid-size.s

4

That is in line with what we usually did in LLD before and sounds good to me.

lld/ELF/InputFiles.h

private:
   void initializeSections(bool ignoreComdats);
   void initializeSymbols();
   void initializeJustSymbols();
   void initializeDwarf();
   InputSectionBase *getRelocTarget(const Elf_Shdr &sec);
   InputSectionBase *createInputSection(const Elf_Shdr &sec);
   StringRef getSectionName(const Elf_Shdr &sec);
 
-  bool shouldMerge(const Elf_Shdr &sec);
+  bool shouldMerge(const Elf_Shdr &sec, StringRef name);
 
   // Each ELF symbol contains a section index which the symbol belongs to.
   // However, because the number of bits dedicated for that is limited, a
   // symbol can directly point to a section only when the section index is
   // equal to or smaller than 65280.
   //
   // If an object file contains more than 65280 sections, the file must
   // contain .symtab_shndx section. The section contains an array of

lld/ELF/InputFiles.cpp

StringRef ObjFile<ELFT>::getShtGroupSignature(ArrayRef<Elf_Shdr> sections,
   // standard, but GNU gold 1.14 (the newest version as of July 2017) or
   // older produce such sections as outputs for the -r option, so we need
   // a bug-compatibility.
   if (signature.empty() && sym.getType() == STT_SECTION)
     return getSectionName(sec);
   return signature;
 }
 
-template <class ELFT> bool ObjFile<ELFT>::shouldMerge(const Elf_Shdr &sec) {
+template <class ELFT>
+bool ObjFile<ELFT>::shouldMerge(const Elf_Shdr &sec, StringRef name) {
   // On a regular link we don't merge sections if -O0 (default is -O1). This
   // sometimes makes the linker significantly faster, although the output will
   // be bigger.
   //
   // Doing the same for -r would create a problem as it would combine sections
   // with different sh_entsize. One option would be to just copy every SHF_MERGE
   // section as is to the output. While this would produce a valid ELF file with
   // usable SHF_MERGE sections, tools like (llvm-)?dwarfdump get confused when
bool ObjFile<ELFT>::shouldMerge(const Elf_Shdr &sec, StringRef name) {
   // sh_entsize. It says that "the member [sh_entsize] contains 0 if
   // the section does not hold a table of fixed-size entries". We know
   // that Rust 1.13 produces a string mergeable section with a zero
   // sh_entsize. Here we just accept it rather than being picky about it.
   uint64_t entSize = sec.sh_entsize;
   if (entSize == 0)
     return false;
   if (sec.sh_size % entSize)
-    fatal(toString(this) +
-          ": SHF_MERGE section size must be a multiple of sh_entsize");
+    fatal(toString(this) + ":(" + name + "): SHF_MERGE section size (" +
+          Twine(sec.sh_size) + ") must be a multiple of sh_entsize (" +
+          Twine(entSize) + ")");
 
   uint64_t flags = sec.sh_flags;
   if (!(flags & SHF_MERGE))
     return false;
   if (flags & SHF_WRITE)
-    fatal(toString(this) + ": writable SHF_MERGE section is not supported");
+    fatal(toString(this) + ":(" + name +
+          "): writable SHF_MERGE section is not supported");
 
   return true;
 }
 
 // This is for --just-symbols.
 //
 // --just-symbols is a very minor feature that allows you to link your
 // output against other existing program, so that if you load both your
if (name == ".note.gnu.build-id" && config->buildId != BuildIdKind::None)
     return &InputSection::discarded;
 
   // The linker merges EH (exception handling) frames and creates a
   // .eh_frame_hdr section for runtime. So we handle them with a special
   // class. For relocatable outputs, they are just passed through.
   if (name == ".eh_frame" && !config->relocatable)
     return make<EhInputSection>(*this, sec, name);
 
-  if (shouldMerge(sec))
+  if (shouldMerge(sec, name))
     return make<MergeInputSection>(*this, sec, name);
   return make<InputSection>(*this, sec, name);
 }
 
 template <class ELFT>
 StringRef ObjFile<ELFT>::getSectionName(const Elf_Shdr &sec) {
   return CHECK(getObj().getSectionName(&sec, sectionStringTable), this);
 }

lld/test/ELF/invalid/merge-invalid-size.s

 // REQUIRES: x86
 // RUN: llvm-mc %s -o %t.o -filetype=obj -triple=x86_64-pc-linux
 // RUN: not ld.lld %t.o -o /dev/null 2>&1 | FileCheck %s
-// CHECK: SHF_MERGE section size must be a multiple of sh_entsize
+// CHECK: merge-invalid-size.s.tmp.o:(.foo): SHF_MERGE section size (2) must be a multiple of sh_entsize (4)
 
 .section .foo,"aM",@progbits,4
 .short 42

lld/test/ELF/invalid/merge-writable.s

 // REQUIRES: x86
 // RUN: llvm-mc %s -o %t.o -filetype=obj -triple=x86_64-pc-linux
 // RUN: not ld.lld %t.o -o /dev/null 2>&1 | FileCheck %s
-// CHECK: writable SHF_MERGE section is not supported
+// CHECK: merge-writable.s.tmp.o:(.foo): writable SHF_MERGE section is not supported
 
 .section .foo,"awM",@progbits,4
 .quad 0