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
