Re: [PATCH net-next v8 03/10] lib: packing: add pack_fields() and unpack_fields()
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: 2024-12-04 23:43:44
On 12/4/24 18:12, Vladimir Oltean wrote:
On Tue, Dec 03, 2024 at 03:53:49PM -0800, Jacob Keller wrote:
Amazing stuff :), I really like the fact that you both keep striving for the best result, even way past the point of cut off of most other ;)
quoted hunk ↗ jump to hunk
diff --git a/scripts/gen_packed_field_checks.c b/scripts/gen_packed_field_checks.c index 09a21afd640b..fabbb741c9a8 100644 --- a/scripts/gen_packed_field_checks.c +++ b/scripts/gen_packed_field_checks.c@@ -9,15 +9,9 @@ int main(int argc, char **argv) { for (int i = 1; i <= MAX_PACKED_FIELD_SIZE; i++) { printf("#define CHECK_PACKED_FIELDS_%d(fields) ({ \\\n", i);
[i] @i in range 1..MAX_PACKED_FIELD_SIZE; inclusive
- printf("\ttypeof(&(fields)[0]) _f = (fields); \\\n");
- printf("\tBUILD_BUG_ON(ARRAY_SIZE(fields) != %d); \\\n", i);
for (int j = 0; j < i; j++)
- printf("\tCHECK_PACKED_FIELD(_f[%d]); \\\n", j);
-
- for (int j = 1; j < i; j++)
- printf("\tCHECK_PACKED_FIELD_OVERLAP(_f[0].startbit < _f[1].startbit, _f[%d], _f[%d]); \\\n",
- j - 1, j);
+ printf("\tCHECK_PACKED_FIELD(fields, %d); \\\n", j);[j] @j < @i
printf("})\n\n");
}
And there's one more thing I tried, which mostly worked. That was to
express CHECK_PACKED_FIELDS_N in terms of CHECK_PACKED_FIELDS_N-1.
This further reduced the auto-generated code size from 1478 lines to 302
lines, which I think is appealing.a lot :)
quoted hunk ↗ jump to hunk
diff --git a/scripts/gen_packed_field_checks.c b/scripts/gen_packed_field_checks.c index fabbb741c9a8..bac85c04ef20 100644 --- a/scripts/gen_packed_field_checks.c +++ b/scripts/gen_packed_field_checks.c@@ -10,9 +10,10 @@ int main(int argc, char **argv) for (int i = 1; i <= MAX_PACKED_FIELD_SIZE; i++) { printf("#define CHECK_PACKED_FIELDS_%d(fields) ({ \\\n", i);
same as [i] above, ok
- for (int j = 0; j < i; j++)
- printf("\tCHECK_PACKED_FIELD(fields, %d); \\\n", j);
+ if (i != 1)
+ printf("\tCHECK_PACKED_FIELDS_%d(fields); \\\n", i - 1);
+ printf("\tCHECK_PACKED_FIELD(fields, %d); \\\n", i);prior to the change CHECK_PACKED_FIELD() was called on values smaller than MAX_PACKED_FIELD_SIZE, compare with [j] above, now you call it also for the MAX one
printf("})\n\n");
}
The problem is that, for some reason, it introduces this sparse warning:
../lib/packing_test.c:436:9: warning: invalid access past the end of 'test_fields' (24 24)
../lib/packing_test.c:448:9: warning: invalid access past the end of 'test_fields' (24 24)off by one error? see above
Nobody accesses past element 6 (ARRAY_SIZE) of test_fields[]. I ran the KUnit with kasan and I saw no warning. The strace warning comes from check_access() in flow.c, but I don't have any energy left today to go further into this. I'm suspecting either a strace bug/false positive, or some sort of variable name aliasing issue which I haven't identified yet.
PS. incremental diff in a single patch is harder to apply, but easier to review, comment both in a single reply == great idea