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

[Qing Zhao <qing.zhao@oracle.com>]

Ping on this patch set.

Thanks a lot!

Qing

> On Feb 16, 2024, at 14:47, Qing Zhao <qing.zhao@oracle.com> wrote:
> 
> Hi,
> 
> This is the 6th version of the patch.
> 
> compare with the 5th version, the only difference is:
> 
> 1. Add the 6th argument to .ACCESS_WITH_SIZE
>   to carry the TYPE of the flexible array.
>   Such information is needed during tree-object-size.cc.
> 
>   previously, we use the result type of the routine
>   .ACCESS_WITH_SIZE to decide the element type of the
>   original array, however, the result type of the routine
>   might be changed during tree optimizations due to 
>   possible type casting in the source code.
> 
> 
> compare with the 4th version, the major difference are:
> 
> 1. Change the return type of the routine .ACCESS_WITH_SIZE 
>   FROM:
>     Pointer to the type of the element of the flexible array;
>   TO:
>     Pointer to the type of the flexible array;
>    And then wrap the call with an indirection reference. 
> 
> 2. Adjust all other parts with this change, (this will simplify the bound sanitizer instrument);
> 
> 3. Add the fixes to the kernel building failures, which include:
>    A. The operator “typeof” cannot return correct type for a->array; 
>    B. The operator “&” cannot return correct address for a->array;
> 
> 4. Correctly handle the case when the value of “counted-by” is zero or negative as following
>   4.1. Update the counted-by doc with the following:
>    When the counted-by field is assigned a negative integer value, the compiler will treat the value as zero. 
>   4.2. Adjust __bdos and array bound sanitizer to handle correctly when “counted-by” is zero. 
> 
> 
> It based on the following proposal:
> 
> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635884.html
> Represent the missing dependence for the "counted_by" attribute and its consumers
> 
> ******The summary of the proposal is:
> 
> * Add a new internal function ".ACCESS_WITH_SIZE" to carry the size information for every reference to a FAM field;
> * In C FE, Replace every reference to a FAM field whose TYPE has the "counted_by" attribute with the new internal function ".ACCESS_WITH_SIZE";
> * In every consumer of the size information, for example, BDOS or array bound sanitizer, query the size information or ACCESS_MODE information from the new internal function;
> * When expansing to RTL, replace the internal function with the actual reference to the FAM field;
> * Some adjustment to ipa alias analysis, and other SSA passes to mitigate the impact to the optimizer and code generation.
> 
> 
> ******The new internal function
> 
>  .ACCESS_WITH_SIZE (REF_TO_OBJ, REF_TO_SIZE, CLASS_OF_SIZE, TYPE_OF_SIZE, ACCESS_MODE, TYPE_OF_REF)
> 
> INTERNAL_FN (ACCESS_WITH_SIZE, ECF_LEAF | ECF_NOTHROW, NULL)
> 
> which returns the "REF_TO_OBJ" same as the 1st argument;
> 
> Both the return type and the type of the first argument of this function have been converted from the incomplete array type to the corresponding pointer type.
> 
> The call to .ACCESS_WITH_SIZE is wrapped with an INDIRECT_REF, whose type is the original imcomplete array type.
> 
> Please see the following link for why:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638793.html
> https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639605.html
> 
> 1st argument "REF_TO_OBJ": The reference to the object;
> 2nd argument "REF_TO_SIZE": The reference to the size of the object,
> 3rd argument "CLASS_OF_SIZE": The size referenced by the REF_TO_SIZE represents
>   0: unknown;
>   1: the number of the elements of the object type;
>   2: the number of bytes;
> 4th argument "TYPE_OF_SIZE": A constant 0 with the TYPE of the object
>  refed by REF_TO_SIZE
> 5th argument "ACCESS_MODE":
>  -1: Unknown access semantics
>   0: none
>   1: read_only
>   2: write_only
>   3: read_write
> 6th argument "TYPE_OF_REF": A constant 0 with the pointer TYPE to
>  the original flexible array type.
> 
> ****** The Patch sets included:
> 
> 1. Provide counted_by attribute to flexible array member field;
>      which includes:
>      * "counted_by" attribute documentation;
>      * C FE handling of the new attribute;
>        syntax checking, error reporting;
>      * testing cases;
> 
> 2. Convert "counted_by" attribute to/from .ACCESS_WITH_SIZE.
>      which includes:
>      * The definition of the new internal function .ACCESS_WITH_SIZE in internal-fn.def.
>      * C FE converts every reference to a FAM with "counted_by" attribute to a call to the internal function .ACCESS_WITH_SIZE.
>        (build_component_ref in c_typeck.cc)
>        This includes the case when the object is statically allocated and initialized.
>        In order to make this working, we should update initializer_constant_valid_p_1 and output_constant in varasm.cc to include calls to .ACCESS_WITH_SIZE.
> 
>        However, for the reference inside "offsetof", ignore the "counted_by" attribute since it's not useful at all. (c_parser_postfix_expression in c/c-parser.cc)
> 	In addtion to "offsetof", for the reference inside operator "typeof" and
>  "alignof", we ignore counted_by attribute too.
>  	When building ADDR_EXPR for the .ACCESS_WITH_SIZE in C FE,
>  replace the call with its first argument.
> 
>      * Convert every call to .ACCESS_WITH_SIZE to its first argument.
>        (expand_ACCESS_WITH_SIZE in internal-fn.cc)
>      * adjust alias analysis to exclude the new internal from clobbering anything.
>        (ref_maybe_used_by_call_p_1 and call_may_clobber_ref_p_1 in tree-ssa-alias.cc)
>      * adjust dead code elimination to eliminate the call to .ACCESS_WITH_SIZE when
>        it's LHS is eliminated as dead code.
>        (eliminate_unnecessary_stmts in tree-ssa-dce.cc)
>      * Provide the utility routines to check the call is .ACCESS_WITH_SIZE and
>        get the reference from the call to .ACCESS_WITH_SIZE.
>        (is_access_with_size_p and get_ref_from_access_with_size in tree.cc)
>      * testing cases. (for offsetof, static initialization, generation of calls to
>        .ACCESS_WITH_SIZE, code runs correctly after calls to .ACCESS_WITH_SIZE are
>        converted back)
> 
> 3. Use the .ACCESS_WITH_SIZE in builtin object size (sub-object only)
>      which includes:
>      * use the size info of the .ACCESS_WITH_SIZE for sub-object.
>      * when the size is a negative integer, treat it as zero.
>      * testing cases. 
> 
> 4 Use the .ACCESS_WITH_SIZE in bound sanitizer
>      which includes:
>      * Instrument array_ref with a call to .ACCESS_WITH_SIZE for bound sanitizer.
>      * when the size is a negative integer, treat it as zero.
>      * testing cases. 
> 
> 5. Add the 6th argument to .ACCESS_WITH_SIZE to carry the TYPE of the flexible array.
>      which includes:
>      * Add the 6th argument to .ACCESS_WITH_SIZE.
>      * use the type of the 6th argument of the routine in tree-object-size.cc
>      * testing case.
> 
> ******Remaining works: 
> 
> 6  Improve __bdos to use the counted_by info in whole-object size for the structure with FAM.
> 7  Emit warnings when the user breaks the requirments for the new counted_by attribute
>   compilation time: -Wcounted-by
>   run time: -fsanitizer=counted-by
>      * The initialization to the size field should be done before the first reference to the FAM field.
>      * the array has at least # of elements specified by the size field all the time during the program.
> 
> I have bootstrapped and regression tested on both x86 and aarch64, no issue.
> Linux kernel linux-6.8-rc4 has been built and exposed one bug with the new counted-by, fixed.
> 
> Let me know your comments.
> 
> thanks.
> 
> Qing
> 
> Qing Zhao (5):
>  Provide counted_by attribute to flexible array member field (PR108896)
>  Convert references with "counted_by" attributes to/from
>    .ACCESS_WITH_SIZE.
>  Use the .ACCESS_WITH_SIZE in builtin object size.
>  Use the .ACCESS_WITH_SIZE in bound sanitizer.
>  Add the 6th argument to .ACCESS_WITH_SIZE