On 8/20/20 3:00 PM, Aldy Hernandez wrote:
First, didn't Marek say in the PR that the diagnostic code should go in
diagnose_mismatched_attributes?
My understanding of the structure of the attribute handling code
is that with just a few exceptions, for C and C++ it's pretty much
all in c-attribs.c. That makes sense to me and I would rather
prefer to avoid spreading attribute-specific logic across other
files. diagnose_mismatched_attributes predates the ability to
access the prior declaration of a function (via node[1] in
attribute handlers). It's shrunk considerably since Marek added
it years ago as a result of the attribute exclusion framework.
If it's possible (I haven't checked) it might make sense to move
the attribute optimize validation logic (the only attribute left
it still handles) from it to handle_optimize_attribute which now
has access to the previous declaration via node[1].
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? ;-)
I like the idea of coming up with general properties for attributes
and validating based on those rather than case by case. An example
along these lines is the attribute exclusion framework I put in
place to reject mutually exclusive attributes (e.g., aligned and
packed). Detection of conflicts between attribute arguments is
a natural extension of the same idea.
But to answer your question: extending the attribute handling
infrastructure does tend to be a lot of work because every change
to struct attribute_spec means updating all back ends (each member
of the struct has to be explicitly initialized in each back end's
attribute_table; that's silly because most of the members take on
default values but that's the way things are set up for now). I'd
like to take some time to think about how to generalize the overall
design to make future enhancements like the one you suggest less
intrusive. Until then, I think this is a worthwhile improvement
to make on its own.
Regardless, here are some random comments.
Thanks for the careful review!
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?
I agree about the name "node." The argument comes from the attribute
handlers: they all take something called a node as their first argument.
It's an array of three elements:
[0] the current declaration or type
[1] the previous declaration or type or null
[2] the current declaration if [0] is a type
validate_attr_args() is called with the same node as the handlers
and uses both node[0] and node[1] to recursively validate the first
one against itself and then against the second. It could be changed
to take two arguments instead of an array (the new "node" and
the original "node," perhaps even under some better name). That
would make it different from every handler but maybe that wouldn't
be a problem.
The newargs argument is also an array, with the second element
being optional. Both elements are used and validated against
the attribute arguments on the same declaration first and again
on the previous one. The array could be split up into two
distinct arguments, say newarg1 and newarg2, or passed in as
std::pair<tree, tree>. I'm not sure I see much of a difference
between the approaches.
I can't think of a good way to call the function twice, once for
each attribute argument. It's part of the validation to make sure
that the second (optional) argument is provided either in both
instances of the same attribute and with the same value, or in
neither. In the case of a conflict, the function also needs to
format both arguments. E.g., for the following declarations:
__attribute__ ((alloc_size (1)) char* f (int, int, int);
__attribute__ ((alloc_size (2, 3))) char* f (int, int, int);
it needs to mention both arguments in the warning:
warning: ignoring attribute 'alloc_size (2, 3)' because it conflicts
with previous 'alloc_size (1)'
...
+
+/* 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.
I've changed the comment to:
/* Convenience wrapper for validate_attr_args to validate a single
attribute argument. Used by handlers for attributes that take
just a single argument. */
Let me know if this isn't what you were looking for.
...
@@ -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?
I renamed it to new_section_name and added old_section_name for
clarity.
+ 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.
I've removed ARG_UNUSED from both name and flags since both are
used. FWIW, being a holdover from when the code was C, ARG_UNUSED
can be removed along with the name of the argument where it's unused
unconditionally. But that's a project for another day.
...
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 loo1
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 :).
Agreed that macros can do that.
...
+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 :)
In tests with lots of global names where I can't generate names
using macros I use some form of mangling to avoid name clashes.
Otherwise I quickly run out of foos and bars. I avoid sequential
numbering because I tend to group test cases by what they have in
common and add to the groups as work on each test, so they quickly
tend to get out of order.
__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?
It tests for the absence of warnings.
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.
Then you should probably try not to look at too many of mine, like
this beauty: ;-)
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/testsuite/gcc.dg/Wstringop-overflow-11.c
Thanks for the feedback!
Attached is a revised patch with the changes above.
Martin
Detect conflicts between incompatible uses of the same attribute (PR c/78666).
Resolves:
PR c/78666 - conflicting attribute alloc_size accepted
PR c/96126 - conflicting attribute section accepted on redeclaration
gcc/c-family/ChangeLog:
PR c/78666
PR c/96126
* c-attribs.c (validate_attr_args): New function.
(validate_attr_arg): Same.
(handle_section_attribute): Call it. Introduce a local variable.
(handle_alloc_size_attribute): Same.
(handle_alloc_align_attribute): Same.
gcc/testsuite/ChangeLog:
PR c/78666
PR c/96126
* gcc.dg/attr-alloc_align-5.c: New test.
* gcc.dg/attr-alloc_size-13.c: New test.
* gcc.dg/attr-section.c: New test.
* c-c++-common/builtin-has-attribute-3.c: Add xfails due to expected
warnings to be cleaned up.
diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 37214831538..486ca00c982 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -720,6 +720,125 @@ 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])
+{
+ /* 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. Used by handlers for attributes that take
+ just a single argument. */
+
+static bool
+validate_attr_arg (tree node[2], tree name, tree newarg)
+{
+ tree argarray[2] = { newarg, NULL_TREE };
+ return validate_attr_args (node, name, argarray);
+}
/* Attribute handlers common to C front ends. */
@@ -1889,11 +2008,13 @@ handle_mode_attribute (tree *node, tree name, tree args,
struct attribute_spec.handler. */
static tree
-handle_section_attribute (tree *node, tree ARG_UNUSED (name), tree args,
- int ARG_UNUSED (flags), bool *no_add_attrs)
+handle_section_attribute (tree *node, tree name, tree args,
+ int flags, bool *no_add_attrs)
{
tree decl = *node;
tree res = NULL_TREE;
+ tree argval = TREE_VALUE (args);
+ const char* new_section_name;
if (!targetm_common.have_named_sections)
{
@@ -1908,7 +2029,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;
@@ -1923,15 +2044,17 @@ handle_section_attribute (tree *node, tree ARG_UNUSED (name), tree args,
goto fail;
}
+ new_section_name = TREE_STRING_POINTER (argval);
+
/* The decl may have already been given a section attribute
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)
- {
- error ("section of %q+D conflicts with previous declaration", *node);
- goto fail;
- }
+ if (const char* const old_section_name = DECL_SECTION_NAME (decl))
+ if (strcmp (old_section_name, new_section_name) != 0)
+ {
+ error ("section of %q+D conflicts with previous declaration",
+ *node);
+ goto fail;
+ }
if (VAR_P (decl)
&& !targetm.have_tls && targetm.emutls.tmpl_section
@@ -1941,6 +2064,9 @@ handle_section_attribute (tree *node, tree ARG_UNUSED (name), tree args,
goto fail;
}
+ if (!validate_attr_arg (node, name, argval))
+ goto fail;
+
res = targetm.handle_generic_attribute (node, name, args, flags,
no_add_attrs);
@@ -1948,7 +2074,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, new_section_name);
return res;
}
@@ -2905,15 +3031,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 +3049,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 +3058,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 +3097,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/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);
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\\\)'" }
+
+
+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 \\\(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" }
