[Qemu-devel] checkpatch.pl question

[Qemu-devel] checkpatch.pl question

[Alexey Kardashevskiy]

Hi!

checkpatch.pl often complains on things like this:

===
ERROR: need consistent spacing around '*' (ctx:WxV)
#57: FILE: hw/misc/vfio.c:4323:
+int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
                                       ^

total: 1 errors, 0 warnings, 46 lines checked
===

Since perl is a write-only language, I cannot understand why :) Any clue?


-- 
Alexey

Re: [Qemu-devel] checkpatch.pl question

[Markus Armbruster]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> Hi!
>
> checkpatch.pl often complains on things like this:
>
> ===
> ERROR: need consistent spacing around '*' (ctx:WxV)
> #57: FILE: hw/misc/vfio.c:4323:
> +int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>                                        ^
>
> total: 1 errors, 0 warnings, 46 lines checked
> ===
>
> Since perl is a write-only language, I cannot understand why :) Any clue?

It misinterprets the '*' as infix operator, because it doesn't recognize
'AddressSpace *as' is a declaration.  Sorry, this isn't much of a clue,
but you didn't provide much of a reproducer.

Re: [Qemu-devel] checkpatch.pl question

[Alexey Kardashevskiy]

On 06/06/2014 04:27 PM, Markus Armbruster wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> Hi!
>>
>> checkpatch.pl often complains on things like this:
>>
>> ===
>> ERROR: need consistent spacing around '*' (ctx:WxV)
>> #57: FILE: hw/misc/vfio.c:4323:
>> +int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>>                                        ^
>>
>> total: 1 errors, 0 warnings, 46 lines checked
>> ===
>>
>> Since perl is a write-only language, I cannot understand why :) Any clue?
> 
> It misinterprets the '*' as infix operator, because it doesn't recognize
> 'AddressSpace *as' is a declaration.  Sorry, this isn't much of a clue,
> but you didn't provide much of a reproducer.
> 

Ooops. Sorry about that.

The failing patch has been posted today:
[PATCH v8 2/4] vfio: Add vfio_container_ioctl()

And the failing chunk is:

diff --git a/include/hw/misc/vfio.h b/include/hw/misc/vfio.h
new file mode 100644
index 0000000..0b26cd8
--- /dev/null
+++ b/include/hw/misc/vfio.h
@@ -0,0 +1,9 @@
+#ifndef VFIO_API_H
+#define VFIO_API_H
+
+#include "qemu/typedefs.h"
+
+extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
+                                int req, void *param);
+
+#endif




-- 
Alexey

Re: [Qemu-devel] checkpatch.pl question

[Markus Armbruster]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 06/06/2014 04:27 PM, Markus Armbruster wrote:
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>> 
>>> Hi!
>>>
>>> checkpatch.pl often complains on things like this:
>>>
>>> ===
>>> ERROR: need consistent spacing around '*' (ctx:WxV)
>>> #57: FILE: hw/misc/vfio.c:4323:
>>> +int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>>>                                        ^
>>>
>>> total: 1 errors, 0 warnings, 46 lines checked
>>> ===
>>>
>>> Since perl is a write-only language, I cannot understand why :) Any clue?
>> 
>> It misinterprets the '*' as infix operator, because it doesn't recognize
>> 'AddressSpace *as' is a declaration.  Sorry, this isn't much of a clue,
>> but you didn't provide much of a reproducer.
>> 
>
> Ooops. Sorry about that.
>
> The failing patch has been posted today:
> [PATCH v8 2/4] vfio: Add vfio_container_ioctl()
>
> And the failing chunk is:
>
> diff --git a/include/hw/misc/vfio.h b/include/hw/misc/vfio.h
> new file mode 100644
> index 0000000..0b26cd8
> --- /dev/null
> +++ b/include/hw/misc/vfio.h
> @@ -0,0 +1,9 @@
> +#ifndef VFIO_API_H
> +#define VFIO_API_H
> +
> +#include "qemu/typedefs.h"
> +
> +extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> +                                int req, void *param);
> +
> +#endif

--debug values=1 produces

188 > . extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
188 > EEVVVVVVVTTTTVVVVVVVVVVVVVVVVVVVVNTTTTTTTTTTTTTTVVCCTTTTTTTTVVVVVVVCC

which suggests it recognizes the declaration just fine.

Copying a few possible victims^Wexperts.

Re: [Qemu-devel] checkpatch.pl question

[Peter Maydell]

On 6 June 2014 08:17, Markus Armbruster <armbru@redhat.com> wrote:
> --debug values=1 produces
>
> 188 > . extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> 188 > EEVVVVVVVTTTTVVVVVVVVVVVVVVVVVVVVNTTTTTTTTTTTTTTVVCCTTTTTTTTVVVVVVVCC
>
> which suggests it recognizes the declaration just fine.
>
> Copying a few possible victims^Wexperts.

It's clearly something to do with it getting confused by the type name,
because you can suppress the warning by just changing it so it has
an "_t" suffix, for instance. In particular notice that the set of allowed
type names constructed by the build_types() subroutine is extremely
limited: it looks to me as if the script makes assumptions based on
kernel style (where 'struct foo' is preferred over a typedef and plain
'foo') that allow it to assume that if it's not one of a very few allowed
formats then it's not a type name.

I find this script extremely hard to understand, though.

thanks
-- PMM

Re: [Qemu-devel] checkpatch.pl question

[Stefan Weil]

Am 07.06.2014 16:58, schrieb Peter Maydell:
> On 6 June 2014 08:17, Markus Armbruster <armbru@redhat.com> wrote:
>> --debug values=1 produces
>>
>> 188 > . extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>> 188 > EEVVVVVVVTTTTVVVVVVVVVVVVVVVVVVVVNTTTTTTTTTTTTTTVVCCTTTTTTTTVVVVVVVCC
>>
>> which suggests it recognizes the declaration just fine.
>>
>> Copying a few possible victims^Wexperts.
> 
> It's clearly something to do with it getting confused by the type name,
> because you can suppress the warning by just changing it so it has
> an "_t" suffix, for instance. In particular notice that the set of allowed
> type names constructed by the build_types() subroutine is extremely
> limited: it looks to me as if the script makes assumptions based on
> kernel style (where 'struct foo' is preferred over a typedef and plain
> 'foo') that allow it to assume that if it's not one of a very few allowed
> formats then it's not a type name.
> 
> I find this script extremely hard to understand, though.
> 
> thanks
> -- PMM
> 

Yes, but that's only part of the story. checkpatch.pl contains some
special handling for the standard C data types and also knows some Linux
data type patterns. It also handles the above case correctly in most
circumstances because there is special code for "*" used in function
argument lists.

Here checkpatch.pl gets confused by a totally different line of the patch:

 type_init(register_vfio_pci_dev_type)

Simply adding a semicolon at the end of that line would help, but is of
course not correct (note that there is already some QEMU code which adds
such a semicolon and which should be fixed). It's generally very
difficult or even impossible for code analysers with a limited view to
analyse macro calls correctly. A human reviewer has the same problem.

checkpatch.pl could be improved to handle the patch correctly: the
type_init line and the line which raises the error message belong to
different files. Obviously some global state variables are not reset
when checkpatch.pl detects patch code belonging to a new file. I'm still
searching which ones. A similar problem might also occur when more than
one patch is checked: this also does not reset all relevant variables
when switching from one patch file to the next one. The original Linux
version of checkpatch.pl shows the same bug.

Regards
Stefan

PS. I just sent a first patch for checkpatch.pl which is totally
unrelated to the above problem, but which I noticed while I investigated it.

Re: [Qemu-devel] checkpatch.pl question

[Peter Maydell]

On 7 June 2014 17:00, Stefan Weil <sw@weilnetz.de> wrote:
> Am 07.06.2014 16:58, schrieb Peter Maydell:
>> It's clearly something to do with it getting confused by the type name,
>> because you can suppress the warning by just changing it so it has
>> an "_t" suffix, for instance. In particular notice that the set of allowed
>> type names constructed by the build_types() subroutine is extremely
>> limited: it looks to me as if the script makes assumptions based on
>> kernel style (where 'struct foo' is preferred over a typedef and plain
>> 'foo') that allow it to assume that if it's not one of a very few allowed
>> formats then it's not a type name.

> Yes, but that's only part of the story. checkpatch.pl contains some
> special handling for the standard C data types and also knows some Linux
> data type patterns. It also handles the above case correctly in most
> circumstances because there is special code for "*" used in function
> argument lists.

Interesting. There are obviously multiple situations where it
fails in this way for different reasons. This is the case I saw yesterday:

@@ -125,19 +125,20 @@ static TCGRegSet tcg_target_available_regs[2];
 static TCGRegSet tcg_target_call_clobber_regs;

 #if TCG_TARGET_INSN_UNIT_SIZE == 1
-static inline void tcg_out8(TCGContext *s, uint8_t v)
+static __attribute__((unused)) inline void tcg_out8(TCGContext *s, uint8_t v)
 {
     *s->code_ptr++ = v;
 }

No macros or previous context for it to get confused by.
Saying "TCGContext_t" instead silences the warning.

The debug output shows that here it's failed to parse the
type as a type:

22 > . static __attribute__((unused)) inline void tcg_out8(TCGContext
*s, uint8_t v)
22 > EEVVVVVVVNNNNNNNNNNNNNNNVVVVVVVVVVVVVVVVTTTTTVVVVVVVVNVVVVVVVVVVVNVCCTTTTTTTTVVV
22 >  ________________________________________________________________B______________

(thanks for pointing out that that existed).

-- PMM

Re: [Qemu-devel] checkpatch.pl question

[Paolo Bonzini]

Il 07/06/2014 18:27, Peter Maydell ha scritto:
> On 7 June 2014 17:00, Stefan Weil <sw@weilnetz.de> wrote:
>> Am 07.06.2014 16:58, schrieb Peter Maydell:
>>> It's clearly something to do with it getting confused by the type name,
>>> because you can suppress the warning by just changing it so it has
>>> an "_t" suffix, for instance. In particular notice that the set of allowed
>>> type names constructed by the build_types() subroutine is extremely
>>> limited: it looks to me as if the script makes assumptions based on
>>> kernel style (where 'struct foo' is preferred over a typedef and plain
>>> 'foo') that allow it to assume that if it's not one of a very few allowed
>>> formats then it's not a type name.
>
>> Yes, but that's only part of the story. checkpatch.pl contains some
>> special handling for the standard C data types and also knows some Linux
>> data type patterns. It also handles the above case correctly in most
>> circumstances because there is special code for "*" used in function
>> argument lists.
>
> Interesting. There are obviously multiple situations where it
> fails in this way for different reasons. This is the case I saw yesterday:
>
> @@ -125,19 +125,20 @@ static TCGRegSet tcg_target_available_regs[2];
>  static TCGRegSet tcg_target_call_clobber_regs;
>
>  #if TCG_TARGET_INSN_UNIT_SIZE == 1
> -static inline void tcg_out8(TCGContext *s, uint8_t v)
> +static __attribute__((unused)) inline void tcg_out8(TCGContext *s, uint8_t v)
>  {
>      *s->code_ptr++ = v;
>  }
>
> No macros or previous context for it to get confused by.
> Saying "TCGContext_t" instead silences the warning.

Given QEMU's coding style, any camelcase identifier (or equivalently and 
identifier containing an uppercase and a lowercase letter) could be 
considered a type.

Paolo