Messages in this thread Patches in this message | | | Date | Tue, 27 Dec 2022 10:55:17 +0100 | From | Uwe Kleine-König <> | Subject | Re: coccinelle: How to remove a return at the end of a void function? |
| |
Hello Julia,
On Mon, Dec 26, 2022 at 05:02:35PM +0100, Julia Lawall wrote: > On Sun, 25 Dec 2022, Uwe Kleine-König wrote: > > > Hello Julia, > > > > first of all thanks for your quick answer. > > > > On Sat, Dec 24, 2022 at 01:28:04PM +0100, Julia Lawall wrote: > > > On Sat, 24 Dec 2022, Uwe Kleine-König wrote: > > > > A simplified spatch looks as follows: > > > > > > > > -------->8-------- > > > > virtual patch > > > > > > > > @p1@ > > > > identifier pdev; > > > > @@ > > > > -int > > > > +void > > > > rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) { > > > > <... > > > > -return 0; > > > > +return; > > > > ...> > > > > } > > > > -------->8-------- > > > > > > > > This results in: > > > > > > > > -------->8-------- > > > > diff -u -p a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c > > > > --- a/drivers/mmc/host/rtsx_usb_sdmmc.c > > > > +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c > > > > @@ -1379,13 +1379,13 @@ static int rtsx_usb_sdmmc_drv_probe(stru > > > > return 0; > > > > } > > > > > > > > -static int rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) > > > > +static void rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) > > > > { > > > > struct rtsx_usb_sdmmc *host = platform_get_drvdata(pdev); > > > > struct mmc_host *mmc; > > > > > > > > if (!host) > > > > - return 0; > > > > + return; > > > > > > > > mmc = host->mmc; > > > > host->host_removal = true; > > > > @@ -1416,7 +1416,7 @@ static int rtsx_usb_sdmmc_drv_remove(str > > > > dev_dbg(&(pdev->dev), > > > > ": Realtek USB SD/MMC module has been removed\n"); > > > > > > > > - return 0; > > > > + return; > > > > } > > > > > > > > #ifdef CONFIG_PM > > > > -------->8-------- > > > > > > > > which is as intended. Now I want to remove the useless "return;" at the > > > > end of the function, however adding > > > > > > > > -------->8-------- > > > > @p2 depends on p1@ > > > > identifier pdev; > > > > @@ > > > > void rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) { > > > > ... > > > > -return; > > > > } > > > > -------->8-------- > > > > > > > > to the spatch doesn't (only) do the intended: > > > > > > The problem is that Coccinelle is following the control-flow through the > > > function, and all of the returns are at the end of a control.flow path. > > > The simple, hacky solution is to change the return;s into some function > > > call Return();, then do like the above for Return(); and then change the > > > Return();s back to return;s > > > > OK, I tried, but somehow coccinelle refuse to work after I introduced > > Return(), even replacing them by return; doesn't work: > > > > -------->8-------- > > virtual patch > > > > @p1@ > > identifier pdev; > > @@ > > -int > > +void > > rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) { > > ... > > -return 0; > > +Return(); > > ... > > } > > > > @p2@ > > identifier pdev; > > @@ > > void rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) { > > ... > > -Return(); > > +return; > > ... > > } > > The problem is that a control-flow path at this point can have multiple > calls to Return(); You pattern only matches when every control-flow path > through the code has exactly one Return().
Ah, ok. This wasn't clear to me from reading the documentation (e.g. https://coccinelle.gitlabpages.inria.fr/website/docs/main_grammar.html)
> You should have one rule that removes the final Return(); > > @p2@ > identifier pdev; > @@ > void rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) { > ... > -Return(); > } > > Then another rule to remove the others: > > @p2@ > identifier pdev; > @@ > void rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) { > <... > -Return(); > +return; > ...> > }
I now have
-------->8-------- virtual patch
@p1@ identifier pdev; @@ -int +void rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) { ... -return 0; +Return(); ... }
@p2@ identifier pdev; @@ void rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) { ... -Return(); }
@p3@ identifier pdev; @@ void rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) { <... -Return(); +return; ...> } -------->8--------
But there p2 suffers from the same problem and only matches code paths with exactly 1 Return(). So the above results in
-------->8-------- diff -u -p a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c --- a/drivers/mmc/host/rtsx_usb_sdmmc.c +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c @@ -1379,13 +1379,13 @@ static int rtsx_usb_sdmmc_drv_probe(stru return 0; }
-static int rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) +static void rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) { struct rtsx_usb_sdmmc *host = platform_get_drvdata(pdev); struct mmc_host *mmc;
if (!host) - return 0; + return;
mmc = host->mmc; host->host_removal = true; @@ -1416,7 +1416,7 @@ static int rtsx_usb_sdmmc_drv_remove(str dev_dbg(&(pdev->dev), ": Realtek USB SD/MMC module has been removed\n");
- return 0; + return; }
#ifdef CONFIG_PM -------->8-------- and only if I remove the "if (!host) return 0;" block before patch generation, the final return is also dropped. I think this is good enough for me, as there are not too many cases like the above. If there is spatch that does the desired change (i.e.
-------->8-------- diff -u -p a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c --- a/drivers/mmc/host/rtsx_usb_sdmmc.c +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c @@ -1379,13 +1379,11 @@ static int rtsx_usb_sdmmc_drv_probe(stru return 0; }
-static int rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) +static void rtsx_usb_sdmmc_drv_remove(struct platform_device *pdev) { struct rtsx_usb_sdmmc *host = platform_get_drvdata(pdev); struct mmc_host *mmc;
if (!host) - return 0; + return;
mmc = host->mmc; host->host_removal = true; @@ -1416,7 +1416,7 @@ static int rtsx_usb_sdmmc_drv_remove(str dev_dbg(&(pdev->dev), ": Realtek USB SD/MMC module has been removed\n"); - - return 0; }
#ifdef CONFIG_PM -------->8-------- ) I'd be happy to hear and learn about it. Thanks Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |[unhandled content-type:application/pgp-signature] | |