diff mbox series

Fix section type of .eh_frame on Linux x86_64

Message ID 20200311212713.vruwfgxhgjwxibmg@google.com
State New
Headers show
Series Fix section type of .eh_frame on Linux x86_64 | expand

Commit Message

Fangrui Song March 11, 2020, 9:27 p.m. UTC
Clang since https://reviews.llvm.org/D73999 will error for the wrong
sh_type.
---
 sysdeps/unix/sysv/linux/x86_64/sigaction.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Adhemerval Zanella March 13, 2020, 12:27 p.m. UTC | #1
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).

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.

> ---
>  sysdeps/unix/sysv/linux/x86_64/sigaction.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/x86_64/sigaction.c b/sysdeps/unix/sysv/linux/x86_64/sigaction.c
> index c58a77c5c6..3b730bc9e3 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/sigaction.c
> +++ b/sysdeps/unix/sysv/linux/x86_64/sigaction.c
> @@ -80,7 +80,7 @@ asm									\
>     "	movq $" #syscall ", %rax\n"					\
>     "	syscall\n"							\
>     ".LEND_" #name ":\n"							\
> -   ".section .eh_frame,\"a\",@progbits\n"				\
> +   ".section .eh_frame,\"a\",@unwind\n"					\
>     ".LSTARTFRAME_" #name ":\n"						\
>     "	.long .LENDCIE_" #name "-.LSTARTCIE_" #name "\n"		\
>     ".LSTARTCIE_" #name ":\n"						\
>
H.J. Lu March 13, 2020, 1:12 p.m. UTC | #2
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.

> 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.
Adhemerval Zanella March 13, 2020, 2:04 p.m. UTC | #3
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.
> 

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.
H.J. Lu March 13, 2020, 2:33 p.m. UTC | #4
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.
> >
>
> 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.
Fangrui Song March 13, 2020, 4:07 p.m. UTC | #5
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.

>> 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.
H.J. Lu March 13, 2020, 4:18 p.m. UTC | #6
On Fri, Mar 13, 2020 at 9:07 AM Fangrui Song <maskray@google.com> 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.
>
> >> 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.

I guess it was for Solaris.
Adhemerval Zanella March 13, 2020, 4:33 p.m. UTC | #7
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).

> 
>>> 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.
Fangrui Song March 13, 2020, 4:56 p.m. UTC | #8
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.
H.J. Lu March 13, 2020, 6:06 p.m. UTC | #9
On Fri, Mar 13, 2020 at 9:56 AM Fangrui Song <maskray@google.com> wrote:
>
> 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.

https://groups.google.com/forum/#!topic/x86-64-abi/7sr4E6THl3g

> 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).
>

Agree.
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/x86_64/sigaction.c b/sysdeps/unix/sysv/linux/x86_64/sigaction.c
index c58a77c5c6..3b730bc9e3 100644
--- a/sysdeps/unix/sysv/linux/x86_64/sigaction.c
+++ b/sysdeps/unix/sysv/linux/x86_64/sigaction.c
@@ -80,7 +80,7 @@  asm									\
    "	movq $" #syscall ", %rax\n"					\
    "	syscall\n"							\
    ".LEND_" #name ":\n"							\
-   ".section .eh_frame,\"a\",@progbits\n"				\
+   ".section .eh_frame,\"a\",@unwind\n"					\
    ".LSTARTFRAME_" #name ":\n"						\
    "	.long .LENDCIE_" #name "-.LSTARTCIE_" #name "\n"		\
    ".LSTARTCIE_" #name ":\n"						\