Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
On Wed, 3 Mar 2021, Mansour Moufid wrote:
> On Tue, Mar 2, 2021 at 4:22 PM Joe Perches wrote:
> >
> > Here is a possible opportunity to reduce data usage in the kernel.
> >
> > $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]'
> > drivers/ | \
> > grep -v __initdata | \
> > wc -l
> > 3250
> >
> > Meaning there are ~3000 declarations of arrays with what appears to be
> > file static const content that are not marked const.
> >
> > So there are many static arrays that could be marked const to move the
> > compiled object code from data to text minimizing the total amount of
> > exposed r/w data.
> >
> > However, I do not know of a mechanism using coccinelle to determine
> > whether or not any of these static declarations are ever modified.
>
> I thought it would be a fun exercise but it got tedious quick.
>
> I don't know how to ignore an attribute like __initdata.
>
> Feel free to refine it:
This adds the consts, but it doesn't check that the array is never
updated, or never stored in a field from which it could get updated.
In my attempt, I tried something like this for the first part:
@r disable optional_qualifier@
type T;
identifier x;
position p != ok.p;
@@
static T x@p[] = { ... };
In principle, this should match cases where there is no const. But it
dones't work. It matches some cases with const too.
Then I tried:
@ok@
type T;
identifier x;
position p;
@@
static const T x@p[] = { ... };
@r@
type T;
identifier x;
position p != ok.p;
@@
static T x@p[] = { ... };
The first rule matches the cases with const, and then the second rule
matches all of the cases with the declared variable at a position
different than that of the first rule. It works if the type T is a simple
type like int, but it doesn't work if it is something more complex, like
struct foo *.
Afterwards, I have the following:
@bad@
position p;
identifier r.x;
expression e,y;
@@
(
x@p[y] = e
|
@p[y]
)
@good@
identifier r.x;
expression y;
position p != bad.p;
@@
x@p[y]
@notgood@
identifier r.x;
position p != good.p;
@@
x@p
@depends on good && !notgood@
identifier r.x;
type r.T;
@@
static
+const
T x[] = { ... };
That is,
* rule bad: Give up if we assign an element or take the address of an
element.
* rule good: Things are going well if we access an element of the array.
If there is no access at all, there is something suspicious, perhaps uses
in macros.
* rule notgood: A montion of the array that is not an element access is
not good either
In the end, if we match good and we don't match notgood, then we can make
the change.
I got stuck on the const problem, and didn't try the __initdata issue.
But one could address with a rule like the rule ok above.
The const problem is at least something worth looking into.
julia
>
>
> @@
> type t;
> identifier x;
> @@
> (
> static const struct { ... } x[];
> |
> static
> + const
> struct { ... } x[];
> |
> static const struct s *x[];
> |
> static
> + const
> struct s *x[];
> |
> static const struct s x[];
> |
> static
> + const
> struct s x[];
> |
> static const t *x[];
> |
> static
> + const
> t *x[];
> |
> static const t x[];
> |
> static
> + const
> t x[];
> )
>
> @@
> type t;
> identifier s, x, y, z;
> assignment operator xx;
> @@
> (
> static const struct { ... } x[] = { ... };
> |
> static
> + const
> struct { ... } x[] = { ... };
> |
> static const struct s *x[] = { ... };
> |
> static
> + const
> struct s *x[] = { ... };
> |
> static const struct s x[] = { ... };
> |
> static
> + const
> struct s x[] = { ... };
> |
> static const t *x[] = { ... };
> |
> static
> + const
> t *x[] = { ... };
> |
> static const t x[] = { ... };
> |
> static
> + const
> t x[] = { ... };
> )
> ... when != x.y xx ...
> when != x[...] xx ...
> when != z = x
> ___
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
On Tue, Mar 2, 2021 at 4:22 PM Joe Perches wrote:
>
> Here is a possible opportunity to reduce data usage in the kernel.
>
> $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]'
> drivers/ | \
> grep -v __initdata | \
> wc -l
> 3250
>
> Meaning there are ~3000 declarations of arrays with what appears to be
> file static const content that are not marked const.
>
> So there are many static arrays that could be marked const to move the
> compiled object code from data to text minimizing the total amount of
> exposed r/w data.
>
> However, I do not know of a mechanism using coccinelle to determine
> whether or not any of these static declarations are ever modified.
I thought it would be a fun exercise but it got tedious quick.
I don't know how to ignore an attribute like __initdata.
Feel free to refine it:
@@
type t;
identifier x;
@@
(
static const struct { ... } x[];
|
static
+ const
struct { ... } x[];
|
static const struct s *x[];
|
static
+ const
struct s *x[];
|
static const struct s x[];
|
static
+ const
struct s x[];
|
static const t *x[];
|
static
+ const
t *x[];
|
static const t x[];
|
static
+ const
t x[];
)
@@
type t;
identifier s, x, y, z;
assignment operator xx;
@@
(
static const struct { ... } x[] = { ... };
|
static
+ const
struct { ... } x[] = { ... };
|
static const struct s *x[] = { ... };
|
static
+ const
struct s *x[] = { ... };
|
static const struct s x[] = { ... };
|
static
+ const
struct s x[] = { ... };
|
static const t *x[] = { ... };
|
static
+ const
t *x[] = { ... };
|
static const t x[] = { ... };
|
static
+ const
t x[] = { ... };
)
... when != x.y xx ...
when != x[...] xx ...
when != z = x
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH v2] coccinelle: misc: add swap script
On Fri, 19 Feb 2021, Denis Efremov wrote:
> Check for opencoded swap() implementation.
>
> Signed-off-by: Denis Efremov
> ---
> Changes in v2:
> - additional patch rule to drop excessive {}
> - fix indentation in patch mode by anchoring ;
>
> scripts/coccinelle/misc/swap.cocci | 101 +
> 1 file changed, 101 insertions(+)
> create mode 100644 scripts/coccinelle/misc/swap.cocci
>
> diff --git a/scripts/coccinelle/misc/swap.cocci
> b/scripts/coccinelle/misc/swap.cocci
> new file mode 100644
> index ..d5da9888c222
> --- /dev/null
> +++ b/scripts/coccinelle/misc/swap.cocci
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Check for opencoded swap() implementation.
> +///
> +// Confidence: High
> +// Copyright: (C) 2021 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers
> +//
> +// Keywords: swap
> +//
> +
> +virtual patch
> +virtual org
> +virtual report
> +virtual context
> +
> +@r depends on !patch@
> +identifier tmp;
> +expression a, b;
> +type T;
> +position p;
> +@@
> +
> +(
> +* T tmp;
> +|
> +* T tmp = 0;
> +|
> +* T *tmp = NULL;
> +)
> +... when != tmp
> +* tmp = a;
> +* a = b;@p
> +* b = tmp;
> +... when != tmp
> +
> +@rpvar depends on patch@
> +identifier tmp;
> +expression a, b;
> +type T;
> +@@
> +
> +(
> +- T tmp;
> +|
> +- T tmp = 0;
> +|
> +- T *tmp = NULL;
> +)
> +... when != tmp
> +- tmp = a;
> +- a = b;
> +- b = tmp
> ++ swap(a, b)
> + ;
> +... when != tmp
> +
> +
> +@rp depends on patch@
> +identifier tmp;
> +expression a, b;
> +@@
> +
> +- tmp = a;
> +- a = b;
> +- b = tmp
> ++ swap(a, b)
> + ;
A rule like the above should also appear in the non-patch case.
> +
> +@depends on (rpvar || rp)@
This needs to be depends on patch && (rpvar || rp). It doesn't make much
sense, because rpvar and rp both depend on patch, but at the moment that
information isn't propagate everywhere that it is needed.
thanks,
julia
> +@@
> +
> +(
> + for (...;...;...)
> +- {
> + swap(...);
> +- }
> +|
> + while (...)
> +- {
> + swap(...);
> +- }
> +|
> + if (...)
> +- {
> + swap(...);
> +- }
> +)
> +
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +coccilib.report.print_report(p[0], "WARNING opportunity for swap()")
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "WARNING opportunity for swap()")
> --
> 2.26.2
>
>
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
On Tue, 2021-03-02 at 22:41 +0100, Julia Lawall wrote:
>
> On Tue, 2 Mar 2021, Joe Perches wrote:
>
> > Here is a possible opportunity to reduce data usage in the kernel.
>
> Does it actually reduce data usage?
Yes, at least for gcc. For instance:
$ gcc --version
gcc (Ubuntu 10.2.0-13ubuntu1) 10.2.0
And with the diff below (x86-64 defconfig with hwmon/pmbus and max20730)
$ diff --git a/drivers/hwmon/pmbus/max20730.c b/drivers/hwmon/pmbus/max20730.c
index 9dd3dd79bc18..b824eab95456 100644
--- a/drivers/hwmon/pmbus/max20730.c
+++ b/drivers/hwmon/pmbus/max20730.c
@@ -434,7 +434,7 @@ static long direct_to_val(u16 w, enum pmbus_sensor_classes >
return d;
}
-static u32 max_current[][5] = {
+static const u32 max_current[][5] = {
[max20710] = { 6200, 8000, 9700, 11600 },
[max20730] = { 13000, 16600, 20100, 23600 },
[max20734] = { 21000, 27000, 32000, 38000 },
$ size drivers/hwmon/pmbus/max20730.o*
textdata bss dec hex filename
9245 256 09501251d drivers/hwmon/pmbus/max20730.o.gcc.new
9149 352 09501251d drivers/hwmon/pmbus/max20730.o.gcc.old
with clang-11 it doesn't seem to make a difference:
$ /usr/bin/clang --version
Ubuntu clang version 11.0.0-2
$ size drivers/hwmon/pmbus/max20730.o*
textdata bss dec hex filename
9166 256 1942324cf
drivers/hwmon/pmbus/max20730.o.clang-11.new
9166 256 1942324cf
drivers/hwmon/pmbus/max20730.o.clang-11.old
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
On Wed, 2021-03-03 at 10:41 +0100, Rasmus Villemoes wrote:
> On 02/03/2021 18.42, Joe Perches wrote:
> > Here is a possible opportunity to reduce data usage in the kernel.
> >
> > $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]'
> > drivers/ | \
> > grep -v __initdata | \
> > wc -l
> > 3250
> >
> > Meaning there are ~3000 declarations of arrays with what appears to be
> > file static const content that are not marked const.
> >
> > So there are many static arrays that could be marked const to move the
> > compiled object code from data to text minimizing the total amount of
> > exposed r/w data.
>
> You can add const if you like, but it will rarely change the generated
> code. gcc is already smart enough to take a static array whose contents
> are provably never modified within the TU and put it in .rodata:
At least some or perhaps even most of the time, true, but the gcc compiler
from v5 through at least v10 seems inconsistent about when it does the
appropriate conversion.
See the example I posted:
https://lore.kernel.org/lkml/6b8b250a06a98ce42120a14824531a8641f5e8aa.ca...@perches.com/
It was a randomly chosen source file conversion btw, I had no prior
knowledge of whether the text/data use would change.
I'm unsure about clang consistently moving static but provably const arrays
from data to text. I rarely use clang. At least for v11 it seems to be
better though. I didn't try 10.1.
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
On 02/03/2021 18.42, Joe Perches wrote:
> Here is a possible opportunity to reduce data usage in the kernel.
>
> $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]'
> drivers/ | \
> grep -v __initdata | \
> wc -l
> 3250
>
> Meaning there are ~3000 declarations of arrays with what appears to be
> file static const content that are not marked const.
>
> So there are many static arrays that could be marked const to move the
> compiled object code from data to text minimizing the total amount of
> exposed r/w data.
You can add const if you like, but it will rarely change the generated
code. gcc is already smart enough to take a static array whose contents
are provably never modified within the TU and put it in .rodata:
static int x = 7;
static int y[2] = {13, 19};
static int p(int a, const int *foo)
{
return a + *foo;
}
int q(int a)
{
int b = p(a, );
return p(b, [b & 1]);
}
$ nm c.o
T q
r y
$ size c.o
textdata bss dec hex filename
111 0 0 111 6f c.o
So x gets optimized away completely, but y isn't so easy to get rid of -
nevertheless, it's never modified and the address doesn't escape the TU,
so gcc treats is as if it was declared const.
I think you'll see the same if you try adding the const on a few of your
real-life examples. (Of course, the control flow may be so convoluted
that gcc isn't able to infer the constness, so I'm not saying it will
never make a difference - only that you shouldn't expect too much.)
Rasmus
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
Hi all!
On 02/03/2021 18:42, Joe Perches wrote:
[...]
> - For instance: (head -10 of the git grep for file statics)
>
> drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 16,
> 8, 4, 2, 1 };
> drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
> drivers/accessibility/speakup/main.c:2059:static spkup_hand spkup_handler[] =
> {
> drivers/accessibility/speakup/speakup_acntpc.c:35:static unsigned int
> synth_portlist[] = { 0x2a8, 0 };
> drivers/accessibility/speakup/speakup_decpc.c:133:static int synth_portlist[]
> = { 0x340, 0x350, 0x240, 0x250, 0 };
> drivers/accessibility/speakup/speakup_dectlk.c:110:static int ap_defaults[] =
> {122, 89, 155, 110, 208, 240, 200, 106, 306};
> drivers/accessibility/speakup/speakup_dectlk.c:111:static int g5_defaults[] =
> {86, 81, 86, 84, 81, 80, 83, 83, 73};
> drivers/accessibility/speakup/speakup_dtlk.c:34:static unsigned int
> synth_portlist[] = {
> drivers/accessibility/speakup/speakup_keypc.c:34:static unsigned int
> synth_portlist[] = { 0x2a8, 0 };
> drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {
>
> For drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32,
> 16, 8, 4, 2, 1 };
Looking at the examples: Just s/^static /static const / in the lines
reported by the grep's above and see if it compiles (and save space)?
MfG,
Bernd
--
Bernd Petrovitsch Email : be...@petrovitsch.priv.at
There is NO CLOUD, just other people's computers. - FSFE
LUGA : http://www.luga.at
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
On Tue, 2 Mar 2021, Bernd Petrovitsch wrote:
> Hi all!
>
> On 02/03/2021 18:42, Joe Perches wrote:
> [...]
> > - For instance: (head -10 of the git grep for file statics)
> >
> > drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32,
> > 16, 8, 4, 2, 1 };
> > drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
> > drivers/accessibility/speakup/main.c:2059:static spkup_hand spkup_handler[]
> > = {
> > drivers/accessibility/speakup/speakup_acntpc.c:35:static unsigned int
> > synth_portlist[] = { 0x2a8, 0 };
> > drivers/accessibility/speakup/speakup_decpc.c:133:static int
> > synth_portlist[] = { 0x340, 0x350, 0x240, 0x250, 0 };
> > drivers/accessibility/speakup/speakup_dectlk.c:110:static int ap_defaults[]
> > = {122, 89, 155, 110, 208, 240, 200, 106, 306};
> > drivers/accessibility/speakup/speakup_dectlk.c:111:static int g5_defaults[]
> > = {86, 81, 86, 84, 81, 80, 83, 83, 73};
> > drivers/accessibility/speakup/speakup_dtlk.c:34:static unsigned int
> > synth_portlist[] = {
> > drivers/accessibility/speakup/speakup_keypc.c:34:static unsigned int
> > synth_portlist[] = { 0x2a8, 0 };
> > drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {
> >
> > For drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = {
> > 32, 16, 8, 4, 2, 1 };
>
> Looking at the examples: Just s/^static /static const / in the lines
> reported by the grep's above and see if it compiles (and save space)?
There is a small risk with compiling that there may be a problem for a
configuration you haven't considered.
julia
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
