On 30.07.26 00:07, Tristan Partin wrote:
The counted_by[0] compiler attribute is fairly new. It was added in GCC
15 and Clang 18. It has been used fairly extensively in the Linux
kernel[0].
To summarize the benefits of the attribute:
- Runtime bounds checking with -DFORTIFY_SOURCE=3 and -fsanitize-bounds
- Accurate reporting of __builtin_dynamic_object_size()
While we don't use __builtin_dynamic_object_size(), I think the runtime
bounds checking improvements are easily worth the little bit of effort
to add the attribute in various locations and review the code. I think
it will improve things for buildfarm animals using ASan due to expanded
coverage.
I took a closer look at this. There are several problems with the
proposed patches.
1) In C++, both gcc and clang have __has_attribute(counted_by) return 1
(true), but the compiler actually rejects the attribute with a warning.
This is not immediately evident in your patch, but it would show up
under cpluspluscheck and whenever we extend this attribute to header
files that happen to get pulled in by C++ source files.
(access/tupdesc.h is an obvious candidate.) Therefore, there needs to be
some #ifndef __cplusplus somewhere.
2) gcc 15 and clang 18 accept the counted_by attribute only for flexible
array members, not for pointers members. (Using it on a pointer causes
an error.) If you want to apply this to pointer members, as your patch
does in buffile.c, you'd have to write a configure test. Or else
restrict it to flexible array members for now.
3) The counted_by attribute requires that, when extending the counted
array, the count field is increased before writing into the new element
at the end. The code dealing with struct BufFile currently doesn't do
that, and so your change in buffile.c fails under -fsanitize=bounds:
../src/backend/storage/file/buffile.c:919:3: runtime error: index 1 out
of bounds for type 'File * __counted_by(numFiles)' (aka 'int *')
(Reproduce with meson configure -Db_sanitize=bounds and meson test ...
--suite regress.)
The code needs to be carefully analyzed and adjusted to fix this. (The
code for the tuplesort.c change appears to be ok.)
4) Although the compilers are flexible with the placement, the most
correct placement of the attribute is at the beginning of the
declaration, like
pg_attribute_counted_by(nTapes) TapeShare tapes[FLEXIBLE_ARRAY_MEMBER];
(Note that the gcc documentation effectively writes it this way.)
Additionally, with this arrangement, we could also make use of the MSVC
_Field_size_ annotation.
5) Minor: The counted_by attribute only takes a single argument, so the
use of __VA_ARGS__ seems excessive.
6) Minor: Awkward wording in comment: "This provides the compiler to
improve ..." -> "enables the compiler ..."?
Suggestion:
- Add C++ guard. (Maybe add annotation in access/tupdesc.h to test.)
- Skip use of the attribute on pointer members for now.
- Make sure cpluspluscheck and -fsanitize=bounds pass.
- Consider the cosmetic adjustments mentioned.
