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

[Wei Huang]

On 03/16/2017 07:04 AM, Alex Bennée wrote:
> 
> Anishka0107 <address@hidden> 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 <address@hidden>
>> ---
>>  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
>