[PATCH] Fix section type of .eh_frame on Linux x86_64

[Fangrui Song]

On 2020-03-13, Adhemerval Zanella wrote:
>
>
>On 13/03/2020 13:07, Fangrui Song wrote:
>> On 2020-03-13, H.J. Lu wrote:
>>> On Fri, Mar 13, 2020 at 7:04 AM Adhemerval Zanella
>>> <adhemerval.zanella@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 13/03/2020 10:12, H.J. Lu wrote:
>>>> > On Fri, Mar 13, 2020 at 5:28 AM Adhemerval Zanella via Libc-alpha
>>>> > <libc-alpha@sourceware.org> wrote:
>>>> >>
>>>> >>
>>>> >>
>>>> >> On 11/03/2020 18:27, Fangrui Song via Libc-alpha wrote:
>>>> >>> Clang since https://reviews.llvm.org/D73999 will error for the wrong
>>>> >>> sh_type.
>>>> >>
>>>> >> It will make eh_frame section of sigaction object to have the
>>>> >> SHT_X86_64_UNWIND, but it seems that 'ld.bfd' at least ignore and
>>>> >> set the resulting eh_frame sh_type for libc.so to SHT_PROGBITS
>>>> >> anyway. The 'as' also still generate SHT_PROGBITS for code that
>>>> >> requires a eh_frame (C++ with exception handling that emits a
>>>> >> gcc_except_table section, for instance).
>>>> >
>>>> > SHT_X86_64_UNWIND was added so that linker can group all
>>>> > SHT_X86_64_UNWIND sections together without checking
>>>> > .eh_frame section name.   Other than that, linker treats .eh_frame like
>>>> > other SHT_PROGBITS sections.   Since .eh_frame is also listed as a
>>>> > special section, similar to .got and .plt, SHT_X86_64_UNWIND isn't
>>>> > really required.
>>>>
>>>> Thanks for the explanation.
>>>>
>>>> >
>>>> >> Setting the eh_frame in assembly routines is not a common practice,
>>>> >> the only other code that I could find that actually does it is
>>>> >> Linux.  For i686 vDSO is also uses 'progbits':
>>>> >>
>>>> >> arch/x86/entry/vdso/vdso32/sigreturn.S
>>>> >>
>>>> >>  36         .section .eh_frame,"a",@progbits
>>>> >>  37 .LSTARTFRAMEDLSI1:
>>>> >>
>>>> >> The change should be ok, but I would like to understand better why
>>>> >> exactly SHT_X86_64_UNWIND should be used for eh_frame, why binutils
>>>> >> does not seems to use it to eh_frame, and why clang is now not
>>>> >> accepting the eh_frame with SHT_PROGBITS type.
>>>> >>
>>>> >
>>>> > Linker should check .eh_frame section name, and optionally check
>>>> > SHT_X86_64_UNWIND.   Because of this, i386 psABI only specifies
>>>> > special .eh_frame section without SHT_386_UNWIND.
>>>> >
>>>> > So I think there is no need to change glibc and lld should be changed.
>>>> >
>>
>> clang integrated assembler is more rigid.
>> https://github.com/llvm/llvm-project/blob/master/llvm/lib/MC/MCObjectFileInfo.cpp#L488
>> .eh_frame is a pre-allocated recognized section.
>> "Redeclaring" (via .section directive) it with different sh_flags or sh_type will error.
>
>The question now is whether this specific test make sense.  From
>H.J.Lu explanation, it seems clang strictness in not allowing
>eh_frame to be defined as SHT_PROGBITS does not make much sense
>because the section will be set a SHT_PROGBITS in any case
>(even if a module defines it as SHT_X86_64_UNWIND).

https://reviews.llvm.org/D73999
The rigidness does not come from more code, but rather, a much simpler check.

   if (Section->getType() != Type)
     Error(loc, "changed section type for " + SectionName + ", expected: 0x" +
                    utohexstr(Section->getType()));
   if (Section->getFlags() != Flags)
     Error(loc, "changed section flags for " + SectionName + ", expected: 0x" +
                    utohexstr(Section->getFlags()));

I confirm that GNU as generate SHT_PROGBITS .eh_frame from .cfi* (Maybe we should teach it to generate SHT_X86_64_UNWIND instead?)
clang generates SHT_X86_64_UNWIND.

My intention is to keep the clang logic simple this way.
As another example, GNU as special cases empty section flags, but we
think the special case is not necessary:
https://github.com/ClangBuiltLinux/linux/issues/913#issuecomment-594157005

Alternatively, we can remove SHT_X86_64_UNWIND from x86-64 psABI, so every arch will use SHT_PROGBITS.
Using a non-special SHT_PROGBITS does not match ELF's spirit but a processor specific SHT_X86_64_UNWIND does not make things better (probably make things worse).

On clang/LLVM side, I'll revert https://reviews.llvm.org/rL252300 and make SHT_X86_64_UNWIND Solaris specific again.
I'll have to make `if (Section->getType() != Type)` uglier by special casing .eh_frame SHT_X86_64_UNWIND to keep
existing object files compatible.

>>
>>>> I was confused why binutils was not throwing a similar issue.  So
>>>> the idea is SHT_X86_64_UNWIND is optional on eh_frame definitions,
>>>> not mandatory, and being optional I agree that compiler/linker
>>>> should not emit an error in such case.
>>>
>>> It is unfortunate that x86-64 psABI specifies both SHT_X86_64_UNWIND
>>> and .eh_frame.  Only one is needed, not both.
>>
>> I know little about GNU as internals
>> (https://sourceware.org/legacy-ml/binutils/2020-02/msg00129.html),
>> but it seems .cfi* generated .eh_frame contents don't conflict with
>> .section declared .eh_frame
>>
>> Though, .section .eh_frame,"a",@unwind; .section .eh_frame,"a",@progbits
>> will error as well.
>>
>> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=d2b2c203e1ff2daf8cfeab0906084f9389e66246 (2004) added SHT_X86_64_UNWIND to binutils.
>> There was no further information why it was added, though.