Re: [PATCH] Fix the bug of multiple cores print

[Corinna Vinschen <vinschen at redhat dot com>]

On Jan 11 22:01, Andrew Pinski wrote:
> On Mon, Jan 11, 2016 at 6:17 PM, Zaibo Xu <xuzaibo@huawei.com> wrote:
> > 1.  _VOID _DEFUN(__sinit, (s), struct _reent *s) in ./newlib/libc/stdio/findfp.c.
> > 2. struct _reent in ./newlib/libc/include/sys/reent.h
> > Reasons: We have fount this bug in debuging the function of printf on
> > Sandybridge of Intel as multiple cores print to the cluster communication
> > port at the same time. After the mending above, this bug is fixed.
> >
> > Signed-off-by: Zaibo Xu <xuzaibo@huawei.com>
> > ---
> >  newlib/libc/include/sys/reent.h | 4 ++--
> >  newlib/libc/stdio/findfp.c      | 3 +++
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/newlib/libc/include/sys/reent.h b/newlib/libc/include/sys/reent.h
> > index 5481ca2..7b882fa 100644
> > --- a/newlib/libc/include/sys/reent.h
> > +++ b/newlib/libc/include/sys/reent.h
> > @@ -382,7 +382,7 @@ struct _reent
> >
> >    char *_emergency;
> >
> > -  int __sdidinit;              /* 1 means stdio has been init'd */
> > +  volatile int __sdidinit;             /* 1 means stdio has been init'd */
> >
> >    int _current_category;       /* unused */
> >    _CONST char *_current_locale;        /* unused */
> > @@ -580,7 +580,7 @@ struct _reent
> >    int _current_category;       /* used by setlocale */
> >    _CONST char *_current_locale;
> >
> > -  int __sdidinit;              /* 1 means stdio has been init'd */
> > +  volatile int __sdidinit;             /* 1 means stdio has been init'd */
> >
> >    void _EXFNPTR(__cleanup, (struct _reent *));
> >
> > diff --git a/newlib/libc/stdio/findfp.c b/newlib/libc/stdio/findfp.c
> > index 975a855..460809a 100644
> > --- a/newlib/libc/stdio/findfp.c
> > +++ b/newlib/libc/stdio/findfp.c
> > @@ -210,6 +210,7 @@ _DEFUN(__sinit, (s),
> >  {
> >    __sinit_lock_acquire ();
> >
> > +  asm volatile("mfence;":::"memory");
> 
> 
> __sinit_lock_acquire  should be enough for the lock here I think.  If
> it is not then the bug is there instead of here.
> 
> >    if (s->__sdidinit)
> >      {
> >        __sinit_lock_release ();
> > @@ -254,8 +255,10 @@ _DEFUN(__sinit, (s),
> >       when the underlying fd 2 is write-only.  */
> >    std (s->_stderr, __SRW | __SNBF, 2, s);
> >
> > +  asm volatile("mfence;":::"memory");
> >    s->__sdidinit = 1;
> >
> > +  asm volatile("mfence;":::"memory");
> >    __sinit_lock_release ();
> 
> __sinit_lock_release should be enough here and if it is not then the
> bug is there instead.

Good point.  How are __sinit_lock_acquire/__sinit_lock_release defined
on that system?


Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Attachment: signature.asc
Description: PGP signature