Re: [PATCH v6 06/15] cpufreq: Add Hardware P-State (HWP) driver

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.07.2023 15:26, Jason Andryuk wrote:
> On Tue, Jul 25, 2023 at 2:27 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 24.07.2023 21:49, Jason Andryuk wrote:
>>> On Mon, Jul 24, 2023 at 12:15 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> On 24.07.2023 14:58, Jason Andryuk wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
>>>>> +#define hwp_err(cpu, fmt, ...) \
>>>>> +    printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, ##__VA_ARGS__)
>>>>> +#define hwp_info(fmt, ...)    printk(XENLOG_INFO "HWP: " fmt, 
>>>>> ##__VA_ARGS__)
>>>>
>>>> I'm not convinced ", ##__VA_ARGS__" is a good construct to use. I notice
>>>> we have a few instances (mainly in code inherited from elsewhere), but I
>>>> think it ought to either be plain C99 style (without the ##) or purely
>>>> the gcc extension form (not using __VA_ARGS__).
>>>
>>> Using plain __VA_ARGS__ doesn't work for the cases without arguments:
>>> arch/x86/acpi/cpufreq/hwp.c:78:53: error: expected expression before ‘)’ 
>>> token
>>>    78 |         printk(XENLOG_DEBUG "HWP: " fmt, __VA_ARGS__);  \
>>>       |                                                     ^
>>> arch/x86/acpi/cpufreq/hwp.c:201:9: note: in expansion of macro ‘hwp_verbose’
>>>   201 |         hwp_verbose("disabled: No energy/performance
>>> preference available");
>>>       |         ^~~~~~~~~~~
>>
>> Of course.
>>
>>> I can use "__VA_OPT__(,) __VA_ARGS__" though.
>>
>> __VA_OPT__ is yet newer than C99, so this is an option only if all
>> compilers we continue to support actually know of this.
> 
> Right, sorry.
> 
>> We have no
>> uses of it in the codebase so far, which suggests you might best go
>> with the longstanding gcc extension here.
> 
> FTAOD, "##__VA_ARGS__" is the longstanding extension?

No. But you've apparently found it ...

>  It's the only
> one I've been able to get to compile.  I'm reading
> https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html and it
> mentions a few different extensions.
> 
> This part seemed promising:
> """
> This has been fixed in C++20, and GNU CPP also has a pair of
> extensions which deal with this problem.
> 
> First, in GNU CPP, and in C++ beginning in C++20, you are allowed to
> leave the variable argument out entirely:
> 
> eprintf ("success!\n")
>      → fprintf(stderr, "success!\n", );
> """
> 
> However, it doesn't seem to actually work for me.  I still get an
> error like the one above for plain __VA_ARGS__.  That is for:
> 
>     #define hwp_err(cpu, fmt, args...) \
>         printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, args)

..., just that you're missing the ##:

    #define hwp_err(cpu, fmt, args...) \
        printk(XENLOG_ERR "HWP: CPU%u error: " fmt, cpu, ## args)

Jan