On 8/24/20 4:59 AM, Aldy Hernandez wrote:
On 8/21/20 1:37 AM, Martin Sebor wrote:
On 8/20/20 3:00 PM, Aldy Hernandez wrote:
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
Ah, the rest of the functions are taking a node pointer. Your patch
threw me off because you use a node[2] instead of a node pointer like
the rest of the functions. Perhaps you should keep to the current style
and pass a node *.
It takes tree node[3] and the -Warray-parameter option (being
reviewed) uses the bound to check for out-of-bounds accesses, both
callers and the callee itself. (C, but not C++, has special syntax
for this: tree node[static 3].)
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.
It looks like node[] carries all the information for the current
attribute and arguments, as well the same information for the previous
attribute. Could your validate function just take:
validate_attr_args (tree *node, tree name)
That way you can save passing a pair of arguments, plus you can save
accumulating said arguments in the handle_* functions.
Or is there something I'm missing here that makes this unfeasible?
If the function didn't the newargs array it would have to extract
the argument(s) from node, duplicating the work already done in
the callers. I.e., figuring out how many arguments the attribute
expects (one or two, depending on the specific attribute), and for
handle_alloc_size_attribute, calling positional_argument (or at
a minimum default_conversion) to convert it to the expected value.
So it's feasible but doesn't seem like a good design.
/* 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]);
If all this section does is extract the attribute from a decl, it would
look cleaner if you abstracted out this functionality into its own
function. I'm a big fan of one function to do one thing.
const char* const namestr = IDENTIFIER_POINTER (name);
prevattr = lookup_attribute (namestr, prevattr);
if (!prevattr)
return true;
Perhaps a better name would be attribute_name_str?
Thanks for the suggestion but I think NAMESTR is good enough: it
should be clear enough from the function argument NAME that it
refers to the string representation of the NAME. There also is
already a pre-existing use of NAMESTR elsewhere and so a precedent
for this choice.
/* 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]);
Does this only work with 2 arguments? What if the attribute takes 3 or
4 arguments? We should make this generic enough to handle any number of
arguments.
It only works with two arguments because one- and two-argument
attribute handlers are the only callers it's suitable for today.
If/when there is a use case for validating more than two arguments
in a general way the function will need to be extended.
At the moment, there are only a handful of attributes that take
three or more arguments: access, format, nonnull and optimize.
Most do their own extensive validation and the simple checking
done by this function isn't really suitable to them, in part
because they can be meaningfully applied to add attributes to
other arguments between one declaration and another. I'm not
sure that generalizing validate_attr_args to try to also validate
these complex attributes would be an improvement over the status
quo. Even if it could be done, it's more ambitious of a project
than what this simple change is trying to do.
/* 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];
}
It looks like you're re-inventing operand_equal_p.
operand_equal_p does a lot more than just compare integer and string
constants for equality, so it has lots more overhead. That said, I'm
very much in favor of code reuse even if comes with a (small) cost so
I tried this:
/* Both arguments must be equal or, for the second pair, neither must
be provided to succeed. */
bool arg1eq = newargs[0] == prevargs[0];
if (!arg1eq)
arg1eq = operand_equal_p (newargs[0], prevargs[0]);
bool arg2eq = newargs[1] == prevargs[1];
if (!arg2eq && newargs[1] && prevargs[1])
arg2eq = operand_equal_p (newargs[1], prevargs[1]);
if (arg1eq && arg2eq)
return true;
Some testing revealed that the code has different semantics for
strings: it compares them including all nuls embedded in them,
while I think the right semantics for attributes is to only consider
strings up and including the first nul (i.e., apply the usual string
semantics). A test case for this corner case is as follows:
__attribute__ ((section ("foo\0bar"))) void f (void);
__attribute__ ((section ("foo"))) void f (void) { }
Without operand_equal_p() GCC accepts this and puts f in section
foo. With the operand_equal_p() change above, it complains:
ignoring attribute ‘section ("foo\x00bar")’ because it conflicts with
previous ‘section ("foor")’ [-Wattributes]
I would rather not change the behavior in this corner case just to
save a few lines of code. If it's thought that string arguments
to attributes (some or all) should be interpreted differently than
in other contexts it seems that such a change should be made
separately and documented in the manual.
/* 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");
I think it would look cleaner if you separated the analysis from the
diagnosing. So perhaps one function that checks if the attribute is
valid (returns true or false), and one function to display all these
warnings.
It's common practice in GCC to do both in the same function:
validate operands and issue warnings for warnings for problems.
In fact, the vast majority of attribute handlers do three things
in the same function: parse, validate, and apply attributes. It
can be helpful to split up big functions the way you suggest but
I think this one is fine the way it is.
Speak of which, I don't know what the consensus was, but if we
absolutely know that a re-declaration of an attribute for a decl is
invalid, I think it should be a hard error.
Is there any case where having conflicting section attributes for a decl
shouldn't merit an error?
I can't think of one. The manual seems clear that an error should
be expected "for incorrect use of supported attributes." Despite
that, there is no consistency in how these things are handled.
Some attribute handlers issue -Wattributes, others give hard errors,
some one or the other under different conditions.
My opinion is that there should be at least two distinct kinds of
diagnostics for attributes, perhaps three: one for the use of
unknown attributes, another for invalid values of arguments to
known attributes, and may also one for invalid syntax (forms,
types, or numbers of arguments) of known attributes. At some
point I'd like to implement and propose this approach but I don't
think this change is the right time for it.
Attached is another revision of this patch with the new helper
function you suggested. Is this version okay to commit?
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..b5d913647e1 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -720,6 +720,134 @@ positional_argument (const_tree fntype, const_tree atname, tree pos,
return pos;
}
+/* Return the first of DECL or TYPE attributes installed in NODE if it's
+ a DECL, or TYPE attributes if it's a TYPE, or null otherwise. */
+
+static tree
+decl_or_type_attrs (tree node)
+{
+ if (DECL_P (node))
+ {
+ if (tree attrs = DECL_ATTRIBUTES (node))
+ return attrs;
+
+ tree type = TREE_TYPE (node);
+ return TYPE_ATTRIBUTES (type);
+ }
+
+ if (TYPE_P (node))
+ return TYPE_ATTRIBUTES (node);
+
+ return NULL_TREE;
+}
+
+/* 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 = decl_or_type_attrs (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 +2017,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 +2038,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 +2053,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 +2073,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 +2083,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 +3040,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 +3058,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 +3067,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 +3106,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" }
