Re: [PATCH net-next v8 03/10] lib: packing: add pack_fields() and unpack_fields()
From: Jacob Keller <jacob.e.keller@intel.com>
Date: 2024-12-04 23:25:15
On 12/4/2024 9:12 AM, Vladimir Oltean wrote:
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.
I figured it out! There are two key issues involved:
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); - 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);
This needs to be i - 1, since arrays are 0-indexed, so this code expands to checking the wrong value. CHECK_PACKED_FIELDS_1 needs to become CHECK_PACKED_FIELD(fields, 0) but this code makes it: CHECK_PACKED_FIELD(fields, 1) Thus, all the array definitions are off-by-one, leading to the last one being out-of-bounds.
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)
Nobody accesses past element 6 (ARRAY_SIZE) of test_fields[]. I ran the
The array size is 6, but we try to access element 6 which is one past
the array... good old off-by-one error :)
There is one further complication which is that the nested statement
expressions ({ ... }) for each CHECK_PACKED_FIELD_N eventually make GCC
confused, as it doesn't seem to keep track of the types very well.
I fixed that by changing the individual CHECK_PACKED_FIELD_N to be
non-statement expressions, and then wrapping their calls in the
builtin_choose_expr() with ({ ... }), which prevents us from creating
too many expression layers for GCC. It actually results in identical
code being evaluated as with the old version but now with a constant
scaling of the text size: 2 lines per additional check. Of course the
complexity scales linearly, but that means our text size no longer
scales with O(n*log(n)) but just as O(N).
Its fantastic improvement, thanks for the suggestion. I'll have v9 out
with these improvements soon.