First, didn't Marek say in the PR that the diagnostic code should go in
diagnose_mismatched_attributes?
An overall comment-- could we write a generic validator rather than
having to special case validation on a case by case basis?
Is there way of marking attributes as immutable if specified on the same
decl? For example, marking that alloc_size, nonnull, sentinel, etc
should never have differing versions of the same attribute for the same
decl name? And then you could go and validate each of those attributes
such marked automatically.
Or is this too much work? Marek, as the C maintainer what are your
thoughts? ;-)
Regardless, here are some random comments.
On 7/9/20 2:01 AM, Martin Sebor via Gcc-patches wrote:
GCC has gotten better at detecting conflicts between various
attributes but it still doesn't do a perfect job of detecting
similar problems due to mismatches between contradictory
arguments to the same attribute. For example,
__attribute ((alloc_size (1))) void* allocate (size_t, size_t);
followed by
__attribute ((alloc_size (2))) void* allocate (size_t, size_t);
is accepted with the former overriding the latter in calls to
the function. Similar problem exists with a few other attributes
that take arguments.
The attached change adds a new utility function that checks for
such mismatches and issues warnings. It also adds calls to it
to detect the problem in attributes alloc_align, alloc_size, and
section. This isn't meant to be a comprehensive fix but rather
a starting point for one.
Tested on x86_64-linux.
Martin
PS I ran into this again while debugging some unrelated changes
and wondering about the behavior in similar situations to mine.
Since the behavior seemed clearly suboptimal I figured I might
as well fix it.
PPS The improved checking triggers warnings in a few calls to
__builtin_has_attribute due to apparent conflicts. I've xfailed
those in the test since it's a known issue with some existing
attributes that should be fixed at some point. Valid uses of
the built-in shouldn't trigger diagnostics except for completely
nonsensical arguments. Unfortunately, the line between valid
and completely nonsensical is a blurry one (GCC either issues
errors, or -Wattributes, or silently ignores some cases
altogether, such as those that are the subject of this patch)
and there is no internal mechanism to control the response.
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 37214831538..bc4f409e346 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -720,6 +725,124 @@ positional_argument (const_tree fntype, const_tree
atname, tree pos,
return pos;
}
+/* Given a pair of NODEs for arbitrary DECLs or TYPEs, validate one or
+ two integral or string attribute arguments NEWARGS to be applied to
+ NODE[0] for the absence of conflicts with the same attribute arguments
+ already applied to NODE[1]. Issue a warning for conflicts and return
+ false. Otherwise, when no conflicts are found, return true. */
+
+static bool
+validate_attr_args (tree node[2], tree name, tree newargs[2])
I think you're doing too much work in one function. Also, I *really*
dislike sending pairs of objects in arrays, especially when they're
called something so abstract as "node" and "newargs".
Would it be possible to make this function only validate one single
argument and call it twice? Or do we gain something by having it do two
things at once?
+{
+ /* First validate the arguments against those already applied to
+ the same declaration (or type). */
+ tree self[2] = { node[0], node[0] };
+ if (node[0] != node[1] && !validate_attr_args (self, name, newargs))
+ return false;
+
+ if (!node[1])
+ return true;
+
+ /* Extract the same attribute from the previous declaration or type. */
+ tree prevattr = NULL_TREE;
+ if (DECL_P (node[1]))
+ {
+ prevattr = DECL_ATTRIBUTES (node[1]);
+ if (!prevattr)
+ {
+ tree type = TREE_TYPE (node[1]);
+ prevattr = TYPE_ATTRIBUTES (type);
+ }
+ }
+ else if (TYPE_P (node[1]))
+ prevattr = TYPE_ATTRIBUTES (node[1]);
+
+ const char* const namestr = IDENTIFIER_POINTER (name);
+ prevattr = lookup_attribute (namestr, prevattr);
+ if (!prevattr)
+ return true;
+
+ /* Extract one or both attribute arguments. */
+ tree prevargs[2];
+ prevargs[0] = TREE_VALUE (TREE_VALUE (prevattr));
+ prevargs[1] = TREE_CHAIN (TREE_VALUE (prevattr));
+ if (prevargs[1])
+ prevargs[1] = TREE_VALUE (prevargs[1]);
+
+ /* Both arguments must be equal or, for the second pair, neither must
+ be provided to succeed. */
+ bool arg1eq, arg2eq;
+ if (TREE_CODE (newargs[0]) == INTEGER_CST)
+ {
+ arg1eq = tree_int_cst_equal (newargs[0], prevargs[0]);
+ if (newargs[1] && prevargs[1])
+ arg2eq = tree_int_cst_equal (newargs[1], prevargs[1]);
+ else
+ arg2eq = newargs[1] == prevargs[1];
+ }
+ else if (TREE_CODE (newargs[0]) == STRING_CST)
+ {
+ const char *s0 = TREE_STRING_POINTER (newargs[0]);
+ const char *s1 = TREE_STRING_POINTER (prevargs[0]);
+ arg1eq = strcmp (s0, s1) == 0;
+ if (newargs[1] && prevargs[1])
+ {
+ s0 = TREE_STRING_POINTER (newargs[1]);
+ s1 = TREE_STRING_POINTER (prevargs[1]);
+ arg2eq = strcmp (s0, s1) == 0;
+ }
+ else
+ arg2eq = newargs[1] == prevargs[1];
+ }
+ else
+ gcc_unreachable ();
+
+ if (arg1eq && arg2eq)
+ return true;
+
+ /* If the two locations are different print a note pointing to
+ the previous one. */
+ const location_t curloc = input_location;
+ const location_t prevloc =
+ DECL_P (node[1]) ? DECL_SOURCE_LOCATION (node[1]) : curloc;
+
+ /* Format the attribute specification for convenience. */
+ char newspec[80], prevspec[80];
+ if (newargs[1])
+ snprintf (newspec, sizeof newspec, "%s (%s, %s)", namestr,
+ print_generic_expr_to_str (newargs[0]),
+ print_generic_expr_to_str (newargs[1]));
+ else
+ snprintf (newspec, sizeof newspec, "%s (%s)", namestr,
+ print_generic_expr_to_str (newargs[0]));
+
+ if (prevargs[1])
+ snprintf (prevspec, sizeof prevspec, "%s (%s, %s)", namestr,
+ print_generic_expr_to_str (prevargs[0]),
+ print_generic_expr_to_str (prevargs[1]));
+ else
+ snprintf (prevspec, sizeof prevspec, "%s (%s)", namestr,
+ print_generic_expr_to_str (prevargs[0]));
+
+ if (warning_at (curloc, OPT_Wattributes,
+ "ignoring attribute %qs because it conflicts "
+ "with previous %qs",
+ newspec, prevspec)
+ && curloc != prevloc)
+ inform (prevloc, "previous declaration here");
+
+ return false;
+}
+
+/* Convenience wrapper for validate_attr_args to validate a single
+ attribute argument. */
+
+static bool
+validate_attr_arg (tree node[2], tree name, tree newarg)
This looks like an entry point to your code from the uses below.
Perhaps you should comment it better. But as I said, maybe we should
only have one version of these-- that handles one attribute argument--
if you agree.
+{
+ tree argarray[2] = { newarg, NULL_TREE };
+ return validate_attr_args (node, name, argarray);
+}
/* Attribute handlers common to C front ends. */
@@ -1894,6 +2017,7 @@ handle_section_attribute (tree *node, tree ARG_UNUSED
(name), tree args,
{
tree decl = *node;
tree res = NULL_TREE;
+ tree argval = TREE_VALUE (args);
if (!targetm_common.have_named_sections)
{
@@ -1908,7 +2032,7 @@ handle_section_attribute (tree *node, tree ARG_UNUSED
(name), tree args,
goto fail;
}
- if (TREE_CODE (TREE_VALUE (args)) != STRING_CST)
+ if (TREE_CODE (argval) != STRING_CST)
{
error ("section attribute argument not a string constant");
goto fail;
@@ -1927,7 +2051,7 @@ handle_section_attribute (tree *node, tree ARG_UNUSED
(name), tree args,
from a previous declaration. Ensure they match. */
if (DECL_SECTION_NAME (decl) != NULL
&& strcmp (DECL_SECTION_NAME (decl),
- TREE_STRING_POINTER (TREE_VALUE (args))) != 0)
+ TREE_STRING_POINTER (argval)) != 0)
{
error ("section of %q+D conflicts with previous declaration", *node);
goto fail;
@@ -1941,6 +2065,9 @@ handle_section_attribute (tree *node, tree ARG_UNUSED
(name), tree args,
goto fail;
}
I appreciate the cleanups to the cryptic TREE_CODE(TREE_VALUE(args)))
idiom, but perhaps we should name the argval variable something more
readable. From the manual it looks like that's the section name.
Perhaps section_name? or section?
+ if (!validate_attr_arg (node, name, argval))
+ goto fail;
+
"name" is defined with ARG_UNUSED in the function. If you are adding a
use, you should remove the ARG_UNUSED.
res = targetm.handle_generic_attribute (node, name, args, flags,
no_add_attrs);
@@ -1948,7 +2075,7 @@ handle_section_attribute (tree *node, tree ARG_UNUSED (name), tree args,
final processing. */
if (!(*no_add_attrs))
{
- set_decl_section_name (decl, TREE_STRING_POINTER (TREE_VALUE (args)));
+ set_decl_section_name (decl, TREE_STRING_POINTER (argval));
return res;
}
@@ -2905,15 +3033,15 @@ handle_malloc_attribute (tree *node, tree name, tree ARG_UNUSED (args),
return NULL_TREE;
}
-/* Handle a "alloc_size" attribute; arguments as in
- struct attribute_spec.handler. */
+/* Handle the "alloc_size (argpos1 [, argpos2])" function type attribute.
+ *NODE is the type of the function the attribute is being applied to. */
static tree
handle_alloc_size_attribute (tree *node, tree name, tree args,
int ARG_UNUSED (flags), bool *no_add_attrs)
{
- tree decl = *node;
- tree rettype = TREE_TYPE (decl);
+ tree fntype = *node;
+ tree rettype = TREE_TYPE (fntype);
if (!POINTER_TYPE_P (rettype))
{
warning (OPT_Wattributes,
@@ -2923,6 +3051,7 @@ handle_alloc_size_attribute (tree *node, tree name, tree
args,
return NULL_TREE;
}
+ tree newargs[2] = { NULL_TREE, NULL_TREE };
for (int i = 1; args; ++i)
{
tree pos = TREE_VALUE (args);
@@ -2931,30 +3060,36 @@ handle_alloc_size_attribute (tree *node, tree name,
tree args,
the argument number in diagnostics (since there's just one
mentioning it is unnecessary and coule be confusing). */
tree next = TREE_CHAIN (args);
- if (tree val = positional_argument (decl, name, pos, INTEGER_TYPE,
+ if (tree val = positional_argument (fntype, name, pos, INTEGER_TYPE,
next || i > 1 ? i : 0))
- TREE_VALUE (args) = val;
+ {
+ TREE_VALUE (args) = val;
+ newargs[i - 1] = val;
+ }
else
{
*no_add_attrs = true;
- break;
+ return NULL_TREE;
}
args = next;
}
+ if (!validate_attr_args (node, name, newargs))
+ *no_add_attrs = true;
+
return NULL_TREE;
}
-/* Handle a "alloc_align" attribute; arguments as in
- struct attribute_spec.handler. */
+
+/* Handle an "alloc_align (argpos)" attribute. */
static tree
handle_alloc_align_attribute (tree *node, tree name, tree args, int,
bool *no_add_attrs)
{
- tree decl = *node;
- tree rettype = TREE_TYPE (decl);
+ tree fntype = *node;
+ tree rettype = TREE_TYPE (fntype);
if (!POINTER_TYPE_P (rettype))
{
warning (OPT_Wattributes,
@@ -2964,9 +3099,12 @@ handle_alloc_align_attribute (tree *node, tree name,
tree args, int,
return NULL_TREE;
}
- if (!positional_argument (*node, name, TREE_VALUE (args), INTEGER_TYPE))
- *no_add_attrs = true;
+ if (tree val = positional_argument (*node, name, TREE_VALUE (args),
+ INTEGER_TYPE))
+ if (validate_attr_arg (node, name, val))
+ return NULL_TREE;
+ *no_add_attrs = true;
return NULL_TREE;
}
diff --git a/gcc/testsuite/gcc.dg/attr-alloc_align-5.c b/gcc/testsuite/gcc.dg/attr-alloc_align-5.c
new file mode 100644
index 00000000000..d6f2bc19da8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/attr-alloc_align-5.c
@@ -0,0 +1,23 @@
+/* PR c/78666 - conflicting attribute alloc_size accepted
+ { dg-do compile }
+ { dg-options "-Wall" } */
+
+#define A(pos) __attribute__ ((alloc_align (pos)))
+
+A (1) char* f2_1 (int, int);
+A (1) char* f2_1 (int, int); // { dg-message "previous declaration
here" }
+
+A (2) char* f2_1 (int, int); // { dg-warning "ignoring attribute
'alloc_align \\\(2\\\)' because it conflicts with previous 'alloc_align \\\(1\\\)'" }
This is a personal preference, so ignore it if you disagree, but I find
tests that #define common idioms hard to read. I know it makes the
patch shorter, but looking at:
A (1) char* f2_1();
...it's almost impossible to know what's going on without hunting the
macro. I also find such tests difficult to debug when I cut and paste
them into a source file to debug a failure I caused :).
+
+
+A (2) char* f2_2 (int, int);
+A (2) char* f2_2 (int, int); // { dg-message "previous declaration
here" }
+
+A (1) char* f2_2 (int, int); // { dg-warning "ignoring attribute
'alloc_align \\\(1\\\)' because it conflicts with previous 'alloc_align \\\(2\\\)'" }
+
+
+A (1) char* f3_1 (int, int, int);
+A (1) char* f3_1 (int, int, int); // { dg-message "previous declaration
here" }
+
+A (2) char* f3_1 (int, int, int); // { dg-warning "ignoring attribute
'alloc_align \\\(2\\\)' because it conflicts with previous 'alloc_align \\\(1\\\)'" }
+A (3) char* f3_1 (int, int, int); // { dg-warning "ignoring attribute
'alloc_align \\\(3\\\)' because it conflicts with previous 'alloc_align \\\(1\\\)'" }
diff --git a/gcc/testsuite/gcc.dg/attr-alloc_size-13.c
b/gcc/testsuite/gcc.dg/attr-alloc_size-13.c
new file mode 100644
index 00000000000..df44f479e07
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/attr-alloc_size-13.c
@@ -0,0 +1,34 @@
+/* PR c/78666 - conflicting attribute alloc_size accepted
+ { dg-do compile }
+ { dg-options "-Wall" } */
+
+#define A(...) __attribute__ ((alloc_size (__VA_ARGS__)))
+
+A (1) char* f2_1 (int, int);
+A (1) A (1) char* f2_1 (int, int);
+
+A (1) char* f2_1 (int, int); // { dg-message "previous declaration
here" }
+
+A (2) char* f2_1 (int, int); // { dg-warning "ignoring attribute
'alloc_size \\\(2\\\)' because it conflicts with previous 'alloc_size \\\(1\\\)'" }
+
+
+A (2) char* f2_2 (int, int);
+A (2) char* f2_2 (int, int); // { dg-message "previous declaration
here" }
+
+A (1) char* f2_2 (int, int); // { dg-warning "ignoring attribute
'alloc_size \\\(1\\\)' because it conflicts with previous 'alloc_size \\\(2\\\)'" }
+
+
+A (1) char* f3_1 (int, int, int);
+A (1) char* f3_1 (int, int, int); // { dg-message "previous declaration
here" }
+
+A (2) char* f3_1 (int, int, int); // { dg-warning "ignoring attribute
'alloc_size \\\(2\\\)' because it conflicts with previous 'alloc_size \\\(1\\\)'" }
+A (3) char* f3_1 (int, int, int); // { dg-warning "ignoring attribute
'alloc_size \\\(3\\\)' because it conflicts with previous 'alloc_size \\\(1\\\)'" }
+A (1, 2) char* f3_1 (int, int, int); // { dg-warning "ignoring attribute
'alloc_size \\\(1, 2\\\)' because it conflicts with previous 'alloc_size \\\(1\\\)'"
}
+A (1, 3) char* f3_1 (int, int, int); // { dg-warning "ignoring attribute
'alloc_size \\\(1, 3\\\)' because it conflicts with previous 'alloc_size \\\(1\\\)'"
}
+
+
+typedef A (2, 3) char* F3_2_3 (int, int, int);
+typedef A (2, 3) char* F3_2_3 (int, int, int);
+typedef A (2, 3) A (2, 3) char* F3_2_3 (int, int, int);
+
+typedef A (1) char* F3_2_3 (int, int, int); // { dg-warning "ignoring attribute 'alloc_size
Similarly here. At this point "typedef A (2, 3) char* F3_2_3 (int,
int, int);" looks like gibberish.
Maybe it would be more readable if you spelled it out, and called the
function something sensical, like foo :)
__attribute__((alloc_size (2, 3)) char *foo (int, int, int);
Also, is the duplicate F3_2_3 typedef definition testing something? Why
are there two exact ones?
I know we don't have explicit requirements for readability of tests.
For example, we have obfuscated code in our testsuite, but I find
readable tests a plus. Again, my opinion.
Aldy
\\\(1\\\)' because it conflicts with previous 'alloc_size \\\(2, 3\\\)'" }
diff --git a/gcc/testsuite/gcc.dg/attr-section.c
b/gcc/testsuite/gcc.dg/attr-section.c
new file mode 100644
index 00000000000..0062b544c71
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/attr-section.c
@@ -0,0 +1,13 @@
+/* PR c/96126 - conflicting attribute section accepted on redeclaration
+ { dg-do compile }
+ { dg-options "-Wall" }
+ { dg-require-named-sections "" } */
+
+__attribute__ ((section ("s1"))) void f1 (void);
+__attribute__ ((section ("s2"))) void f1 (void); // { dg-warning "ignoring attribute 'section
\\\(\"s2\"\\\)' because it conflicts with previous 'section \\\(\"s1\"\\\)'" }
+
+__attribute__ ((section ("s3"), section ("s4")))
+void f2 (void); // { dg-error "conflicts" }
+
+__attribute__ ((section ("s5"))) __attribute ((section ("s6")))
+void f3 (void); // { dg-error "conflicts" }
diff --git a/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c
b/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c
index 2a59501b9f3..5736bab9443 100644
--- a/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c
+++ b/gcc/testsuite/c-c++-common/builtin-has-attribute-3.c
@@ -75,7 +75,7 @@ void test_alloc_align (void)
A (0, fnone, alloc_align (1)); /* { dg-warning "\\\[-Wattributes" } */
A (0, falloc_size_1, alloc_align (1));
A (1, falloc_align_1, alloc_align (1));
- A (0, falloc_align_2, alloc_align (1));
+ A (0, falloc_align_2, alloc_align (1)); /* { dg-bogus "\\\[-Wattributes"
"pr?????" { xfail *-*-* } }" */
A (1, falloc_align_2, alloc_align (2));
}
@@ -88,26 +88,26 @@ void test_alloc_size_malloc (void)
A (0, falloc_align_1, alloc_size (1));
A (0, falloc_align_2, alloc_size (1));
A (1, falloc_size_1, alloc_size (1));
- A (0, falloc_size_1, alloc_size (2));
- A (0, falloc_size_2, alloc_size (1));
+ A (0, falloc_size_1, alloc_size (2)); /* { dg-bogus "\\\[-Wattributes"
"pr?????" { xfail *-*-* } }" */
+ A (0, falloc_size_2, alloc_size (1)); /* { dg-bogus "\\\[-Wattributes"
"pr?????" { xfail *-*-* } }" */
A (1, falloc_size_2, alloc_size (2));
A (1, falloc_size_2_4, alloc_size);
/* It would probably make more sense to have the built-in return
true only when both alloc_size arguments match, not just one
or the other. */
- A (0, falloc_size_2_4, alloc_size (1));
- A (1, falloc_size_2_4, alloc_size (2));
- A (0, falloc_size_2_4, alloc_size (3));
- A (1, falloc_size_2_4, alloc_size (4));
+ A (0, falloc_size_2_4, alloc_size (1)); /* { dg-bogus "\\\[-Wattributes"
"pr?????" { xfail *-*-* } }" */
+ A (1, falloc_size_2_4, alloc_size (2)); /* { dg-bogus "\\\[-Wattributes"
"pr?????" { xfail *-*-* } }" */
+ A (0, falloc_size_2_4, alloc_size (3)); /* { dg-bogus "\\\[-Wattributes"
"pr?????" { xfail *-*-* } }" */
+ A (1, falloc_size_2_4, alloc_size (4)); /* { dg-bogus "\\\[-Wattributes"
"pr?????" { xfail *-*-* } }" */
A (1, falloc_size_2_4, alloc_size (2, 4));
extern ATTR (alloc_size (3))
void* fmalloc_size_3 (int, int, int);
A (1, fmalloc_size_3, alloc_size);
- A (0, fmalloc_size_3, alloc_size (1));
- A (0, fmalloc_size_3, alloc_size (2));
+ A (0, fmalloc_size_3, alloc_size (1)); /* { dg-bogus "\\\[-Wattributes"
"pr?????" { xfail *-*-* } }" */
+ A (0, fmalloc_size_3, alloc_size (2)); /* { dg-bogus "\\\[-Wattributes"
"pr?????" { xfail *-*-* } }" */
A (1, fmalloc_size_3, alloc_size (3));
A (0, fmalloc_size_3, malloc);
