Discussion:
[Cocci] Compacting parameters into fewer lines
Timur Tabi
2018-07-31 14:27:58 UTC
Permalink
Coccinelle does a pretty good job at compating parameters into fewer
lines, but it's not always aggressive enough. Is there a tuning
parameter I can try?

For example, this is good:

- DBG_PRINTF((
- DBG_MODULE_GLOBAL, DBG_LEVEL_ERRORS,
- "NVRM: Failed to allocate object handle in %s\n",
- __FUNCTION__));
+ NV_PRINTF(LEVEL_ERROR,
+ "Failed to allocate object handle in %s\n",
+ __FUNCTION__);

But here, the call to NV_PRINTF only needs two lines, not three.
Parameters 1 and 2 can fit on the same line, or parameters 2 and 3 can
be put on the same line. In this case, it didn't even try:

- DBG_PRINTF((
- DBG_MODULE_GLOBAL, DBG_LEVEL_ERRORS,
- "NVRM: Unable to alloc device in %s\n", __FUNCTION__));
+ NV_PRINTF(LEVEL_ERROR,
+ "Unable to alloc device in %s\n",
+ __FUNCTION__);

Here, it compacted a little bit, but it could have done more. This
could have fit in one line:

- DBG_PRINTF((DBG_MODULE_OS, DEBUGLEVEL_TRACEINFO,
- "NVGVI: %s(), Null state\n",
- __FUNCTION__));
+ NV_PRINTF(LEVEL_INFO, "%s(), Null state\n",
+ __FUNCTION__);
Julia Lawall
2018-07-31 14:33:40 UTC
Permalink
Post by Timur Tabi
Coccinelle does a pretty good job at compating parameters into fewer
lines, but it's not always aggressive enough. Is there a tuning
parameter I can try?
"--max-width", Arg.Set_int Flag_parsing_c.max_width,
" column limit for generated code";
Post by Timur Tabi
- DBG_PRINTF((
- DBG_MODULE_GLOBAL, DBG_LEVEL_ERRORS,
- "NVRM: Failed to allocate object handle in %s\n",
- __FUNCTION__));
+ NV_PRINTF(LEVEL_ERROR,
+ "Failed to allocate object handle in %s\n",
+ __FUNCTION__);
But here, the call to NV_PRINTF only needs two lines, not three.
Parameters 1 and 2 can fit on the same line, or parameters 2 and 3 can
- DBG_PRINTF((
- DBG_MODULE_GLOBAL, DBG_LEVEL_ERRORS,
- "NVRM: Unable to alloc device in %s\n", __FUNCTION__));
+ NV_PRINTF(LEVEL_ERROR,
+ "Unable to alloc device in %s\n",
+ __FUNCTION__);
Here, it compacted a little bit, but it could have done more. This
- DBG_PRINTF((DBG_MODULE_OS, DEBUGLEVEL_TRACEINFO,
- "NVGVI: %s(), Null state\n",
- __FUNCTION__));
+ NV_PRINTF(LEVEL_INFO, "%s(), Null state\n",
+ __FUNCTION__);
In this case, I think it is just leaving __FUNCTION__ where it is.

Please always send a semantic patch and a .c file if you have a concern
about something, even if you have sent them before. Then I can treat the
problems one by one, and don't have to search around in my mailbox for the
relevant information.

thanks,
julia
Timur Tabi
2018-07-31 15:03:14 UTC
Permalink
Post by Julia Lawall
Post by Timur Tabi
Coccinelle does a pretty good job at compating parameters into fewer
lines, but it's not always aggressive enough. Is there a tuning
parameter I can try?
"--max-width", Arg.Set_int Flag_parsing_c.max_width,
" column limit for generated code";
$ spatch --sp-file ~/sw/dev/gpu_drv/chips_a/nv_printf.cocci
--max-width 80 --dir . > /tmp/n.patch
/usr/lib/coccinelle/spatch: unknown option '--max-width'.

$ spatch --version
spatch version 1.0.4 with Python support and with PCRE support
Post by Julia Lawall
Post by Timur Tabi
- DBG_PRINTF((DBG_MODULE_OS, DEBUGLEVEL_TRACEINFO,
- "NVGVI: %s(), Null state\n",
- __FUNCTION__));
+ NV_PRINTF(LEVEL_INFO, "%s(), Null state\n",
+ __FUNCTION__);
In this case, I think it is just leaving __FUNCTION__ where it is.
So what decides when it moves a parameter and when it doesn't?
Post by Julia Lawall
Please always send a semantic patch and a .c file if you have a concern
about something, even if you have sent them before. Then I can treat the
problems one by one, and don't have to search around in my mailbox for the
relevant information.
Attached.
Julia Lawall
2018-07-31 15:18:41 UTC
Permalink
Post by Timur Tabi
Post by Julia Lawall
Post by Timur Tabi
Coccinelle does a pretty good job at compating parameters into fewer
lines, but it's not always aggressive enough. Is there a tuning
parameter I can try?
"--max-width", Arg.Set_int Flag_parsing_c.max_width,
" column limit for generated code";
$ spatch --sp-file ~/sw/dev/gpu_drv/chips_a/nv_printf.cocci
--max-width 80 --dir . > /tmp/n.patch
/usr/lib/coccinelle/spatch: unknown option '--max-width'.
$ spatch --version
spatch version 1.0.4 with Python support and with PCRE support
Post by Julia Lawall
Post by Timur Tabi
- DBG_PRINTF((DBG_MODULE_OS, DEBUGLEVEL_TRACEINFO,
- "NVGVI: %s(), Null state\n",
- __FUNCTION__));
+ NV_PRINTF(LEVEL_INFO, "%s(), Null state\n",
+ __FUNCTION__);
In this case, I think it is just leaving __FUNCTION__ where it is.
So what decides when it moves a parameter and when it doesn't?
I think that if a parameter is not in danger of overflowing and is not
modified it won't move. It's not optimally formatting the argument list,
just avoiding going past the specified number of columns.

For example, some people may prefer one argument per line, while others
prefer to pack them as much as possible. It doesn't try to figure out
this preference.

julia
Post by Timur Tabi
Post by Julia Lawall
Please always send a semantic patch and a .c file if you have a concern
about something, even if you have sent them before. Then I can treat the
problems one by one, and don't have to search around in my mailbox for the
relevant information.
Attached.
Timur Tabi
2018-07-31 15:34:58 UTC
Permalink
Post by Julia Lawall
I think that if a parameter is not in danger of overflowing and is not
modified it won't move. It's not optimally formatting the argument list,
just avoiding going past the specified number of columns.
For example, some people may prefer one argument per line, while others
prefer to pack them as much as possible. It doesn't try to figure out
this preference.
That makes sense. However, when the cocci script shortens a line, the
lines now look funny. Also, it appears that since coccinelle already
has the ability to reformat function calls, it would be nice if we could
tap into that feature to do a sort of automatic reformatting.

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
Julia Lawall
2018-07-31 19:21:04 UTC
Permalink
Post by Timur Tabi
Post by Julia Lawall
Post by Timur Tabi
Coccinelle does a pretty good job at compating parameters into fewer
lines, but it's not always aggressive enough. Is there a tuning
parameter I can try?
"--max-width", Arg.Set_int Flag_parsing_c.max_width,
" column limit for generated code";
$ spatch --sp-file ~/sw/dev/gpu_drv/chips_a/nv_printf.cocci
--max-width 80 --dir . > /tmp/n.patch
/usr/lib/coccinelle/spatch: unknown option '--max-width'.
$ spatch --version
spatch version 1.0.4 with Python support and with PCRE support
Post by Julia Lawall
Post by Timur Tabi
- DBG_PRINTF((DBG_MODULE_OS, DEBUGLEVEL_TRACEINFO,
- "NVGVI: %s(), Null state\n",
- __FUNCTION__));
+ NV_PRINTF(LEVEL_INFO, "%s(), Null state\n",
+ __FUNCTION__);
In this case, I think it is just leaving __FUNCTION__ where it is.
So what decides when it moves a parameter and when it doesn't?
Post by Julia Lawall
Please always send a semantic patch and a .c file if you have a concern
about something, even if you have sent them before. Then I can treat the
problems one by one, and don't have to search around in my mailbox for the
relevant information.
Attached.
Actually, you only sent the .cocci file. I need a .c file too. The
pretty printing may be quite sensitive to the specific layout of the code.

julia
SF Markus Elfring
2018-07-31 16:12:35 UTC
Permalink
Post by Timur Tabi
Attached.
I have taken this transformation approach as an opportunity to take another look
at implementation details for further software development considerations.


@rule1@
expression x;
expression list y;
@@
-DBG_PRINTF
+NV_PRINTF
(
- (x),
y);


You repeated additions of identifiers in a few SmPL rules. I would find it nicer
to combine corresponding deletions into SmPL disjunctions (when you would like
to avoid to fiddle with regular expressions as SmPL constraints).


@A depends on rule1@
@@
NV_PRINTF(...,
- \(DBG_LEVEL_INFO \| DEBUGLEVEL_TRACEINFO \| DBG_LEVEL_SETUPINFO \| DEBUGLEVEL_SETUPINFO\)
+ LEVEL_INFO
, ...);

@B depends on rule1@
@@
NV_PRINTF(...,
- \(DBG_LEVEL_USERERRORS \| DEBUGLEVEL_USERERRORS\)
+ LEVEL_NOTICE
, ...);

@C depends on rule1@
@@
NV_PRINTF(...,
- \(DBG_LEVEL_WARNINGS \| DEBUGLEVEL_WARNINGS\)
+ LEVEL_WARNING
, ...);

@D depends on rule1@
@@
NV_PRINTF(...,
- \(DBG_LEVEL_ERRORS \| DEBUGLEVEL_ERRORS\)
+ LEVEL_ERROR
, ...);


Do you find such SmPL code adjustments useful?

Regards,
Markus
Timur Tabi
2018-07-31 19:48:34 UTC
Permalink
Post by SF Markus Elfring
@rule1@
expression x;
expression list y;
@@
-DBG_PRINTF
+NV_PRINTF
(
- (x),
y);
This doesn't work:

- DBG_PRINTF((DBG_MODULE_OS, DEBUGLEVEL_ERRORS,
- "NVRM: x86emu: int $%d (eax = %08x)\n", num, M.x86.R_EAX));
+ NV_PRINTF();
Post by SF Markus Elfring
@A depends on rule1@
@@
NV_PRINTF(...,
- \(DBG_LEVEL_INFO \| DEBUGLEVEL_TRACEINFO \| DBG_LEVEL_SETUPINFO \| DEBUGLEVEL_SETUPINFO\)
+ LEVEL_INFO
, ...);
This does work, thanks.
Post by SF Markus Elfring
Do you find such SmPL code adjustments useful?
Yes, thank you. I always appreciate optimization suggestions.

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
SF Markus Elfring
2018-08-01 06:08:45 UTC
Permalink
Post by SF Markus Elfring
@rule1@
expression x;
expression list y;
@@
-DBG_PRINTF
+NV_PRINTF
           (
-          (x),
            y);
Do you care if a macro (or function) parameter is optionally enclosed by parentheses?
-        DBG_PRINTF((DBG_MODULE_OS, DEBUGLEVEL_ERRORS,
-            "NVRM: x86emu: int $%d (eax = %08x)\n", num, M.x86.R_EAX));
+        NV_PRINTF();
Does this test result need any further clarification for the desired
software behaviour on an other change granularity?
I always appreciate optimization suggestions.
I have got another software development idea for this transformation approach.

How do you think about to improve the replacement specification a bit more
by combining the variants A till D into a single SmPL rule which will use
a nested SmPL disjunction?

Regards,
Markus
Julia Lawall
2018-08-01 06:17:46 UTC
Permalink
Post by SF Markus Elfring
How do you think about to improve the replacement specification a bit more
by combining the variants A till D into a single SmPL rule which will use
a nested SmPL disjunction?
I haven't actually looked at the semantic patch code, but I would point out
that making a disjunction is not always a good idea. The semantics of a
disjunction is that if an earlier pattern matches, then the later patterns
are not considered. The semantics of a succession of rules is that later
patterns are applied to the result of earlier patterns. These are clearly
not the same things. At the expression level, ensuring that subsequent
patterns don't apply if earlier ones do entails quite a lot of work, so a
disjunction may not be very efficient. This performance issue is much less
at the statement level.

If there are no semantics or performance issues involved, then I would be
inclined to think that the way the person first decided to write the rule is
the best way, because that is how they thought about the problem.

julia
Timur Tabi
2018-08-02 20:32:11 UTC
Permalink
Post by SF Markus Elfring
Do you care if a macro (or function) parameter is optionally enclosed by parentheses?
It's not optional. The calls to DBG_PRINTF use double parens, and need
to be replaced with single parens when using NV_PRINTF.
Post by SF Markus Elfring
-        DBG_PRINTF((DBG_MODULE_OS, DEBUGLEVEL_ERRORS,
-            "NVRM: x86emu: int $%d (eax = %08x)\n", num, M.x86.R_EAX));
+        NV_PRINTF();
Does this test result need any further clarification for the desired
software behaviour on an other change granularity?
I don't understand that.

The purpose of the script is to convert

DBG_PRINTF((DBG_MODULE_OS, DEBUGLEVEL_ERRORS, "NVRM: x86emu: int $%d
(eax = %08x)\n", num, M.x86.R_EAX));

into

NV_PRINTF(LEVEL_ERRORS, "x86emu: int $%d (eax = %08x)\n", num, M.x86.R_EAX);
Post by SF Markus Elfring
How do you think about to improve the replacement specification a bit more
by combining the variants A till D into a single SmPL rule which will use
a nested SmPL disjunction?
I don't know what a "nested SmPL disjunction" is.
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
Loading...