[PATCH] checkpatch.pl: Add warning for harmful goto labels

[PATCH] checkpatch.pl: Add warning for harmful goto labels

[L. Alberto Giménez]

Issue a warning for too broad goto labels that may make the code to
follow the wrong exit path, thus causing hard to debug bugs.

Signed-off-by: L. Alberto Giménez <agimenez@sysvalve.es>
---
 scripts/checkpatch.pl | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d124359..e8ce220 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -444,6 +444,11 @@ our $allowed_asm_includes = qr{(?x:
 )};
 # memory.h: ARM has a custom one
 
+our @goto_harmful_labels = qw(
+	out
+	fail
+	);
+
 # Load common spelling mistakes and build regular expression list.
 my $misspellings;
 my %spelling_fix;
@@ -2702,6 +2707,14 @@ sub process {
 			}
 		}
 
+		if ($sline =~ /goto (.*);/) {
+			my $label = $1;
+			if (grep { /^$label$/ } @goto_harmful_labels) {
+				WARN("HARMFUL_GOTO_LABEL",
+					"Goto label '$label' is considered harmful\n" . $herecurr);
+			}
+		}
+
 # discourage the addition of CONFIG_EXPERIMENTAL in #if(def).
 		if ($line =~ /^\+\s*\#\s*if.*\bCONFIG_EXPERIMENTAL\b/) {
 			WARN("CONFIG_EXPERIMENTAL",
-- 
2.1.4

Re: [PATCH] checkpatch.pl: Add warning for harmful goto labels

[Richard Weinberger]

On Sat, Mar 21, 2015 at 10:16 PM, L. Alberto Giménez
<agimenez@sysvalve.es> wrote:
> Issue a warning for too broad goto labels that may make the code to
> follow the wrong exit path, thus causing hard to debug bugs.
>
> Signed-off-by: L. Alberto Giménez <agimenez@sysvalve.es>
> ---
>  scripts/checkpatch.pl | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d124359..e8ce220 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -444,6 +444,11 @@ our $allowed_asm_includes = qr{(?x:
>  )};
>  # memory.h: ARM has a custom one
>
> +our @goto_harmful_labels = qw(
> +       out
> +       fail
> +       );
> +
>  # Load common spelling mistakes and build regular expression list.
>  my $misspellings;
>  my %spelling_fix;
> @@ -2702,6 +2707,14 @@ sub process {
>                         }
>                 }
>
> +               if ($sline =~ /goto (.*);/) {
> +                       my $label = $1;
> +                       if (grep { /^$label$/ } @goto_harmful_labels) {
> +                               WARN("HARMFUL_GOTO_LABEL",
> +                                       "Goto label '$label' is considered harmful\n" . $herecurr);

Huh? Since when?

rw@azrael:~/linux (for-v4.1/uml_misc $)> git grep -e "goto out;" | wc -l
26667
rw@azrael:~/linux (for-v4.1/uml_misc $)> git grep -e "goto fail;" | wc -l
3733

What is next? Variable name "i" considered harmful?
Can we please stop this nonsense.

-- 
Thanks,
//richard

Re: [PATCH] checkpatch.pl: Add warning for harmful goto labels

[Peter Hurley]

Hi Alberto,

On 03/21/2015 05:16 PM, L. Alberto Giménez wrote:
> Issue a warning for too broad goto labels that may make the code to
> follow the wrong exit path, thus causing hard to debug bugs.

What compiler allowed duplicate goto labels?

Regards,
Peter Hurley

Re: [PATCH] checkpatch.pl: Add warning for harmful goto labels

[L. Alberto Giménez]

On Sat, Mar 21, 2015 at 10:40:46PM +0100, Richard Weinberger wrote:
> Huh? Since when?

There are a lot of cases where a too generic goto label for cleanup
causes a bug or makes debugging harder.

Last time was this G+ post, by Dan Carpenter:

https://plus.google.com/106378716002406849458/posts/DfuAkt8szf2


> rw@azrael:~/linux (for-v4.1/uml_misc $)> git grep -e "goto out;" | wc -l
> 26667
> rw@azrael:~/linux (for-v4.1/uml_misc $)> git grep -e "goto fail;" | wc -l
> 3733

If something is already in the kernel code, does that mean that it's OK?
I honestly don't think so, and I think that goto labels for cleanup exit
paths should be a little more descriptive.

> What is next? Variable name "i" considered harmful?

No one complained about that so far. I might add that to checkpatch.pl
if needed.

> Can we please stop this nonsense.

It's just a proposal for a warning. If it is really not needed, it won't
be applied and life will go on :)

-- 
L. Alberto Giménez
GnuPG key ID 0xDD4E27AB

Re: [PATCH] checkpatch.pl: Add warning for harmful goto labels

[L. Alberto Giménez]

On Sat, Mar 21, 2015 at 05:41:04PM -0400, Peter Hurley wrote:
> Hi Alberto,
> 
> On 03/21/2015 05:16 PM, L. Alberto Giménez wrote:
> > Issue a warning for too broad goto labels that may make the code to
> > follow the wrong exit path, thus causing hard to debug bugs.
> 
> What compiler allowed duplicate goto labels?

Humans are bad at following code paths, and making modifications to such
code may lead to errors if the code is not descriptive enough, please
check my reply to Richard for an explanation.

Regards,
-- 
L. Alberto Giménez
GnuPG key ID 0xDD4E27AB

Re: [PATCH] checkpatch.pl: Add warning for harmful goto labels

[Richard Weinberger]

Am 21.03.2015 um 23:06 schrieb L. Alberto Giménez:
> On Sat, Mar 21, 2015 at 10:40:46PM +0100, Richard Weinberger wrote:
>> Huh? Since when?
> 
> There are a lot of cases where a too generic goto label for cleanup
> causes a bug or makes debugging harder.
> 
> Last time was this G+ post, by Dan Carpenter:
> 
> https://plus.google.com/106378716002406849458/posts/DfuAkt8szf2
> 
> 
>> rw@azrael:~/linux (for-v4.1/uml_misc $)> git grep -e "goto out;" | wc -l
>> 26667
>> rw@azrael:~/linux (for-v4.1/uml_misc $)> git grep -e "goto fail;" | wc -l
>> 3733
> 
> If something is already in the kernel code, does that mean that it's OK?
> I honestly don't think so, and I think that goto labels for cleanup exit
> paths should be a little more descriptive.

I disagree. out and exit are perfectly fine labels.

>> What is next? Variable name "i" considered harmful?
> 
> No one complained about that so far. I might add that to checkpatch.pl
> if needed.

I *really* *really* hope you're kidding.

>> Can we please stop this nonsense.
> 
> It's just a proposal for a warning. If it is really not needed, it won't
> be applied and life will go on :)

checkpatch.pl is already more than annoying. It used to be a nice tool but
it becomes more and more an harassment for guys who actually work on the kernel.

Thanks,
//richard

Re: [PATCH] checkpatch.pl: Add warning for harmful goto labels

[Joe Perches]

On Sat, 2015-03-21 at 23:12 +0100, Richard Weinberger wrote:
> Am 21.03.2015 um 23:06 schrieb L. Alberto Giménez:
> > There are a lot of cases where a too generic goto label for cleanup
> > causes a bug or makes debugging harder.
[]
> > If something is already in the kernel code, does that mean that it's OK?
> > I honestly don't think so, and I think that goto labels for cleanup exit
> > paths should be a little more descriptive.
> 
> I disagree. out and exit are perfectly fine labels.

I agree with you Richard.

While the form of the patch is fine, but content is not.

There might be a case for a coccinelle style patch that
looks for more than a single label in a function and looks
at the label name choices, but I think it'd be pretty
dubious at best.

> > It's just a proposal for a warning. If it is really not needed, it won't
> > be applied and life will go on :)

In that case, it'd be nicer to preface the patch subject with RFC

> checkpatch.pl is already more than annoying. It used to be a nice tool but
> it becomes more and more an harassment for guys who actually work on the kernel.

Richard, what sub-optimal messages do you think checkpatch
produces by default?

cheers,  Joe

Re: [PATCH] checkpatch.pl: Add warning for harmful goto labels

[L. Alberto Giménez]

On Sat, Mar 21, 2015 at 07:56:39PM -0700, Joe Perches wrote:
> []
> > > If something is already in the kernel code, does that mean that it's OK?
> > > I honestly don't think so, and I think that goto labels for cleanup exit
> > > paths should be a little more descriptive.
> > 
> > I disagree. out and exit are perfectly fine labels.
> 
> I agree with you Richard.
> 
[...]
> > > It's just a proposal for a warning. If it is really not needed, it won't
> > > be applied and life will go on :)
> 
> In that case, it'd be nicer to preface the patch subject with RFC

Yes, you are right, sorry about that.

Thank you for the feedback guys!

And Richard, of course I was kidding about the 'i' variable. I enjoy
sarcasm myself too :)

Best regards,
-- 
L. Alberto Giménez
GnuPG key ID 0xDD4E27AB