[ELF] - Fix for PR26968 - i386 lld produces incorrect fatal error "SHF_MERGE section size must be a multiple of sh_entsize"

[grimar]

Summary

Accordind to PR26968 description,

"In a FreeBSD/i386 buildworld many objects have SHF_MERGE with sh_entsize == 0, and this shouldn't be a fatal error. "
Probably we do not want to spread the fix on different targets as looks these objects are just broken on i386.
So patch fixes it only for I386 target.

[emaste]

ELF/InputFiles.cpp

188

This is no longer true; the objects with sh_entsize == 0 were due to a bug with lld -r that has since been fixed. So we no longer encounter this issue on FreeBSD/i386. Thus, this issue comes down to a question of what should lld do if it encounters atypical / unusual input.

From http://www.sco.com/developers/gabi/1998-04-29/ch4.sheader.html, sh_entsize

contains 0 if the section does not hold a table of fixed-size entries.

https://docs.oracle.com/cd/E19120-01/open.solaris/819-0690/ggdlu/index.html suggests that mergeable string sections may have sh_entsize == 0.

In my opinion restoring rL263664 is reasonable; objects with SHF_MERGE and sh_entsize == 0 are not invalid, and the linker is free to just avoid the merge optimization in this case.

[ruiu]

ELF/InputFiles.cpp

188

It sounds like

SHF_STRINGS sections: sh_entsize of 0 is valid and should be treated as if 1.
non-SHF_STRINGS sections: sh_entsize of 0 is invalid (?) and should be treated as non-mergeable sections.

[joerg]

Are the problematic sections empty or not?

[emaste]

Are the problematic sections empty or not?

We're arguing a hypothetical here, so there's no definitive answer. But they were not empty in the case that triggered the PR, where an old lld -r generated sh_entsize == 0 sections.

[grimar]

ELF/InputFiles.cpp

188

So issue just no longer exist for FreeBSD. I assumed it is still a problem for BSD because it is not very clear from PR page, and so if it is not anymore, do we really need to do something ? Probably there are no such objects in real world ?

[emaste]

So issue just no longer exist for FreeBSD. I assumed it is still a problem for BSD because it is not very clear from PR page,

Sorry for any confusion. To be clear, this is currently not an issue in FreeBSD.

Here's what happened:

• I made these sections non-mergeable rL263660/rL263664
• The bug in lld -r was fixed
• My change was reverted in rL263944
• Rui closed the PR with the comment "handling them as non-mergeable sections is a reasonable behavior, so let's close this bug as expected"

so I re-opened the PR, because lld right now does not handle them as non-mergeable sections.

and so if it is not anymore, do we really need to do something ? Probably there are no such objects in real world ?

I believe we should restore rL263664:

• Such objects may be rare/unlikely, but it is valid to have a mergeable section with sh_entsize == 0
• Handling them as non-mergeable is explicitly valid behaviour (not merging a mergeable section is just a missed optimization)
• The cost of rL263664 in terms of code complexity, size or run time, is trivial

That said, I don't care much either way and it's fine with me if we just close the PR. We should make an explicit decision though, not have the PR closed based on a misunderstanding.

[grimar]

In D23713#522472, @emaste wrote:

I believe we should restore rL263664:

• Such objects may be rare/unlikely, but it is valid to have a mergeable section with sh_entsize == 0
• Handling them as non-mergeable is explicitly valid behaviour (not merging a mergeable section is just a missed optimization)
• The cost of rL263664 in terms of code complexity, size or run time, is trivial

That said, I don't care much either way and it's fine with me if we just close the PR. We should make an explicit decision though, not have the PR closed based on a misunderstanding.

Thanks for clarification Ed !
I agree that if we want to land something, this patch is not a option since fixes only i386,
so restore a rL263664 is probably better option, I am abadoning this one.

ELF/InputFiles.cpp

if (Sec.sh_size == 0)
     return false;
 
   uintX_t Flags = Sec.sh_flags;
   if (!(Flags & SHF_MERGE))
     return false;
   if (Flags & SHF_WRITE)
     fatal(getFilename(this) + ": writable SHF_MERGE section is not supported");
   uintX_t EntSize = Sec.sh_entsize;
+
+  // In a FreeBSD/i386 world many objects have SHF_MERGE with sh_entsize == 0.
+  if (!EntSize && Config->EMachine == EM_386)
+    return false;
   if (!EntSize || Sec.sh_size % EntSize)
     fatal(getFilename(this) +
           ": SHF_MERGE section size must be a multiple of sh_entsize");
 
   // Don't try to merge if the alignment is larger than the sh_entsize and this
   // is not SHF_STRINGS.
   //
   // Since this is not a SHF_STRINGS, we would need to pad after every entity.

test/ELF/invalid-elf.test

 # RUN: not ld.lld %p/Inputs/invalid-shstrndx.so -o %t2 2>&1 | \
 # RUN:   FileCheck --check-prefix=INVALID-SECTION-INDEX %s
 # INVALID-SECTION-INDEX: Invalid section index
 
 # RUN: not ld.lld %p/Inputs/invalid-shentsize-zero.elf -o %t2 2>&1 | \
 # RUN:   FileCheck --check-prefix=INVALID-SHENTSIZE-ZERO %s
 # INVALID-SHENTSIZE-ZERO: SHF_MERGE section size must be a multiple of sh_entsize
 
+## In a FreeBSD/i386 world many objects have SHF_MERGE with sh_entsize == 0.
+# RUN: ld.lld %p/Inputs/invalid-shentsize-zero-i386.elf -o %t2
+# RUN: llvm-readobj %t2 > /dev/null
+
 # RUN: not ld.lld %p/Inputs/invalid-multiple-eh-relocs.elf -o %t2 2>&1 | \
 # RUN:   FileCheck --check-prefix=INVALID-EH-RELOCS %s
 # INVALID-EH-RELOCS: multiple relocation sections to .eh_frame are not supported
 
 .long foo