On 10/8/18 5:22 PM, Martin Sebor wrote:
> Attached is an updated patch with the INTEGRAL_TYPE_P test added
> to detect constant non-integer arguments like (void*)0, and with
> quoting made unconditional. I also removed the pretty printer
> business to avoid the "value%s" format, and modified the manual
> to clarify when each of the attributes are applicable and what
> their meaningful argument values are.
>
> On 10/07/2018 04:38 PM, Martin Sebor wrote:
>> While still testing an enhancement in the area of attributes
>> I ran across bugs and inconsistencies in how different handlers
>> deal with positional arguments. The bugs are either an ICE due
>> to the handlers not consistently converting positional arguments
>> to constants (i.e., CONST_DEL to INTEGER_CST in C++) for which
>> downstream code is unprepared (PR 87541), or errors for valid
>> code (PR 87542), or failing to diagnose invalid arguments.
>> The other inconsistencies are simply in responding to the same
>> invalid condition differently for different attributes .
>>
>> The attached patch introduces a new function to do validate
>> positional arguments in a uniform way and replaces the existing
>> handling with it.
>>
>> As a consequence of the handling being made consistent a number
>> of tests needed adjusting. In addition, some invalid arguments
>> that were previously rejected with a hard error are diagnosed
>> with just a warning (invalid argument values in attribute format),
>> and in one other instance what previously triggered a warning is
>> now accepted without one (attribute alloc_size on a function
>> without a prototype). I'd be up tightening things up if that's
>> preferable as long it's done consistently.
>>
>> Tested on x86_64-linux.
>>
>> Martin
>>
>> PS It would be nice to have a general policy for how to respond
>> to invalid attribute arguments (warning or error). Even better,
>> it would be ideal to move the validation from the front-ends and
>> back-ends into the middle-end. I.e., describe attribute arguments
>> in more detail in struct attribute_spec and have decl_attributes
>> validate nut just the number of arguments but also their types
>> and, when appropriate, expected values.
>
>
> gcc-87541.diff
>
> PR c++/87541 - ICE using a constant decl as an attribute alloc_size argument
> PR c++/87542 - bogus error on attribute format with a named constant argument
>
> gcc/ChangeLog:
>
> PR c++/87541
> PR c++/87542
> * tree.c (type_argument_type): New function.
> * tree.h (type_argument_type): Declare it.
> * gcc/doc/extend.texi (alloc_align): Update and clarify.
> (alloc_size, nonnull, sentinel): Same.
>
> gcc/c-family/ChangeLog:
>
> PR c++/87541
> PR c++/87542
> * c-attribs.c (positional_argument): New function.
> (handle_alloc_size_attribute): Use it and simplify.
> (handle_alloc_align_attribute): Same.
> (handle_assume_aligned_attribute): Same.
> (handle_nonnull_attribute): Same.
> * c-common.c (check_function_arguments): Pass fntype to
> check_function_format.
> * c-common.h (check_function_format): Add an argument.
> (PosArgFlags, positional_argument): Declare new type and function.
> * c-format.c (decode_format_attr): Add arguments.
> (check_format_string, get_constant): Same.
> (convert_format_name_to_system_name): Adjust.
>
> gcc/testsuite/ChangeLog:
>
> PR c++/87541
> PR c++/87542
> * g++.dg/ext/attr-alloc_size.C: New test.
> * c-c++-common/pr71574.c: Adjust diagnostics.
> * c-c++-common/attributes-1.c: Same.
> * gcc.dg/attr-alloc_align-2.c: Same.
> * gcc.dg/attr-alloc_align-4.c: New test.
> * gcc.dg/attr-alloc_size-2.c: Adjust diagnostics.
> * gcc.dg/attr-alloc_size.c: Same.
> * gcc.dg/attr-assume_aligned-4.c: New test.
> * gcc.dg/format/attr-3.c: Adjust diagnostics.
> * gcc.dg/nonnull-2.c: Same.
> * obj-c++.dg/attributes/method-format-1.mm: Same.
> * obj-c++.dg/attributes/method-nonnull-1.mm: Same.
> * objc.dg/attributes/method-format-1.m: same.
> * objc.dg/attributes/method-nonnull-1.m: Same.
> Index: gcc/c-family/c-common.c
> @@ -1322,6 +1323,18 @@ extern int tm_attr_to_mask (tree);
> extern tree tm_mask_to_attr (int);
> extern tree find_tm_attribute (tree);
>
> +/* A bitmap of flags to positional_argument. */
> +enum PosArgFlags {
> + /* Consider positional attribute argument value zero valid. */
> + posarg_zero = 1,
> + /* Consider positional attribute argument value valid if it refers
> + to the ellipsis (i.e., beyond the last typed argument). */
> + posarg_ellipsis = 2
> +};
> +
> +extern tree positional_argument (const_tree, const_tree, tree, tree_code,
> + int = 0, int = PosArgFlags ());
> +
> extern enum flt_eval_method
> excess_precision_mode_join (enum flt_eval_method, enum flt_eval_method);
No camel case. Make the enum type lower case and its values upper case.
> @@ -326,17 +331,18 @@ static bool
> }
> }
>
> - if (!get_constant (format_num_expr, &info->format_num, validated_p))
> - {
> - error ("format string has invalid operand number");
> - return false;
> - }
> + if (tree val = get_constant (fntype, atname, *format_num_expr,
> + 2, &info->format_num, 0, validated_p))
> + *format_num_expr = val;
> + else
> + return false;
Is it really a good idea to be modifying something inside of ARGS like
this? At the very least the function comments neeed updating.
>
> - if (!get_constant (first_arg_num_expr, &info->first_arg_num, validated_p))
> - {
> - error ("%<...%> has invalid operand number");
> - return false;
> - }
> + if (tree val = get_constant (fntype, atname, *first_arg_num_expr,
> + 3, &info->first_arg_num,
> + (posarg_zero | posarg_ellipsis), validated_p))
> + *first_arg_num_expr = val;
> + else
> + return false;
Similarly.
>
> +/* Return the type of the function TYPE's argument ARGNO if known.
> + For vararg function's where ARGNO refers to one of the variadic
> + arguments return null. Otherwise, return a void_type_node for
> + out-of-bounds ARGNO. */
> +
> +tree
> +type_argument_type (const_tree type, unsigned argno)
You might consider calling the argument "fntype". That makes it a
little clearer what you're iterating over.
> +{
> + /* Treat zero the same as an out-of-bounds argument number. */
> + if (!argno)
> + return void_type_node;
> +
> + unsigned i = 1;
> +
> + for (tree t = TYPE_ARG_TYPES (type); ; t = TREE_CHAIN (t), ++i)
There's already iterators to walk over TYPE_ARG_TYPES. See
FOREACH_FUNCTION_ARGS
Mostly good, probably requires one round of updating and we'll be good
to go.
jeff
