Re: [Cocci] [PATCH] coccinelle: api: add sprintf() support to device_attr_show

Re: [Cocci] [PATCH] coccinelle: api: add sprintf() support to device_attr_show

[Denis Efremov]

Hi,

Markus, I think that CCing new people and spam them with mails they
are obviously not interested in doesn't bring an additional value to
the discussion. linux-kernel and cocci mailing lists are enough
in my opinion. This also will allow us to keep "threaded" mail
order.

On 8/14/20 11:30 AM, Markus Elfring wrote:
>> Interesting enough that with this patch coccinelle starts to skip
>> patch generation in some cases. For example, it skips patch for
>> drivers/base/core.c This is an unexpected result for me.
> 
> Would you like to point questionable differences for such patch hunks out?

Without this patch the script generates:
$ spatch -D patch --no-includes --include-headers --cocci-file scripts/coccinelle/api/device_attr_show.cocci drivers/base/core.c
--- drivers/base/core.c
+++ /tmp/cocci-output-63510-2f17ff-core.c
@@ -1713,7 +1713,7 @@ ssize_t device_show_ulong(struct device
char *buf)
{
struct dev_ext_attribute *ea = to_ext_attr(attr);
-       return snprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
+       return scnprintf(buf, PAGE_SIZE, "%lx\n", *(unsigned long *)(ea->var));
}
EXPORT_SYMBOL_GPL(device_show_ulong);

@@ -1743,7 +1743,7 @@ ssize_t device_show_int(struct device *d
{
struct dev_ext_attribute *ea = to_ext_attr(attr);

-       return snprintf(buf, PAGE_SIZE, "%d\n", *(int *)(ea->var));
+       return scnprintf(buf, PAGE_SIZE, "%d\n", *(int *)(ea->var));
}
EXPORT_SYMBOL_GPL(device_show_int);

@@ -1764,7 +1764,7 @@ ssize_t device_show_bool(struct device *
{
struct dev_ext_attribute *ea = to_ext_attr(attr);

-       return snprintf(buf, PAGE_SIZE, "%d\n", *(bool *)(ea->var));
+       return scnprintf(buf, PAGE_SIZE, "%d\n", *(bool *)(ea->var));
}
EXPORT_SYMBOL_GPL(device_show_bool);

With this patch it generates nothing. I would expect spatch to generate
a different patch with sprintf instead of scnprintf, because I think 
... is enough to match "*(int *)(ea->var)". Even if it can't match sprintf
pattern it should fallback to scnprintf pattern.

> You propose to use a nested SmPL disjunction for desired adjustments.
> I suggest to start a corresponding case distinction behind
> the key word “return” instead of repeating it three times.

It doesn't work.

Thanks,
Denis
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] coccinelle: api: add sprintf() support to device_attr_show

[Denis Efremov]

On 8/14/20 3:30 PM, Markus Elfring wrote:
>>> You propose to use a nested SmPL disjunction for desired adjustments.
>>> I suggest to start a corresponding case distinction behind
>>> the key word “return” instead of repeating it three times.
>>
>> It doesn't work.
> 
> How do you think about to apply a SmPL rule variant like the following?
> 
> @rp depends on patch@
> identifier show, dev, attr, buf;
> constant str;
> @@
> 
> ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> 	<...
> 	return
> (
> -		snprintf
> +		sprintf
> 			(
> 			buf,
> -			\(PAGE_SIZE\|PAGE_SIZE - 1\),
> (			str
> |
> (			"%i"\|"%i\n"\|"%li"\|"%li\n"\|"%lli"\|"%lli\n"\|
> 			"%d"\|"%d\n"\|"%ld"\|"%ld\n"\|"%lld"\|"%lld\n"\|
> 			"%u"\|"%u\n"\|"%lu"\|"%lu\n"\|"%llu"\|"%llu\n"\|
> 			"%x"\|"%x\n"\|"%lx"\|"%lx\n"\|"%llx"\|"%llx\n"\|
> 			"%X"\|"%X\n"\|"%lX"\|"%lX\n"\|"%llX"\|"%llX\n"\|
> 			"0x%x"\|"0x%x\n"\|"0x%lx"\|"0x%lx\n"\|"0x%llx"\|"0x%llx\n"\|
> 			"0x%X"\|"0x%X\n"\|"0x%lX"\|"0x%lX\n"\|"0x%llX"\|"0x%llX\n"\|
> 			"%02x\n"\|"%03x\n"\|"%04x\n"\|"%08x\n"\|
> 			"%02X\n"\|"%03X\n"\|"%04X\n"\|"%08X\n"\|
> 			"0x%02x\n"\|"0x%03x\n"\|"0x%04x\n"\|"0x%08x\n"\|
> 			"0x%02X\n"\|"0x%03X\n"\|"0x%04X\n"\|"0x%08X\n"\|
> 			"%zd"\|"%zd\n"\|"%zu"\|"%zu\n"\|"%zx"\|"%zx\n"\|
> 			"%c"\|"%c\n"\|"%p"\|"%p\n"\|"%pU\n"\|"%pUl\n"\|"%hu\n"
> )			,
> 			...
> )
> 			)
> |
> -		snprintf
> +		scnprintf
> 			(...)
> );
> 	...>
> }
> 

3 levels of nested disjunctions makes this pattern completely unreadable
and gives no comparable benefits. I don't think we should care much about
number of characters in the kernel sources, gzip will do a better job
anyway.


Thanks,
Denis
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Searching for format strings with SmPL disjunctions

[Markus Elfring]

Can a source code search pattern like the following become ever usable?

@display@
@@
*"%"
("d"
|"ld"
|"lld"
)
("\n"
|""
)


elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch --parse-cocci show_usage_of_selected_format_strings.cocci
…
minus: parse error:
  File "show_usage_of_selected_format_strings.cocci", line 4, column 0, charpos = 18
  around = '(',
  whole content = ("d"


Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Searching for format strings with SmPL disjunctions

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]



On Sun, 16 Aug 2020, Markus Elfring wrote:

> Can a source code search pattern like the following become ever usable?
>
> @display@
> @@
> *"%"
> ("d"
> |"ld"
> |"lld"
> )
> ("\n"
> |""
> )
>
>
> elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch --parse-cocci show_usage_of_selected_format_strings.cocci
> …
> minus: parse error:
>   File "show_usage_of_selected_format_strings.cocci", line 4, column 0, charpos = 18
>   around = '(',
>   whole content = ("d"

There are no short term plans for this.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Searching for format strings with SmPL disjunctions

[Markus Elfring]

> There are no short term plans for this.

Can long term development considerations be influenced for advanced data processing
of format strings?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] Searching for format strings with SmPL disjunctions

[Markus Elfring]

How do you think about to extend a source code search pattern like the following
by using a regular expression as a SmPL constraint?

@display@
format F =~ "l{0,2}[diuxX]";
@@
*\("%@F@" \| "%@F@\n"\)


Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] [PATCH] coccinelle: api: add sprintf() support to device_attr_show

[Julia Lawall]

> With this patch it generates nothing. I would expect spatch to generate
> a different patch with sprintf instead of scnprintf, because I think
> ... is enough to match "*(int *)(ea->var)". Even if it can't match sprintf
> pattern it should fallback to scnprintf pattern.

It's a known bug, explained in tests/failing_andany.cocci.  In your case,
it is because the last pattern in the disjunction can match things that
the second pattern can match, and because of an optimization done for <...
...>.  I will see if it can be fixed.  Thanks for the report.

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api: add sprintf() support to device_attr_show

[Markus Elfring]

> I will see if it can be fixed.

How will the issue “failing tests - TODO” evolve further?
https://github.com/coccinelle/coccinelle/commit/f2d7ec9006c89610bd1aab4662fcf100e3e6d469#diff-13ff769079511ec7b5dddef7143b2b93R1

How do the comments there fit to undesirable effects for SmPL disjunctions?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api: add sprintf() support to device_attr_show

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]



On Mon, 17 Aug 2020, Markus Elfring wrote:

> > I will see if it can be fixed.
>
> How will the issue “failing tests - TODO” evolve further?
> https://github.com/coccinelle/coccinelle/commit/f2d7ec9006c89610bd1aab4662fcf100e3e6d469#diff-13ff769079511ec7b5dddef7143b2b93R1

failing_andany.cocci is solved with the fix for the current problem.

>
> How do the comments there fit to undesirable effects for SmPL disjunctions?

The problem has nothing to do with disjunctions.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api: add sprintf() support to device_attr_show

[Markus Elfring]

> The problem has nothing to do with disjunctions.

Can missing source code matches trigger the consequence that questionable
branches would be applied there?

Will the clarification for the issue “failing tests - TODO” be continued?

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api: add sprintf() support to device_attr_show

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 408 bytes --]



On Mon, 17 Aug 2020, Markus Elfring wrote:

> > The problem has nothing to do with disjunctions.
>
> Can missing source code matches trigger the consequence that questionable
> branches would be applied there?

No.  The problem causes transformations that should be applied not to be
applied.

julia

>
> Will the clarification for the issue “failing tests - TODO” be continued?
>
> Regards,
> Markus
>

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api: add sprintf() support to device_attr_show

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 677 bytes --]



On Mon, 17 Aug 2020, Markus Elfring wrote:

> >> Can missing source code matches trigger the consequence that questionable
> >> branches would be applied there?
> >
> > No.
>
> I imagine that corresponding views will need further clarifications.

What clarifications?  In the failing_andany example the problem is that
less code is transformed than should be.  So no "quationable branches" are
applied.

julia

>
>
> > The problem causes transformations that should be applied not to be applied.
>
> I reported errors according to the software category “failing tests - TODO”.
> Thus I am still looking for solutions in affected application areas.
>
> Regards,
> Markus
>

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api: add sprintf() support to device_attr_show

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 660 bytes --]



On Mon, 17 Aug 2020, Markus Elfring wrote:

> > What clarifications?
>
> Did you notice that my suggestion for the SmPL script variant “scripts/coccinelle/api/device_attr_show.cocci”
> can generate a patch?
> https://lore.kernel.org/cocci/0a9015b6-9037-59c5-31f2-cd2b32c86b50@linux.com/
>
> Unfortunately, it seems that this transformation approach produces also
> an inappropriate test result so far.
> Do we expect that the first part of the SmPL disjunction should match
> (for the source file “drivers/base/core.c” for example)?

This semantic patch triggers the same problem as the original one, and
should benefit from the same solution.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api: add sprintf() support to device_attr_show

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1841 bytes --]



On Sat, 5 Sep 2020, Markus Elfring wrote:

> >>> What clarifications?
> >>
> >> Did you notice that my suggestion for the SmPL script variant “scripts/coccinelle/api/device_attr_show.cocci”
> >> can generate a patch?
> >> https://lore.kernel.org/cocci/0a9015b6-9037-59c5-31f2-cd2b32c86b50@linux.com/
> >>
> >> Unfortunately, it seems that this transformation approach produces also
> >> an inappropriate test result so far.
> >> Do we expect that the first part of the SmPL disjunction should match
> >> (for the source file “drivers/base/core.c” for example)?
> >
> > This semantic patch triggers the same problem as the original one,
>
> This view is interesting.
>
>
> > and should benefit from the same solution.
>
> I noticed the commit “improve andany optimization” yesterday.
> https://github.com/coccinelle/coccinelle/commit/1a5502d4be8db0623b5acead974943098bdcbf57
>
> The change description contains the following information.
>
> “…
> It also makes the test failing_andany work, which did not work previously.
> …”
>
> I would hope for further positive consequences. Thus I have tried
> the software “Coccinelle 1.0.8-00168-gc4048513” out again.
> But I observe that the expected patch is still not generated
> if a special SmPL disjunction should be applied in my test case.
> The expected test result is displayed if a source code search
> should be performed according to extracted code from the first branch
> of such a disjunction (so that no disjunction is specified in
> the SmPL script variant).
>
> How will the clarification be continued?

The clarification will be continued when you provide a small semantic
patch and a C file that causes a problem.  I'm not going to hunt in years
of mail archives to find it.  If you want something to happen you have to
provide that information now.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api: add sprintf() support to device_attr_show

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]



On Sat, 5 Sep 2020, Markus Elfring wrote:

> > The clarification will be continued when you provide a small semantic
> > patch and a C file that causes a problem.
>
> I imagine that a patch review was still running.
>
>
> > I'm not going to hunt in years
>
> This action is not needed here.
>
>
> > of mail archives to find it.
>
> Have you got ongoing difficulties with safe issue tracking?
>
> Please use the information also from available archive interfaces.
> https://lore.kernel.org/cocci/0a9015b6-9037-59c5-31f2-cd2b32c86b50@linux.com/
> https://systeme.lip6.fr/pipermail/cocci/2020-August/008120.html
>
>
> > If you want something to happen you have to provide that information now.
>
> Would you like to test the software any further with the attached SmPL script variant
> (and the previously mentioned Linux source file “drivers/base/core.c” for example)?

That is a big file.  What part of it is of concern?

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

Re: [Cocci] coccinelle: api: add sprintf() support to device_attr_show

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]



On Sat, 5 Sep 2020, Markus Elfring wrote:

> > The clarification will be continued when you provide a small semantic
> > patch and a C file that causes a problem.
>
> I imagine that a patch review was still running.
>
>
> > I'm not going to hunt in years
>
> This action is not needed here.
>
>
> > of mail archives to find it.
>
> Have you got ongoing difficulties with safe issue tracking?
>
> Please use the information also from available archive interfaces.
> https://lore.kernel.org/cocci/0a9015b6-9037-59c5-31f2-cd2b32c86b50@linux.com/
> https://systeme.lip6.fr/pipermail/cocci/2020-August/008120.html
>
>
> > If you want something to happen you have to provide that information now.
>
> Would you like to test the software any further with the attached SmPL script variant
> (and the previously mentioned Linux source file “drivers/base/core.c” for example)?

It doesn't work because you persist in thinking that you can put a
disjunction around anything.  This is not correct.  You can only put
disjuctions around expressions, statements, types, etc.  But not fragments
of an expression list.  You have

\(str\|huge_regexp,...\)

This is matched against a single expression, which can either be str or a
comma expression consisting of something that matches your huge regular
expression followed by another expression.

There is no problem here.  There will be no changes with respect to this
issue.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

[Cocci] [PATCH] coccinelle: api: add sprintf() support to device_attr_show

[Denis Efremov]

It's safe to use sprintf() for simple cases in device_attr_show
type of functions. Add support for sprintf() in patch mode to
the device_attr_show.cocci script to print numbers and pointers.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
Interesting enough that with this patch coccinelle starts to skip
patch generation in some cases. For example, it skips patch for
drivers/base/core.c This is an unexpected result for me.

 scripts/coccinelle/api/device_attr_show.cocci | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/scripts/coccinelle/api/device_attr_show.cocci b/scripts/coccinelle/api/device_attr_show.cocci
index d8ec4bb8ac41..1248b8c76cfe 100644
--- a/scripts/coccinelle/api/device_attr_show.cocci
+++ b/scripts/coccinelle/api/device_attr_show.cocci
@@ -30,15 +30,45 @@ ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
 
 @rp depends on patch@
 identifier show, dev, attr, buf;
+constant str;
 @@
 
 ssize_t show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	<...
+(
+	return
+-		snprintf
++		sprintf
+			(buf,
+-			\(PAGE_SIZE\|PAGE_SIZE - 1\),
+			str);
+|
+	return
+-		snprintf
++		sprintf
+			(buf,
+-			\(PAGE_SIZE\|PAGE_SIZE - 1\),
+			\("%i"\|"%i\n"\|"%li"\|"%li\n"\|"%lli"\|"%lli\n"\|
+			  "%d"\|"%d\n"\|"%ld"\|"%ld\n"\|"%lld"\|"%lld\n"\|
+			  "%u"\|"%u\n"\|"%lu"\|"%lu\n"\|"%llu"\|"%llu\n"\|
+			  "%x"\|"%x\n"\|"%lx"\|"%lx\n"\|"%llx"\|"%llx\n"\|
+			  "%X"\|"%X\n"\|"%lX"\|"%lX\n"\|"%llX"\|"%llX\n"\|
+			  "0x%x"\|"0x%x\n"\|"0x%lx"\|"0x%lx\n"\|"0x%llx"\|"0x%llx\n"\|
+			  "0x%X"\|"0x%X\n"\|"0x%lX"\|"0x%lX\n"\|"0x%llX"\|"0x%llX\n"\|
+			  "%02x\n"\|"%03x\n"\|"%04x\n"\|"%08x\n"\|
+			  "%02X\n"\|"%03X\n"\|"%04X\n"\|"%08X\n"\|
+			  "0x%02x\n"\|"0x%03x\n"\|"0x%04x\n"\|"0x%08x\n"\|
+			  "0x%02X\n"\|"0x%03X\n"\|"0x%04X\n"\|"0x%08X\n"\|
+			  "%zd"\|"%zd\n"\|"%zu"\|"%zu\n"\|"%zx"\|"%zx\n"\|
+			  "%c"\|"%c\n"\|"%p"\|"%p\n"\|"%pU\n"\|"%pUl\n"\|"%hu\n"\),
+			...);
+|
 	return
 -		snprintf
 +		scnprintf
 			(...);
+)
 	...>
 }
 
-- 
2.26.2

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci