Hi Hi Tristan,
On Thu, 24 Sep 2026 at 16:17, "Tristan Partin" <[email protected]> wrote: > On Wed Sep 23, 2026 at 6:33 AM CDT, Japin Li wrote: >> >> Hi, Tristan >> >> Thanks for updating the patches. >> >> On Wed, 23 Sep 2026 at 07:26, "Tristan Partin" <[email protected]> wrote: >>> On Tue Sep 22, 2026 at 5:28 AM UTC, Peter Eisentraut wrote: >>>> 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. >>> >>> I would love to understand the rationale for returning 1 when the >>> compiler will just throw a warning anyway. Fixed. >>> >>>> 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. >>> >>> I like the idea of restricting it to flexible array members for now. >>> It'll make for an easier review. Maybe in a subsequent patch we can >>> raise the minimum compiler versions of using pg_attribute_counted_by() >>> to GCC 16 and Clang 21. >>> >>>> 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.) >>> >>> Good catch. In the upcoming changes, I ran test suites with >>> -fsanitize=bounds, and found one place that needed a fix. Note that >>> changes to buffile.c are not currently in scope for this patchset since >>> it wasn't a flexible array member. >>> >>>> 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.) >>> >>> The second patch uses postfix notation, but subsequent patches enable >>> support for prefix notation. I'll let you be the judge of whether to >>> commit prefix or postfix. Commits 3 & 4 are genuine improvements, though >>> they do also enable prefix support. >>> >>>> Additionally, with this arrangement, we could also make use of the MSVC >>>> _Field_size_ annotation. >>> >>> This is good motivation. >>> >>>> 5) Minor: The counted_by attribute only takes a single argument, so the >>>> use of __VA_ARGS__ seems excessive. >>> >>> I think I just blindly copied surrounding macro code and forgot to >>> change it. Fixed in this new version. >>> >>>> 6) Minor: Awkward wording in comment: "This provides the compiler to >>>> improve ..." -> "enables the compiler ..."? >>> >>> Fixed. >>> >>>> 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. >>> >>> Thanks for the review. >>> >> >> I tested it locally, and all tests passed. >> >> I found some places that could use the new pg_attribute_counted_by() >> attribute >> e.g., in heapam_xlog.h, xact.h, etc. Are those intentional omissions? >> >> Was this an oversight, or is pg_attribute_counted_by not needed here? >> >> Haven't checked everywhere yet. If it is oversight, I'll check it later. > > I originally had these as well, but my understanding is that these > structures are populated from the filesystem, so in the event of data > corruption, the count field may no longer be accurate and could cause > a runtime failure similar to what Peter mentioned above in his review. > It's possible that I am being too cautious though. Let me know what you > think. I should have brought this up in my last email, but now is as > good a time as any to discuss! Thanks for the clear explanation. I agree with you on the cautious approach — especially for structures that may be populated from the filesystem, or where the count field does not strictly represent the allocated/accessible length of the flexible array member. While reviewing remaining FAMs that do not yet use pg_attribute_counted_by(), two cases looked like solid candidates: 1. BackgroundWorkerArray in bgworker.c typedef struct BackgroundWorkerArray { int total_slots; uint32 parallel_register_count; uint32 parallel_terminate_count; BackgroundWorkerSlot slot[FLEXIBLE_ARRAY_MEMBER]; } BackgroundWorkerArray; The total_slots is set once at shared-memory initialization (based on max_worker_processes) and never changes afterwards. All accesses are of the form slot[i] with i < total_slots. The invariant is therefore trivial to maintain. 2. vbits in contrib/pg_visibility/pg_visibility.c typedef struct vbits { BlockNumber next; BlockNumber count; uint8 bits[FLEXIBLE_ARRAY_MEMBER]; } vbits; The structure is allocated and filled in one place (collect_visibility_data), with count equal to the number of blocks. Subsequent use is strictly read-only and always checks next < count before indexing into bits[]. Both are purely internal, the count field matches the actual array bound, and the update order is safe. I think it would be worthwhile to annotate them. Thoughts? -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
