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)

----------------------------------------------------------------

----------------------------------------------------------------

Igor Mammedov (1):
  ide: ahci: unparent children buses before freeing their memory

John Snow (9):
  IDE: replace DEBUG_IDE with tracing system
  IDE: Add register hints to tracing
  IDE: add tracing for data ports
  ATAPI: Replace DEBUG_IDE_ATAPI with tracing events
  IDE: replace DEBUG_AIO with trace events
  AHCI: Replace DPRINTF with trace-events
  AHCI: Rework IRQ constants
  AHCI: pretty-print FIS to buffer instead of stderr
  AHCI: remove DPRINTF macro

Thomas Huth (1):
  hw/ide/microdrive: Mark the dscm1xxxx device with user_creatable =
    false

 Makefile.objs             |   1 +
 hw/ide/ahci.c             | 244 ++++++++++++++++++++++++----------------------
 hw/ide/ahci_internal.h    |  44 +++++++--
 hw/ide/atapi.c            |  69 +++++--------
 hw/ide/cmd646.c           |  10 +-
 hw/ide/core.c             | 185 +++++++++++++++++++++++------------
 hw/ide/microdrive.c       |   3 +
 hw/ide/pci.c              |  17 +---
 hw/ide/piix.c             |  11 +--
 hw/ide/trace-events       | 111 +++++++++++++++++++++
 hw/ide/via.c              |  10 +-
 include/hw/ide/internal.h |   8 +-
 12 files changed, 441 insertions(+), 272 deletions(-)
 create mode 100644 hw/ide/trace-events

-- 
2.9.5

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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.