On Mon, Dec 09, 2013 at 08:05:15PM +0100, Stefan Weil wrote:
> Hi,
>
> this patch was recently committed to git master.
>
> It now causes problems with gcc-4.7 -Werror=clobbered. See more comments
> below.
>
> Am 06.12.2013 16:54, schrieb Paolo Bonzini:
> > From: Marcelo Tosatti <mtosa...@redhat.com>
> >
> > v4: s/fail/failed/ (Peter Maydell)
> >
> > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> > Signed-off-by: Marcelo Tosatti <mtosa...@redhat.com>
> > ---
> > exec.c | 59
> > +++++++++++++++++++++++++++++++++++++++++++-----------
> > qemu-options.hx | 2 -
> > vl.c | 4 ---
> > 3 files changed, 47 insertions(+), 18 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index 95c4356..f4b9ef2 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -904,6 +904,13 @@ static long gethugepagesize(const char *path)
> > return fs.f_bsize;
> > }
> >
> > +static sigjmp_buf sigjump;
> > +
> > +static void sigbus_handler(int signal)
> > +{
> > + siglongjmp(sigjump, 1);
> > +}
> > +
> > static void *file_ram_alloc(RAMBlock *block,
> > ram_addr_t memory,
> > const char *path)
> > @@ -913,9 +920,6 @@ static void *file_ram_alloc(RAMBlock *block,
> > char *c;
> > void *area;
> > int fd;
> > -#ifdef MAP_POPULATE
> > - int flags;
> > -#endif
> > unsigned long hpagesize;
> >
> > hpagesize = gethugepagesize(path);
> > @@ -963,21 +967,52 @@ static void *file_ram_alloc(RAMBlock *block,
> > if (ftruncate(fd, memory))
> > perror("ftruncate");
> >
> > -#ifdef MAP_POPULATE
> > - /* NB: MAP_POPULATE won't exhaustively alloc all phys pages in the case
> > - * MAP_PRIVATE is requested. For mem_prealloc we mmap as MAP_SHARED
> > - * to sidestep this quirk.
> > - */
> > - flags = mem_prealloc ? MAP_POPULATE | MAP_SHARED : MAP_PRIVATE;
> > - area = mmap(0, memory, PROT_READ | PROT_WRITE, flags, fd, 0);
> > -#else
> > area = mmap(0, memory, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
> > -#endif
> > if (area == MAP_FAILED) {
> > perror("file_ram_alloc: can't mmap RAM pages");
> > close(fd);
> > return (NULL);
> > }
> > +
> > + if (mem_prealloc) {
> > + int ret, i;
> > + struct sigaction act, oldact;
> > + sigset_t set, oldset;
> > +
> > + memset(&act, 0, sizeof(act));
> > + act.sa_handler = &sigbus_handler;
> > + act.sa_flags = 0;
> > +
> > + ret = sigaction(SIGBUS, &act, &oldact);
> > + if (ret) {
> > + perror("file_ram_alloc: failed to install signal handler");
> > + exit(1);
> > + }
> > +
> > + /* unblock SIGBUS */
> > + sigemptyset(&set);
> > + sigaddset(&set, SIGBUS);
> > + pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
> > +
>
>
> The sigsetjmp instruction causes the compiler to assume that 'area'
> might be clobbered (sigsetjmp is declared to return twice, and gcc does
> not now anything about its semantics).
>
> > + if (sigsetjmp(sigjump, 1)) {
> > + fprintf(stderr, "file_ram_alloc: failed to preallocate
> > pages\n");
> > + exit(1);
> > + }
> > +
> > + /* MAP_POPULATE silently ignores failures */
>
> Yes, but why this comment in this context? Here we don't use
> MAP_POPULATE and can get SIGBUS (which is not silent). Or am I wrong?
It explains why MAP_POPULATE cannot be used.
> > + for (i = 0; i < (memory/hpagesize)-1; i++) {
> > + memset(area + (hpagesize*i), 0, 1);
> > + }
> > +
> > + ret = sigaction(SIGBUS, &oldact, NULL);
> > + if (ret) {
> > + perror("file_ram_alloc: failed to reinstall signal handler");
> > + exit(1);
> > + }
> > +
> > + pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> > + }
> > +
> > block->fd = fd;
> > return area;
> > }
>
>
> This is the warning shown by newer gcc versions:
>
> qemu/exec.c:921:11: error:
> variable ‘area’ might be clobbered by ‘longjmp’ or ‘vfork’
> [-Werror=clobbered]
>
> I want to get support for -Wextra, and -Wclobbered is part of -Wextra.
>
> Of course we could disable -Wclobbered and still use the other
> advantages of -Wextra,
> but this might hide real problems with clobbered variables in the future.
>
> Declaring the local variable 'area' to be volatile also fixes the warning.
>
> Any opinion how this problem should be solved?
>
> Thanks,
> Stefan
The warning is spurious, obviously. Don't see a problem with 'volatile' for
area pointer.
