| Message ID | 1386345276-9803-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <mtosatti@redhat.com> > > v4: s/fail/failed/ (Peter Maydell) > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Marcelo Tosatti <mtosatti@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? > + 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
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 <mtosatti@redhat.com> > > > > v4: s/fail/failed/ (Peter Maydell) > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Marcelo Tosatti <mtosatti@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.
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); + + if (sigsetjmp(sigjump, 1)) { + fprintf(stderr, "file_ram_alloc: failed to preallocate pages\n"); + exit(1); + } + + /* MAP_POPULATE silently ignores failures */ + 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; } diff --git a/qemu-options.hx b/qemu-options.hx index 8b94264..eafc022 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -228,7 +228,6 @@ STEXI Allocate guest RAM from a temporarily created file in @var{path}. ETEXI -#ifdef MAP_POPULATE DEF("mem-prealloc", 0, QEMU_OPTION_mem_prealloc, "-mem-prealloc preallocate guest memory (use with -mem-path)\n", QEMU_ARCH_ALL) @@ -237,7 +236,6 @@ STEXI @findex -mem-prealloc Preallocate memory when using -mem-path. ETEXI -#endif DEF("k", HAS_ARG, QEMU_OPTION_k, "-k language use keyboard layout (for example 'fr' for French)\n", diff --git a/vl.c b/vl.c index 8d5d874..d191f51 100644 --- a/vl.c +++ b/vl.c @@ -188,9 +188,7 @@ static int display_remote; const char* keyboard_layout = NULL; ram_addr_t ram_size; const char *mem_path = NULL; -#ifdef MAP_POPULATE int mem_prealloc = 0; /* force preallocation of physical target memory */ -#endif int nb_nics; NICInfo nd_table[MAX_NICS]; int autostart; @@ -3206,11 +3204,9 @@ int main(int argc, char **argv, char **envp) case QEMU_OPTION_mempath: mem_path = optarg; break; -#ifdef MAP_POPULATE case QEMU_OPTION_mem_prealloc: mem_prealloc = 1; break; -#endif case QEMU_OPTION_d: log_mask = optarg; break;