checkpatch.pl: disable arch-specific test for linux-user

Message ID 1474919934-21518-1-git-send-email-riku.voipio@linaro.org
State New
Headers show

Commit Message

Riku Voipio Sept. 26, 2016, 7:58 p.m.
From: Riku Voipio <riku.voipio@linaro.org>


Linux-user and bsd-user code needs lots of arch-specific ifdefs,
so disable the warning.

Signed-off-by: Riku Voipio <riku.voipio@linaro.org>

---
 scripts/checkpatch.pl | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

-- 
2.1.4

Comments

Peter Maydell Sept. 26, 2016, 9:08 p.m. | #1
On 26 September 2016 at 12:58,  <riku.voipio@linaro.org> wrote:
> From: Riku Voipio <riku.voipio@linaro.org>

>

> Linux-user and bsd-user code needs lots of arch-specific ifdefs,

> so disable the warning.

>

> Signed-off-by: Riku Voipio <riku.voipio@linaro.org>

> ---

>  scripts/checkpatch.pl | 5 +++--

>  1 file changed, 3 insertions(+), 2 deletions(-)

>

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

> index dde3f5f..98a007f 100755

> --- a/scripts/checkpatch.pl

> +++ b/scripts/checkpatch.pl

> @@ -2405,8 +2405,9 @@ sub process {

>                 }

>  # check of hardware specific defines

>  # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases

> -# where they might be necessary.

> -               if ($line =~ m@^.\s*\#\s*if.*\b__@) {

> +# where they might be necessary. Skip test on linux-user and bsd-user

> +# where arch defines are needed

> +               if (!($realfile =~ /^(linux|bsd)-user/) &&  $line =~ m@^.\s*\#\s*if.*\b__@) {

>                         ERROR("architecture specific defines should be avoided\n" .  $herecurr);

>                 }


Do you have some examples of the false positives you want
to suppress here? For new code I would hope that we can
handle host-arch-specifics by having new files (or just
new #defines etc) in linux-user/host/$ARCH/ rather than
inline #ifdeffery in the main files.

thanks
-- PMM
Riku Voipio Sept. 26, 2016, 11:36 p.m. | #2
On 27 September 2016 at 00:08, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 26 September 2016 at 12:58,  <riku.voipio@linaro.org> wrote:

>> From: Riku Voipio <riku.voipio@linaro.org>

>>

>> Linux-user and bsd-user code needs lots of arch-specific ifdefs,

>> so disable the warning.

>>

>> Signed-off-by: Riku Voipio <riku.voipio@linaro.org>

>> ---

>>  scripts/checkpatch.pl | 5 +++--

>>  1 file changed, 3 insertions(+), 2 deletions(-)

>>

>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

>> index dde3f5f..98a007f 100755

>> --- a/scripts/checkpatch.pl

>> +++ b/scripts/checkpatch.pl

>> @@ -2405,8 +2405,9 @@ sub process {

>>                 }

>>  # check of hardware specific defines

>>  # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases

>> -# where they might be necessary.

>> -               if ($line =~ m@^.\s*\#\s*if.*\b__@) {

>> +# where they might be necessary. Skip test on linux-user and bsd-user

>> +# where arch defines are needed

>> +               if (!($realfile =~ /^(linux|bsd)-user/) &&  $line =~ m@^.\s*\#\s*if.*\b__@) {

>>                         ERROR("architecture specific defines should be avoided\n" .  $herecurr);

>>                 }

>

> Do you have some examples of the false positives you want

> to suppress here? For new code I would hope that we can

> handle host-arch-specifics by having new files (or just

> new #defines etc) in linux-user/host/$ARCH/ rather than

> inline #ifdeffery in the main files.


One example from your patch:

https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05650.html

And another from Laurent:

https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg06486.html

Every new syscall will comes with "#ifdef TARGET_NR_foo and
defined(__NR_foo)", while host/target combos catch up. Now, most
TARGET_NR_foo's are needed only for unicore32, but the __NR_foo
defines will be needed for a very long time.

Riku
Peter Maydell Sept. 27, 2016, 12:21 a.m. | #3
On 26 September 2016 at 16:36, Riku Voipio <riku.voipio@linaro.org> wrote:
> On 27 September 2016 at 00:08, Peter Maydell <peter.maydell@linaro.org> wrote:

>> Do you have some examples of the false positives you want

>> to suppress here? For new code I would hope that we can

>> handle host-arch-specifics by having new files (or just

>> new #defines etc) in linux-user/host/$ARCH/ rather than

>> inline #ifdeffery in the main files.

>

> One example from your patch:

>

> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05650.html

>

> And another from Laurent:

>

> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg06486.html

>

> Every new syscall will comes with "#ifdef TARGET_NR_foo and

> defined(__NR_foo)", while host/target combos catch up. Now, most

> TARGET_NR_foo's are needed only for unicore32, but the __NR_foo

> defines will be needed for a very long time.


Oh, I see; I don't think of the __NR_foo as being "architecture
specific". I think we'd be better off specifically whitelisting
those in checkpatch rather than turning off the whole check
for linux-user.

thanks
-- PMM
Riku Voipio Sept. 27, 2016, 1:36 p.m. | #4
On 27 September 2016 at 14:58, Paolo Bonzini <pbonzini@redhat.com> wrote:
>

>

> On 26/09/2016 21:58, riku.voipio@linaro.org wrote:

>> From: Riku Voipio <riku.voipio@linaro.org>

>>

>> Linux-user and bsd-user code needs lots of arch-specific ifdefs,

>> so disable the warning.

>>

>> Signed-off-by: Riku Voipio <riku.voipio@linaro.org>

>> ---

>>  scripts/checkpatch.pl | 5 +++--

>>  1 file changed, 3 insertions(+), 2 deletions(-)

>>

>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl

>> index dde3f5f..98a007f 100755

>> --- a/scripts/checkpatch.pl

>> +++ b/scripts/checkpatch.pl

>> @@ -2405,8 +2405,9 @@ sub process {

>>               }

>>  # check of hardware specific defines

>>  # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases

>> -# where they might be necessary.

>> -             if ($line =~ m@^.\s*\#\s*if.*\b__@) {

>> +# where they might be necessary. Skip test on linux-user and bsd-user

>> +# where arch defines are needed

>> +             if (!($realfile =~ /^(linux|bsd)-user/) &&  $line =~ m@^.\s*\#\s*if.*\b__@) {

>>                       ERROR("architecture specific defines should be avoided\n" .  $herecurr);

>>               }

>>

>>

>

> Hi Riku,

>

> I have already posted a pull request that degrades this to a warning.

>

> Later on we may make it an error except for some files and/or patterns.

> For linux-user I think that __NR_* should be definitely allowed, but a

> blanket permission is not necessary.


I'll update my patch against your PR and to check not only for
linux-user dir, but also match __NR_.

Riku

Patch hide | download patch | download mbox

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index dde3f5f..98a007f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2405,8 +2405,9 @@  sub process {
 		}
 # check of hardware specific defines
 # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases
-# where they might be necessary.
-		if ($line =~ m@^.\s*\#\s*if.*\b__@) {
+# where they might be necessary. Skip test on linux-user and bsd-user
+# where arch defines are needed
+		if (!($realfile =~ /^(linux|bsd)-user/) &&  $line =~ m@^.\s*\#\s*if.*\b__@) {
 			ERROR("architecture specific defines should be avoided\n" .  $herecurr);
 		}