Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2023-08-25 11:24, Qing Zhao wrote:
> This is the 3rd version of the patch, per our discussion based on the
> review comments for the 1st and 2nd version, the major changes in this
> version are:

Hi Qing,

I hope the review was helpful.  Overall, a couple of things to consider:

1. How would you handle potential reordering between assignment of the 
size to the counted_by field with the __bdos call that may consume it? 
You'll probably need to express some kind of dependency there or in the 
worst case, insert a barrier to disallow reordering.

2. How would you handle signedness of the size field?  The size gets 
converted to sizetype everywhere it is used and overflows/underflows may 
produce interesting results.  Do you want to limit the types to unsigned 
or do you want to add a disclaimer in the docs?  The former seems like 
the *right* thing to do given that it is a new feature; best to enforce 
the cleaner habit at the outset.

Thanks,
Sid

> 
> ***Against 1st version:
> 1. change the name "element_count" to "counted_by";
> 2. change the parameter for the attribute from a STRING to an
> Identifier;
> 3. Add logic and testing cases to handle anonymous structure/unions;
> 4. Clarify documentation to permit the situation when the allocation
> size is larger than what's specified by "counted_by", at the same time,
> it's user's error if allocation size is smaller than what's specified by
> "counted_by";
> 5. Add a complete testing case for using counted_by attribute in
> __builtin_dynamic_object_size when there is mismatch between the
> allocation size and the value of "counted_by", the expecting behavior
> for each case and the explanation on why in the comments.
> 
> ***Against 2rd version:
> 1. Identify a tree node sharing issue and fixed it in the routine
>     "component_ref_get_counted_ty" of tree.cc;
> 2. Update the documentation and testing cases with the clear usage
>     of the fomula to compute the allocation size:
> MAX (sizeof (struct A), offsetof (struct A, array[0]) + counted_by * sizeof(element))
>     (the algorithm used in tree-object-size.cc is correct).
> 
> In this set of patches, the major functionality provided is:
> 
> 1. a new attribute "counted_by";
> 2. use this new attribute in bound sanitizer;
> 3. use this new attribute in dynamic object size for subobject size;
> 
> As discussed, I plan to add two more separate patches sets after this initial
> patch set is approved and committed.
> 
> set 1. A new warning option and a new sanitizer option for the user error
>        when the allocation size is smaller than the value of "counted_by".
> set 2. An improvement to __builtin_dynamic_object_size  for whole-object
>        size of the structure with FAM annaoted with counted_by.
> 
> there are also some existing bugs in tree-object-size.cc identified
> during the study, and PRs were filed to record them. these bugs will
> be fixed seperately with individual patches:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111030
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111040
> 
> Bootstrapped and regression tested on both aarch64 and X86, no issue.
> 
> Please see more details on the description of this work on:
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619708.html
> 
> and more discussions on
> https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626376.html
> 
> Okay for committing?
> 
> thanks.
> 
> Qing
> 
> Qing Zhao (3):
>    Provide counted_by attribute to flexible array member field (PR108896)
>    Use the counted_by atribute info in builtin object size [PR108896]
>    Use the counted_by attribute information in bound sanitizer[PR108896]
> 
>   gcc/c-family/c-attribs.cc                     |  54 ++++-
>   gcc/c-family/c-common.cc                      |  13 ++
>   gcc/c-family/c-common.h                       |   1 +
>   gcc/c-family/c-ubsan.cc                       |  16 ++
>   gcc/c/c-decl.cc                               |  79 +++++--
>   gcc/doc/extend.texi                           |  77 +++++++
>   .../gcc.dg/flex-array-counted-by-2.c          |  74 ++++++
>   .../gcc.dg/flex-array-counted-by-3.c          | 210 ++++++++++++++++++
>   gcc/testsuite/gcc.dg/flex-array-counted-by.c  |  40 ++++
>   .../ubsan/flex-array-counted-by-bounds-2.c    |  27 +++
>   .../ubsan/flex-array-counted-by-bounds.c      |  46 ++++
>   gcc/tree-object-size.cc                       |  37 ++-
>   gcc/tree.cc                                   | 133 +++++++++++
>   gcc/tree.h                                    |  15 ++
>   14 files changed, 797 insertions(+), 25 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by-2.c
>   create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by-3.c
>   create mode 100644 gcc/testsuite/gcc.dg/flex-array-counted-by.c
>   create mode 100644 gcc/testsuite/gcc.dg/ubsan/flex-array-counted-by-bounds-2.c
>   create mode 100644 gcc/testsuite/gcc.dg/ubsan/flex-array-counted-by-bounds.c
>