* [PATCH] improve validation of attribute arguments (PR c/78666)
@ 2020-07-09 0:01 Martin Sebor
2020-07-16 22:53 ` [PING][PATCH] " Martin Sebor
2020-08-20 21:00 ` [PATCH] " Aldy Hernandez
0 siblings, 2 replies; 12+ messages in thread
From: Martin Sebor @ 2020-07-09 0:01 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1669 bytes --]
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.
[-- Attachment #2: gcc-78666.diff --]
[-- Type: text/x-patch, Size: 15996 bytes --]
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.
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])
+{
+ /* 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)
+{
+ 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;
}
+ if (!validate_attr_arg (node, name, argval))
+ goto fail;
+
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\\\)'" }
+
+
+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" }
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);
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PING][PATCH] improve validation of attribute arguments (PR c/78666)
2020-07-09 0:01 [PATCH] improve validation of attribute arguments (PR c/78666) Martin Sebor
@ 2020-07-16 22:53 ` Martin Sebor
2020-07-26 17:41 ` [PING 2][PATCH] " Martin Sebor
2020-08-20 21:00 ` [PATCH] " Aldy Hernandez
1 sibling, 1 reply; 12+ messages in thread
From: Martin Sebor @ 2020-07-16 22:53 UTC (permalink / raw)
To: gcc-patches, Jeff Law
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-July/549686.html
(Jeff, I forgot to mention this patch when we spoke earlier today.)
On 7/8/20 6:01 PM, Martin Sebor 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PING 2][PATCH] improve validation of attribute arguments (PR c/78666)
2020-07-16 22:53 ` [PING][PATCH] " Martin Sebor
@ 2020-07-26 17:41 ` Martin Sebor
2020-08-10 16:50 ` [PING 3][PATCH] " Martin Sebor
0 siblings, 1 reply; 12+ messages in thread
From: Martin Sebor @ 2020-07-26 17:41 UTC (permalink / raw)
To: gcc-patches, Jeff Law
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-July/549686.html
On 7/16/20 4:53 PM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-July/549686.html
>
> (Jeff, I forgot to mention this patch when we spoke earlier today.)
>
> On 7/8/20 6:01 PM, Martin Sebor 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.
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PING 3][PATCH] improve validation of attribute arguments (PR c/78666)
2020-07-26 17:41 ` [PING 2][PATCH] " Martin Sebor
@ 2020-08-10 16:50 ` Martin Sebor
0 siblings, 0 replies; 12+ messages in thread
From: Martin Sebor @ 2020-08-10 16:50 UTC (permalink / raw)
To: gcc-patches, Jeff Law
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-July/549686.html
On 7/26/20 11:41 AM, Martin Sebor wrote:
> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-July/549686.html
>
> On 7/16/20 4:53 PM, Martin Sebor wrote:
>> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-July/549686.html
>>
>> (Jeff, I forgot to mention this patch when we spoke earlier today.)
>>
>> On 7/8/20 6:01 PM, Martin Sebor 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.
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] improve validation of attribute arguments (PR c/78666)
2020-07-09 0:01 [PATCH] improve validation of attribute arguments (PR c/78666) Martin Sebor
2020-07-16 22:53 ` [PING][PATCH] " Martin Sebor
@ 2020-08-20 21:00 ` Aldy Hernandez
2020-08-20 23:37 ` Martin Sebor
1 sibling, 1 reply; 12+ messages in thread
From: Aldy Hernandez @ 2020-08-20 21:00 UTC (permalink / raw)
To: Martin Sebor, gcc-patches; +Cc: Marek Polacek
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);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] improve validation of attribute arguments (PR c/78666)
2020-08-20 21:00 ` [PATCH] " Aldy Hernandez
@ 2020-08-20 23:37 ` Martin Sebor
2020-08-24 10:59 ` Aldy Hernandez
0 siblings, 1 reply; 12+ messages in thread
From: Martin Sebor @ 2020-08-20 23:37 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches; +Cc: Marek Polacek
[-- Attachment #1: Type: text/plain, Size: 10368 bytes --]
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
[-- Attachment #2: gcc-78666.diff --]
[-- Type: text/x-patch, Size: 16883 bytes --]
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" }
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] improve validation of attribute arguments (PR c/78666)
2020-08-20 23:37 ` Martin Sebor
@ 2020-08-24 10:59 ` Aldy Hernandez
2020-08-24 16:45 ` Martin Sebor
0 siblings, 1 reply; 12+ messages in thread
From: Aldy Hernandez @ 2020-08-24 10:59 UTC (permalink / raw)
To: Martin Sebor, gcc-patches; +Cc: Marek Polacek
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 *.
> 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?
> /* 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?
> /* 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.
> /* 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.
> /* 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.
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'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.
I agree. Using the C++ idiom of nameless argument is cleaner and
catches these sort of things automatically.
>>> +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.
Yeah, but this isn't the case. You only have one name in this test, so
there's no need to make it hard to read.
> 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
Yes, and these sort of tests have been challenging to work with when
debugging the ranger. When you see an error in testsuite for the above
test, you have no idea what went wrong, short of running the
preprocessor on it, and hunting for the error. It makes finding and
fixing regressions harder.
Aldy
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] improve validation of attribute arguments (PR c/78666)
2020-08-24 10:59 ` Aldy Hernandez
@ 2020-08-24 16:45 ` Martin Sebor
2020-08-25 5:51 ` Aldy Hernandez
2020-09-09 21:44 ` [PING][PATCH] " Martin Sebor
0 siblings, 2 replies; 12+ messages in thread
From: Martin Sebor @ 2020-08-24 16:45 UTC (permalink / raw)
To: Aldy Hernandez, gcc-patches; +Cc: Marek Polacek
[-- Attachment #1: Type: text/plain, Size: 12317 bytes --]
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
[-- Attachment #2: gcc-78666.diff --]
[-- Type: text/x-patch, Size: 17096 bytes --]
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" }
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] improve validation of attribute arguments (PR c/78666)
2020-08-24 16:45 ` Martin Sebor
@ 2020-08-25 5:51 ` Aldy Hernandez
2020-09-09 21:44 ` [PING][PATCH] " Martin Sebor
1 sibling, 0 replies; 12+ messages in thread
From: Aldy Hernandez @ 2020-08-25 5:51 UTC (permalink / raw)
To: Martin Sebor, gcc-patches; +Cc: Marek Polacek
> Attached is another revision of this patch with the new helper
> function you suggested. Is this version okay to commit?
I don't actually have the ability to grant check-in in this area. I
just figured that implementing some of these suggestions might make it
more palatable to reviewers who do.
The patch is much better, but IMO it is a bit hard to read and not
generic enough, but I defer to the relevant maintainers.
Aldy
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PING][PATCH] improve validation of attribute arguments (PR c/78666)
2020-08-24 16:45 ` Martin Sebor
2020-08-25 5:51 ` Aldy Hernandez
@ 2020-09-09 21:44 ` Martin Sebor
2020-09-15 23:15 ` Joseph Myers
1 sibling, 1 reply; 12+ messages in thread
From: Martin Sebor @ 2020-09-09 21:44 UTC (permalink / raw)
To: gcc-patches; +Cc: Aldy Hernandez, Marek Polacek
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552500.html
Aldy provided a bunch of comments on this patch but I'm still looking
for a formal approval.
Martin
On 8/24/20 10:45 AM, Martin Sebor wrote:
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PING][PATCH] improve validation of attribute arguments (PR c/78666)
2020-09-09 21:44 ` [PING][PATCH] " Martin Sebor
@ 2020-09-15 23:15 ` Joseph Myers
2020-09-16 20:25 ` Martin Sebor
0 siblings, 1 reply; 12+ messages in thread
From: Joseph Myers @ 2020-09-15 23:15 UTC (permalink / raw)
To: Martin Sebor; +Cc: gcc-patches, Marek Polacek
On Wed, 9 Sep 2020, Martin Sebor via Gcc-patches wrote:
> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552500.html
>
> Aldy provided a bunch of comments on this patch but I'm still looking
> for a formal approval.
This patch is OK.
> > 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.
I think that for at least the section attribute, embedded NULs are
suspect. In that case, so are wide strings, but for some attributes wide
strings should be accepted and handled sensibly (but aren't, see bug
91182).
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PING][PATCH] improve validation of attribute arguments (PR c/78666)
2020-09-15 23:15 ` Joseph Myers
@ 2020-09-16 20:25 ` Martin Sebor
0 siblings, 0 replies; 12+ messages in thread
From: Martin Sebor @ 2020-09-16 20:25 UTC (permalink / raw)
To: Joseph Myers; +Cc: gcc-patches, Marek Polacek
On 9/15/20 5:15 PM, Joseph Myers wrote:
> On Wed, 9 Sep 2020, Martin Sebor via Gcc-patches wrote:
>
>> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552500.html
>>
>> Aldy provided a bunch of comments on this patch but I'm still looking
>> for a formal approval.
>
> This patch is OK.
>
>>> 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.
>
> I think that for at least the section attribute, embedded NULs are
> suspect. In that case, so are wide strings, but for some attributes wide
> strings should be accepted and handled sensibly (but aren't, see bug
> 91182).
I agree. Clang has the same "bug" as GCC would if it used
operand_equal_p().
Besides embedded nuls, other characters are problematic and cause
(sometimes confusing) assembler errors. Examples are the empty
string, embedded whitespace, or mismatched quotes. It would be
nice to do some basic validation and print a nice warning for
the most common problems.
Martin
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-09-16 20:25 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-09 0:01 [PATCH] improve validation of attribute arguments (PR c/78666) Martin Sebor
2020-07-16 22:53 ` [PING][PATCH] " Martin Sebor
2020-07-26 17:41 ` [PING 2][PATCH] " Martin Sebor
2020-08-10 16:50 ` [PING 3][PATCH] " Martin Sebor
2020-08-20 21:00 ` [PATCH] " Aldy Hernandez
2020-08-20 23:37 ` Martin Sebor
2020-08-24 10:59 ` Aldy Hernandez
2020-08-24 16:45 ` Martin Sebor
2020-08-25 5:51 ` Aldy Hernandez
2020-09-09 21:44 ` [PING][PATCH] " Martin Sebor
2020-09-15 23:15 ` Joseph Myers
2020-09-16 20:25 ` Martin Sebor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).