On 08/17/2017 10:03 AM, Martin Sebor wrote:
>
> First, there is the risk that someone will write code based
> on the pure declaration even if there's no intervening call
> to the function between it and the const one. Code tends to
> be sloppy, and it's also not uncommon to declare the same
> function more than once, for whatever reason. (The ssa-ccp-2.c
> test is an example of the latter.)
True. But I think if we get into a state where the semantics of the
implementation are incompatible with the listed attributes (and we can
reliably detect that) then we should issue a separate and distinct
warning. For example a pure function that reads global memory should
issue a warning/error.
ISTM such a warning/error would be a separate and distinct patch than
detection of attribute conflicts.
>
> Second, there are cases of attribute conflicts that GCC already
> points out that are much more benign in their effects (the ones
> I know about are always_inline/noinline and cold/hot). In light
> of the risk above it seems only helpful to include const/pure in
> the same set.
always_inline/noinline conflicts can cause build failures in codebases
that require certain functions to be inlined. We can argue about
whether or not that's a good policy for those codebases, but they
certainly exist. I could envision the hot/cold case generating a link
error if the linker script shoved them into different memory segments
and getting them wrong caused an overflow.
But these IMHO are corner cases and should not drive major decisions here.
>
> Third, I couldn't find another pair of attributes that GCC would
> deliberately handle this way (silently accept both but prefer one
> over the other), and introducing a special case just for these
> two seemed like a wart.
Seems reasonable.
> I do still have the question whether diagnosing attribute
> conflicts under -Wattributes is right. The manual implies
> that -Wattributes is for attributes in the wrong places or
> on the wrong entities, and that -Wignored-attributes should
> be expected instead when GCC decides to drop one for some
> reason.
This seems like it ought to be a distinct option given the way
-Wattributes is documented. Alternately we can tweak the meaning of
-Wattributes. I don't have strong opinions here.
>
> It is a little unfortunate that many -Wattributes warnings
> print just "attribute ignored" (and have done so for years).
> I think they should all be enhanced to also print why the
> attribute is ignored (e.g., "'packed' attribute on function
> declarations ignored/not valid/not supported" or something
> like that). Those that ignore attributes that would
> otherwise be valid e.g., because they conflict with other
> specifiers of the same attribute but with a different
> operand might then be candidate for changing to
> -Wignored-attributes.
Seems like a reasonable follow-up if you're interested.
>
> Thanks
> Martin
>
>
> gcc-81544-1.diff
>
>
> PR c/81544 - attribute noreturn and warn_unused_result on the same function
> accepted
>
> gcc/c/ChangeLog:
>
> PR c/81544
> * c-decl.c (c_decl_attributes): Look up existing declaration and
> pass it to decl_attributes.
>
> gcc/c-family/ChangeLog:
>
> PR c/81544
> * c-attribs.c (attr_aligned_exclusions): New array.
> (attr_alloc_exclusions, attr_cold_hot_exclusions): Same.
> (attr_common_exclusions, attr_const_pure_exclusions): Same.
> (attr_gnu_inline_exclusions, attr_inline_exclusions): Same.
> (attr_noreturn_exclusions, attr_returns_twice_exclusions): Same.
> (attr_warn_unused_result_exclusions): Same.
> (handle_hot_attribute, handle_cold_attribute): Simplify.
> (handle_const_attribute): Warn on function returning void.
> (handle_pure_attribute): Same.
> * c-warn.c (diagnose_mismatched_attributes): Simplify.
>
> gcc/testsuite/ChangeLog:
>
> PR c/81544
> * c-c++-common/Wattributes-2.c: New test.
> * c-c++-common/Wattributes.c: New test.
> * c-c++-common/attributes-3.c: Adjust.
> * gcc.dg/attr-noinline.c: Adjust.
> * gcc.dg/pr44964.c: Same.
> * gcc.dg/torture/pr42363.c: Same.
> * gcc.dg/tree-ssa/ssa-ccp-2.c: Same.
>
> gcc/ChangeLog:
>
> PR c/81544
> * attribs.c (empty_attribute_table): Initialize new member of
> struct attribute_spec.
> (decl_attributes): Add argument. Handle mutually exclusive
> combinations of attributes.
> * attribs.h (decl_attributes): Add default argument.
> * tree-core.h (attribute_spec::exclusions, exclude): New type and
> member.
> * doc/extend.texi (Common Function Attributes): Update const and pure.
> diff --git a/gcc/attribs.h b/gcc/attribs.h
> index d4a790b..a9eba0a 100644
> --- a/gcc/attribs.h
> +++ b/gcc/attribs.h
> @@ -31,7 +31,7 @@ extern void init_attributes (void);
> from tree.h. Depending on these flags, some attributes may be
> returned to be applied at a later stage (for example, to apply
> a decl attribute to the declaration rather than to its type). */
> -extern tree decl_attributes (tree *, tree, int);
> +extern tree decl_attributes (tree *, tree, int, tree = NULL_TREE);
So we do allow default arguments. Though there seems to be some
preference for just defining an overload instead. The arguments for
using overloads instead of a default argument here aren't real
compelling though.
Anyway, consider using an overload. If you think the default argument
is cleaner, I won't object.
>
> extern bool cxx11_attribute_p (const_tree);
> extern tree get_attribute_name (const_tree);
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index 0d9ab2d..8a157f6 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see
> #include "tree-iterator.h"
> #include "opts.h"
> #include "gimplify.h"
> +#include "bitmap.h"
I don't immediately see where you need bitmap.h in here, but I could
have missed it. It's not a problem if you need it, but if not, let's
avoid adding it unnecessarily.
> @@ -1667,14 +1769,18 @@ check_cxx_fundamental_alignment_constraints (tree
> node,
> struct attribute_spec.handler. */
>
> static tree
> -handle_aligned_attribute (tree *node, tree ARG_UNUSED (name), tree args,
> +handle_aligned_attribute (tree *node, tree name, tree args,
> int flags, bool *no_add_attrs)
> {
> tree decl = NULL_TREE;
> tree *type = NULL;
> - int is_type = 0;
> + bool is_type = false;
> tree align_expr;
> - int i;
> +
> + /* The last (already pushed) declaration with all validated attributes
> + merged in or the current about-to-be-pushed one if one hassn't been
s/hassn't/hasn't/
I'd like to echo Joseph's comment WRT the mirroring of exclusions.
Ideally setting up the structure so that happens automatically is best,
but alternately, testing for symmetry is acceptable. If asymmetry is
desirable, it needs to be documented.
Largely it looks good. I think the biggest issue is the exclusion
tables and how to deal with cases where we need to ensure symmetry vs
any cases where asymmetry is expected/desirable.
Jeff
