Message ID | CAATtE7D_q4qd4Y9uTP-JmfJZ_zGT01HYYtKZW0L3_Q+v3TCegA@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | Fix BZ 23606 -- Missing ENDBR32 in sysdeps/i386/start.S | expand |
On Fri, Sep 7, 2018 at 12:06 AM, Terry Guo <terry.xpguo@gmail.com> wrote: > Hello, > > The patch is to fix this issue by wrapping the _start function with > ENTRY and END just like what we did for 64bit target. Tested with > glibc 32bit build, there is no regression. OK for trunk? > > BR, > Terry > > > 2018-09-07 H.J. Lu <hongjiu.lu@intel.com> > Xuepeng Guo <xuepeng.guo@intel.com> > > [BZ #23606] > * sysdeps/i386/start.S: Wrap _start function with ENTRY and END. > > > diff --git a/sysdeps/i386/start.S b/sysdeps/i386/start.S > index 91035fa83f..e35e9bd31b 100644 > --- a/sysdeps/i386/start.S > +++ b/sysdeps/i386/start.S > @@ -52,10 +52,11 @@ > NULL > */ > > - .text > - .globl _start > - .type _start,@function > -_start: > +#include <sysdep.h> > + > +ENTRY (_start) > + /* Clearing frame pointer is insufficient, use CFI. */ > + cfi_undefined (eip) > /* Clear the frame pointer. The ABI suggests this be done, to mark > the outermost frame obviously. */ > xorl %ebp, %ebp > @@ -131,6 +132,7 @@ _start: > 1: movl (%esp), %ebx > ret > #endif > +END (_start) > > /* To fulfill the System V/i386 ABI we need this symbol. Yuck, it's so > meaningless since we don't support machines < 80386. */ LGTM. Please check it in. If you don't have an account, please send me an attachment of "git format-patch". I will check it in for you. Thanks.
On 09/07/2018 09:06 AM, Terry Guo wrote: > + /* Clearing frame pointer is insufficient, use CFI. */ > + cfi_undefined (eip) Isn't this a separate fix? The ChangeLog or the commit message should mention ENDBR32 (“Use ENTRY to insert ENDBR32 when building for CET” or something like that). Thanks, Florian
On Wed, Sep 12, 2018 at 12:43 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 09/07/2018 09:06 AM, Terry Guo wrote: >> >> + /* Clearing frame pointer is insufficient, use CFI. */ >> + cfi_undefined (eip) > > > Isn't this a separate fix? Since _start now includes CFI, without "cfi_undefined (eip)", unwinder may not terminate at _start and one unwind test will fail. > The ChangeLog or the commit message should mention ENDBR32 (“Use ENTRY to > insert ENDBR32 when building for CET” or something like that). > Please also write ChangeLog as * sysdeps/i386/start.S (_start): ...
On 09/12/2018 01:57 PM, H.J. Lu wrote: > On Wed, Sep 12, 2018 at 12:43 AM, Florian Weimer <fweimer@redhat.com> wrote: >> On 09/07/2018 09:06 AM, Terry Guo wrote: >>> >>> + /* Clearing frame pointer is insufficient, use CFI. */ >>> + cfi_undefined (eip) >> >> >> Isn't this a separate fix? > > Since _start now includes CFI, without "cfi_undefined (eip)", unwinder may not > terminate at _start and one unwind test will fail. Ah! Please include this information in the commit message or ChangeLog entry, too. Thanks, Florian
On Wed, Sep 12, 2018 at 4:59 AM, Florian Weimer <fweimer@redhat.com> wrote: > On 09/12/2018 01:57 PM, H.J. Lu wrote: >> >> On Wed, Sep 12, 2018 at 12:43 AM, Florian Weimer <fweimer@redhat.com> >> wrote: >>> >>> On 09/07/2018 09:06 AM, Terry Guo wrote: >>>> >>>> >>>> + /* Clearing frame pointer is insufficient, use CFI. */ >>>> + cfi_undefined (eip) >>> >>> >>> >>> Isn't this a separate fix? >> >> >> Since _start now includes CFI, without "cfi_undefined (eip)", unwinder >> may not >> terminate at _start and one unwind test will fail. > > > Ah! Please include this information in the commit message or ChangeLog > entry, too. This is the patch I am going to check in.
On 09/12/2018 05:20 PM, H.J. Lu wrote:
> This is the patch I am going to check in.
Nice commit message and ChangeLog entry, assuming that the lines with #
survive the git comment filter. 8-)
Thanks,
Florian
diff --git a/sysdeps/i386/start.S b/sysdeps/i386/start.S index 91035fa83f..e35e9bd31b 100644 --- a/sysdeps/i386/start.S +++ b/sysdeps/i386/start.S @@ -52,10 +52,11 @@ NULL */ - .text - .globl _start - .type _start,@function -_start: +#include <sysdep.h> + +ENTRY (_start) + /* Clearing frame pointer is insufficient, use CFI. */ + cfi_undefined (eip) /* Clear the frame pointer. The ABI suggests this be done, to mark the outermost frame obviously. */ xorl %ebp, %ebp @@ -131,6 +132,7 @@ _start: 1: movl (%esp), %ebx ret #endif +END (_start) /* To fulfill the System V/i386 ABI we need this symbol. Yuck, it's so meaningless since we don't support machines < 80386. */