On Tue, 15 Mar 2022 at 02:20, Andrew Deason <adea...@sinenomine.net> wrote:
>
> On older Solaris releases, we didn't get a protype for madvise, and so
> util/osdep.c provides its own prototype. Some time between the public
> Solaris 11.4 release and Solaris 11.4.42 CBE, we started getting an
> madvise prototype that looks like this:
>
> extern int madvise(void *, size_t, int);
>
> which conflicts with the prototype in util/osdeps.c. Instead of always
> declaring this prototype, check if we're missing the madvise()
> prototype, and only declare it ourselves if the prototype is missing.
>
> The 'missing_madvise_proto' meson check contains an obviously wrong
> prototype for madvise. So if that code compiles and links, we must be
> missing the actual prototype for madvise.
>
> Signed-off-by: Andrew Deason <adea...@sinenomine.net>
> ---
> To be clear, I'm okay with removing the prototype workaround
> unconditionally; I'm just not sure if there's enough consensus on doing
> that.
>
> The "missing prototype" check is based on getting a compiler error on a
> conflicting prototype, since this just seems more precise and certain
> than getting an error from a missing prototype (needs
> -Werror=missing-prototypes or -Werror). But I can do it the other way
> around if needed.
Seems a reasonable approach to me.
> Changes since v1:
> - madvise prototype check changed to not be platforms-specific, and turned
> into
> CONFIG_MADVISE_MISSING_PROTOTYPE.
>
> meson.build | 17 +++++++++++++++--
> util/osdep.c | 3 +++
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 2d6601467f..ff5fce693e 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1705,25 +1705,38 @@ config_host_data.set('CONFIG_EVENTFD', cc.links('''
> int main(void) { return eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); }'''))
> config_host_data.set('CONFIG_FDATASYNC', cc.links(gnu_source_prefix + '''
> #include <unistd.h>
> int main(void) {
> #if defined(_POSIX_SYNCHRONIZED_IO) && _POSIX_SYNCHRONIZED_IO > 0
> return fdatasync(0);
> #else
> #error Not supported
> #endif
> }'''))
> -config_host_data.set('CONFIG_MADVISE', cc.links(gnu_source_prefix + '''
> +
> +has_madvise = cc.links(gnu_source_prefix + '''
> #include <sys/types.h>
> #include <sys/mman.h>
> #include <stddef.h>
> - int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }'''))
> + int main(void) { return madvise(NULL, 0, MADV_DONTNEED); }''')
Since this is a little bit tricky, I think a comment here will help
future readers:
# Older Solaris/Illumos provide madvise() but forget to prototype it.
# In this case has_madvise will be true (the test program links despite
# a compile warning). To detect the missing-prototype case, we try
# again with a definitely-bogus prototype. This will only compile
# if the system headers don't provide the prototype; otherwise the
# conflicting prototypes will cause a compiler error.
> +missing_madvise_proto = false
> +if has_madvise
> + missing_madvise_proto = cc.links(gnu_source_prefix + '''
> + #include <sys/types.h>
> + #include <sys/mman.h>
> + #include <stddef.h>
> + extern int madvise(int);
> + int main(void) { return madvise(0); }''')
> +endif
> +config_host_data.set('CONFIG_MADVISE', has_madvise)
> +config_host_data.set('CONFIG_MADVISE_MISSING_PROTOTYPE',
> missing_madvise_proto)
> +#ifdef HAVE_MADVISE_MISSING_PROTOTYPE
> /* 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
As you note, this doesn't match the name we picked in meson.build.
I don't feel very strongly about the name (we certainly don't manage
consistency across the project about CONFIG_ vs HAVE_ !), but my suggestion
is HAVE_MADVISE_WITHOUT_PROTOTYPE.
Can you put the prototype in include/qemu/osdep.h, please?
(Exact location not very important as long as it's inside
the extern-C block, but I suggest just under the bit where we
define SIGIO for __HAIKU__.)
This means moving the comment, which will then want fixing up to
our coding style, which these days is
/*
* line 1
* line 2
*/
for multi-line comments.
thanks
-- PMM
