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 <adeason@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.

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); }''')
+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)
+
 config_host_data.set('CONFIG_MEMFD', cc.links(gnu_source_prefix + '''
   #include <sys/mman.h>
   int main(void) { return memfd_create("foo", MFD_ALLOW_SEALING); }'''))
 config_host_data.set('CONFIG_OPEN_BY_HANDLE', cc.links(gnu_source_prefix + '''
   #include <fcntl.h>
   #if !defined(AT_EMPTY_PATH)
   # error missing definition
   #else
   int main(void) { struct file_handle fh; return open_by_handle_at(0, &fh, 0); }
   #endif'''))
diff --git a/util/osdep.c b/util/osdep.c
index 7c4deda6fe..560ce9111a 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -21,20 +21,23 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 
 /* Needed early for CONFIG_BSD etc. */
 
 #ifdef CONFIG_SOLARIS
 #include <sys/statvfs.h>
+#endif
+
+#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
 
 #include "qemu-common.h"
 #include "qemu/cutils.h"
 #include "qemu/sockets.h"
 #include "qemu/error-report.h"
 #include "qemu/madvise.h"
-- 
2.11.0

On Mon, 14 Mar 2022 21:20:23 -0500
Andrew Deason <adeason@sinenomine.net> wrote:

>  #ifdef CONFIG_SOLARIS
>  #include <sys/statvfs.h>
> +#endif
> +
> +#ifdef HAVE_MADVISE_MISSING_PROTOTYPE

Oh goodness, that's not the right name is it. I'll wait a little bit to
see if there are any other comments, but clearly that needs to be fixed.

(I am testing that the meson checks behave as expected (by editing out
the proto in mman.h), but I didn't run the whole build when doing that.)

-- 
Andrew Deason
adeason@sinenomine.net

On Tue, 15 Mar 2022 at 02:20, Andrew Deason <adeason@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 <adeason@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

On Tue, 15 Mar 2022 18:33:33 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> 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.

I don't think it matters much, but just to mention, the prototype is in
there, but it's deliberately hidden by some #ifdef logic for (older?)
POSIX/XPG compliance or something. I sometimes try to phrase this in a
way that reflects that, but it's hard so I probably won't care.

> > +#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__.)

Okay, but this will cause callers that call madvise() directly to
"work", even though they're not going through the qemu_madvise wrapper.
There's one area in cross-platform code you noted before, in
softmmu/physmem.c, and that does cause the same build error if the
prototype is missing. (I'm going to add another commit to make that use
the wrapper in the next patchset.)

I assume that's not a concern unless I hear otherwise; just pointing it
out.

And all other comments will be addressed; thanks.

-- 
Andrew Deason
adeason@sinenomine.net

On Tue, 15 Mar 2022 at 19:16, Andrew Deason <adeason@sinenomine.net> wrote:
>
> On Tue, 15 Mar 2022 18:33:33 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> > 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__.)
>
> Okay, but this will cause callers that call madvise() directly to
> "work", even though they're not going through the qemu_madvise wrapper.
> There's one area in cross-platform code you noted before, in
> softmmu/physmem.c, and that does cause the same build error if the
> prototype is missing. (I'm going to add another commit to make that use
> the wrapper in the next patchset.)
>
> I assume that's not a concern unless I hear otherwise; just pointing it
> out.

Yeah, that's fine. The idea is that osdep.h is where we fix up this
kind of odd system-header bug, and we do it for everywhere, because
otherwise it's too easy to forget to put in the "make this work
on the oddball platform" code where it's needed.

If you add the patch to change physmem.c, please cc: the author
of the commit that added it (commit cdfa56c551bb) -- it looks
a bit complicated so it's possible it is intentional.

-- PMM