Message ID | 20170916010330.10435-1-jsnow@redhat.com |
---|---|
State | New |
Headers | show |
Hi, This series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PULL 00/11] Ide patches Message-id: 20170916010330.10435-1-jsnow@redhat.com Type: series === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' bc6172dc94 AHCI: remove DPRINTF macro 2ea9b926e3 AHCI: pretty-print FIS to buffer instead of stderr d23d77942b AHCI: Rework IRQ constants c3e3883d53 AHCI: Replace DPRINTF with trace-events a3f8dc5d3c IDE: replace DEBUG_AIO with trace events 3c992d8a98 ATAPI: Replace DEBUG_IDE_ATAPI with tracing events 3beaa3f939 IDE: add tracing for data ports 1994e49cc7 IDE: Add register hints to tracing a446948ea3 IDE: replace DEBUG_IDE with tracing system 8d2b13d3da hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable = false 9bc5607864 ide: ahci: unparent children buses before freeing their memory === OUTPUT BEGIN === Checking PATCH 1/11: ide: ahci: unparent children buses before freeing their memory... Checking PATCH 2/11: hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable = false... Checking PATCH 3/11: IDE: replace DEBUG_IDE with tracing system... ERROR: spaces required around that '|' (ctx:VxV) #146: FILE: hw/ide/core.c:1197: + if (reg_num != 7 && (s->status & (BUSY_STAT|DRQ_STAT))) { ^ total: 1 errors, 0 warnings, 337 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 4/11: IDE: Add register hints to tracing... Checking PATCH 5/11: IDE: add tracing for data ports... Checking PATCH 6/11: ATAPI: Replace DEBUG_IDE_ATAPI with tracing events... Checking PATCH 7/11: IDE: replace DEBUG_AIO with trace events... Checking PATCH 8/11: AHCI: Replace DPRINTF with trace-events... ERROR: Hex numbers must be prefixed with '0x' #548: FILE: hw/ide/trace-events:91: +handle_reg_h2d_fis_pmp(void *s, int port, char b0, char b1, char b2) "ahci(%p)[%d]: Port Multiplier not supported, FIS: 0x%02x-%02x-%02x" ERROR: Hex numbers must be prefixed with '0x' #549: FILE: hw/ide/trace-events:92: +handle_reg_h2d_fis_res(void *s, int port, char b0, char b1, char b2) "ahci(%p)[%d]: Reserved flags set in H2D Register FIS, FIS: 0x%02x-%02x-%02x" ERROR: Hex numbers must be prefixed with '0x' #555: FILE: hw/ide/trace-events:98: +handle_cmd_unhandled_fis(void *s, int port, uint8_t b0, uint8_t b1, uint8_t b2) "ahci(%p)[%d]: unhandled FIS type. cmd_fis: 0x%02x-%02x-%02x" total: 3 errors, 0 warnings, 496 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 9/11: AHCI: Rework IRQ constants... Checking PATCH 10/11: AHCI: pretty-print FIS to buffer instead of stderr... WARNING: line over 80 characters #60: FILE: hw/ide/ahci.c:1206: + char *pretty_fis = ahci_pretty_buffer_fis(ide_state->io_buffer, 0x10); total: 0 errors, 1 warnings, 60 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 11/11: AHCI: remove DPRINTF macro... === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org
On 16 September 2017 at 02:03, John Snow <jsnow@redhat.com> wrote: > The following changes since commit 5faf2d376af3cb4eb92da44c2580e08d39832caa: > > Merge remote-tracking branch 'remotes/huth/tags/check-20170915' into staging (2017-09-15 20:29:44 +0100) > > are available in the git repository at: > > https://github.com/jnsnow/qemu.git tags/ide-pull-request > > for you to fetch changes up to 2a94e34d3ecef91727f467cc012587c632099d40: > > AHCI: remove DPRINTF macro (2017-09-15 20:36:18 -0400) > > ---------------------------------------------------------------- > > ---------------------------------------------------------------- Hi; I'm afraid this doesn't build with clang: /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error: comparison of unsigned enum expression >= 0 is always true [-Werror,-Wtautological-compare] if (enval >= 0 && enval < IDE_DMA__COUNT) { ~~~~~ ^ ~ 1 error generated. (It's impdef whether an enum with all positive values is a signed type or unsigned type, so just deleting the comparison against 0 would also be wrong...) thanks -- PMM
On 09/16/2017 09:34 AM, Peter Maydell wrote: > Hi; I'm afraid this doesn't build with clang: > > /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error: > comparison of unsigned enum expression >= 0 is always true > [-Werror,-Wtautological-compare] > if (enval >= 0 && enval < IDE_DMA__COUNT) { > ~~~~~ ^ ~ > 1 error generated. > > (It's impdef whether an enum with all positive values is > a signed type or unsigned type, so just deleting the > comparison against 0 would also be wrong...) But if ((unsigned)enval < IDE_DMA__COUNT) { should work, regardless of the signedness of the enum.
On 09/16/2017 10:34 AM, Peter Maydell wrote: > On 16 September 2017 at 02:03, John Snow <jsnow@redhat.com> wrote: >> The following changes since commit 5faf2d376af3cb4eb92da44c2580e08d39832caa: >> >> Merge remote-tracking branch 'remotes/huth/tags/check-20170915' into staging (2017-09-15 20:29:44 +0100) >> >> are available in the git repository at: >> >> https://github.com/jnsnow/qemu.git tags/ide-pull-request >> >> for you to fetch changes up to 2a94e34d3ecef91727f467cc012587c632099d40: >> >> AHCI: remove DPRINTF macro (2017-09-15 20:36:18 -0400) >> >> ---------------------------------------------------------------- >> >> ---------------------------------------------------------------- > > Hi; I'm afraid this doesn't build with clang: > > /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error: > comparison of unsigned enum expression >= 0 is always true > [-Werror,-Wtautological-compare] > if (enval >= 0 && enval < IDE_DMA__COUNT) { > ~~~~~ ^ ~ > 1 error generated. > > (It's impdef whether an enum with all positive values is > a signed type or unsigned type, so just deleting the > comparison against 0 would also be wrong...) > > thanks > -- PMM > Huh, impdef in the general case, but is it undefined for gnu99? I'm wondering why Clang can be so certain about this comparison being useless. Is this a Clang "bug"? --js
On 18 September 2017 at 18:55, John Snow <jsnow@redhat.com> wrote: > On 09/16/2017 10:34 AM, Peter Maydell wrote: >> Hi; I'm afraid this doesn't build with clang: >> >> /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error: >> comparison of unsigned enum expression >= 0 is always true >> [-Werror,-Wtautological-compare] >> if (enval >= 0 && enval < IDE_DMA__COUNT) { >> ~~~~~ ^ ~ >> 1 error generated. >> >> (It's impdef whether an enum with all positive values is >> a signed type or unsigned type, so just deleting the >> comparison against 0 would also be wrong...) > Huh, impdef in the general case, but is it undefined for gnu99? I'm > wondering why Clang can be so certain about this comparison being > useless. Is this a Clang "bug"? My guess is that clang as an implementation picks unsigned in this case, that it then effectively lowers all the enums to just being integer arithmetic, and then the warning pass coming along later doesn't know that the unsigned thing it's comparing is an enum. I think you could argue that it would at least be helpful if clang didn't warn about comparisons that only happen to be useless for this particular platform/impdef choice but are useful for the same code compiled with a different compiler. thanks -- PMM
On 18 September 2017 at 19:00, Peter Maydell <peter.maydell@linaro.org> wrote: > On 18 September 2017 at 18:55, John Snow <jsnow@redhat.com> wrote: >> On 09/16/2017 10:34 AM, Peter Maydell wrote: >>> Hi; I'm afraid this doesn't build with clang: >>> >>> /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error: >>> comparison of unsigned enum expression >= 0 is always true >>> [-Werror,-Wtautological-compare] >>> if (enval >= 0 && enval < IDE_DMA__COUNT) { >>> ~~~~~ ^ ~ >>> 1 error generated > I think you could argue that it would at least be helpful > if clang didn't warn about comparisons that only happen > to be useless for this particular platform/impdef choice > but are useful for the same code compiled with a different > compiler. A bit of googling and some experimentation reveals that clang deliberately suppresses this warning in the special case of comparing against an enum value which happens to be zero (but not for literal constant zero!). So this will be fine: if (enval >= IDE_DMA_READ && enval < IDE_DMA__COUNT) (or more sensibly you'd want to define an enum constant for IDE_DMA__FIRST or something rather than relying on READ being 0.) (found here: http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html ) thanks -- PMM
On 18/09/17 19:14, Peter Maydell wrote: > On 18 September 2017 at 19:00, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 18 September 2017 at 18:55, John Snow <jsnow@redhat.com> wrote: >>> On 09/16/2017 10:34 AM, Peter Maydell wrote: >>>> Hi; I'm afraid this doesn't build with clang: >>>> >>>> /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error: >>>> comparison of unsigned enum expression >= 0 is always true >>>> [-Werror,-Wtautological-compare] >>>> if (enval >= 0 && enval < IDE_DMA__COUNT) { >>>> ~~~~~ ^ ~ >>>> 1 error generated > > >> I think you could argue that it would at least be helpful >> if clang didn't warn about comparisons that only happen >> to be useless for this particular platform/impdef choice >> but are useful for the same code compiled with a different >> compiler. > > A bit of googling and some experimentation reveals that > clang deliberately suppresses this warning in the special > case of comparing against an enum value which happens to > be zero (but not for literal constant zero!). So this will > be fine: > if (enval >= IDE_DMA_READ && enval < IDE_DMA__COUNT) > > (or more sensibly you'd want to define an enum constant > for IDE_DMA__FIRST or something rather than relying on > READ being 0.) > > (found here: > http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html > ) Doing a git pull and even with the applied version of this patch I get a build failure on my local gcc-4.7: cc -I/home/build/src/qemu/git/qemu/hw/ide -Ihw/ide -I/home/build/src/qemu/git/qemu/tcg -I/home/build/src/qemu/git/qemu/tcg/i386 -I/home/build/src/qemu/git/qemu/linux-headers -I/home/build/src/qemu/git/qemu/linux-headers -I. -I/home/build/src/qemu/git/qemu -I/home/build/src/qemu/git/qemu/accel/tcg -I/home/build/src/qemu/git/qemu/include -I/usr/include/pixman-1 -I/home/build/src/qemu/git/qemu/dtc/libfdt -Werror -pthread -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-all -I/usr/include/p11-kit-1 -I/usr/include/libpng12 -I/home/build/src/qemu/git/qemu/tests -MMD -MP -MT hw/ide/core.o -MF hw/ide/core.d -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g -c -o hw/ide/core.o hw/ide/core.c hw/ide/core.c: In function ‘IDE_DMA_CMD_str’: hw/ide/core.c:71:5: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits] cc1: all warnings being treated as errors make: *** [hw/ide/core.o] Error 1 Are there any other workarounds for this at all? ATB, Mark.
On 09/20/2017 01:02 PM, Mark Cave-Ayland wrote: > On 18/09/17 19:14, Peter Maydell wrote: > >> On 18 September 2017 at 19:00, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 18 September 2017 at 18:55, John Snow <jsnow@redhat.com> wrote: >>>> On 09/16/2017 10:34 AM, Peter Maydell wrote: >>>>> Hi; I'm afraid this doesn't build with clang: >>>>> >>>>> /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:70:15: error: >>>>> comparison of unsigned enum expression >= 0 is always true >>>>> [-Werror,-Wtautological-compare] >>>>> if (enval >= 0 && enval < IDE_DMA__COUNT) { >>>>> ~~~~~ ^ ~ >>>>> 1 error generated >> >> >>> I think you could argue that it would at least be helpful >>> if clang didn't warn about comparisons that only happen >>> to be useless for this particular platform/impdef choice >>> but are useful for the same code compiled with a different >>> compiler. >> >> A bit of googling and some experimentation reveals that >> clang deliberately suppresses this warning in the special >> case of comparing against an enum value which happens to >> be zero (but not for literal constant zero!). So this will >> be fine: >> if (enval >= IDE_DMA_READ && enval < IDE_DMA__COUNT) >> >> (or more sensibly you'd want to define an enum constant >> for IDE_DMA__FIRST or something rather than relying on >> READ being 0.) >> >> (found here: >> http://clang-developers.42468.n3.nabble.com/Possibly-invalid-enum-tautology-warning-td3233140.html >> ) > > Doing a git pull and even with the applied version of this patch I get a > build failure on my local gcc-4.7: > > cc -I/home/build/src/qemu/git/qemu/hw/ide -Ihw/ide > -I/home/build/src/qemu/git/qemu/tcg > -I/home/build/src/qemu/git/qemu/tcg/i386 > -I/home/build/src/qemu/git/qemu/linux-headers > -I/home/build/src/qemu/git/qemu/linux-headers -I. > -I/home/build/src/qemu/git/qemu > -I/home/build/src/qemu/git/qemu/accel/tcg > -I/home/build/src/qemu/git/qemu/include -I/usr/include/pixman-1 > -I/home/build/src/qemu/git/qemu/dtc/libfdt -Werror -pthread > -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include > -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE > -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings > -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv > -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs > -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers > -Wold-style-declaration -Wold-style-definition -Wtype-limits > -fstack-protector-all -I/usr/include/p11-kit-1 > -I/usr/include/libpng12 -I/home/build/src/qemu/git/qemu/tests -MMD -MP > -MT hw/ide/core.o -MF hw/ide/core.d -O2 -U_FORTIFY_SOURCE > -D_FORTIFY_SOURCE=2 -g -c -o hw/ide/core.o hw/ide/core.c > hw/ide/core.c: In function ‘IDE_DMA_CMD_str’: > hw/ide/core.c:71:5: error: comparison of unsigned expression >= 0 is > always true [-Werror=type-limits] > cc1: all warnings being treated as errors > make: *** [hw/ide/core.o] Error 1 > > Are there any other workarounds for this at all? > > > ATB, > > Mark. > Guh. From which distro does your GCC 4.7 hail? Regardless, I suppose I will revert to Eric's workaround, though I like the way it reads an awful lot less. --js
On 20/09/17 18:55, John Snow wrote: > Guh. From which distro does your GCC 4.7 hail? > > Regardless, I suppose I will revert to Eric's workaround, though I like > the way it reads an awful lot less. Thanks John - it's just a standard Debian Wheezy installation on amd64. ATB, Mark.