All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] puv3: always compile-check debug printf
@ 2017-03-16  4:30 Anishka0107
  2017-03-16 10:37 ` no-reply
  2017-03-16 12:04 ` Alex Bennée
  0 siblings, 2 replies; 4+ messages in thread
From: Anishka0107 @ 2017-03-16  4:30 UTC (permalink / raw)
  To: jsnow, stefanha, qemu-devel; +Cc: 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] [PATCH] puv3: always compile-check debug printf
  2017-03-16  4:30 [Qemu-devel] [PATCH] puv3: always compile-check debug printf Anishka0107
@ 2017-03-16 10:37 ` no-reply
  2017-03-16 12:04 ` Alex Bennée
  1 sibling, 0 replies; 4+ messages in thread
From: no-reply @ 2017-03-16 10:37 UTC (permalink / raw)
  To: rimjhim0107; +Cc: famz, jsnow, stefanha, qemu-devel

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] [PATCH] puv3: always compile-check debug printf
  2017-03-16  4:30 [Qemu-devel] [PATCH] puv3: always compile-check debug printf Anishka0107
  2017-03-16 10:37 ` no-reply
@ 2017-03-16 12:04 ` Alex Bennée
  2017-03-16 16:25   ` Wei Huang
  1 sibling, 1 reply; 4+ messages in thread
From: Alex Bennée @ 2017-03-16 12:04 UTC (permalink / raw)
  To: Anishka0107; +Cc: jsnow, stefanha, qemu-devel


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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [Qemu-devel] [PATCH] puv3: always compile-check debug printf
  2017-03-16 12:04 ` Alex Bennée
@ 2017-03-16 16:25   ` Wei Huang
  0 siblings, 0 replies; 4+ messages in thread
From: Wei Huang @ 2017-03-16 16:25 UTC (permalink / raw)
  To: Alex Bennée, Anishka0107; +Cc: jsnow, qemu-devel, stefanha



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
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2017-03-16 16:26 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-16  4:30 [Qemu-devel] [PATCH] puv3: always compile-check debug printf Anishka0107
2017-03-16 10:37 ` no-reply
2017-03-16 12:04 ` Alex Bennée
2017-03-16 16:25   ` Wei Huang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.