On 10/16/2018 04:35 PM, Jeff Law wrote:
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.
Done.
As an aside, I almost thought that after nearly fours years
I've adjusted to most reviewers' preferences but I'm clearly
not quite there yet. As usual, this is not mentioned in
the coding conventions (except for C++ template parameters
where it is the preferred spelling), and there are also
examples of different styles in GCC, including the one
I chose. For instance, in c-format.c the format_type enum
uses all lowercase enumerators. There are also quite a few
(over a hundred in fact) examples of CamelCase enums in
various front-ends, back-ends, and other parts of GCC.
@@ -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.
That's what the code does. My patch doesn't change it, it just
adds a new variable. I added a comment to mention it nonetheless.
- 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.
Same as above. The original code passed &info->first_arg_num
to get_constant and changed info->first_arg_num there. I just
introduced a temporary, first_arg_num_expr, and set it to
the address of info->first_arg_num.
+/* 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.
Done. I copied the function, including the argument name, from
from type_num_arguments (const_tree type), so I've changed that
one to match.
+{
+ /* 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
As I explained above, I just copied another function just above
the one I added.
Besides the one in the function I copied there are six other
loops in this file and just one use of FOREACH_FUNCTION_ARGS.
I also think the macro is harder to understand and poor style.
It requires declaring an extra variable outside the scope of
the loop even if it isn't used. So I kept the loop as is.
Retested on x86_64-linux.
Martin