Re: [PATCH v4 00/10] linux-user: Detect and report host crashes

[Helge Deller]

On 9/19/23 09:40, Michael Tokarev wrote:
> 18.09.2023 17:05, Helge Deller wrote:
> > On 9/12/23 12:34, Michael Tokarev wrote:
> > > 12.09.2023 12:45, Helge Deller:
> > >
> > > > /usr/bin/ld: 
> > > > /usr/lib/gcc/x86_64-redhat-linux/13/../../../../lib64/libc.a(abort.o): in 
> > > > function `abort':
> > > > (.text.unlikely+0x0): multiple definition of `abort'; 
> > > > libqemu-aarch64-linux-user.fa.p/linux-user_signal.c.o:/srv/_build/../../home/cvs/qemu/qemu/linux-user/signal.c:723:
> > > >  first defined here
> > >
> > > [PATCH v4 03/10] linux-user: Use die_with_signal with abort
> > >
> > > Sigh.
> > >
> > > I'd be double-cautious when overriding system functions like this, - it's
> > > almost always a bad idea.
> >
> > Richard, I'm not sure, but with that change:
> >
> > -void abort(void)
> > +void  __attribute__((weak)) abort(void)
> >
> > it will at least successfully link the binary. Not sure which effects it has,
> > but probably not worse than before your patch series...
>
> With weak here, ld should peak abort() from glibc, basically reverting this
> patch:
>
> linux-user: Use die_with_signal with abort
> +/*
> + * The system abort() will raise SIGABRT, which will get caught and deferred
> + * by host_signal_handler.  Returning into system abort will try harder.
> + * Eventually, on x86, it will execute HLT, which raises SIGSEGV.  This goes
> + * back into host_signal_handler, through a different path which may longjmp
> + * back to the main loop.  This often explodes.
> + */
> +void abort(void)
> +{
> +    die_with_signal(SIGABRT);
> +}
> +
>
> I think it's better to revert it now.

Maybe.
I only did static compile builds, and not even for all platforms.
Since I assume Richard did test it in his builds (which probably were
linux-user built as dynamic executable) seem to have worked for him,
the code probably works on those binaries.
If this function is left out on static builds (because libc.a already
provides this function), then there is no difference (aka same not-optimal
behaviour) as without this patch.
So, keeping this patch may help for dynamic linux-user executables at least.

> Probably the right solution is to use qemu_abort() (and qemu_assert() etc),
> and maybe #define abort(x) qemu_abort(x).  Even if some way to redefine
> abort like the above will work on glibc, it does not mean it will work
> on *bsd and in other contexts.

True. That's probably the better solution.

> Yes, providing its own abort() is tempting due to its simplicity.
> But it is a grey area at best.

Helge