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.
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h
index 3f79c389a90..33805f13afe 100644
--- a/src/include/access/heapam_xlog.h
+++ b/src/include/access/heapam_xlog.h
@@ -141,7 +141,7 @@ typedef struct xl_heap_truncate
Oid dbId;
uint32 nrelids;
uint8 flags;
- Oid relids[FLEXIBLE_ARRAY_MEMBER];
+ pg_attribute_counted_by(nrelids) Oid relids[FLEXIBLE_ARRAY_MEMBER];
} xl_heap_truncate;
#define SizeOfHeapTruncate (offsetof(xl_heap_truncate, relids))
@@ -194,7 +194,7 @@ typedef struct xl_heap_multi_insert
{
uint8 flags;
uint16 ntuples;
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ pg_attribute_counted_by(ntuples) OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
} xl_heap_multi_insert;
#define SizeOfHeapMultiInsert offsetof(xl_heap_multi_insert, offsets)
@@ -402,7 +402,7 @@ typedef struct xlhp_freeze_plan
typedef struct xlhp_freeze_plans
{
uint16 nplans;
- xlhp_freeze_plan plans[FLEXIBLE_ARRAY_MEMBER];
+ pg_attribute_counted_by(nplans) xlhp_freeze_plan plans[FLEXIBLE_ARRAY_MEMBER];
} xlhp_freeze_plans;
/*
@@ -415,7 +415,7 @@ typedef struct xlhp_freeze_plans
typedef struct xlhp_prune_items
{
uint16 ntargets;
- OffsetNumber data[FLEXIBLE_ARRAY_MEMBER];
+ pg_attribute_counted_by(ntargets) OffsetNumber data[FLEXIBLE_ARRAY_MEMBER];
} xlhp_prune_items;
@@ -470,7 +470,7 @@ typedef struct xl_heap_inplace
Oid tsId; /* MyDatabaseTableSpace */
bool relcacheInitFileInval; /* invalidate relcache init files */
int nmsgs; /* number of shared inval msgs */
- SharedInvalidationMessage msgs[FLEXIBLE_ARRAY_MEMBER];
+ pg_attribute_counted_by(nmsgs) SharedInvalidationMessage msgs[FLEXIBLE_ARRAY_MEMBER];
} xl_heap_inplace;
#define MinSizeOfHeapInplace (offsetof(xl_heap_inplace, nmsgs) + sizeof(int))
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index a8cbdf247c8..1d94e38b7c3 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -220,7 +220,7 @@ typedef struct xl_xact_assignment
{
TransactionId xtop; /* assigned XID's top-level XID */
int nsubxacts; /* number of subtransaction XIDs */
- TransactionId xsub[FLEXIBLE_ARRAY_MEMBER]; /* assigned subxids */
+ pg_attribute_counted_by(nsubxacts) TransactionId xsub[FLEXIBLE_ARRAY_MEMBER]; /* assigned subxids */
} xl_xact_assignment;
#define MinSizeOfXactAssignment offsetof(xl_xact_assignment, xsub)
@@ -262,14 +262,14 @@ typedef struct xl_xact_dbinfo
typedef struct xl_xact_subxacts
{
int nsubxacts; /* number of subtransaction XIDs */
- TransactionId subxacts[FLEXIBLE_ARRAY_MEMBER];
+ pg_attribute_counted_by(nsubxacts) TransactionId subxacts[FLEXIBLE_ARRAY_MEMBER];
} xl_xact_subxacts;
#define MinSizeOfXactSubxacts offsetof(xl_xact_subxacts, subxacts)
typedef struct xl_xact_relfilelocators
{
int nrels; /* number of relations */
- RelFileLocator xlocators[FLEXIBLE_ARRAY_MEMBER];
+ pg_attribute_counted_by(nrels) RelFileLocator xlocators[FLEXIBLE_ARRAY_MEMBER];
} xl_xact_relfilelocators;
#define MinSizeOfXactRelfileLocators offsetof(xl_xact_relfilelocators, xlocators)
@@ -296,14 +296,14 @@ typedef struct xl_xact_stats_item
typedef struct xl_xact_stats_items
{
int nitems;
- xl_xact_stats_item items[FLEXIBLE_ARRAY_MEMBER];
+ pg_attribute_counted_by(nitems) xl_xact_stats_item items[FLEXIBLE_ARRAY_MEMBER];
} xl_xact_stats_items;
#define MinSizeOfXactStatsItems offsetof(xl_xact_stats_items, items)
typedef struct xl_xact_invals
{
int nmsgs; /* number of shared inval msgs */
- SharedInvalidationMessage msgs[FLEXIBLE_ARRAY_MEMBER];
+ pg_attribute_counted_by(nmsgs) SharedInvalidationMessage msgs[FLEXIBLE_ARRAY_MEMBER];
} xl_xact_invals;
#define MinSizeOfXactInvals offsetof(xl_xact_invals, msgs)
