[Qemu-devel] [PATCH] puv3: always compile-check debug printf

[Qemu-devel] [PATCH] puv3: always compile-check debug printf

[Anishka0107]

    To prevent bitrot of the format string of the debug statement, files with
  conditional debug statements should ensure that printf is compiled always,
  and enclosed within if(0) statements and not in #ifdef.

Signed-off-by: Anishka Gupta <rimjhim0107@gmail.com>
---
 include/hw/unicore32/puv3.h | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/hw/unicore32/puv3.h b/include/hw/unicore32/puv3.h
index 5a4839f..e268484 100644
--- a/include/hw/unicore32/puv3.h
+++ b/include/hw/unicore32/puv3.h
@@ -41,10 +41,14 @@
 #define PUV3_IRQS_OST0          (26)
 
 /* All puv3_*.c use DPRINTF for debug. */
-#ifdef DEBUG_PUV3
-#define DPRINTF(fmt, ...) printf("%s: " fmt , __func__, ## __VA_ARGS__)
-#else
-#define DPRINTF(fmt, ...) do {} while (0)
-#endif
+#define DEBUG_PUV3 0
+
+#define DPRINTF(fmt, ...)
+    if (DEBUG_PUV3) {
+        fprintf(stderr, "%s: " fmt , __func__, ## __VA_ARGS__)
+    }
+    else {
+        do {} while (0)
+    }
 
 #endif /* QEMU_HW_PUV3_H */
-- 
2.5.0

Re: [Qemu-devel] [PATCH] puv3: always compile-check debug printf

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH] puv3: always compile-check debug printf
Message-id: 1489638621-31978-1-git-send-email-rimjhim0107@gmail.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
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'
016f432 puv3: always compile-check debug printf

=== OUTPUT BEGIN ===
Checking PATCH 1/1: puv3: always compile-check debug printf...
ERROR: else should follow close brace '}'
#32: FILE: include/hw/unicore32/puv3.h:50:
+    }
+    else {

total: 1 errors, 0 warnings, 19 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.

=== 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

Re: [Qemu-devel] [PATCH] puv3: always compile-check debug printf

[Alex Bennée]

Anishka0107 <rimjhim0107@gmail.com> writes:

>     To prevent bitrot of the format string of the debug statement, files with
>   conditional debug statements should ensure that printf is compiled always,
>   and enclosed within if(0) statements and not in #ifdef.
>
> Signed-off-by: Anishka Gupta <rimjhim0107@gmail.com>
> ---
>  include/hw/unicore32/puv3.h | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/include/hw/unicore32/puv3.h b/include/hw/unicore32/puv3.h
> index 5a4839f..e268484 100644
> --- a/include/hw/unicore32/puv3.h
> +++ b/include/hw/unicore32/puv3.h
> @@ -41,10 +41,14 @@
>  #define PUV3_IRQS_OST0          (26)
>
>  /* All puv3_*.c use DPRINTF for debug. */
> -#ifdef DEBUG_PUV3
> -#define DPRINTF(fmt, ...) printf("%s: " fmt , __func__, ## __VA_ARGS__)
> -#else
> -#define DPRINTF(fmt, ...) do {} while (0)
> -#endif
> +#define DEBUG_PUV3 0
> +
> +#define DPRINTF(fmt, ...)
> +    if (DEBUG_PUV3) {
> +        fprintf(stderr, "%s: " fmt , __func__, ## __VA_ARGS__)
> +    }
> +    else {
> +        do {} while (0)
> +    }

This is incorrect. It's fine not to have an else leg as the compiler
will dead-code away the if (0) part. However you still need to wrap in a
do {} while to avoid problems with trailing ;'s. Also you need line
continuations for defining macros.

Did you actually compile test this patch? I suspect it wouldn't build
due to the missing ;s for your fprintf and do while.

See hw/misc/imx6_src.c for a debug printf I recently converted.

>
>  #endif /* QEMU_HW_PUV3_H */


--
Alex Bennée

Re: [Qemu-devel] [PATCH] puv3: always compile-check debug printf

[Wei Huang]

On 03/16/2017 07:04 AM, Alex Bennée wrote:
> 
> Anishka0107 <rimjhim0107@gmail.com> writes:
> 
>>     To prevent bitrot of the format string of the debug statement, files with
>>   conditional debug statements should ensure that printf is compiled always,
>>   and enclosed within if(0) statements and not in #ifdef.
>>
>> Signed-off-by: Anishka Gupta <rimjhim0107@gmail.com>
>> ---
>>  include/hw/unicore32/puv3.h | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/hw/unicore32/puv3.h b/include/hw/unicore32/puv3.h
>> index 5a4839f..e268484 100644
>> --- a/include/hw/unicore32/puv3.h
>> +++ b/include/hw/unicore32/puv3.h
>> @@ -41,10 +41,14 @@
>>  #define PUV3_IRQS_OST0          (26)
>>
>>  /* All puv3_*.c use DPRINTF for debug. */
>> -#ifdef DEBUG_PUV3
>> -#define DPRINTF(fmt, ...) printf("%s: " fmt , __func__, ## __VA_ARGS__)
>> -#else
>> -#define DPRINTF(fmt, ...) do {} while (0)
>> -#endif
>> +#define DEBUG_PUV3 0
>> +
>> +#define DPRINTF(fmt, ...)
>> +    if (DEBUG_PUV3) {
>> +        fprintf(stderr, "%s: " fmt , __func__, ## __VA_ARGS__)
>> +    }
>> +    else {
>> +        do {} while (0)
>> +    }
> 
> This is incorrect. It's fine not to have an else leg as the compiler
> will dead-code away the if (0) part. However you still need to wrap in a
> do {} while to avoid problems with trailing ;'s. Also you need line
> continuations for defining macros.
> 
> Did you actually compile test this patch? I suspect it wouldn't build
> due to the missing ;s for your fprintf and do while.

On top of what Alex pointed out, I think "PATCH v2" you posted is a fix
to this one. You should always post a complete patch for review.

If you compile QEMU with DEBUG_PUV3 enabled, you will notice compilation
errors in hw/dma/puv3_dma.c. Maybe you can fix them altogether.

qemu-upstream.git/include/hw/unicore32/puv3.h:46:34: error: format ‘%x’
expects argument of type ‘unsigned int’, but argument 3 has type ‘hwaddr
{aka long unsigned int}’ [-Werror=format=]
 #define DPRINTF(fmt, ...) printf("%s: " fmt , __func__, ## __VA_ARGS__)
                                  ^
hw/dma/puv3_dma.c:47:5: note: in expansion of macro ‘DPRINTF’
     DPRINTF("offset 0x%x, value 0x%x\n", offset, ret);
     ^~~~~~~


> 
> See hw/misc/imx6_src.c for a debug printf I recently converted.
> 
>>
>>  #endif /* QEMU_HW_PUV3_H */
> 
> 
> --
> Alex Bennée
>