Re: [1/2] string: Add stracpy and stracpy_pad mechanisms
> The rule now properly checks that the third argument is the size of the > first argument. This made a small reduction in the number of results. Thanks for your SmPL script adjustments. Will deviations from this restriction become more interesting? > \(strscpy\|strlcpy\)(e1.f, e2, i2)@p Would you like to take the function “strscpy_pad” also into account for source code transformations with the macro “stracpy_pad”? Regards, Markus
Re: [1/2] string: Add stracpy and stracpy_pad mechanisms
> Huh? Rule 2 is important, to ensure that ths size is correct. I assume that the dependency of the replacement on the data structure check can become clearer. Regards, Markus
Re: [Cocci] [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms
> @r@ > identifier f,i1,i2; > struct i1 e1; > expression e2; > position p; > @@ > \(strscpy\|strlcpy\)(e1.f, e2, i2)@p I have got the impression that the replacement can work also without an inherited position variable at the end. How do you think about to omit this SmPL rule then? Can it be nicer to reduce duplicate SmPL code a bit? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [1/2] string: Add stracpy and stracpy_pad mechanisms
>>> @@ >>> ( >>> -x = strlcpy >>> +stracpy >>> (e1.f, e2 >>> -, i2 >>> )@p; >>> ... when != x >>> >>> | >> >> I wonder about the deletion of the assignment target. >> Should the setting of such a variable be usually preserved? > > If it is a local variable and never subsequently used, it doesn't seem > very useful. Such an explanation is easier to understand. * How do you think about the possibility that it was (accidentally) forgotten to use such a local variable? * Your transformation can result in an intentionally unused return value. Would you like point any more source code places out where values are unused so far? Regards, Markus
Re: [Cocci] [PATCH 1/2] string: Add stracpy and stracpy_pad mechanisms
> New version. I check for non-use of the return value of strlcpy and > address some issues that affected the matching of the case where the first > argument involves a pointer dereference. I suggest to take another look at corresponding implementation details of the shown SmPL script. > \(strscpy\|strlcpy\)(e1.f, e2, i2)@p Can the data access operator “->” (arrow) matter also here? > @@ > identifier r.i1,r.i2; > type T; > @@ > struct i1 { ... T i1[i2]; ... } Will an additional SmPL rule name be helpful for this part? > @@ > ( > -x = strlcpy > +stracpy > (e1.f, e2 > -, i2 > )@p; > ... when != x > > | I wonder about the deletion of the assignment target. Should the setting of such a variable be usually preserved? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [v4 3/3] coccinelle: Add script to check for platform_get_irq() excessive prints
>> Will further software development considerations become more interesting >> also around a contribution like “Coccinelle: Add a SmPL script for >> the reconsideration of redundant dev_err() calls”? >> https://lore.kernel.org/lkml/2744a3fc-9e67-8113-1dd9-43669e063...@web.de/ >> https://lore.kernel.org/patchwork/patch/1095937/ >> https://lkml.org/lkml/2019/7/1/145 >> https://systeme.lip6.fr/pipermail/cocci/2019-July/006071.html >> > > Did this patch ever get merged? Not yet. - I hope that this addition is still in the usual patch review queue. > It seems better to amend that patch instead of introduce another one. Further extensions can become more challenging for this software area. How would you like to adjust common implementation details? Regards, Markus
Re: [v4 3/3] coccinelle: Add script to check for platform_get_irq() excessive prints
>>> +if (ret != -EPROBE_DEFER) >> >> Is it appropriate to treat this error code check as optional >> by the shown transformation approach? >> Can this case distinction be omitted? > > I don't know what you mean here. I suggest to take another look at the importance and relevance of this specific source code search detail (including SmPL disjunctions). > Do you want me to drop this part so that EPROBE_DEFER checks don't get > removed? No, not at the moment. - But I am still looking for further clarification of the desired software design. So I am curious how a corresponding agreement will evolve. Regards, Markus
Re: [Cocci] [PATCH v4 3/3] coccinelle: Add script to check for platform_get_irq() excessive prints
> +@script:python depends on org@ > +p1 << r.p1; > +@@ > + > +cocci.print_main(p1) Will an additional message be helpful at this place? Will further software development considerations become more interesting also around a contribution like “Coccinelle: Add a SmPL script for the reconsideration of redundant dev_err() calls”? https://lore.kernel.org/lkml/2744a3fc-9e67-8113-1dd9-43669e063...@web.de/ https://lore.kernel.org/patchwork/patch/1095937/ https://lkml.org/lkml/2019/7/1/145 https://systeme.lip6.fr/pipermail/cocci/2019-July/006071.html Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [v4 2/3] treewide: Remove dev_err() usage after platform_get_irq()
> > > struct platform_device *E; How much does this specification matter for the parameters of the mentioned functions (in the SmPL script)? Will the selection of function names be sufficient for the discussed source code search pattern? > > Can you teach it to remove curly braces when it's appropriate? > > (see below for examples) > > I don't know if that works. Such an adjustment depends on additional development efforts. > Is there some sort of tidy script I can run on my patches to do this? You can add corresponding case distinctions to your transformation approach for the semantic patch language (on demand), can't you? Regards, Markus
Re: [PATCH v4 3/3] coccinelle: Add script to check for platform_get_irq() excessive prints
I would prefer to concentrate the usage of SmPL disjunctions on changing implementation details so that the specification of duplicate code can be avoided. > +( > +platform_get_irq(E, ...) > +| > +platform_get_irq_byname(E, ...) > +); Function names: +(platform_get_irq +|platform_get_irq_byname +)(E, ...); > +if ( \( ret < 0 \| ret <= 0 \) ) Comparison operators: +if (ret \( < \| <= \) 0) > +if (ret != -EPROBE_DEFER) Is it appropriate to treat this error code check as optional by the shown transformation approach? Can this case distinction be omitted? Regards, Markus
Re: [Cocci] Coccinelle for Go
> But I also write tools that use the Go AST package to rewrite Go, Would you like to share any more experiences around related software? > and have never felt the lack of coccinelle for Go. I find this impression interesting. How much did you get used to functionality which is supported by the semantic patch language? > I'm curious what you see as the use case. Are you looking for wider support of a known programming interface? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Coccinelle for Go
> We are looking into using Coccinelle for refactoring in Go Thanks for your interest. > Does Coccinelle include support for Go? Not yet. > * Is there any plan to add support for the same? Would you like to contribute significant development resources for corresponding software extensions? > * What would it take to add this support? Remarkable desire for collateral evolution. > Any documentation regarding this? Yes, of course. Did you notice information sources like the following already? * Paper “Computation tree logic with variables and witnesses” http://coccinelle.lip6.fr/papers/popl09.pdf https://doi.org/10.1145/1480881.1480897 * Manual for the semantic patch language https://github.com/coccinelle/coccinelle/blob/ed1eb8e06f800739d3992158d36945c0c4c6f0c7/docs/manual/cocci_syntax.tex#L50 * Clarification request: Support for more programming languages? https://systeme.lip6.fr/pipermail/cocci/2016-July/003445.html https://lore.kernel.org/cocci/76bf1017-d629-d44a-5493-0dcccbbfa...@users.sourceforge.net/ How do you think about to improve related technologies? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v3] Coccinelle: semantic code search for “use after …”
> Finally, this patch finds use-after-free issues for a node. > (implemented by the r_use_after_put rule) I suggest to take another look also at information from a clarification attempt on a topic like “Checking statement order for patch generation with SmPL support”. https://systeme.lip6.fr/pipermail/cocci/2017-September/004483.html https://lore.kernel.org/cocci/alpine.DEB.2.20.1709071519240.3168@hadrien/ Under which circumstances will it become safer to develop SmPL script variants for such source code search patterns? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [v3] Coccinelle: semantic code search for “use after …”
> Finally, this patch finds use-after-free issues for a node. > (implemented by the r_use_after_put rule) I suggest to take another look also at information from a clarification attempt on a topic like “Checking statement order for patch generation with SmPL support”. https://systeme.lip6.fr/pipermail/cocci/2017-September/004483.html https://lore.kernel.org/cocci/alpine.DEB.2.20.1709071519240.3168@hadrien/ Under which circumstances will it become safer to develop SmPL script variants for such source code search patterns? Regards, Markus
Re: [Cocci] [v3] coccinelle: semantic code search for missing of_node_put
> 2), Then SmPL A generates another SmPL B based on the function name list; This would be a general data processing possibility. Another option would be to let SmPL scripts to import relevant data from external files or to query facts from databases. > You expect the entire process above to be automated. I hope that this can be achieved finally. > This idea may be interesting, Thanks for your feedback. > but it can't be done now, I got an other view. - Why is your view so limited at the moment? > and it will introduce uncontrollable factors. I suggest to take additional design options into account so that you might get more control on some factors. Which software development challenges are still waiting for better solutions? > We agree with julia's comments: > I would prefer not to put semantic patches that involve iteration into the > kernel, for simplicity. I guess that this kind of change reluctance can be also adjusted. Some source code analysis approaches can look simple enough while advanced ones will show more of the inherent complexity. > Our file is called of_node_put.cocci, which contains three rules: r_miss_put, > r_miss_put_ext and r_use_after_put. This combination is interesting, isn't it? > If you separate them, it seems inappropriate. * Would you like to be able to let each source code analysis task to be executed on its own? * I guess that it can become possible with additional development efforts to support also a mixture of analysis patterns. * The patch subject “… missing …” does probably not fit to the detection “use after …”. >>> v3: delete the global set, … >> >> To which previous implementation detail do you refer here? > > Here is an improvement based on julia's comments: > https://lkml.org/lkml/2019/7/5/55 I would find an other description clearer then. * Drop of functions around “add_if_not_present” * Omission of iteration functionality Are any more adjustments worth to be explicitly mentioned in this patch change log? > Here are some improvements. Are you going to contribute further patch versions? > Adding an asterisk here is more convenient to use, This might be. - I wonder how good additional data fit to supported output formats. > it can mark the location of the code of interest, such as: I know its functionality also. - I got the impression that the use of SmPL asterisks will be safe for the operation mode “context”. >>> +... when != e = (T)x >>> +when != true x == NULL >> >> Will assignment exclusions get any more software development attention? >> https://lore.kernel.org/lkml/03cc4df5-ce7f-ba91-36b5-687fec8c7...@web.de/ >> https://lore.kernel.org/patchwork/patch/1095169/#1291892 >> https://lkml.org/lkml/2019/6/29/193 Will this aspect evolve further anyhow? >> You propose once more to use a SmPL conjunction in the rule “r_miss_put_ext”. >> I am also still waiting for a definitive explanation on the applicability >> of this combination. Would you like to clarify this software detail any more? >>> +@r_use_after_put exists@ >>> +expression r_put.E, subE<=r_put.E; >> >> I have got an understanding difficulty around the interpretation >> of the shown SmPL constraint. >> How will the clarification be continued? More helpful information? > +| > + f(...,c,...,(T)E,...) I would interpret such passing of a pointer for a device node as an undesirable “use after free (or put)”. Will this SmPL disjunction need further adjustments? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [v3] coccinelle: semantic code search for missing of_node_put
>> Why would you like to keep this SmPL code in the commit description? > > I don't know indetail what you are proposing, I imagine that you can get more interesting software development ideas from links to previous messages. I hope that the desired clarification can become more constructive. How are the chances to move such code into SmPL script files? > but I would prefer not to put semantic patches that involve iteration > into the kernel, for simplicity. This view is also interesting. But I hope that this functionality will become more helpful if we can agree on value combinations which should be iterated for powerful source code analysis. >> I would prefer software evolution in an other direction. >> https://lore.kernel.org/lkml/44be5924-26ca-5106-aa25-3cbc3343a...@web.de/ >> https://lkml.org/lkml/2019/7/4/21 Would you like to add any more advices for affected software components? Regards, Markus
Re: [v3] coccinelle: semantic code search for missing of_node_put
>> Why would you like to keep this SmPL code in the commit description? > > I don't know indetail what you are proposing, I imagine that you can get more interesting software development ideas from links to previous messages. I hope that the desired clarification can become more constructive. How are the chances to move such code into SmPL script files? > but I would prefer not to put semantic patches that involve iteration > into the kernel, for simplicity. This view is also interesting. But I hope that this functionality will become more helpful if we can agree on value combinations which should be iterated for powerful source code analysis. >> I would prefer software evolution in an other direction. >> https://lore.kernel.org/lkml/44be5924-26ca-5106-aa25-3cbc3343a...@web.de/ >> https://lkml.org/lkml/2019/7/4/21 Would you like to add any more advices for affected software components? Regards, Markus
Re: [PATCH v3] coccinelle: semantic code search for missing of_node_put
> We find these functions by using the following script: Why would you like to keep this SmPL code in the commit description? I would prefer software evolution in an other direction. https://lore.kernel.org/lkml/44be5924-26ca-5106-aa25-3cbc3343a...@web.de/ https://lkml.org/lkml/2019/7/4/21 > @initialize:ocaml@ > @@ > > let relevant_str = "use of_node_put() on it when done" I see further possibilities to improve this data processing approach. https://lore.kernel.org/lkml/904b9362-cd01-ffc9-600b-0c4884861...@web.de/ https://lore.kernel.org/patchwork/patch/1095169/#1291378 https://lkml.org/lkml/2019/6/28/326 I am missing more constructive answers for mentioned development concerns. > And this patch also looks for places … Does a SmPL script perform an action? > Finally, this patch finds use-after-free issues for a node. > (implemented by the r_use_after_put rule) This software extension is another interesting contribution. But I imagine that a separate SmPL script can be more helpful for this source code search pattern. > v3: delete the global set, … To which previous implementation detail do you refer here? > +virtual report > +virtual org > + > +@initialize:python@ > +@@ > + > +report_miss_prefix = "ERROR: missing of_node_put; acquired a node pointer > with refcount incremented on line " > +report_miss_suffix = ", but without a corresponding object release within > this function." > +org_miss_main = "acquired a node pointer with refcount incremented" > +org_miss_sec = "needed of_node_put" > +report_use_after_put = "ERROR: use-after-free; reference preceded by > of_node_put on line " > +org_use_after_put_main = "of_node_put" > +org_use_after_put_sec = "reference" If you would insist on the usage of these variables, they should be applied only for the selected analysis operation mode. I would expect corresponding SmPL dependency specifications. https://github.com/coccinelle/coccinelle/blob/b4509f6e7fb06d5616bb19dd5a110b203fd0e566/docs/manual/cocci_syntax.tex#L559 > +@r_miss_put exists@ > +local idexpression struct device_node *x; > +expression e, e1; > +position p1, p2; > +statement S; > +type T, T1; > +@@ > + > +* x = @p1\(of_find_all_nodes\| The usage of the SmPL asterisk functionality can fit to the operation mode “context”. https://bottest.wiki.kernel.org/coccicheck#modes Would you like to add any corresponding SmPL details? Under which circumstances will remaining programming concerns be clarified for such SmPL disjunctions? > +... when != e = (T)x > +when != true x == NULL Will assignment exclusions get any more software development attention? https://lore.kernel.org/lkml/03cc4df5-ce7f-ba91-36b5-687fec8c7...@web.de/ https://lore.kernel.org/patchwork/patch/1095169/#1291892 https://lkml.org/lkml/2019/6/29/193 > +when != of_node_put(x) … > +) > +& > +x = f(...) > +... > +if (<+...x...+>) S > +... > +of_node_put(x); > +) You propose once more to use a SmPL conjunction in the rule “r_miss_put_ext”. I am also still waiting for a definitive explanation on the applicability of this combination. > +@r_put@ > +expression E; > +position p1; > +@@ > + > +* of_node_put@p1(E) I guess that this SmPL code will need further adjustments. > +@r_use_after_put exists@ > +expression r_put.E, subE<=r_put.E; I have got an understanding difficulty around the interpretation of the shown SmPL constraint. How will the clarification be continued? Regards, Markus
Re: [PATCH] scripts: coccinelle: add check for uncessary double unlikely() calls
My software development attention was caught also by your proposal. I would appreciate a message subject without a typo. > +virtual patch > +virtual context > +virtual org > +virtual report These metavariables are not used in the subsequent code in this approach for the semantic patch language. Fine-tuning might become relevant also for the change specifications. https://lore.kernel.org/lkml/1559767582-11081-1-git-send-email-i...@metux.net/ Would you like to contribute an extended script version? Regards, Markus
Re: [Cocci] NULL pointer constraints vs. compiler checks
> we recently have lots of patches for either adding missing NULL checks, > removing unneeded ones, etc. Would you like to point any more concrete change examples out? > It seems that's an non-trivial terrain, This area can be also challenging. > so I'm thinking about potential compiler support. Can any more development tools help? > Does anybody know whether gcc has some way for not-null constraints ? Will you get corresponding answers better from other information systems than the mailing list here? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Compiling with OCaml 4.08
> Has anyone attempted to port coccinelle to OCaml 4.08? I am unsure also about this detail. > It seems to require multiple complex changes. I am waiting on corresponding software improvements according to a report like “Checking dependencies for current OCaml compiler version”. https://systeme.lip6.fr/pipermail/cocci/2019-June/006029.html https://lore.kernel.org/cocci/5e66ab8c-2232-ca87-b5bf-c93b3c1a1...@web.de/ Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: Coccinelle: Checking the deletion of duplicate of_node_put() calls with SmPL
> But I wonder at the moment why it does not work (as expected) for the original > complete source file. I discovered that a diff hunk (or usable patch?) is generated if the return statement is deleted (or commented out) before the jump label which refers to a potentially unwanted function call at the mentioned place. How will the support evolve for automatic adjustment of such source code combinations by the semantic patch language (Coccinelle software)? Regards, Markus
Re: Coccinelle: Checking the deletion of duplicate of_node_put() calls with SmPL
> 110: ierr_out: > 111: of_node_put(trng); ---> double released here > ... > > This issue was detected by using the Coccinelle software. Such a detection of a questionable source code place can be nice and helpful. I constructed another script variant for the semantic patch language. @deletion@ expression x; identifier target; @@ of_node_put(x); if (...) goto target; ... when any target: -of_node_put(x); I observe then that this adjustment approach can generate the desired patch for a source code extract. elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch ../janitor/delete_duplicate_of_node_put1.cocci crypto4xx_trng-excerpt1.c … - of_node_put(trng); … But I wonder at the moment why it does not work (as expected) for the original complete source file. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/amcc/crypto4xx_trng.c?id=5ad18b2e60b75c7297a998dea702451d33a052ed#n71 https://elixir.bootlin.com/linux/v5.2/source/drivers/crypto/amcc/crypto4xx_trng.c#L71 I am curious on further software development ideas. Regards, Markus
Re: Coccinelle: Handling of SmPL disjunctions
>>> How do you think about to increase the matching granularity >>> for this functionality? >> >> No idea what this means. Disjunctions are expanded up to the level of the >> nodes in the control-flow graph. > > We have got different expectations for working with such nodes > for possible (data flow) analysis. Will the chances become better to clarify this functionality? Which software components are involved so far? Regards, Markus
Re: Coccinelle: api: add devm_platform_ioremap_resource script
>> I will apply with Julia's Signed-off-by instead of Acked-by. >> I will also add SPDX tag. >> >> Is this OK? > > Yes, thanks. Will the clarification for following implementation details get any more software development attention? https://systeme.lip6.fr/pipermail/cocci/2019-June/005975.html https://lore.kernel.org/lkml/7b4fe770-dadd-80ba-2ba4-0f2bc9098...@web.de/ * The flag “IORESOURCE_MEM” * Exclusion of variable assignments by SmPL when constraints Regards, Markus
Re: [v2] coccinelle: semantic code search for missing of_node_put
>> @script:python depends on report && r1@ > > No need to depend on r1. That is guaranteed by the inheritance on the > metavariables below. Will this information become more helpful for the completion of the corresponding software documentation? Regards, Markus
Re: [Cocci] Pretty-printing of code for ternary operators?
>> elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch >> ../janitor/use_call_with_ternary_operator1.cocci ksm-excerpt1.c >> … >> +list_add_tail(&mm_slot->mm_list, >> + (ksm_run & KSM_RUN_UNMERGE) ? &ksm_mm_head.mm_list : >> &ksm_scan.mm_slot->mm_list); > > And this is unsatisfactory because why? Such a transformation result could be acceptable if the generated line length would be tolerated. But the second function parameter should be reformatted if we would like to care for the Linux coding style in such an use case finally. > I don't think ? should be used if it is going to end up over multiple lines. I got an other software development opinion. > In any case, I already commented that you proposed code was unreadable. I am also curious on how the clarification for a patch on a topic like “NFS: Less function calls in show_pnfs()” will evolve further. https://lore.kernel.org/patchwork/patch/1096614/#1293018 https://lore.kernel.org/lkml/ed599239-a6a2-1cc6-14ce-d5bb41e91...@web.de/ https://lkml.org/lkml/2019/7/3/173 Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Pretty-printing of code for ternary operators?
>> Would you like to achieve any improvements for automatic code beautification? >> https://github.com/coccinelle/coccinelle/issues/37 > > If you want to report a problem, I became curious again if more contributors would become interested to influence a possibly known software situation a bit more. > you must include a semantic patch @replacement@ expression check, context, x1, x2; identifier action; @@ -if (check) - action(context, x1); -else - action(context, x2); +action(context, (check) ? x1 : x2); > and C code that gives an unsatisfactory result. Source file example: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/ksm.c?id=550d1f5bda33fa3b203d8cf8df1396825dbfd213#n2500 https://elixir.bootlin.com/linux/v5.2-rc7/source/mm/ksm.c#L2500 // SPDX-License-Identifier: GPL-2.0-only // Deleted part int __ksm_enter(struct mm_struct *mm) { struct mm_slot *mm_slot; int needs_wakeup; mm_slot = alloc_mm_slot(); if (!mm_slot) return -ENOMEM; /* Check ksm_run too? Would need tighter locking */ needs_wakeup = list_empty(&ksm_mm_head.mm_list); spin_lock(&ksm_mmlist_lock); insert_to_mm_slots_hash(mm, mm_slot); // Deleted part if (ksm_run & KSM_RUN_UNMERGE) list_add_tail(&mm_slot->mm_list, &ksm_mm_head.mm_list); else list_add_tail(&mm_slot->mm_list, &ksm_scan.mm_slot->mm_list); spin_unlock(&ksm_mmlist_lock); // Deleted part return 0; } // Deleted part Another test result: elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch ../janitor/use_call_with_ternary_operator1.cocci ksm-excerpt1.c … + list_add_tail(&mm_slot->mm_list, + (ksm_run & KSM_RUN_UNMERGE) ? &ksm_mm_head.mm_list : &ksm_scan.mm_slot->mm_list); … How likely is it that the combined source code for a single function parameter will fit into known line length limitations? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] Pretty-printing of code for ternary operators?
Hello, The semantic patch language supports also to convert conditional statements into usages of the ternary operator. How much can the Coccinelle software help with pretty-printing of generated source code in this area? Would you like to achieve any improvements for automatic code beautification? https://github.com/coccinelle/coccinelle/issues/37 Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [v2] coccinelle: semantic code search for missing of_node_put
> We tested and found that both <...x...> and <+... x ...+> variants work fine. Is the difference in the functionality from this SmPL construct clear already? > We use <... x ...> instead of <+... x ...+> here to eliminate the following > false positives: Do we stumble on another software design challenge? For which function parameter will the specified variable be required finally? > 486 asd = v4l2_async_notifier_add_fwnode_subdev( > 487 &camss->notifier, of_fwnode_handle(remote), ---> > v4l2_async_notifier_add_fwnode_subdev will pass remote to camss->notifier. > 488 sizeof(*csd)); Should any more special cases be taken better into account? Regards, Markus
Re: [v2] coccinelle: semantic code search for missing of_node_put
> We will also provide an example written in Python later. Will the code move from the commit description into a file for your next patch version? > We first use this script to find out all the function names to be processed, I am still curious on how the output format selection will become clearer for the potentially desired automatic data conversion. > and then copy these function names into r1. Would this action be performed by another software build script? >>> +@initialize:python@ >>> +@@ >>> + >>> +seen = set() >>> + >>> +def add_if_not_present (p1, p2): >> >> It seems that you would like to use iteration functionality. I am waiting on another constructive answer for this implementation detail. >>> +x = @p1\(of_find_all_nodes\| >> >> I would find this SmPL disjunction easier to read without the usage >> of extra backslashes. >> >> +x = >> +(of_… >> +|of_… >> +)@p1(...); >> >> >> Which sort criteria were applied for the generation of the shown >> function name list? > > As julia pointed out, your current writing is not compiled. * It can be needed for a while to specify the mentioned position variable at an other place. * Would you like to adjust the SmPL coding style here? * Will the application of sort criteria be clarified for such identifier lists? >>> +if (x == NULL || ...) S >>> +... when != e = (T)x >>> +when != true x == NULL … > Our previous version used the "when any" clause, so we need > "when != true x == NULL". I suggest to reconsider further aspects for such constraints. > We can delete this code exclusion specification for this version. I would find another assignment exclusion more appropriate at this place. Regards, Markus
Re: [v2] Coccinelle: Suppression of warnings?
> By the way: do we have any mechanism for explicitly suppressing > individual warnings (some kind of annotation), I do not know such an accepted specification interface (for the handling together with SmPL scripts) so far. How would you identify possibly unwanted messages in a safe way? > when the maintainer is sure that some particular case is a false-positive ? Do you know any specific source code places already where you would get concerned about the confidence and relevance of the provided information? Regards, Markus
[PATCH v2] Coccinelle: Add a SmPL script for the reconsideration of redundant dev_err() calls
From: Markus Elfring Date: Mon, 1 Jul 2019 10:00:39 +0200 The function “devm_ioremap_resource” contains appropriate error reporting. Thus it can be questionable to present another error message at other places. Provide design options for the adjustment of affected source code by the means of the semantic patch language (Coccinelle software). Signed-off-by: Markus Elfring --- v2: Suggestions from Julia Lawall were integrated. * Application of the SmPL construct “<+... … ...+>” * Replacement of a return specification by a statement metavariable. * Different coding style for a branch of a SmPL disjunction. * Usage of a specific function name in two messages. .../coccinelle/misc/redundant_dev_err.cocci | 62 +++ 1 file changed, 62 insertions(+) create mode 100644 scripts/coccinelle/misc/redundant_dev_err.cocci diff --git a/scripts/coccinelle/misc/redundant_dev_err.cocci b/scripts/coccinelle/misc/redundant_dev_err.cocci new file mode 100644 index ..7998defb04f3 --- /dev/null +++ b/scripts/coccinelle/misc/redundant_dev_err.cocci @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: GPL-2.0 +/// Reconsider a function call for redundant error reporting. +// +// Keywords: dev_err redundant device error messages +// Confidence: Medium + +virtual patch +virtual context +virtual org +virtual report + +@display depends on context@ +expression e; +@@ + e = devm_ioremap_resource(...); + if (IS_ERR(e)) + { + <+... +* dev_err(...); + ...+> + } + +@deletion depends on patch@ +expression e; +statement s; +@@ + e = devm_ioremap_resource(...); + if (IS_ERR(e)) +( +-{ +- dev_err(...); +s +-} +| + { + <+... +- dev_err(...); + ...+> + } +) + +@or depends on org || report@ +expression e; +position p; +@@ + e = devm_ioremap_resource(...); + if (IS_ERR(e)) + { + <+... dev_err@p(...); ...+> + } + +@script:python to_do depends on org@ +p << or.p; +@@ +coccilib.org.print_todo(p[0], +"WARNING: An error message is probably not needed here because the devm_ioremap_resource() function contains appropriate error reporting.") + +@script:python reporting depends on report@ +p << or.p; +@@ +coccilib.report.print_report(p[0], + "WARNING: An error message is probably not needed here because the devm_ioremap_resource() function contains appropriate error reporting.") -- 2.22.0
Re: [v2] Coccinelle: Testing SmPL constraints
> Please actually try things out before declaring them to be useless. This feedback provides also another opportunity for collateral evolution in some directions. I am curious on how involved uncertainty can be fixed around possibly different interpretation for provided software functionality. The SmPL construct “... when …” is mentioned in an area (of the section “Basic transformations” in the software documentation) which is introduced with the wording “The grammar for the minus or plus slice of a transformation is as follows:”. https://github.com/coccinelle/coccinelle/blob/c6d7554edf7c4654aeae4d33c3f040e300682f23/docs/manual/cocci_syntax.tex#L1033 I got the impression that the corresponding meaning is not explained in this information source so far. The published example “Reference counter: the of_xxx API” can be interesting then to some degree for the explanation of the discussed development efforts. https://github.com/coccinelle/coccinelle/blob/175de16bc7e535b6a89a62b81a673b0d0cd7075c/docs/manual/examples.tex#L320 If the available application documentation is still too limited (and incomplete because it is also work in progress), it is probably usual that SmPL code occasionally tries to express expectations which are not covered by an evolving software implementation. How would you like to improve the situation further? * Is it certain that a search is performed only for the source code “x == NULL” (and corresponding isomorphisms) by the SmPL constraint “when != true” (after a successful null pointer check was detected in this use case)? * Would you like to test any functionality which should work in different ways than you might see from the original OCaml source code? https://github.com/coccinelle/coccinelle/issues/134 Regards, Markus
Re: [v2] coccinelle: semantic code search for missing of_node_put
+if (x == NULL || ...) S +... when != e = (T)x +when != true x == NULL … > I assume that it was added because it was found to be useful. We can get different software development opinions also on this implementation detail. > Please actually try things out before declaring them to be useless. Further clarification of desirable software behaviour will help. I dare to express doubts around the SmPL functionality “when != true x == NULL”. Would any more contributors like to share additional insights for the safer application of the semantic patch language? Is a reassignment of such local variable an usual precondition for the discussed programming concern? Regards, Markus
Re: [v2] coccinelle: semantic code search for missing of_node_put
>> +if (x == NULL || ...) S >> +... when != e = (T)x >> +when != true x == NULL > > I wonder if this code exclusion specification is really required > after a null pointer was checked before. I would like to add another view for this implementation detail. The when constraint can express a software desire which can be reasonable to some degree. You would like to be sure that a null pointer will not occur after a corresponding check succeeded. * But I feel unsure about the circumstances under which the Coccinelle software can determine this aspect actually. * I find that it can eventually make sense only after the content of the local variable (which is identified by “x”) was modified. Thus I would find the exclusion of assignments more useful at this place. Regards, Markus
Re: [v2] coccinelle: semantic code search for missing of_node_put
>> +x = >> +(of_… >> +|of_… >> +)@p1(...); > > Did you actually test this? I doubt that a position metavariable can be > put on a ) of a disjunction. Would you ever like to support this possibility? >> +|return >> +(x >> +|of_fwnode_handle(x) >> +); > > The original code is much more readable. We have got different views around such specification variants. > The internal representation will be the same. I imagine that the Coccinelle software might evolve into additional directions. Regards, Markus
Re: [v2] coccinelle: semantic code search for missing of_node_put
>> +x = >> +(of_… >> +|of_… >> +)@p1(...); > > Did you actually test this? I doubt that a position metavariable can be > put on a ) of a disjunction. Would you ever like to support this possibility? >> +|return >> +(x >> +|of_fwnode_handle(x) >> +); > > The original code is much more readable. We have got different views around such specification variants. > The internal representation will be the same. I imagine that the Coccinelle software might evolve into additional directions. Regards, Markus
Re: [PATCH v2] coccinelle: semantic code search for missing of_node_put
> The counter must be decremented after the last usage of a device node. Thanks for your next try to improve the software situation also in this area. > We find these functions by using the following SmPL: Would it be nicer to use the word “script” also here? > > @initialize:ocaml@ > @@ How do you think about to describe the chosen algorithm in a way for contributors who might not be so familiar with this programming language? Will any information from previous discussions become relevant for a better commit description? > let relevant_str = "use of_node_put() on it when done" Will such a literal need further development and software documentation considerations? > let contains s1 s2 = > let re = Str.regexp_string s2 > in > try ignore (Str.search_forward re s1 0); true > with Not_found -> false > > let relevant_functions = ref [] > > let add_function f c = > if not (List.mem f !relevant_functions) > then > begin > let s = String.concat " " I find such a concatenation suspicious after the space character is used also for a string splitting before. Can this delimiter be omitted for the combination? > ( > (List.map String.lowercase_ascii > (List.filter > (function x -> > Str.string_match > (Str.regexp "[a-zA-Z_\\(\\)][-a-zA-Z0-9_\\(\\)]*$") > x 0) (Str.split (Str.regexp "[ .;\t\n]+") c in > if contains s relevant_str I would prefer to use the string constant in the called function directly instead of passing it as another parameter. > then >Printf.printf "Found relevant function: %s\n" f; >relevant_functions := f :: !relevant_functions; > end I find your choice for an output format unclear at the moment. I imagine that the corresponding data processing of these function names will need fine-tuning. I am missing the software component for the conversion of this identifier list into a disjunction for the SmPL rule “r1”. > And this patch also looks for places where an of_node_put() Does a patch or a script perform an action? > call is on some paths but not on others. Let us look at mentioned implementation details. > +@initialize:python@ > +@@ > + > +seen = set() > + > +def add_if_not_present (p1, p2): It seems that you would like to use iteration functionality. https://github.com/coccinelle/coccinelle/blob/99e081e9b89d49301b7bd2c5e5aac88c66eaaa6a/docs/manual/cocci_syntax.tex#L1826 How will it matter here? > +def display_report(p1, p2): > +if add_if_not_present(p1[0].line, p2[0].line): > + coccilib.report.print_report(p2[0], > +"ERROR: missing of_node_put; acquired a > node pointer with refcount incremented on line " > ++ p1[0].line > ++ ", but without a corresponding object > release within this function.") > + > +def display_org(p1, p2): > +cocci.print_main("acquired a node pointer with refcount incremented", p1) > +cocci.print_secs("needed of_node_put", p2) Can it be helpful to specify SmPL dependencies for these functions according to the applied operation mode? > +x = @p1\(of_find_all_nodes\| I would find this SmPL disjunction easier to read without the usage of extra backslashes. +x = +(of_… +|of_… +)@p1(...); Which sort criteria were applied for the generation of the shown function name list? > +if (x == NULL || ...) S > +... when != e = (T)x > +when != true x == NULL I wonder if this code exclusion specification is really required after a null pointer was checked before. > +| > +return x; > +| > +return of_fwnode_handle(x); Can a nested SmPL disjunction be helpful at such places? +|return +(x +|of_fwnode_handle(x) +); > +when != v4l2_async_notifier_add_fwnode_subdev(<...x...>) Would the specification variant “<+... x ...+>” be relevant for the parameter selection? > +& > +x = f(...) > +... > +if (<+...x...+>) S > +... > +of_node_put(x); You propose once more to use a SmPL conjunction in the rule “r2”. How does it fit to the previous exclusion specification “when != of_node_put(x)”? Regards, Markus
Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?
>> Where did the Coccinelle software get the impression that anything >> would be added too often at the end of such a function implementation? > > Without the semantic patch and the C source code, You have access to data from both files already. > I can't answer the question. Is it more convenient to check test results for attachments again? Regards, Markus @replacement@ expression info, result; identifier target, work; type t != void; @@ t work(...) { <+... if (...) ( -{ -result = info; goto - target + e_nodev ; -} | { ... -result = info; goto - target + e_nodev ; } ) ...+> target: ... return result; +e_nodev: +result = info; +goto target; } // SPDX-License-Identifier: GPL-2.0-or-later // Deleted part static int megasas_mgmt_ioctl_fw(struct file *file, unsigned long arg) { struct megasas_iocpacket __user *user_ioc = (struct megasas_iocpacket __user *)arg; struct megasas_iocpacket *ioc; struct megasas_instance *instance; int error; ioc = memdup_user(user_ioc, sizeof(*ioc)); if (IS_ERR(ioc)) return PTR_ERR(ioc); instance = megasas_lookup_instance(ioc->host_no); if (!instance) { error = -ENODEV; goto out_kfree_ioc; } /* Block ioctls in VF mode */ if (instance->requestorId && !allow_vf_ioctls) { error = -ENODEV; goto out_kfree_ioc; } if (atomic_read(&instance->adprecovery) == MEGASAS_HW_CRITICAL_ERROR) { dev_err(&instance->pdev->dev, "Controller in crit error\n"); error = -ENODEV; goto out_kfree_ioc; } if (instance->unload == 1) { error = -ENODEV; goto out_kfree_ioc; } if (down_interruptible(&instance->ioctl_sem)) { error = -ERESTARTSYS; goto out_kfree_ioc; } if (megasas_wait_for_adapter_operational(instance)) { error = -ENODEV; goto out_up; } error = megasas_mgmt_fw_ioctl(instance, user_ioc, ioc); out_up: up(&instance->ioctl_sem); out_kfree_ioc: kfree(ioc); return error; } ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?
>> replacement: already tagged token: > > You try to add two things one one token, which is not allowed. How do you think about to clarify why a source file adjustment like the following can let my SmPL script succeed in a test configuration? elfring@Sonne:~/Projekte/Coccinelle/Probe> diff -u megaraid_sas-excerpt1.c megaraid_sas-excerpt2.c … @@ -32,24 +32,7 @@ goto out_kfree_ioc; } - if (instance->unload == 1) { - error = -ENODEV; - goto out_kfree_ioc; - } - - if (down_interruptible(&instance->ioctl_sem)) { - error = -ERESTARTSYS; - goto out_kfree_ioc; - } - - if (megasas_wait_for_adapter_operational(instance)) { - error = -ENODEV; - goto out_up; - } - - error = megasas_mgmt_fw_ioctl(instance, user_ioc, ioc); -out_up: - up(&instance->ioctl_sem); +// Deleted part out_kfree_ioc: kfree(ioc); Where did the Coccinelle software get the impression that anything would be added too often at the end of such a function implementation? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?
>>> You can try with the option --debug. >>> Or if that doesn't help with the option --verbose-ctl-engine. >> >> How can these parameters help to clarify undesirable run time >> characteristics? > > How about actually looking at the results yourself? I did that. - I do not find these extra data so helpful at the moment. The influence of presented background information on software execution speed is still unclear. >> replacement: already tagged token: > > You try to add two things one one token, which is not allowed. Are there any other software development challenges to consider for the insertion of a bit of source code at a function implementation end? … out_kfree_ioc: kfree(ioc); return error; } … Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?
> Maybe there are too many metavariable bindings. Under which circumstances would such a situation really occur? How do you think about my SmPL script example for the shown use case? > You can try with the option --debug. > Or if that doesn't help with the option --verbose-ctl-engine. How can these parameters help to clarify undesirable run time characteristics? Is it eventually easier to explain the following information? elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch --debug megaraid_sas-excerpt1.c ~/Projekte/Coccinelle/janitor/move_error_code_assignment_to_function_end1.cocci … replacement: already tagged token: C code context … Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Moving exception handling code to the end of a function implementation with SmPL?
> * The complete source file seems to be very challenging for testing > the run time characteristics. How are the chances to clarify the different test results for this source code transformation approach with the software combination “Coccinelle 1.0.7-00211-geaa13d59-dirty (OCaml 4.07.1)”? elfring@Sonne:~/Projekte/Linux/next-patched> spatch --profile ~/Projekte/Coccinelle/janitor/move_error_code_assignment_to_function_end1.cocci drivers/scsi/megaraid/megaraid_sas_base.c … timeout (we abort) … profiling result … *full_engine : 200.279202 sec 1 count *bigloop : 199.492610 sec 1 count *Rule replacement: 199.492608 sec 1 count *process_a_ctl_a_env_a_toplevel : 198.390209 sec 1 count *mysat : 198.390177 sec 1 count *ctl : 198.389955 sec 1 count process_a_ctl_a_env_a_toplevel : 1.102299 sec171 count … Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] More precise distinction of types for source code searches?
> @@ > expression x; > constant c1,c2; > @@ > > x = c1; The SmPL manual contains the promising wording “As metavariables are bound and inherited across rules, …”. https://github.com/coccinelle/coccinelle/blob/c6d7554edf7c4654aeae4d33c3f040e300682f23/docs/manual/cocci_syntax.tex#L179 The mentioned binding and inheritance can still become clearer. I guess that the Coccinelle software constructs corresponding internal data structures. The application experience shows that specific matched values can be directly reused in subsequent SmPL rules already. > ( > x = c1; Can it make sense then to support the direct access to a matched item also as a constraint within the same SmPL rule? How do you think about to work with backreferences to known data for further checking (or exclusion) of such source code? > | > *x = c2; > ) Will any SmPL constraint extensions result in the consequence to construct a special metavariable type? Will the software situation evolve further around the usage of such SmPL disjunctions? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] More precise distinction of types for source code searches?
> @@ > expression x; > constant c1,c2; > @@ > > x = c1; > ( > x = c1; > | > *x = c2; > ) Thanks for your suggestion of the possible usage of a SmPL disjunction. * Does it indicate a search attempt to match the first assignment statement twice (for the implementation of exclusion of duplicate source code)? * How many implementation details will become relevant for the corresponding documentation of the Coccinelle software? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] More precise distinction of types for source code searches?
> The expected difference can trigger the need to express this detail > by the usage of two identifiers based on the same metavariable type. I guess that this wording should be clarified a bit more according to the current software development status around the semantic patch language. A selection of different metavariable types is supported already. https://github.com/coccinelle/coccinelle/blob/c6d7554edf7c4654aeae4d33c3f040e300682f23/docs/manual/cocci_syntax.tex#L199 So it seems to be occasionally appropriate to use metavariables with the same type while different names are chosen then for the corresponding variable declaration. Another SmPL script example: @test@ constant c1, c2; @@ x = *c1 ; x = *c2 ; Such a source code search specification is too generic so far when you would really like to determine if these assignments (or its elements) are equivalent or even identical (or not). I am looking again for possibilities to improve language distinctions here. * How can a single metavariable remember a mapping to a previous instance from its type? * How will it become possible to match only a subset of a known base type? * How often do you want to exclude something because of a previous match with a similar type? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] More precise distinction of types for source code searches?
Hello, The semantic patch language supports metavariables. It can occur then that a source code search should be performed for items which should be different while they belong still to the same data type. (If these items would be identical, the software situation would provide opportunities to simplify corresponding source code.) The expected difference can trigger the need to express this detail by the usage of two identifiers based on the same metavariable type. SmPL script example: @display@ constant c1, c2; identifier x; @@ x = *c1 ; x = *c2 ; Source file example: int main(void) { int x; x = 1; x = 0; return x; } The used constants are integers and additional properties can be expressed for such numbers. These properties can eventually handled with scripted SmPL constraints. But I have got the impression that other metavariable types provide even more software development challenges for safe distinction of relevant items. I would like to point the detail out once more that also these metavariables can be of the same kind then with the consequence that it is not guaranteed that such metavariables will only match different source code. The support for the introduction of additional subtypes on demand might be too limited by the evolving software. How do you think about this use case? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking redundant variable initialisations with SmPL?
Am 23.06.19 um 15:13 schrieb Julia Lawall: > > > On Sun, 23 Jun 2019, Markus Elfring wrote: > >>> Try >>> >>> ... when any >>> >>> just before the final ). In some circumstaces the parser doesn't accept >>> an expression at the end of a sequence like you have here. >> >> Thanks for your quick response. >> >> The addition of such a SmPL ellipsis helps somehow. >> But I am still not pleased with the generated transformation result. >> So I am curious on how this software clarification will evolve further. > > I am not pleased is not especially descriptive. Partly, yes. > If you have a question about something, provide the semantic patch The discussed SmPL script “variant show_questionable_variable_initialisation5.cocci” (which is another work in progress): @display@ binary operator bo1, bo2; expression action, e1, e2 != e1, e3, e4, e5; identifier var; statement es1, is2, es2, is3, es3; type t; @@ ( t var = e1; <+... if (...) { var = e2; ... } else es1 ...+> if ( \( var \| var bo1 e3 \) ) is2 else es2 | t var * = e1 ; ... when != if ( \( var \| var bo2 e4 \) ) is3 else es3 when != action(..., var, ...) when != switch (var) { default: ... } when exists ( *e5 = <+... var ...+> | *var = e5 ) ... when any ) > and the code on which you get an undesired result. We have got access to the original source file which is another update candidate. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/lkdtm/core.c?id=4b972a01a7da614b4796475f933094751a295a2f#n386 https://elixir.bootlin.com/linux/v5.2-rc6/source/drivers/misc/lkdtm/core.c#L386 Intermediate test result: … @@ -284,8 +284,6 @@ static ssize_t lkdtm_debugfs_entry(struc const char __user *user_buf, size_t count, loff_t *off) { - struct crashpoint *crashpoint = file_inode(f)->i_private; - const struct crashtype *crashtype = NULL; char *buf; int err; @@ -303,13 +301,11 @@ static ssize_t lkdtm_debugfs_entry(struc buf[count] = '\0'; strim(buf); - crashtype = find_crashtype(buf); free_page((unsigned long)buf); if (!crashtype) return -EINVAL; - err = lkdtm_register_cpoint(crashpoint, crashtype); if (err < 0) return err; > I believe that you put a * on an assignment with var on the right hand side, I would like to stress the change possibility to delete a value for a variable initialisation if it would be really unused in the analysed function implementations. > which at least doesn't correspond to my understanding of what you > are trying to do, so perhaps that is the problem. 1. The patch hunk for the function “lkdtm_debugfs_entry” points out that the preprocessor symbol “NULL” could be omitted in this case. 2. Now I am missing the proposed deletion of the code “ = -EINVAL” for the function “lkdtm_module_init” (according to my SmPL patch attempt). See also a patch on the topic “[next] lkdtm: remove redundant initialization of ret”: https://lkml.org/lkml/2019/6/14/265 https://lore.kernel.org/patchwork/patch/1088971/ https://lore.kernel.org/lkml/20190614094311.24024-1-colin.k...@canonical.com/ We hope still on software development progress for the issue “Addition of do-while support for SmPL”. https://systeme.lip6.fr/pipermail/cocci/2019-April/005786.html https://lore.kernel.org/cocci/caagqs2ujute9tx4ai66aup8sy4zkryqexzkbioy8+hkmvug...@mail.gmail.com/ Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking redundant variable initialisations with SmPL?
> Try > > ... when any > > just before the final ). In some circumstaces the parser doesn't accept > an expression at the end of a sequence like you have here. Thanks for your quick response. The addition of such a SmPL ellipsis helps somehow. But I am still not pleased with the generated transformation result. So I am curious on how this software clarification will evolve further. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking redundant variable initialisations with SmPL?
> Try > > ... when any > > just before the final ). In some circumstaces the parser doesn't accept > an expression at the end of a sequence like you have here. Thanks for your quick response. The addition of such a SmPL ellipsis helps somehow. But I am still not pleased with the generated transformation result. So I am curious on how this software clarification will evolve further. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking redundant variable initialisations with SmPL?
> Yu can do whatever you want, but you will get lotsof false positives if > you keep it. If you really want a star on the declaration then you need > to make two rules. The first that finds the positions of the places that > match and the second that only puts a * when there is both a matched > declaration and a matched reinitialization. I hoped that the following SmPL script variant can be another approximation for the desired solution on the discussed source code search pattern. @display@ binary operator bo1, bo2; expression action, e1, e2 != e1, e3, e4, e5; identifier var; statement es1, is2, es2, is3, es3; type t; @@ ( t var = e1; <+... if (...) { var = e2; ... } else es1 ...+> if ( \( var \| var bo1 e3 \) ) is2 else es2 | t var * = e1 ; ... when != if ( \( var \| var bo2 e4 \) ) is3 else es3 when != action(..., var, ...) when != switch (var) { default: ... } when exists ( *e5 = <+... var ...+> | *var = e5 ) ) But I stumble on the following error message. elfring@Sonne:~/Projekte/Linux/next-patched> git checkout next-20190620 && spatch drivers/misc/lkdtm/core.c ~/Projekte/Coccinelle/janitor/show_questionable_variable_initialisation5.cocci … minus: parse error: File "/home/elfring/Projekte/Coccinelle/janitor/show_questionable_variable_initialisation5.cocci", line 36, column 0, charpos = 492 around = ')', whole content = ) I observed then that each of the two main branches in the shown SmPL disjunction can work as expected. So I wonder even more why the combination can not be parsed by the software “Coccinelle 1.0.7-00211-geaa13d59 (OCaml 4.07.1)” so far. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking redundant variable initialisations with SmPL?
> Yu can do whatever you want, but you will get lotsof false positives if > you keep it. If you really want a star on the declaration then you need > to make two rules. The first that finds the positions of the places that > match and the second that only puts a * when there is both a matched > declaration and a matched reinitialization. I hoped that the following SmPL script variant can be another approximation for the desired solution on the discussed source code search pattern. @display@ binary operator bo1, bo2; expression action, e1, e2 != e1, e3, e4, e5; identifier var; statement es1, is2, es2, is3, es3; type t; @@ ( t var = e1; <+... if (...) { var = e2; ... } else es1 ...+> if ( \( var \| var bo1 e3 \) ) is2 else es2 | t var * = e1 ; ... when != if ( \( var \| var bo2 e4 \) ) is3 else es3 when != action(..., var, ...) when != switch (var) { default: ... } when exists ( *e5 = <+... var ...+> | *var = e5 ) ) But I stumble on the following error message. elfring@Sonne:~/Projekte/Linux/next-patched> git checkout next-20190620 && spatch drivers/misc/lkdtm/core.c ~/Projekte/Coccinelle/janitor/show_questionable_variable_initialisation5.cocci … minus: parse error: File "/home/elfring/Projekte/Coccinelle/janitor/show_questionable_variable_initialisation5.cocci", line 36, column 0, charpos = 492 around = ')', whole content = ) I observed then that each of the two main branches in the shown SmPL disjunction can work as expected. So I wonder even more why the combination can not be parsed by the software “Coccinelle 1.0.7-00211-geaa13d59 (OCaml 4.07.1)” so far. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking redundant variable initialisations with SmPL?
>>> It could be helpful to replace the last line by: >>> >>> ( >>> e3 = <+...var...+> >> >> Can this SmPL specification make sense as another when constraint? > > No. I imagine that a few extensions like the following can become safer. when != do ds while( \( var bo e3 \| var \) ); when != switch(var) { ... default: ... } Unfortunately, the software combination “Coccinelle 1.0.7-00211-geaa13d59 (OCaml 4.07.1)” does not like such an approach at the moment. … minus: parse error: File "/home/elfring/Projekte/Coccinelle/janitor/show_questionable_variable_initialisation3.cocci", line 12, column 27, charpos = 295 around = '...', … If I try the specification “when != switch(var) { default: ... }” out, the information “not supported” is provided. > When is about the code between the code that matches what is before > or after. If you put when, you will get a false positives for var = var + 1. It seems that more analysis constraints should be taken into account finally. I find also that it should be ensured then that metavariables like “e3” and “var” will refer to different source code. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking redundant variable initialisations with SmPL?
>>> In that case, it would also be beneficial to remove the * >> >> I find the asterisk required here > > Yu can do whatever you want, but you will get lotsof false positives if > you keep it. If you really want a star on the declaration I would prefer to use a minus character for the specification that an unused value should be deleted if corresponding change confidence can be achieved for the discussed source code analysis pattern also by the means of the semantic patch language. > then you need to make two rules. The first that finds the positions of the > places > that match and the second that only puts a * when there is both a matched > declaration and a matched reinitialization. But I wold imagine that from > the reinitialization, it would be easy to find the declaration, so it > doesn't seem worth further complicating the rule. This explanation sounds promising. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking redundant variable initialisations with SmPL?
> It could be helpful to replace the last line by: > > ( > e3 = <+...var...+> Can this SmPL specification make sense as another when constraint? > | > * var = e3 > ) > > In that case, it would also be beneficial to remove the * I find the asterisk required here > on the variable declaration so that a potentially unused value is marked for the discussed variable initialisation. > because that will be activated regardless of which branch matches > in the disjunction. Will further data flow analysis influence such a view any more? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking redundant variable initialisations with SmPL?
>> elfring@Sonne:~/Projekte/Linux/next-patched> spatch >> ~/Projekte/Coccinelle/janitor/show_questionable_variable_initialisation1.cocci >> drivers/misc/lkdtm/core.c >> … >> exn while in timeout_function >> Fatal error: exception Coccinelle_modules.Common.Impossible(56) >> >> >> How do you think about the software situation? > > This problem is now fixed. Another aspect was improved also for the Coccinelle software. The following SmPL script variant can point source code places out for further considerations. @display@ binary operator bo; expression e1, e2, e3, call; identifier var; statement is, es; type t; @@ *t var = e1; ... when != if ( \( var bo e2 \| var \) ) is else es when != call(..., var, ...) when exists *var = e3 But it seems that data flow analysis would be needed to exclude remaining false positives for such a source code search pattern. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] Checking dependencies for current OCaml compiler version
Hello, I have tried out to build the current Coccinelle software again based on the OPAM switch “4.08.0”. Unfortunately, I stumble on the following error message. … ocamlfind ocamlc -c -no-alias-deps -I . stdcompat__pervasives.ml -o stdcompat__pervasives.cmo File "stdcompat__pervasives.ml", line 1, characters 8-18: 1 | include Pervasives ^^ Alert deprecated: module Stdlib.Pervasives Use Stdlib instead. If you need to stay compatible with OCaml < 4.07, you can use the stdlib-shims library: https://github.com/ocaml/stdlib-shims File "stdcompat__pervasives.ml", line 1: Error: The implementation stdcompat__pervasives.ml does not match the interface stdcompat__pervasives.cmi: The value `protect' is required but not provided File "stdcompat__pervasives_s.mli", line 55, characters 2-60: Expected declaration make[2]: *** [Makefile:1370: stdcompat__pervasives.cmo] Fehler 2 make[2]: Verzeichnis „/home/elfring/Projekte/Coccinelle/20160205/bundles/stdcompat/stdcompat-8“ wird verlassen make[1]: *** [Makefile:3: all] Fehler 2 … How will the software situation be improved then? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] Moving exception handling code to the end of a function implementation with SmPL?
Hello, I became interested in another source code transformation again. I would like to move a bit of common code to the end of a function implementation with the help of the following script for the semantic patch language. @replacement@ expression info, result; identifier target, work; type t != void; @@ t work(...) { <+... if (...) ( -{ -result = info; goto - target + e_nodev ; -} | { ... -result = info; goto - target + e_nodev ; } ) ...+> target: ... return result; +e_nodev: +result = info; +goto target; } The implementation of the function “megasas_mgmt_ioctl_fw” looks like an update candidate. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/megaraid/megaraid_sas_base.c?id=4ae004a9bca8bef118c2b4e76ee31c7df4514f18#n7742 https://elixir.bootlin.com/linux/v5.2-rc5/source/drivers/scsi/megaraid/megaraid_sas_base.c#L7742 * I extracted it into a test source file. Unfortunately, I stumble on the error message “replacement: already tagged token: C code context” then. * If I delete a bit more source code for this example, the shown transformation approach can work as expected. * The complete source file seems to be very challenging for testing the run time characteristics. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Coccinelle: Add a SmPL script for the reconsideration of redundant dev_err() calls
> I still don't see the point of specifying return. Why not just S, where S > is a statement metavariable? Do you find the following SmPL change specification more appropriate? @deletion depends on patch@ expression e; statement s; @@ e = devm_ioremap_resource(...); if (IS_ERR(e)) ( -{ - dev_err(...); s -} | { <+... - dev_err(...); ...+> } ) Will any additional constraints become relevant? >> Would this approach need a version check for the Coccinelle software? > > Why would that be necessary? I guess that the application of SmPL disjunctions for if statements can trigger development concerns. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Coccinelle: Add a SmPL script for the reconsideration of redundant dev_err() calls
> I still don't see the point of specifying return. Why not just S, where S > is a statement metavariable? Do you find the following SmPL change specification more appropriate? @deletion depends on patch@ expression e; statement s; @@ e = devm_ioremap_resource(...); if (IS_ERR(e)) ( -{ - dev_err(...); s -} | { <+... - dev_err(...); ...+> } ) Will any additional constraints become relevant? >> Would this approach need a version check for the Coccinelle software? > > Why would that be necessary? I guess that the application of SmPL disjunctions for if statements can trigger development concerns. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: Coccinelle: Add a SmPL script for the reconsideration of redundant dev_err() calls
> I think that something like > > if (IS_ERR(e)) > { > <+... > *dev_err(...) > ...+> > } > > would be more appropriate. Whether there is a return or not doesn't > really matter. Do you find the following SmPL change specification useful and acceptable? @deletion depends on patch@ expression e; @@ e = devm_ioremap_resource(...); if (IS_ERR(e)) ( -{ - dev_err(...); return (...); -} |{ <+... - dev_err(...); ...+> } ) Would this approach need a version check for the Coccinelle software? Regards, Markus
Re: [Cocci] Coccinelle: Add a SmPL script for the reconsideration of redundant dev_err() calls
>> Would you prefer to clarify a more advanced approach? > > I think that something like > > if (IS_ERR(e)) > { > <+... > *dev_err(...) > ...+> > } > > would be more appropriate. This SmPL construct can be more powerful. > Whether there is a return or not doesn't really matter. Such an adjustment can be helpful for a few operation modes. But the number of statements in the if branch will influence the possibility for the deletion of curly braces together with redundant dev_err() calls by the SmPL patch mode. >> Would you like to get the relevant function name dynamically determined? > > I have no idea what you consider "the relevant function name" to be. > If it is always devm_ioremap_resource then it would seem that it does not > need to be dynamically determined. Do other functions share the same error reporting strategy so that any more collateral software evolution can happen? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: Coccinelle: Add a SmPL script for the reconsideration of redundant dev_err() calls
>> +@display depends on context@ >> +expression e; >> +@@ >> + e = devm_ioremap_resource(...); >> + if (IS_ERR(e)) >> + { >> +* dev_err(...); >> +return (...); >> + } > > Why do you assume that there is exactly one dev_err and one return after > the test? I propose to start with the addition of a simple source code search pattern. Would you prefer to clarify a more advanced approach? >> +@script:python to_do depends on org@ >> +p << or.p; >> +@@ >> +coccilib.org.print_todo(p[0], >> +"WARNING: An error message is probably not needed >> here because the previously called function contains appropriate error >> reporting.") > > "the previously called function" would be better as "devm_ioremap_resource". Would you like to get the relevant function name dynamically determined? Regards, Markus
Re: Coccinelle: Add a SmPL script for the reconsideration of redundant dev_err() calls
>> +@display depends on context@ >> +expression e; >> +@@ >> + e = devm_ioremap_resource(...); >> + if (IS_ERR(e)) >> + { >> +* dev_err(...); >> +return (...); >> + } > > Why do you assume that there is exactly one dev_err and one return after > the test? I propose to start with the addition of a simple source code search pattern. Would you prefer to clarify a more advanced approach? >> +@script:python to_do depends on org@ >> +p << or.p; >> +@@ >> +coccilib.org.print_todo(p[0], >> +"WARNING: An error message is probably not needed >> here because the previously called function contains appropriate error >> reporting.") > > "the previously called function" would be better as "devm_ioremap_resource". Would you like to get the relevant function name dynamically determined? Regards, Markus
[Cocci] [PATCH] Coccinelle: Add a SmPL script for the reconsideration of redundant dev_err() calls
From: Markus Elfring Date: Thu, 20 Jun 2019 19:12:53 +0200 The function “devm_ioremap_resource” contains appropriate error reporting. Thus it can be questionable to present another error message at other places. Provide design options for the adjustment of affected source code by the means of the semantic patch language (Coccinelle software). Signed-off-by: Markus Elfring --- .../coccinelle/misc/redundant_dev_err.cocci | 53 +++ 1 file changed, 53 insertions(+) create mode 100644 scripts/coccinelle/misc/redundant_dev_err.cocci diff --git a/scripts/coccinelle/misc/redundant_dev_err.cocci b/scripts/coccinelle/misc/redundant_dev_err.cocci new file mode 100644 index ..aeb228280276 --- /dev/null +++ b/scripts/coccinelle/misc/redundant_dev_err.cocci @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: GPL-2.0 +/// Reconsider a function call for redundant error reporting. +// +// Keywords: dev_err redundant device error messages +// Confidence: Medium + +virtual patch +virtual context +virtual org +virtual report + +@display depends on context@ +expression e; +@@ + e = devm_ioremap_resource(...); + if (IS_ERR(e)) + { +* dev_err(...); +return (...); + } + +@deletion depends on patch@ +expression e; +@@ + e = devm_ioremap_resource(...); + if (IS_ERR(e)) +-{ +- dev_err(...); +return (...); +-} + +@or depends on org || report@ +expression e; +position p; +@@ + e = devm_ioremap_resource(...); + if (IS_ERR(e)) + { +dev_err@p(...); +return (...); + } + +@script:python to_do depends on org@ +p << or.p; +@@ +coccilib.org.print_todo(p[0], +"WARNING: An error message is probably not needed here because the previously called function contains appropriate error reporting.") + +@script:python reporting depends on report@ +p << or.p; +@@ +coccilib.report.print_report(p[0], + "WARNING: An error message is probably not needed here because the previously called function contains appropriate error reporting.") -- 2.22.0 ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] Checking redundant variable initialisations with SmPL?
Hello, A patch on a topic like “[next] lkdtm: remove redundant initialization of ret” caught also my software development attention. https://lkml.org/lkml/2019/6/14/265 https://lore.kernel.org/patchwork/patch/1088971/ https://lore.kernel.org/lkml/20190614094311.24024-1-colin.k...@canonical.com/ I hoped that the following script for the semantic patch language can point such an update candidate also out. @display@ binary operator bo; expression e1, e2, e3; identifier var, work; statement is, es; type t; @@ *t var = e1; ... when != if (var bo e2) is else es *var = ( work(...) | e3 ) elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/janitor/show_questionable_variable_initialisation1.cocci drivers/misc/lkdtm/core.c … exn while in timeout_function Fatal error: exception Coccinelle_modules.Common.Impossible(56) How do you think about the software situation? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Handling of pointer expressions by SmPL
>> Would you like to add this information to the software documentation? > > Not really. The documentation can't defend against everyone's imagination. Will it become helpful to mention the metavariable type “expression *” also in the SmPL manual? https://github.com/coccinelle/coccinelle/blob/cad4c0705f9e37f501531e133d3a47bc56ed0ce2/docs/manual/cocci_syntax.tex#L67 The following SmPL script variant can work as expected then. @display@ expression* ex; identifier zm =~ "_zmalloc"; statement is, es; @@ ex = zm(...); if (ex) is else es *memset(ex, 0, ...); elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/janitor/show_questionable_memset3.cocci drivers/staging/rtl8723bs/core/rtw_ap.c … Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Handling of pointer expressions by SmPL
>> Would you like to add this information to the software documentation? > > Not really. The documentation can't defend against everyone's imagination. Will it become helpful to mention the metavariable type “expression *” also in the SmPL manual? https://github.com/coccinelle/coccinelle/blob/cad4c0705f9e37f501531e133d3a47bc56ed0ce2/docs/manual/cocci_syntax.tex#L67 The following SmPL script variant can work as expected then. @display@ expression* ex; identifier zm =~ "_zmalloc"; statement is, es; @@ ex = zm(...); if (ex) is else es *memset(ex, 0, ...); elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/janitor/show_questionable_memset3.cocci drivers/staging/rtl8723bs/core/rtw_ap.c … Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Handling of pointer expressions by SmPL
> You can't specify pointer dereferences by metavariables. Thanks for another clarification for a detail by the Coccinelle software. I assumed that this would be useful functionality for the specification “expression *X;”. > The pattern is only in the pattern. It is not partly in the metavariables. Would you like to add this information to the software documentation? * Does the asterisk express only a specific restriction on the metavariable type then? * Which source code would you match (or exclude) for pointer expressions? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Handling of pointer expressions by SmPL
>>> Metavariables have types. Here X has pointer type. >> >> Our understanding of this software detail seems to match. >> >> But I assume that the asterisk can be treated in a different way by >> the isomorphism definition language in comparison to the semantic >> patch language. >> How much does context-dependent interpretation matter here? >> >> >>> There is no need for X to match a pointer that is dereferenced. >> >> I got further development imaginations around the places where >> you would specify pointer dereferences by metavariables instead of >> leaving asterisks in source code search fragments. > > This is incorrect. Which detail do you find inappropriate from my feedback? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Handling of pointer expressions by SmPL
>> https://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L151 >> >> It looks like that it should refer to a pointer dereference >> (according to a view for the semantic patch language). >> But the really desired meaning might be different for the support >> of pointer expressions by the isomorphism definition language. > > Metavariables have types. Here X has pointer type. Our understanding of this software detail seems to match. But I assume that the asterisk can be treated in a different way by the isomorphism definition language in comparison to the semantic patch language. How much does context-dependent interpretation matter here? > There is no need for X to match a pointer that is dereferenced. I got further development imaginations around the places where you would specify pointer dereferences by metavariables instead of leaving asterisks in source code search fragments. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Handling of pointer expressions by SmPL
> How would Coccinelle know that a pointer is expected if there is no asterisk? I am still trying to achieve a better understanding around the interpretation of the specification “expression *X;”. https://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L151 It looks like that it should refer to a pointer dereference (according to a view for the semantic patch language). But the really desired meaning might be different for the support of pointer expressions by the isomorphism definition language. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking for a null pointer with SmPL
> How would Coccinelle know that a pointer is expected if there is no asterisk? SmPL expression metavariables can handle also pointer variables from C code, can't they? Type distinctions have got other consequences for isomorphism definitions. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking for a null pointer with SmPL
> Isomorphisms have a cost, so the provided isomorphism only converts !X to > X == NULL when X is a pointer. It seems that the Coccinelle software has got restrictions on the interpretation of pointers so far. Is a pointer recognised for application of isomorphisms by the semantic patch language only when the metavariable type contains an asterisk (instead of expecting that SmPL expression metavariables can benefit from this functionality already without an explicit “*”)? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: drivers: Inline code in devm_platform_ioremap_resource() from two functions
>> Would you like to check the software circumstances once more >> for the generation of a similar code structure by a C compiler >> (or optimiser)? > > As said: unfortunately, I don't have the time to do that I became curious if you would like to adjust your software development attention a bit more also in this area. > - you'd have to tell us, what exactly you've got in mind. I try to point possibilities out to improve the combination of two functions. > If it's just about some error checks which happen to be redundant in a > particular case, you'll have to show that this case is a *really* hot > path (eg. irq, syscall, scheduling, etc) - but I don't see that here. 1. May the check “resource_type(res) == IORESOURCE_MEM” be performed in a local loop? 2. How hot do you find the null pointer check for the device input parameter of the function “devm_ioremap_resource”? > Any actual measurements on how your patch improves that ? Not yet. - Which benchmarks would you trust here? > Look, I understand that you'd like to squeeze out maximum performance, I hope so. > but this has to be practically maintainable. This can be achieved if more contributors would find proposed adjustments helpful for another software transformation. Regards, Markus
Re: [Cocci] Checking for a null pointer with SmPL
> If you turn off isomorphisms, it will only match exactly what you have > written. This setting can occasionally be appropriate. > Isomorphisms have a cost, so the provided isomorphism only converts !X to > X == NULL when X is a pointer. If you want to make an isomorphism that > does something else, please feel free. If data type information can not be taken into account for the transformation of a source code search specification at this point, it can become interesting to reactivate selected “isomorphism” rules on demand only after type information is available at a later data processing step. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking for a null pointer with SmPL
> As I already told you, the isomorphisms are applied before matching > against the C code. At that time, this information is not available. Can the software situation be clarified also by omitting Coccinelle's isomorphisms? (Can this functionality be temporarily switched off for a specific SmPL rule?) https://github.com/coccinelle/coccinelle/blob/c6d7554edf7c4654aeae4d33c3f040e300682f23/docs/manual/cocci_syntax.tex#L125 How relevant is it for the handling of a source code search specification like “if (ex) is else es” by the semantic patch language (if you would like to support the omission of the code part “== NULL” at all)? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] drivers: Inline code in devm_platform_ioremap_resource() from two functions
>> Two function calls were combined in this function implementation. >> Inline corresponding code so that extra error checks can be avoided here. > > What exactly is the purpose of this ? I suggest to take another look at the need and relevance of involved error checks in the discussed function combination. > Looks like a notable code duplication ... This can be. > I thought we usually try to reduce this, instead of introducing new ones. Would you like to check the software circumstances once more for the generation of a similar code structure by a C compiler (or optimiser)? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking for a null pointer with SmPL
>> Example: >> struct setkey_parm *psetkeyparm; > > As I already told you, the isomorphisms are applied before matching > against the C code. At that time, this information is not available. Under which circumstances will the Coccinelle software become able to take available data type information completely into account? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking for a null pointer with SmPL
>> Can the mentioned SmPL script variant work also without taking extra >> software transformations by isomorphism specifications into account? > > Well, you already know that it doesn't. So oviously the answe is that it > cannot. At the moment. > If you indicate that the expression ex is a pointer then maybe it will work. Should the available data type information be sufficient for the handling of pointers in the discussed use case? Example: struct setkey_parm *psetkeyparm; Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking for a null pointer with SmPL
> Isomorphisms happen in advance, not during the matching process. Can the mentioned SmPL script variant work also without taking extra software transformations by isomorphism specifications into account? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking for a null pointer with SmPL
>> Should an isomorphism specification like “not_ptr2” help here? >> https://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L157 > > No, because Coccinelle does not know that it is a pointer. > How should it know? Can the software determine that the expression metavariable refers to a pointer variable (within the implementation of a function like “rtw_ap_set_key”)? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/rtl8723bs/core/rtw_ap.c?id=9e0babf2c06c73cda2c0cd37a1653d823adb40ec#n1479 https://elixir.bootlin.com/linux/v5.2-rc5/source/drivers/staging/rtl8723bs/core/rtw_ap.c#L1479 Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Checking for a null pointer with SmPL
> In the second case, Coccinelle has no way of knowing that ex is a pointer, Can the software support pointer expressions better? > so it doesn't feel motivated to consider that ex should be compared to NULL. Should an isomorphism specification like “not_ptr2” help here? https://github.com/coccinelle/coccinelle/blob/19ee1697bf152d37a78a20cefe148775bf4b0e0d/standard.iso#L157 > I would imagine that you would have gooten at least some relevant > information if you had tried spatch --parse-cocci. This data display can give useful hints. Are we waiting also for a more complete software documentation in such an application area? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] Checking for a null pointer with SmPL
Hello, A patch on a topic like “staging/rtl8723bs/core/rtw_ap: Remove redundant call to memset” caught also my software development attention. https://lkml.org/lkml/2019/6/15/220 https://lore.kernel.org/patchwork/patch/1089416/ https://lore.kernel.org/lkml/20190616033527.GA14062@hari-Inspiron-1545/ The following script for the semantic patch language points the shown change possibility out as expected. @display@ expression ex; identifier zm =~ "_zmalloc"; statement is; @@ ex = zm(...); if (ex == NULL) is *memset(ex, 0, ...); I would expect that the following SmPL script can work in a similar way. @display@ expression ex; identifier zm =~ "_zmalloc"; statement is, es; @@ ex = zm(...); if (ex) is else es *memset(ex, 0, ...); But this approach does not point an update candidate out at the moment. How do you think about the software situation? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] drivers: Inline code in devm_platform_ioremap_resource() from two functions
>> Two function calls were combined in this function implementation. >> Inline corresponding code so that extra error checks can be avoided here. > > I don't see any point to this at all. Would you like to take another look at corresponding design options? How do you think about to check run time characteristics any more? > By inlining the code, you have created a clone, > which will introduce extra work to maintain in the future. Would you find the shown software transformation acceptable if a C compiler will be able to generate a similar code structure? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[PATCH] drivers: Inline code in devm_platform_ioremap_resource() from two functions
From: Markus Elfring Date: Fri, 14 Jun 2019 11:05:33 +0200 Two function calls were combined in this function implementation. Inline corresponding code so that extra error checks can be avoided here. Signed-off-by: Markus Elfring --- drivers/base/platform.c | 39 ++- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 4d1729853d1a..baadca72f949 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -80,8 +80,8 @@ struct resource *platform_get_resource(struct platform_device *dev, EXPORT_SYMBOL_GPL(platform_get_resource); /** - * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform - * device + * devm_platform_ioremap_resource + * Achieve devm_ioremap_resource() functionality for a platform device * * @pdev: platform device to use both for memory resource lookup as well as *resource management @@ -91,10 +91,39 @@ EXPORT_SYMBOL_GPL(platform_get_resource); void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev, unsigned int index) { - struct resource *res; + u32 i; - res = platform_get_resource(pdev, IORESOURCE_MEM, index); - return devm_ioremap_resource(&pdev->dev, res); + for (i = 0; i < pdev->num_resources; i++) { + struct resource *res = &pdev->resource[i]; + + if (resource_type(res) == IORESOURCE_MEM && index-- == 0) { + struct device *dev = &pdev->dev; + resource_size_t size = resource_size(res); + void __iomem *dest; + + if (!devm_request_mem_region(dev, +res->start, +size, +dev_name(dev))) { + dev_err(dev, + "can't request region for resource %pR\n", + res); + return IOMEM_ERR_PTR(-EBUSY); + } + + dest = devm_ioremap(dev, res->start, size); + if (!dest) { + dev_err(dev, + "ioremap failed for resource %pR\n", + res); + devm_release_mem_region(dev, res->start, size); + dest = IOMEM_ERR_PTR(-ENOMEM); + } + + return dest; + } + } + return IOMEM_ERR_PTR(-EINVAL); } EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource); #endif /* CONFIG_HAS_IOMEM */ -- 2.22.0
Re: [Cocci] Wider usage of “fresh identifiers”?
> Apparently I'm missing this obscure line in the documentation: > > "Fresh identifier metavariables must only be used in + code." > > Any suggestions on how to work around this, other than use Python? Would you get into the mood to adjust this software situation any more? (Increase OCaml development?) How do you think about to check the mentioned restriction in more detail? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: Coccinelle: api: add devm_platform_ioremap_resource script
>> The flag “IORESOURCE_MEM” is passed as the second parameter for the call >> of the function “platform_get_resource” in this refactoring. > > In that particular case, we maybe should consider separate inline > helpers instead of passing this is a parameter. > > Maybe it would even be more efficient to have completely separate > versions of devm_platform_ioremap_resource(), so we don't even have > to pass that parameter on stack. Would you like to add another function which should be called instead of the combination of the functions “platform_get_resource” and “devm_ioremap_resource”? Regards, Markus
Re: [Cocci] Working with pipes for parallel SmPL data processing?
Am 10.06.19 um 23:00 schrieb Julia Lawall: > > > On Sun, 9 Jun 2019, Markus Elfring wrote: > >> Hello, >> >> The Coccinelle software supports also a variant of parallel data processing >> after the parameter “--jobs” was passed. >> https://github.com/coccinelle/coccinelle/blob/7ec31ed1fadf738bc487ccefdc63bfe0598f44cc/docs/manual/spatch_options.tex#L745 >> >> It is nice when it works to distribute analysis on source files to some >> processors. >> Unfortunately, undesirable software behaviour can be observed if a database >> like “PostgreSQL 11.3-7.1” would be used in such a system configuration. >> Thus I imagine that it can be occasionally appropriate to perform desired >> parallel data processing by the usage of a detailed pipeline instead. >> https://ocaml.github.io/ocamlunix/pipes.html >> >> How do you think about another application for this kind of >> inter-process communication? > > When you use -j, with a value bigger than one > you fork processes that thus the child processes write to > a different region of memory than the parent process. This can be. But such forked processes have got access also to copied data from the the parent process. > I don't have any interest in connecting Coccinelle to a database, I am curious if any other users of your software can adjust the corresponding development interests. > so I'm not going to spend any time on debugging your issue. I hope that collateral software evolution can happen if more advices will be taken better into account from existing software documentation. https://docs.sqlalchemy.org/en/13/core/pooling.html#using-connection-pools-with-multiprocessing The data storage is working already if known database connections would be performed only by a single process (instead of background processes). > Maybe you can use a hashtable, I would find an other data structure more helpful for the storage of analysis results from parallel source code processing. > then use merge in a finalize, Thanks for such feedback. > and the write the information to the database in the parent thread. This is also my software development idea for working with a detailed process pipeline. > The use of merge is illustrated in eg tests/merge_vars.cocci I would about the detail that a test source file is empty here. https://github.com/coccinelle/coccinelle/commit/0207001b5241092e7b22cf4f99b22db13c01a24d#diff-27862a1fe13b63e871b358e9fc54b3b7 I find the usage of Coccinelle's merge functionality unclear at the moment. Can such an area become better documented? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Improving parallel SmPL data processing for database support
> https://github.com/coccinelle/coccinelle/issues/50 Would you like to adjust the situation around better software collaboration? https://docs.sqlalchemy.org/en/13/core/pooling.html#using-connection-pools-with-multiprocessing Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] Working with pipes for parallel SmPL data processing?
Hello, The Coccinelle software supports also a variant of parallel data processing after the parameter “--jobs” was passed. https://github.com/coccinelle/coccinelle/blob/7ec31ed1fadf738bc487ccefdc63bfe0598f44cc/docs/manual/spatch_options.tex#L745 It is nice when it works to distribute analysis on source files to some processors. Unfortunately, undesirable software behaviour can be observed if a database like “PostgreSQL 11.3-7.1” would be used in such a system configuration. Thus I imagine that it can be occasionally appropriate to perform desired parallel data processing by the usage of a detailed pipeline instead. https://ocaml.github.io/ocamlunix/pipes.html How do you think about another application for this kind of inter-process communication? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: Coccinelle: api: add devm_platform_ioremap_resource script
>>> +- e1 = devm_ioremap_resource(arg4, id); >>> ++ e1 = devm_platform_ioremap_resource(arg1, arg3); >> >> Can the following specification variant matter for the shown SmPL >> change approach? >> >> + e1 = >> +- devm_ioremap_resource(arg4, id >> ++ devm_platform_ioremap_resource(arg1, arg3 >> + ); > > In the latter case, the original formatting of e1 will be preserved. I would like to point the possibility out to express only required changes also by SmPL specifications. > But there is not usually any interesting formatting on the left side of an > assignment (ie typically no newlines or comments). Is there any need to trigger additional source code reformatting? > I can see no purpose to factorizing the right parenthesis. These characters at the end of such a function call should be kept unchanged. I got another software development concern according to the discussed software update “drivers: provide devm_platform_ioremap_resource()” (from 2019-02-21). https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/platform.c?id=7945f929f1a77a1c8887a97ca07f87626858ff42 The flag “IORESOURCE_MEM” is passed as the second parameter for the call of the function “platform_get_resource” in this refactoring. Should this detail be specified also in the proposed script for the semantic patch language instead of using the metavariable “arg2” in SmPL disjunctions? How do you think about to delete error detection and corresponding exception handling code for the previous function call? Is the SmPL code specification “when != id” really sufficient for the exclusion of variable reassignments here? Regards, Markus
Re: Coccinelle: api: add devm_platform_ioremap_resource script
> +- e1 = devm_ioremap_resource(arg4, id); > ++ e1 = devm_platform_ioremap_resource(arg1, arg3); Can the following specification variant matter for the shown SmPL change approach? + e1 = +- devm_ioremap_resource(arg4, id ++ devm_platform_ioremap_resource(arg1, arg3 + ); Regards, Markus
Re: [Cocci] Handling of designated initialisers by SmPL?
> Would you like to try another SmPL script variant out? @replacement@ constant text; expression value; @@ .driver = { - .name = text, - .of_match_table = value, - }, + .name = text, + .of_match_table = of_match_ptr(value), + }, How do you think about the following response by the Coccinelle software? elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/Probe/Weigelt9.cocci sound/soc/codecs/pcm3060-i2c.c … minus: parse error: File "/home/elfring/Projekte/Coccinelle/Probe/Weigelt9.cocci", line 5, column 1, charpos = 51 around = '.', whole content = .driver = { Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] How to replace obsolete #ifdef's?
> Just wrapping the assignment into the macro call turned out to be easy. I am curious how your transformation approaches will evolve further. > But I haven't found a way to remove the now #ifdef :( The support for preprocessor functionality is limited by the semantic patch language so far. Would you like to try another SmPL script variant out? @replacement@ constant text; expression value; identifier my_name; type driver_type; @@ static driver_type my_name = { .driver = { -.name = text, -.of_match_table = value, -}, +.name = text, +.of_match_table = of_match_ptr(value), +}, ... }; Do you find the following test result acceptable finally? elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/Probe/Weigelt8.cocci sound/soc/codecs/pcm3060-i2c.c init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h … @@ -45,9 +45,7 @@ MODULE_DEVICE_TABLE(of, pcm3060_of_match static struct i2c_driver pcm3060_i2c_driver = { .driver = { .name = "pcm3060", -#ifdef CONFIG_OF - .of_match_table = pcm3060_of_match, -#endif /* CONFIG_OF */ + .of_match_table = of_match_ptr(pcm3060_of_match), }, .id_table = pcm3060_i2c_id, .probe = pcm3060_i2c_probe, Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] How to replace obsolete #ifdef's?
>> Will similar software updates become more challenging for the initially >> described handling of designated initialisers for known data structures? > > This is in the spirit of the solution I already proposed. Would you like to compare transformation results also for the following SmPL change specification? @replacement@ constant text; expression value; identifier my_name; type driver_type; @@ static driver_type my_name = { - .driver = { -.name = text, -.of_match_table = value + .driver = { +.name = text, +.of_match_table = of_match_ptr(value) , }, ... }; Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] How to replace obsolete #ifdef's?
>> Will similar software updates become more challenging for the initially >> described handling of designated initialisers for known data structures? > > This is in the spirit of the solution I already proposed. How do you think about the following SmPL transformation approach? @replacement@ constant text; expression value; identifier my_name; type driver_type; @@ static driver_type my_name = { .driver = { -.name = text, +.name = text, .of_match_table = + of_match_ptr( value + ) , }, ... }; Test result: elfring@Sonne:~/Projekte/Linux/next-patched> spatch ~/Projekte/Coccinelle/Probe/Weigelt5.cocci sound/soc/codecs/pcm3060-i2c.c … @@ -43,10 +43,10 @@ MODULE_DEVICE_TABLE(of, pcm3060_of_match #endif /* CONFIG_OF */ static struct i2c_driver pcm3060_i2c_driver = { +#ifdef CONFIG_OF .driver = { .name = "pcm3060", -#ifdef CONFIG_OF - .of_match_table = pcm3060_of_match, + .of_match_table = of_match_ptr(pcm3060_of_match), #endif /* CONFIG_OF */ }, .id_table = pcm3060_i2c_id, I would find it questionable that the software combination “Coccinelle 1.0.7-00206-gfdcc6d79 (OCaml 4.07.1)” suggests to move a conditional preprocessor statement to an other source code place. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] How to replace obsolete #ifdef's?
> but not: > > @@ > @@ > > - #ifdef FOO > bar(); > - #endif The deletion of these preprocessor statements is not directly supported in the shown way by the Coccinelle software at the moment. But I got another code transformation idea which would not be so convenient. The semantic patch language supports the metavariable type “statement list” for a while. https://github.com/coccinelle/coccinelle/blob/cad4c0705f9e37f501531e133d3a47bc56ed0ce2/docs/manual/cocci_syntax.tex#L209 I imagine then that the function call can be “intentionally” deleted and added back in such a simple SmPL script example. * Should any extra C code vanish after such a special adjustment because specific parts were not restored? * Can it make sense here to change a bit of code even if it was originally intended to keep it untouched? Will similar software updates become more challenging for the initially described handling of designated initialisers for known data structures? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] Improving parallel SmPL data processing for database support
Hello, I occasionally use the software “SQLAlchemy” for easy storage of source code data. I noticed once more that information was not permanently stored if the parameter “-j 4” was passed to the program “spatch” while the database “PostgreSQL 11.3-7.1” was selected as the desired storage system. The used scripts for the semantic patch language can still work as expected if they are executed only by single processor instead. Can any more software developers get into the mood then to find the system details out which hinder the application of parallel data processing in such cases? https://github.com/coccinelle/coccinelle/issues/50 Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci