| Message ID | 73f0c9915473d9e4b3681fb5cc55144291a43192.1581698101.git.jpoimboe@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: ingenic: Make unreachable path more robust | expand |
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 >
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)?
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(); > } > } > >
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
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; } }
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; > } > } > >
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(); } }
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(-)