[PATCH] pinctrl: ingenic: Make unreachable path more robust

[PATCH] pinctrl: ingenic: Make unreachable path more robust

[Josh Poimboeuf]

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

Re: [PATCH] pinctrl: ingenic: Make unreachable path more robust

[Paul Cercueil]

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
> 

Re: [PATCH] pinctrl: ingenic: Make unreachable path more robust

[Josh Poimboeuf]

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

Re: [PATCH] pinctrl: ingenic: Make unreachable path more robust

[Randy Dunlap]

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

Re: [PATCH] pinctrl: ingenic: Make unreachable path more robust

[Paul Cercueil]

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

Re: [PATCH] pinctrl: ingenic: Make unreachable path more robust

[Josh Poimboeuf]

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;
 		}
 	}
 

Re: [PATCH] pinctrl: ingenic: Make unreachable path more robust

[Paul Cercueil]

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;
>  		}
>  	}
> 
>