Re: [PATCH] propagate attributes to local redeclaration (PR 99420)
On Thu, 8 Apr 2021, Martin Sebor via Gcc-patches wrote:
> There's another similar piece of code in pushdecl() that I didn't
> touch, although I couldn't come up with a test case showing it's
> necessary. Both hunks go back ages so I wonder if they might have
> been obviated by other improvements.
The other similar code in pushdecl is executed in cases where there are
multiple declarations of the same identifier in the same scope, e.g.:
int f (void);
void
g (void)
{
int f (void);
int f (void);
}
That particular example isn't interesting, but the idea is that the type
the declaration ends up getting is based on only visible type information
(if an intermediate scope had a variable "int f;" with automatic scope,
for example, the file-scope declaration wouldn't be visible, so the type
within the scope of the inner declarations should be the composite only of
the types of those declarations and not that of the file-scope
declaration).
I expect that the attribute handling currently there for built-in
functions only is because there were problems in some cases if a built-in
function were referenced without its built-in attributes. As the
attributes don't affect the function type in standard terms, it's
certainly OK, and improves diagnostic quality, to include attributes from
declarations that aren't visible.
It's possible that the piece of code you're changing always ensures that
the attributes are copied from the built-in function to the first
declaration in the inner scope (even when any file scope / external scope
declaration is shadowed), and, if composite_type and duplicate_decls
always preserve attributes, this might mean the code you're not changing
doesn't actually need its attribute handling because the attributes are
always present (for all functions, given your patch, or for built-in
functions, without it) even without that handling. So if something
changed that did make that code unnecessary, it might have been a fix in
composite_type or duplicate_decls.
The patch is OK.
--
Joseph S. Myers
jos...@codesourcery.com
Re: [PATCH] propagate attributes to local redeclaration (PR 99420)
On 4/13/21 10:07 AM, Jeff Law wrote: On 4/8/2021 4:15 PM, Martin Sebor via Gcc-patches wrote: The C front end ordinarily merges function type attributes upon the redeclaration of a function but it doesn't do that for those at local scope, unless the declaration refers to a built-in. Because the new -Warray-parameter warning relies on the internal access attribute on the type of the function added by the C front end for parameters declared using the array notation, it triggers when it sees a redeclaration of a function in a local scope even when both declarations use the same array form, issuing a false positive. The same problem affects other similar redeclarations involving attributes, such as unused_result, causing false negatives there. (Clang and ICC behave as I expect.) The attached patch lets the front end propagate the type attributes for all redeclarations, resolving this class of problems for all affected attributes. There's another similar piece of code in pushdecl() that I didn't touch, although I couldn't come up with a test case showing it's necessary. Both hunks go back ages so I wonder if they might have been obviated by other improvements. What I'd want to know is why the code didn't copy the attributes to the redeclarations. Did the git history provide any hints? Does the hunk of code post-date creation of the public lists? If so, was there any discussion in the public lists of the change that might give insights? The conditional the patch removes was added by Joseph in r86636 to fix PR c/13801. The bug deals with extern declarations in nested scopes. This issue is about declarations in unrelated scopes. The (very long) discussion of the changes is below but I couldn't find anything relevant to the problem(s) my change tries to solve: https://gcc.gnu.org/legacy-ml/gcc-patches/2004-08/msg00085.html FWIW, with or without my patch, GCC still isn't entirely consistent in how it handles attributes on extern redeclarations in different scopes, either with itself or with other compilers. Martin
Re: [PATCH] propagate attributes to local redeclaration (PR 99420)
On 4/8/2021 4:15 PM, Martin Sebor via Gcc-patches wrote: The C front end ordinarily merges function type attributes upon the redeclaration of a function but it doesn't do that for those at local scope, unless the declaration refers to a built-in. Because the new -Warray-parameter warning relies on the internal access attribute on the type of the function added by the C front end for parameters declared using the array notation, it triggers when it sees a redeclaration of a function in a local scope even when both declarations use the same array form, issuing a false positive. The same problem affects other similar redeclarations involving attributes, such as unused_result, causing false negatives there. (Clang and ICC behave as I expect.) The attached patch lets the front end propagate the type attributes for all redeclarations, resolving this class of problems for all affected attributes. There's another similar piece of code in pushdecl() that I didn't touch, although I couldn't come up with a test case showing it's necessary. Both hunks go back ages so I wonder if they might have been obviated by other improvements. What I'd want to know is why the code didn't copy the attributes to the redeclarations. Did the git history provide any hints? Does the hunk of code post-date creation of the public lists? If so, was there any discussion in the public lists of the change that might give insights? Jeff
[PATCH] propagate attributes to local redeclaration (PR 99420)
The C front end ordinarily merges function type attributes upon
the redeclaration of a function but it doesn't do that for those
at local scope, unless the declaration refers to a built-in.
Because the new -Warray-parameter warning relies on the internal
access attribute on the type of the function added by the C front
end for parameters declared using the array notation, it triggers
when it sees a redeclaration of a function in a local scope even
when both declarations use the same array form, issuing a false
positive.
The same problem affects other similar redeclarations involving
attributes, such as unused_result, causing false negatives there.
(Clang and ICC behave as I expect.)
The attached patch lets the front end propagate the type attributes
for all redeclarations, resolving this class of problems for all
affected attributes.
There's another similar piece of code in pushdecl() that I didn't
touch, although I couldn't come up with a test case showing it's
necessary. Both hunks go back ages so I wonder if they might have
been obviated by other improvements.
Martin
PR c/99420 - bogus -Warray-parameter on a function redeclaration in function scope
PR c/99972 - missing -Wunused-result on a call to a locally redeclared warn_unused_result function
gcc/c/ChangeLog:
PR c/99420
PR c/99972
* c-decl.c (pushdecl): Always propagate type attribute.
gcc/testsuite/ChangeLog:
PR c/99420
PR c/99972
* gcc.dg/Warray-parameter-9.c: New test.
* gcc.dg/Wnonnull-6.c: New test.
* gcc.dg/Wreturn-type3.c: New test.
* gcc.dg/Wunused-result.c: New test.
* gcc.dg/attr-noreturn.c: New test.
* gcc.dg/attr-returns-nonnull.c: New test.
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index 3b2241bfd97..677bfd73a50 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -3263,11 +3263,10 @@ pushdecl (tree x)
else
thistype = type;
b->u.type = TREE_TYPE (b->decl);
- if (TREE_CODE (b->decl) == FUNCTION_DECL
- && fndecl_built_in_p (b->decl))
- thistype
- = build_type_attribute_variant (thistype,
- TYPE_ATTRIBUTES (b->u.type));
+ /* Propagate the type attributes to the decl. */
+ thistype
+ = build_type_attribute_variant (thistype,
+ TYPE_ATTRIBUTES (b->u.type));
TREE_TYPE (b->decl) = thistype;
bind (name, b->decl, scope, /*invisible=*/false, /*nested=*/true,
locus);
diff --git a/gcc/testsuite/gcc.dg/Warray-parameter-9.c b/gcc/testsuite/gcc.dg/Warray-parameter-9.c
new file mode 100644
index 000..b5d3d963c88
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Warray-parameter-9.c
@@ -0,0 +1,54 @@
+/* PR c/99420 - bogus -Warray-parameter on a function redeclaration
+ in function scope
+ { dg-do compile }
+ { dg-options "-Wall" } */
+
+extern int a1[1], a2[2], a3[3], a4[4];
+
+void fa1 (int [1]); // { dg-message "previously declared as 'int\\\[1]'" }
+void fa1 (int [1]);
+
+
+void nested_decl (void)
+{
+ void fa2 (int [2]);
+
+ fa2 (a1); // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+ fa2 (a2);
+ fa2 (a3);
+
+ void fa3 (int [3]);
+
+ fa3 (a2); // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+ fa3 (a3);
+}
+
+
+void nested_redecl (void)
+{
+ void fa1 (int [2]); // { dg-warning "argument 1 of type 'int\\\[2]' with mismatched bound" }
+
+ fa1 (a1 + 1); // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+ fa1 (a1);
+
+ void fa2 (int [2]); // { dg-bogus "\\\[-Warray-parameter" }
+
+ fa2 (a1); // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+ fa2 (a2);
+ fa2 (a3);
+
+ void fa3 (int [3]); // { dg-bogus "\\\[-Warray-parameter" }
+
+ fa3 (a2); // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+ fa3 (a3);
+
+ void fa4 (int [4]);
+}
+
+void fa4 (int [5]); // { dg-warning "\\\[-Warray-parameter" }
+
+void call_fa4 (void)
+{
+ fa4 (a4);
+ fa4 (a3); // { dg-warning "\\\[-Warray-bounds|-Wstringop-overflow" }
+}
diff --git a/gcc/testsuite/gcc.dg/Wnonnull-6.c b/gcc/testsuite/gcc.dg/Wnonnull-6.c
new file mode 100644
index 000..48f09da996f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wnonnull-6.c
@@ -0,0 +1,93 @@
+/* Verify that attribute nonnull on global and local function declarations
+ or those to pointers to functions is merged.
+ { dg-do compile }
+ { dg-options "-Wall" } */
+
+void fnonnull_local_local (void)
+{
+ extern __attribute__ ((nonnull)) void fnonnull1 (void*);
+
+ fnonnull1 (0);// { dg-warning "\\\[-Wnonnull" }
+}
+
+void gnonnull_local_local (void)
+{
+ extern void fnonnull1 (void*);
+
+ fnonnull1 (0);// { dg-warning "\\\[-Wnonnull" }
+}
+
+
+void fnonnull_local_global (void)
+{
+ extern __attribute__ ((nonnull)) void fnonnull2 (void*);
+
+ fnonnull2 (0);// { dg-warning "\\\[-Wnonnull" }
+}
+
+extern void fnonnull2 (void*);
+
+void gnonnull_local_global (void)
+{
+ fnonnull2 (0);// { dg-warning "\\\[-Wnonnull" }
+}
+
+
+extern __attribute__ ((nonnull)) void fnonnull3 (voi
