LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] pinctrl: ingenic: Make unreachable path more robust
@ 2020-02-14 16:37 Josh Poimboeuf
  2020-02-14 19:02 ` Paul Cercueil
  2020-02-14 21:52 ` Randy Dunlap
  0 siblings, 2 replies; 7+ messages in thread
From: Josh Poimboeuf @ 2020-02-14 16:37 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Linus Walleij, linux-gpio, linux-kernel, Peter Zijlstra, Randy Dunlap

In the second loop of ingenic_pinconf_set(), it annotates the switch
default case as unreachable().  The annotation is technically correct,
because that same case would have resulted in an early return in the
previous loop.

However, if a bug were to get introduced later, for example if an
additional case were added to the first loop without adjusting the
second loop, it would result in nasty undefined behavior: most likely
the function's generated code would fall through to the next function.

Another issue is that, while objtool normally understands unreachable()
annotations, there's one special case where it doesn't: when the
annotation occurs immediately after a 'ret' instruction.  That happens
to be the case here because unreachable() is immediately before the
return.

So change the unreachable() to BUG() so that the unreachable code, if
ever executed, would panic instead of introducing undefined behavior.
This also makes objtool happy.

This fixes the following objtool warning:

  drivers/pinctrl/pinctrl-ingenic.o: warning: objtool: ingenic_pinconf_set() falls through to next function ingenic_pinconf_group_set()

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 drivers/pinctrl/pinctrl-ingenic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c
index 96f04d121ebd..6b61ac6cd4d2 100644
--- a/drivers/pinctrl/pinctrl-ingenic.c
+++ b/drivers/pinctrl/pinctrl-ingenic.c
@@ -2158,7 +2158,7 @@ static int ingenic_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 			break;
 
 		default:
-			unreachable();
+			BUG();
 		}
 	}
 
-- 
2.21.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] pinctrl: ingenic: Make unreachable path more robust
  2020-02-14 16:37 [PATCH] pinctrl: ingenic: Make unreachable path more robust Josh Poimboeuf
@ 2020-02-14 19:02 ` Paul Cercueil
  2020-02-14 20:37   ` Josh Poimboeuf
  2020-02-14 21:52 ` Randy Dunlap
  1 sibling, 1 reply; 7+ messages in thread
From: Paul Cercueil @ 2020-02-14 19:02 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Linus Walleij, linux-gpio, linux-kernel, Peter Zijlstra, Randy Dunlap

Hi Josh,


Le ven., févr. 14, 2020 at 10:37, Josh Poimboeuf <jpoimboe@redhat.com> 
a écrit :
> In the second loop of ingenic_pinconf_set(), it annotates the switch
> default case as unreachable().  The annotation is technically correct,
> because that same case would have resulted in an early return in the
> previous loop.
> 
> However, if a bug were to get introduced later, for example if an
> additional case were added to the first loop without adjusting the
> second loop, it would result in nasty undefined behavior: most likely
> the function's generated code would fall through to the next function.
> 
> Another issue is that, while objtool normally understands 
> unreachable()
> annotations, there's one special case where it doesn't: when the
> annotation occurs immediately after a 'ret' instruction.  That happens
> to be the case here because unreachable() is immediately before the
> return.
> 
> So change the unreachable() to BUG() so that the unreachable code, if
> ever executed, would panic instead of introducing undefined behavior.
> This also makes objtool happy.

I don't like the idea that you change this driver's code just to work 
around a bug in objtool, and I don't like the idea of working around a 
future bug that shouldn't be introduced in the first place.

-Paul

> 
> This fixes the following objtool warning:
> 
>   drivers/pinctrl/pinctrl-ingenic.o: warning: objtool: 
> ingenic_pinconf_set() falls through to next function 
> ingenic_pinconf_group_set()
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  drivers/pinctrl/pinctrl-ingenic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-ingenic.c 
> b/drivers/pinctrl/pinctrl-ingenic.c
> index 96f04d121ebd..6b61ac6cd4d2 100644
> --- a/drivers/pinctrl/pinctrl-ingenic.c
> +++ b/drivers/pinctrl/pinctrl-ingenic.c
> @@ -2158,7 +2158,7 @@ static int ingenic_pinconf_set(struct 
> pinctrl_dev *pctldev, unsigned int pin,
>  			break;
> 
>  		default:
> -			unreachable();
> +			BUG();
>  		}
>  	}
> 
> --
> 2.21.1
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] pinctrl: ingenic: Make unreachable path more robust
  2020-02-14 19:02 ` Paul Cercueil
@ 2020-02-14 20:37   ` Josh Poimboeuf
  2020-02-15  2:37     ` Paul Cercueil
  0 siblings, 1 reply; 7+ messages in thread
From: Josh Poimboeuf @ 2020-02-14 20:37 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Linus Walleij, linux-gpio, linux-kernel, Peter Zijlstra, Randy Dunlap

On Fri, Feb 14, 2020 at 04:02:18PM -0300, Paul Cercueil wrote:
> Hi Josh,
> 
> 
> Le ven., févr. 14, 2020 at 10:37, Josh Poimboeuf <jpoimboe@redhat.com> a
> écrit :
> > In the second loop of ingenic_pinconf_set(), it annotates the switch
> > default case as unreachable().  The annotation is technically correct,
> > because that same case would have resulted in an early return in the
> > previous loop.
> > 
> > However, if a bug were to get introduced later, for example if an
> > additional case were added to the first loop without adjusting the
> > second loop, it would result in nasty undefined behavior: most likely
> > the function's generated code would fall through to the next function.
> > 
> > Another issue is that, while objtool normally understands unreachable()
> > annotations, there's one special case where it doesn't: when the
> > annotation occurs immediately after a 'ret' instruction.  That happens
> > to be the case here because unreachable() is immediately before the
> > return.
> > 
> > So change the unreachable() to BUG() so that the unreachable code, if
> > ever executed, would panic instead of introducing undefined behavior.
> > This also makes objtool happy.
> 
> I don't like the idea that you change this driver's code just to work around
> a bug in objtool, and I don't like the idea of working around a future bug
> that shouldn't be introduced in the first place.

It's not an objtool bug.  It's a byproduct of the fact that GCC's
undefined behavior is inscrutable, and there's no way to determine that
it actually *wants* to jump to a random function.

And anyway, regardless of objtool, the patch is meant to make the code
more robust.

Do you not agree that BUG (defined behavior) is more robust than
unreachable (undefined behavior)?

-- 
Josh


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] pinctrl: ingenic: Make unreachable path more robust
  2020-02-14 16:37 [PATCH] pinctrl: ingenic: Make unreachable path more robust Josh Poimboeuf
  2020-02-14 19:02 ` Paul Cercueil
@ 2020-02-14 21:52 ` Randy Dunlap
  1 sibling, 0 replies; 7+ messages in thread
From: Randy Dunlap @ 2020-02-14 21:52 UTC (permalink / raw)
  To: Josh Poimboeuf, Paul Cercueil
  Cc: Linus Walleij, linux-gpio, linux-kernel, Peter Zijlstra

On 2/14/20 8:37 AM, Josh Poimboeuf wrote:
> In the second loop of ingenic_pinconf_set(), it annotates the switch
> default case as unreachable().  The annotation is technically correct,
> because that same case would have resulted in an early return in the
> previous loop.
> 
> However, if a bug were to get introduced later, for example if an
> additional case were added to the first loop without adjusting the
> second loop, it would result in nasty undefined behavior: most likely
> the function's generated code would fall through to the next function.
> 
> Another issue is that, while objtool normally understands unreachable()
> annotations, there's one special case where it doesn't: when the
> annotation occurs immediately after a 'ret' instruction.  That happens
> to be the case here because unreachable() is immediately before the
> return.
> 
> So change the unreachable() to BUG() so that the unreachable code, if
> ever executed, would panic instead of introducing undefined behavior.
> This also makes objtool happy.
> 
> This fixes the following objtool warning:
> 
>   drivers/pinctrl/pinctrl-ingenic.o: warning: objtool: ingenic_pinconf_set() falls through to next function ingenic_pinconf_group_set()
> 
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested

Thanks.

> ---
>  drivers/pinctrl/pinctrl-ingenic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c
> index 96f04d121ebd..6b61ac6cd4d2 100644
> --- a/drivers/pinctrl/pinctrl-ingenic.c
> +++ b/drivers/pinctrl/pinctrl-ingenic.c
> @@ -2158,7 +2158,7 @@ static int ingenic_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
>  			break;
>  
>  		default:
> -			unreachable();
> +			BUG();
>  		}
>  	}
>  
> 

-- 
~Randy


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] pinctrl: ingenic: Make unreachable path more robust
  2020-02-14 20:37   ` Josh Poimboeuf
@ 2020-02-15  2:37     ` Paul Cercueil
  2020-02-17 15:18       ` Josh Poimboeuf
  0 siblings, 1 reply; 7+ messages in thread
From: Paul Cercueil @ 2020-02-15  2:37 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Linus Walleij, linux-gpio, linux-kernel, Peter Zijlstra, Randy Dunlap



Le ven., févr. 14, 2020 at 14:37, Josh Poimboeuf <jpoimboe@redhat.com> 
a écrit :
> On Fri, Feb 14, 2020 at 04:02:18PM -0300, Paul Cercueil wrote:
>>  Hi Josh,
>> 
>> 
>>  Le ven., févr. 14, 2020 at 10:37, Josh Poimboeuf 
>> <jpoimboe@redhat.com> a
>>  écrit :
>>  > In the second loop of ingenic_pinconf_set(), it annotates the 
>> switch
>>  > default case as unreachable().  The annotation is technically 
>> correct,
>>  > because that same case would have resulted in an early return in 
>> the
>>  > previous loop.
>>  >
>>  > However, if a bug were to get introduced later, for example if an
>>  > additional case were added to the first loop without adjusting the
>>  > second loop, it would result in nasty undefined behavior: most 
>> likely
>>  > the function's generated code would fall through to the next 
>> function.
>>  >
>>  > Another issue is that, while objtool normally understands 
>> unreachable()
>>  > annotations, there's one special case where it doesn't: when the
>>  > annotation occurs immediately after a 'ret' instruction.  That 
>> happens
>>  > to be the case here because unreachable() is immediately before 
>> the
>>  > return.
>>  >
>>  > So change the unreachable() to BUG() so that the unreachable 
>> code, if
>>  > ever executed, would panic instead of introducing undefined 
>> behavior.
>>  > This also makes objtool happy.
>> 
>>  I don't like the idea that you change this driver's code just to 
>> work around
>>  a bug in objtool, and I don't like the idea of working around a 
>> future bug
>>  that shouldn't be introduced in the first place.
> 
> It's not an objtool bug.  It's a byproduct of the fact that GCC's
> undefined behavior is inscrutable, and there's no way to determine 
> that
> it actually *wants* to jump to a random function.
> 
> And anyway, regardless of objtool, the patch is meant to make the code
> more robust.
> 
> Do you not agree that BUG (defined behavior) is more robust than
> unreachable (undefined behavior)?

It's a dead code path. That would be an undefined behaviour, if it was 
taken, but it's not.

-Paul



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] pinctrl: ingenic: Make unreachable path more robust
  2020-02-15  2:37     ` Paul Cercueil
@ 2020-02-17 15:18       ` Josh Poimboeuf
  2020-02-20  1:36         ` Paul Cercueil
  0 siblings, 1 reply; 7+ messages in thread
From: Josh Poimboeuf @ 2020-02-17 15:18 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Linus Walleij, linux-gpio, linux-kernel, Peter Zijlstra, Randy Dunlap

On Fri, Feb 14, 2020 at 11:37:04PM -0300, Paul Cercueil wrote:
> > >  I don't like the idea that you change this driver's code just to
> > > work around
> > >  a bug in objtool, and I don't like the idea of working around a
> > > future bug
> > >  that shouldn't be introduced in the first place.
> > 
> > It's not an objtool bug.  It's a byproduct of the fact that GCC's
> > undefined behavior is inscrutable, and there's no way to determine that
> > it actually *wants* to jump to a random function.
> > 
> > And anyway, regardless of objtool, the patch is meant to make the code
> > more robust.
> > 
> > Do you not agree that BUG (defined behavior) is more robust than
> > unreachable (undefined behavior)?
> 
> It's a dead code path. That would be an undefined behaviour, if it was
> taken, but it's not.

Given your confidence that humans don't introduce bugs, would you
recommend that we

  s/BUG()/unreachable()/

tree-wide?

Another option would be to remove the unreachable() statement, which
would actually improve the generated code by making it more compact (16
bytes of i-cache savings), on top of removing the "fallthrough to next
function" nastiness.

diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c
index 96f04d121ebd..13c7d3351ed5 100644
--- a/drivers/pinctrl/pinctrl-ingenic.c
+++ b/drivers/pinctrl/pinctrl-ingenic.c
@@ -2158,7 +2158,8 @@ static int ingenic_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 			break;
 
 		default:
-			unreachable();
+			/* unreachable */
+			break;
 		}
 	}
 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] pinctrl: ingenic: Make unreachable path more robust
  2020-02-17 15:18       ` Josh Poimboeuf
@ 2020-02-20  1:36         ` Paul Cercueil
  0 siblings, 0 replies; 7+ messages in thread
From: Paul Cercueil @ 2020-02-20  1:36 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Linus Walleij, linux-gpio, linux-kernel, Peter Zijlstra, Randy Dunlap

Hi Josh,

Le lun., févr. 17, 2020 at 09:18, Josh Poimboeuf <jpoimboe@redhat.com> 
a écrit :
> On Fri, Feb 14, 2020 at 11:37:04PM -0300, Paul Cercueil wrote:
>>  > >  I don't like the idea that you change this driver's code just 
>> to
>>  > > work around
>>  > >  a bug in objtool, and I don't like the idea of working around a
>>  > > future bug
>>  > >  that shouldn't be introduced in the first place.
>>  >
>>  > It's not an objtool bug.  It's a byproduct of the fact that GCC's
>>  > undefined behavior is inscrutable, and there's no way to 
>> determine that
>>  > it actually *wants* to jump to a random function.
>>  >
>>  > And anyway, regardless of objtool, the patch is meant to make the 
>> code
>>  > more robust.
>>  >
>>  > Do you not agree that BUG (defined behavior) is more robust than
>>  > unreachable (undefined behavior)?
>> 
>>  It's a dead code path. That would be an undefined behaviour, if it 
>> was
>>  taken, but it's not.
> 
> Given your confidence that humans don't introduce bugs, would you
> recommend that we
> 
>   s/BUG()/unreachable()/
> 
> tree-wide?

Of course not.

> Another option would be to remove the unreachable() statement, which
> would actually improve the generated code by making it more compact 
> (16
> bytes of i-cache savings), on top of removing the "fallthrough to next
> function" nastiness.

I'd prefer that, yes.

-Paul

> diff --git a/drivers/pinctrl/pinctrl-ingenic.c 
> b/drivers/pinctrl/pinctrl-ingenic.c
> index 96f04d121ebd..13c7d3351ed5 100644
> --- a/drivers/pinctrl/pinctrl-ingenic.c
> +++ b/drivers/pinctrl/pinctrl-ingenic.c
> @@ -2158,7 +2158,8 @@ static int ingenic_pinconf_set(struct 
> pinctrl_dev *pctldev, unsigned int pin,
>  			break;
> 
>  		default:
> -			unreachable();
> +			/* unreachable */
> +			break;
>  		}
>  	}
> 
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-14 16:37 [PATCH] pinctrl: ingenic: Make unreachable path more robust Josh Poimboeuf
2020-02-14 19:02 ` Paul Cercueil
2020-02-14 20:37   ` Josh Poimboeuf
2020-02-15  2:37     ` Paul Cercueil
2020-02-17 15:18       ` Josh Poimboeuf
2020-02-20  1:36         ` Paul Cercueil
2020-02-14 21:52 ` Randy Dunlap

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git