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

[Paul Cercueil]

Le ven., févr. 14, 2020 at 14:37, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 
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@xxxxxxxxxx> 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