| Message ID | CAJ+F1CLzuv4CevvT+gM_ZOnC67_=-3EpRiQBoQXqNYDiFLBw+A@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16 January 2018 at 11:58, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Tue, Jan 16, 2018 at 12:25 PM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> On 15 January 2018 at 23:35, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> The following changes since commit 997eba28a3ed5400a80f754bf3a1c8044b75b9ff: >>> >>> Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180111' into staging (2018-01-11 14:34:41 +0000) >>> >>> are available in the git repository at: >>> >>> >>> git://github.com/bonzini/qemu.git tags/for-upstream >>> >>> for you to fetch changes up to ff9adba50bf8a4c080b8aee9be2314ef179a7b5f: >>> >>> ucontext: annotate coroutine stack for ASAN (2018-01-12 15:21:14 +0100) >>> >>> ---------------------------------------------------------------- >>> * QemuMutex tracing improvements (Alex) >>> * ram_addr_t optimization (David) >>> * SCSI fixes (Fam, Stefan, me) >>> * do {} while (0) fixes (Eric) >>> * KVM fix for PMU (Jan) >>> * memory leak fixes from ASAN (Marc-André) >>> * migration fix for HPET, icount, loadvm (Maria, Pavel) >>> * hflags fixes (me, Tao) >>> * block/iscsi uninitialized variable (Peter L.) >>> * full support for GMainContexts in character devices (Peter Xu) >>> * more boot-serial-test (Thomas) >>> * Memory leak fix (Zhecheng) >> >> Various build failures, I'm afraid: >> > > damn, sorry.. > >> x86-64/Linux/gcc: >> >> configure produces an error message: >> >> ERROR: ASAN build enabled, but ASAN header is too old. >> Without code annotation, the report may be inferior. >> >> even though this configure did not explicitly request ASAN. >> Then configure seems to exit successfully anyway, since the >> build proceeds. > > ASAN is enabled by default if available when --enable-debug. We could > add more flags if that helps. Configure switches should work like this: * default: use feature if present, but don't complain if not present or not usable * --enable-foo: use feature. if feature not present, complain and fail configure * --disable-foo: don't test for or use feature >> For some reason the build creates config-target.h a lot: > One per target no? (the build should be more silent than before) Oh, I guess that makes sense. It's a bit odd that the message neither gives the target part of the filename nor has those entering/leaving directory messages... >> >> Then it runs configure again, this time without the ERROR message, >> and eventually fails with: >> >> CC hw/display/exynos4210_fimd.o >> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c: In >> function ‘fimd_get_buffer_id’: >> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c:1105:5: >> error: case label does not reduce to an integer constant >> case FIMD_WINCON_BUF2_STAT: > > never saw that error, hmm interesting. This is related to > -fsanitize=address ? Is this on Debian stable? Ubuntu xenial (16.04.3 LTS). No idea if it's related to the sanitizer or not. > >> ^ >> >> On sparc64 host configure fails: > Hmm, I guess the check -fsanitize=address doesn't return an error > unless -Werror is given. Perhaps it needs: > > diff --git a/configure b/configure > index f5550f3289..ba68c550c9 100755 > --- a/configure > +++ b/configure > @@ -5190,7 +5190,7 @@ fi > > have_asan=no > write_c_skeleton > -if compile_prog "-fsanitize=address" ""; then > +if compile_prog "-Werror -fsanitize=address" ""; then > have_asan=yes > fi > > @@ -5207,7 +5207,7 @@ int main(void) { > return 0; > } > EOF > -if compile_prog "-fsanitize=address" "" ; then > +if compile_prog "-Werror -fsanitize=address" "" ; then > have_asan_iface_fiber=yes > fi > Looks plausible. thanks -- PMM
On 16/01/2018 13:06, Peter Maydell wrote: >> ASAN is enabled by default if available when --enable-debug. We could >> add more flags if that helps. > Configure switches should work like this: > * default: use feature if present, but don't complain if not present > or not usable > * --enable-foo: use feature. if feature not present, complain and > fail configure > * --disable-foo: don't test for or use feature > However, --enable-debug has never worked like this (the "default" part)... Paolo
On 16 January 2018 at 13:41, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 16/01/2018 13:06, Peter Maydell wrote: >>> ASAN is enabled by default if available when --enable-debug. We could >>> add more flags if that helps. >> Configure switches should work like this: >> * default: use feature if present, but don't complain if not present >> or not usable >> * --enable-foo: use feature. if feature not present, complain and >> fail configure >> * --disable-foo: don't test for or use feature >> > > However, --enable-debug has never worked like this (the "default" part)... True, but -g, no optimization isn't really something we want to default to :-) I think the general principle that unless the user specifically said they cared about the address sanitizer we shouldn't complain if it happens not to work on this host is still a good one. thanks -- PMM
Hi On Tue, Jan 16, 2018 at 1:06 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 16 January 2018 at 11:58, Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: >> Hi >> >> On Tue, Jan 16, 2018 at 12:25 PM, Peter Maydell >> <peter.maydell@linaro.org> wrote: >>> On 15 January 2018 at 23:35, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> The following changes since commit 997eba28a3ed5400a80f754bf3a1c8044b75b9ff: >>>> >>>> Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180111' into staging (2018-01-11 14:34:41 +0000) >>>> >>>> are available in the git repository at: >>>> >>>> >>>> git://github.com/bonzini/qemu.git tags/for-upstream >>>> >>>> for you to fetch changes up to ff9adba50bf8a4c080b8aee9be2314ef179a7b5f: >>>> >>>> ucontext: annotate coroutine stack for ASAN (2018-01-12 15:21:14 +0100) >>>> >>>> ---------------------------------------------------------------- >>>> * QemuMutex tracing improvements (Alex) >>>> * ram_addr_t optimization (David) >>>> * SCSI fixes (Fam, Stefan, me) >>>> * do {} while (0) fixes (Eric) >>>> * KVM fix for PMU (Jan) >>>> * memory leak fixes from ASAN (Marc-André) >>>> * migration fix for HPET, icount, loadvm (Maria, Pavel) >>>> * hflags fixes (me, Tao) >>>> * block/iscsi uninitialized variable (Peter L.) >>>> * full support for GMainContexts in character devices (Peter Xu) >>>> * more boot-serial-test (Thomas) >>>> * Memory leak fix (Zhecheng) >>> >>> Various build failures, I'm afraid: >>> >> >> damn, sorry.. >> >>> x86-64/Linux/gcc: >>> >>> configure produces an error message: >>> >>> ERROR: ASAN build enabled, but ASAN header is too old. >>> Without code annotation, the report may be inferior. >>> >>> even though this configure did not explicitly request ASAN. >>> Then configure seems to exit successfully anyway, since the >>> build proceeds. >> >> ASAN is enabled by default if available when --enable-debug. We could >> add more flags if that helps. > > Configure switches should work like this: > * default: use feature if present, but don't complain if not present > or not usable > * --enable-foo: use feature. if feature not present, complain and > fail configure > * --disable-foo: don't test for or use feature Would that be enough to drop the "ERROR:" prefix ? >>> For some reason the build creates config-target.h a lot: > >> One per target no? (the build should be more silent than before) > > Oh, I guess that makes sense. It's a bit odd that the message neither > gives the target part of the filename nor has those entering/leaving > directory messages... > >>> >>> Then it runs configure again, this time without the ERROR message, >>> and eventually fails with: >>> >>> CC hw/display/exynos4210_fimd.o >>> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c: In >>> function ‘fimd_get_buffer_id’: >>> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c:1105:5: >>> error: case label does not reduce to an integer constant >>> case FIMD_WINCON_BUF2_STAT: >> >> never saw that error, hmm interesting. This is related to >> -fsanitize=address ? Is this on Debian stable? Interesting, looks like a bug in gcc ubsan that doesn't happen with recent versions (related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80550 but probably a different bug). Adding a cast is enough: -#define FIMD_WINCON_BUF2_STAT ((0 << 21) | (1 << 31)) +#define FIMD_WINCON_BUF2_STAT (uint32_t)((0 << 21) | (1 << 31)) It looks like there are no other cases like this > > Ubuntu xenial (16.04.3 LTS). No idea if it's related to the > sanitizer or not. > >> >>> ^ >>> >>> On sparc64 host configure fails: > >> Hmm, I guess the check -fsanitize=address doesn't return an error >> unless -Werror is given. Perhaps it needs: >> >> diff --git a/configure b/configure >> index f5550f3289..ba68c550c9 100755 >> --- a/configure >> +++ b/configure >> @@ -5190,7 +5190,7 @@ fi >> >> have_asan=no >> write_c_skeleton >> -if compile_prog "-fsanitize=address" ""; then >> +if compile_prog "-Werror -fsanitize=address" ""; then >> have_asan=yes >> fi >> >> @@ -5207,7 +5207,7 @@ int main(void) { >> return 0; >> } >> EOF >> -if compile_prog "-fsanitize=address" "" ; then >> +if compile_prog "-Werror -fsanitize=address" "" ; then >> have_asan_iface_fiber=yes >> fi >> > > Looks plausible. Actually, it probably needs also $CPU_CFLAGS Paolo, would you drop "build-sys: add some sanitizers when --enable-debug if possible" & "ucontext: annotate coroutine stack for ASAN" from the series? I'll send a new version for those 2. Thanks
On 16/01/2018 14:47, Peter Maydell wrote: > On 16 January 2018 at 13:41, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 16/01/2018 13:06, Peter Maydell wrote: >>>> ASAN is enabled by default if available when --enable-debug. We could >>>> add more flags if that helps. >>> Configure switches should work like this: >>> * default: use feature if present, but don't complain if not present >>> or not usable >>> * --enable-foo: use feature. if feature not present, complain and >>> fail configure >>> * --disable-foo: don't test for or use feature >>> >> >> However, --enable-debug has never worked like this (the "default" part)... > > True, but -g, no optimization isn't really something we want to > default to :-) Same for ASAN. :-) > I think the general principle that unless the user > specifically said they cared about the address sanitizer we shouldn't > complain if it happens not to work on this host is still a good one. Yes, I agree. So we need two options: * --enable-asan defaults to not used, but also fails configure if ASAN is not available/usable. * if we want to have --enable-debug enable ASAN, it should however _not_ fail configure if ASAN is not available/usable. (I am not sure anymore it's a good idea). The questions are: * should fiber support be required for --enable-asan? What is the difference in the quality of the reports? * if not, and assuming --enable-debug tries to enable ASAN, should --enable-debug complain if fiber support is not required? Should --enable-debug enable ASAN if fiber support is not available? * if --enable-debug does *not* try to enable ASAN, should test-debug add --enable-asn? (I think so). Paolo
On 16 January 2018 at 13:50, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Interesting, looks like a bug in gcc ubsan that doesn't happen with > recent versions (related to > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80550 but probably a > different bug). Adding a cast is enough: > > -#define FIMD_WINCON_BUF2_STAT ((0 << 21) | (1 << 31)) > +#define FIMD_WINCON_BUF2_STAT (uint32_t)((0 << 21) | (1 << 31)) > > It looks like there are no other cases like this Rather than casting it's probably simpler to use "1U" to get that shift to be the right type. thanks -- PMM
Hi On Tue, Jan 16, 2018 at 2:54 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 16/01/2018 14:47, Peter Maydell wrote: >> On 16 January 2018 at 13:41, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> On 16/01/2018 13:06, Peter Maydell wrote: >>>>> ASAN is enabled by default if available when --enable-debug. We could >>>>> add more flags if that helps. >>>> Configure switches should work like this: >>>> * default: use feature if present, but don't complain if not present >>>> or not usable >>>> * --enable-foo: use feature. if feature not present, complain and >>>> fail configure >>>> * --disable-foo: don't test for or use feature >>>> >>> >>> However, --enable-debug has never worked like this (the "default" part)... >> >> True, but -g, no optimization isn't really something we want to >> default to :-) > > Same for ASAN. :-) > >> I think the general principle that unless the user >> specifically said they cared about the address sanitizer we shouldn't >> complain if it happens not to work on this host is still a good one. > > Yes, I agree. > > So we need two options: > > * --enable-asan defaults to not used, but also fails configure if ASAN > is not available/usable. and --enable-ubsan etc ... I wish we would avoid the multiplication of configure options, and use good default values instead for --enable-debug. But if it's not possible, let's add more options. However, it would be great if ASAN can be enabled by default, it seems too few developers care, even though it should be strongly recommended. > > * if we want to have --enable-debug enable ASAN, it should however _not_ > fail configure if ASAN is not available/usable. (I am not sure anymore > it's a good idea). > > The questions are: > > * should fiber support be required for --enable-asan? What is the > difference in the quality of the reports? It's not required, but helps to detect more leaks. It also removes some warnings in some cases: Before: elmarco@boraha:~/src/qemu/build (asan *%)$ tests/test-coroutine -p /basic/lifecycle /basic/lifecycle: ==20781==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! ==20781==WARNING: ASan is ignoring requested __asan_handle_no_return: stack top: 0x7ffcb184d000; bottom 0x7ff6c4cfd000; size: 0x0005ecb50000 (25446121472) False positive error reports may follow For details see https://github.com/google/sanitizers/issues/189 OK After: tests/test-coroutine -p /basic/lifecycle /basic/lifecycle: ==21110==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! OK > * if not, and assuming --enable-debug tries to enable ASAN, should > --enable-debug complain if fiber support is not required? Should > --enable-debug enable ASAN if fiber support is not available? I propose to simply print a warning during configure > > * if --enable-debug does *not* try to enable ASAN, should test-debug add > --enable-asn? (I think so). The other way around? (tbh, I am not fond of all the configure options - if you need fine-grained configuration, you can overwrite the various *FLAGS..)
On 16/01/2018 15:22, Marc-André Lureau wrote: > Hi > > On Tue, Jan 16, 2018 at 2:54 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 16/01/2018 14:47, Peter Maydell wrote: >>> On 16 January 2018 at 13:41, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> On 16/01/2018 13:06, Peter Maydell wrote: >>>>>> ASAN is enabled by default if available when --enable-debug. We could >>>>>> add more flags if that helps. >>>>> Configure switches should work like this: >>>>> * default: use feature if present, but don't complain if not present >>>>> or not usable >>>>> * --enable-foo: use feature. if feature not present, complain and >>>>> fail configure >>>>> * --disable-foo: don't test for or use feature >>>>> >>>> >>>> However, --enable-debug has never worked like this (the "default" part)... >>> >>> True, but -g, no optimization isn't really something we want to >>> default to :-) >> >> Same for ASAN. :-) >> >>> I think the general principle that unless the user >>> specifically said they cared about the address sanitizer we shouldn't >>> complain if it happens not to work on this host is still a good one. >> >> Yes, I agree. >> >> So we need two options: >> >> * --enable-asan defaults to not used, but also fails configure if ASAN >> is not available/usable. > > and --enable-ubsan etc ... > > I wish we would avoid the multiplication of configure options, and use > good default values instead for --enable-debug. But if it's not > possible, let's add more options. However, it would be great if ASAN > can be enabled by default, it seems too few developers care, even > though it should be strongly recommended. We can add --enable-sanitizer then instead of being specific to ASAN. It could also support --enable-sanitizer=asan,ubsan but that's for later. >> >> * if we want to have --enable-debug enable ASAN, it should however _not_ >> fail configure if ASAN is not available/usable. (I am not sure anymore >> it's a good idea). >> >> The questions are: >> >> * should fiber support be required for --enable-asan? What is the >> difference in the quality of the reports? > > It's not required, but helps to detect more leaks. It also removes > some warnings in some cases: Ok, if it's not a flood of warnings it's okay. > >> * if not, and assuming --enable-debug tries to enable ASAN, should >> --enable-debug complain if fiber support is not required? Should >> --enable-debug enable ASAN if fiber support is not available? > > I propose to simply print a warning during configure Yes, it could do the same as --enable-asan. >> * if --enable-debug does *not* try to enable ASAN, should test-debug add >> --enable-asn? (I think so). > > The other way around? I mean - IMO, test-debug should enable sanitizers even if --enable-sanitizer and --enable-debug are completely separate. Paolo
On 16/01/2018 14:50, Marc-André Lureau wrote: > Hi > > On Tue, Jan 16, 2018 at 1:06 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 16 January 2018 at 11:58, Marc-André Lureau >> <marcandre.lureau@gmail.com> wrote: >>> Hi >>> >>> On Tue, Jan 16, 2018 at 12:25 PM, Peter Maydell >>> <peter.maydell@linaro.org> wrote: >>>> On 15 January 2018 at 23:35, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>>> The following changes since commit 997eba28a3ed5400a80f754bf3a1c8044b75b9ff: >>>>> >>>>> Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180111' into staging (2018-01-11 14:34:41 +0000) >>>>> >>>>> are available in the git repository at: >>>>> >>>>> >>>>> git://github.com/bonzini/qemu.git tags/for-upstream >>>>> >>>>> for you to fetch changes up to ff9adba50bf8a4c080b8aee9be2314ef179a7b5f: >>>>> >>>>> ucontext: annotate coroutine stack for ASAN (2018-01-12 15:21:14 +0100) >>>>> >>>>> ---------------------------------------------------------------- >>>>> * QemuMutex tracing improvements (Alex) >>>>> * ram_addr_t optimization (David) >>>>> * SCSI fixes (Fam, Stefan, me) >>>>> * do {} while (0) fixes (Eric) >>>>> * KVM fix for PMU (Jan) >>>>> * memory leak fixes from ASAN (Marc-André) >>>>> * migration fix for HPET, icount, loadvm (Maria, Pavel) >>>>> * hflags fixes (me, Tao) >>>>> * block/iscsi uninitialized variable (Peter L.) >>>>> * full support for GMainContexts in character devices (Peter Xu) >>>>> * more boot-serial-test (Thomas) >>>>> * Memory leak fix (Zhecheng) >>>> >>>> Various build failures, I'm afraid: >>>> >>> >>> damn, sorry.. >>> >>>> x86-64/Linux/gcc: >>>> >>>> configure produces an error message: >>>> >>>> ERROR: ASAN build enabled, but ASAN header is too old. >>>> Without code annotation, the report may be inferior. >>>> >>>> even though this configure did not explicitly request ASAN. >>>> Then configure seems to exit successfully anyway, since the >>>> build proceeds. >>> >>> ASAN is enabled by default if available when --enable-debug. We could >>> add more flags if that helps. >> >> Configure switches should work like this: >> * default: use feature if present, but don't complain if not present >> or not usable >> * --enable-foo: use feature. if feature not present, complain and >> fail configure >> * --disable-foo: don't test for or use feature > > Would that be enough to drop the "ERROR:" prefix ? > >>>> For some reason the build creates config-target.h a lot: >> >>> One per target no? (the build should be more silent than before) >> >> Oh, I guess that makes sense. It's a bit odd that the message neither >> gives the target part of the filename nor has those entering/leaving >> directory messages... >> >>>> >>>> Then it runs configure again, this time without the ERROR message, >>>> and eventually fails with: >>>> >>>> CC hw/display/exynos4210_fimd.o >>>> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c: In >>>> function ‘fimd_get_buffer_id’: >>>> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c:1105:5: >>>> error: case label does not reduce to an integer constant >>>> case FIMD_WINCON_BUF2_STAT: >>> >>> never saw that error, hmm interesting. This is related to >>> -fsanitize=address ? Is this on Debian stable? > > Interesting, looks like a bug in gcc ubsan that doesn't happen with > recent versions (related to > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80550 but probably a > different bug). Adding a cast is enough: > > -#define FIMD_WINCON_BUF2_STAT ((0 << 21) | (1 << 31)) > +#define FIMD_WINCON_BUF2_STAT (uint32_t)((0 << 21) | (1 << 31)) > > It looks like there are no other cases like this > > > >> >> Ubuntu xenial (16.04.3 LTS). No idea if it's related to the >> sanitizer or not. >> >>> >>>> ^ >>>> >>>> On sparc64 host configure fails: >> >>> Hmm, I guess the check -fsanitize=address doesn't return an error >>> unless -Werror is given. Perhaps it needs: >>> >>> diff --git a/configure b/configure >>> index f5550f3289..ba68c550c9 100755 >>> --- a/configure >>> +++ b/configure >>> @@ -5190,7 +5190,7 @@ fi >>> >>> have_asan=no >>> write_c_skeleton >>> -if compile_prog "-fsanitize=address" ""; then >>> +if compile_prog "-Werror -fsanitize=address" ""; then >>> have_asan=yes >>> fi >>> >>> @@ -5207,7 +5207,7 @@ int main(void) { >>> return 0; >>> } >>> EOF >>> -if compile_prog "-fsanitize=address" "" ; then >>> +if compile_prog "-Werror -fsanitize=address" "" ; then >>> have_asan_iface_fiber=yes >>> fi >>> >> >> Looks plausible. > > Actually, it probably needs also $CPU_CFLAGS > > Paolo, would you drop "build-sys: add some sanitizers when > --enable-debug if possible" & "ucontext: annotate coroutine stack for > ASAN" from the series? I'll send a new version for those 2. Yes, that was already my plan. Thanks for confirming! Paolo
On 16 January 2018 at 14:26, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 16/01/2018 15:22, Marc-André Lureau wrote: >>> * if not, and assuming --enable-debug tries to enable ASAN, should >>> --enable-debug complain if fiber support is not required? Should >>> --enable-debug enable ASAN if fiber support is not available? >> >> I propose to simply print a warning during configure > > Yes, it could do the same as --enable-asan. The thing you want to avoid here is having one of my standard build configs produce new text saying 'warning' or 'error', because that will flag up to me as a build failure and I'll bounce the pullreq... thanks -- PMM
diff --git a/configure b/configure index f5550f3289..ba68c550c9 100755 --- a/configure +++ b/configure @@ -5190,7 +5190,7 @@ fi have_asan=no write_c_skeleton -if compile_prog "-fsanitize=address" ""; then +if compile_prog "-Werror -fsanitize=address" ""; then have_asan=yes fi @@ -5207,7 +5207,7 @@ int main(void) { return 0; } EOF -if compile_prog "-fsanitize=address" "" ; then +if compile_prog "-Werror -fsanitize=address" "" ; then have_asan_iface_fiber=yes fi