Re: [PATCH 1/2] util/osdep: Avoid madvise proto on modern Solaris

[Peter Maydell]

On Mon, 14 Mar 2022 at 18:18, Andrew Deason <adeason@sinenomine.net> wrote:
>
> On Mon, 14 Mar 2022 16:36:00 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
> > On Mon, 14 Mar 2022 at 16:12, Andrew Deason <adeason@sinenomine.net> wrote:
> > >  #ifdef CONFIG_SOLARIS
> > >  #include <sys/statvfs.h>
> > > +#ifndef HAVE_MADVISE_PROTO
> > >  /* See MySQL bug #7156 (http://bugs.mysql.com/bug.php?id=7156) for
> > >     discussion about Solaris header problems */
> > >  extern int madvise(char *, size_t, int);
> > >  #endif
> > > +#endif
> >
> > Rather than keeping this inside a CONFIG_SOLARIS and only doing
> > the meson.build test if targetos == sunos, I would prefer it if we
> > unconditionally determined two things in meson.build:
> >  (1) do we have madvise in the usual way? (this is what we would
> >      want CONFIG_MADVISE to be, and might even be what it actually is)
> >  (2) do we have madvise but only if we provide a prototype for it
> >      ourselves? (maybe CONFIG_MADVISE_NO_PROTO)
>
> CONFIG_MADVISE is set if we can cc.links() something that calls
> madvise(). If we're missing the prototype, that will fail with -Werror,
> but I expect succeeds otherwise. If cc.links() just uses the cflags for
> the build, then it seems like it might succeed or fail depending on
> --enable-werror.

Mmm. I think that we wrote it that way because it was a straight
translation to meson of the previous configure-script madvise
detection code. I think it is equivalent to
config_host_data.set('CONFIG_MEMALIGN', cc.has_function('memalign'))
which also effectively does a pure "does this link?" test.
So maybe we want to keep CONFIG_MEMALIGN the way it is and add
a CONFIG_MISSING_MEMALIGN_PROTOTYPE or something. I think that
this is rapidly getting out of my depth as far as meson is concerned,
though.

> I see some other tests give -Werror as an explicit
> extra argument (HAVE_BROKEN_SIZE_MAX, and something for fuzzing); should
> this be doing the same to make sure it fails for a missing prototype?
>
> Also just to mention, if we don't care about older Solaris, the
> prototype can just be unconditionally removed. It's pretty annoying to
> even try to build qemu from git on Solaris 11.4 and earlier, because
> many of the build requirements need to be installed/compiled manually
> (notably python 3.6+, but iirc also ninja, meson, etc). So I haven't
> really tried; there may be many other build issues there.

Hard to say whether we do or don't care. We have had some
mailing list threads in the past. I think we might also care
a bit about Illumos, which might still have some issues that
Solaris proper has since fixed. Sometimes it's easier to hang on
to the portability workaround than to try to find out :-)

> > Side note: do you know why CONFIG_SOLARIS includes sys/statvfs.h ?
> > Is that unrelated to madvise() ?
>
> I think so, it was added in commit 605686cd7ac, which predates madvise()
> in that file. It does look like it could be removed from a quick glance.

Yeah, I think in commit 4a1418e07bdcfaa31 in 2009 we removed kqemu
support, which was the only thing using statvfs() in that file,
but forgot to remove the #include. Since that's an entirely separate
thing from madvise, feel free to either ignore it or send a separate patch.

-- PMM