[v1] puv3: always compile-check debug printf

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

Posted by Anishka0107 8 years, 10 months ago

    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

Posted by no-reply@patchew.org 8 years, 10 months ago

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

Posted by Alex Bennée 8 years, 10 months ago

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

Posted by Wei Huang 8 years, 10 months ago


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
>