> On Oct 5, 2023, at 4:08 PM, Siddhesh Poyarekar <siddh...@gotplt.org> wrote: > > 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.
Good point! So, your example in the respond to [V3][PATCH 2/3]Use the counted_by atribute info in builtin object size [PR108896]: “ Maybe another test where the allocation, size assignment and __bdos call happen in the same function, where the allocator is not recognized by gcc: void * __attribute__ ((noinline)) alloc (size_t sz) { return __builtin_malloc (sz); } void test (size_t sz) { array_annotated = alloc (sz); array_annotated->b = sz; return __builtin_dynamic_object_size (array_annotated->c, 1); } The interesting thing to test (and ensure in the codegen) is that the assignment to array_annotated->b does not get reordered to below the __builtin_dynamic_object_size call since technically there is no data dependency between the two. “ Will test on this. Not sure whether the current GCC alias analysis is able to distinguish one field of a structure from another field of the same structure, if YES, then We need to add an explicit dependency edge from the write to “array_annotated->b” to the call to “__builtin_dynamic_object_size(array_annotated->c,1)”. I will check on this and see how to resolve this issue. I guess the possible solution is that we can add an implicit ref to “array_annotated->b” at the call to “__builtin_dynamic_object_size(array_annotated->c, 1)” if the counted_by attribute is available. That should resolve the issue. Richard, what do you think on this? > > 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. As I replied to Martin in another email, I plan to do the following to resolve this issue: 1. No specification for signed or unsigned for counted_by field. 2. Add a sanitizer option -fsanitize=counted-by-bound to catch the cases when the size of the counted-by is not positive. Then, we will be consistent with the handling of VLA. So, I will not change anything for the current patch. However, I will add the sanitizer option in a followup patch set. Let me know your opinion. thanks. Qing > > 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