On 09/12/2017 11:06 AM, Jeff Law wrote:
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.
I agree. This warning is for the case when the implementation
is not visible and the mismatch between the declaration and its
definition cannot be detected. E.g., when the user of a function
defined in another translation unit declares it based on
a different (e.g., earlier) version of it, or based on
a misunderstanding of its guarantees.
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.
The consequence of a const/pure conflict can be "wrong" code (i.e.,
generating code for a call to a function declared const when its
definition is, in fact, only pure, will have unexpected effects).
When both the definition and the mismatched declaration are visible
the mismatch between it and its declaration could be detected (as
a future enhancement). But without it, the second best and, IMO,
second most helpful thing GCC can do for two declarations of the
same function, one const and the other pure, is to issue a warning
about the mismatched guarantees the declarations provide to the
caller. (The ideal would be to use the pure attribute instead
of const and also emit a warning.)
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.
Okay.
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.
Okay.
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.
A default argument effectively adds an overload without the overhead
of defining one (i.e., a single declaration and definition vs one
for each) so I find it preferable (AFAICS, it's also in line with
how other similar cases are handled elsewhere in GCC).
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.
Thanks. It's not needed so I've removed it.
@@ -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.
I hope my response to Joseph along with the updated patch resolves
this concern. If not, let me know.
Thanks
Martin
