Обсуждение: Debian 12 gcc warning
On Debian 12, gcc version 12.2.0 (Debian 12.2.0-14) generates a warning
on PG 13 to current, but only with -O1 optimization level, and not at
-O0/-O2/-O3:
clauses.c: In function ‘recheck_cast_function_args’:
clauses.c:4293:19: warning: ‘actual_arg_types’ may be used uninitialized [-Wmaybe-uninitialized]
4293 | rettype = enforce_generic_type_consistency(actual_arg_types,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4294 |
declared_arg_types,
|
~~~~~~~~~~~~~~~~~~~
4295 | nargs,
| ~~~~~~
4296 |
funcform->prorettype,
|
~~~~~~~~~~~~~~~~~~~~~
4297 | false);
| ~~~~~~
In file included from clauses.c:45:
../../../../src/include/parser/parse_coerce.h:82:17: note: by argument 1 of type ‘const Oid *’ {aka ‘const unsigned
int*’} to ‘enforce_generic_type_consistency’ declared here
82 | extern Oid enforce_generic_type_consistency(const Oid *actual_arg_types,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
clauses.c:4279:33: note: ‘actual_arg_types’ declared here
4279 | Oid actual_arg_types[FUNC_MAX_ARGS];
| ^~~~~~~~~~~~~~~~
The code is:
static void
recheck_cast_function_args(List *args, Oid result_type,
Oid *proargtypes, int pronargs,
HeapTuple func_tuple)
{
Form_pg_proc funcform = (Form_pg_proc) GETSTRUCT(func_tuple);
int nargs;
Oid actual_arg_types[FUNC_MAX_ARGS];
Oid declared_arg_types[FUNC_MAX_ARGS];
Oid rettype;
ListCell *lc;
if (list_length(args) > FUNC_MAX_ARGS)
elog(ERROR, "too many function arguments");
nargs = 0;
foreach(lc, args)
{
actual_arg_types[nargs++] = exprType((Node *) lfirst(lc));
}
Assert(nargs == pronargs);
memcpy(declared_arg_types, proargtypes, pronargs * sizeof(Oid));
--> rettype = enforce_generic_type_consistency(actual_arg_types,
declared_arg_types,
nargs,
funcform->prorettype,
false);
/* let's just check we got the same answer as the parser did ... */
I don't see a clean way of avoiding the warning except by initializing
the array, which seems wasteful.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
On Mon, Aug 28, 2023 at 03:37:20PM -0400, Bruce Momjian wrote:
> I don't see a clean way of avoiding the warning except by initializing
> the array, which seems wasteful.
Or just initialize the array with a {0}?
--
Michael
Вложения
On Tue, Aug 29, 2023 at 07:30:15AM +0900, Michael Paquier wrote:
> On Mon, Aug 28, 2023 at 03:37:20PM -0400, Bruce Momjian wrote:
> > I don't see a clean way of avoiding the warning except by initializing
> > the array, which seems wasteful.
>
> Or just initialize the array with a {0}?
Uh, doesn't that set all elements to zero? See:
https://stackoverflow.com/questions/2589749/how-to-initialize-array-to-0-in-c
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
On Tue, 29 Aug 2023 at 07:37, Bruce Momjian <bruce@momjian.us> wrote:
> nargs = 0;
> foreach(lc, args)
> {
> actual_arg_types[nargs++] = exprType((Node *) lfirst(lc));
> }
Does it still produce the warning if you form the above more like?
nargs = list_length(args);
for (int i = 0; i < nargs; i++)
actual_arg_types[i] = exprType((Node *) list_nth(args, i));
I'm just not sure if it's unable to figure out if at least nargs
elements is set or if it won't be happy until all 100 elements are
set.
David
On Tue, Aug 29, 2023 at 11:55:48AM +1200, David Rowley wrote:
> On Tue, 29 Aug 2023 at 07:37, Bruce Momjian <bruce@momjian.us> wrote:
> > nargs = 0;
> > foreach(lc, args)
> > {
> > actual_arg_types[nargs++] = exprType((Node *) lfirst(lc));
> > }
>
> Does it still produce the warning if you form the above more like?
>
> nargs = list_length(args);
> for (int i = 0; i < nargs; i++)
> actual_arg_types[i] = exprType((Node *) list_nth(args, i));
>
> I'm just not sure if it's unable to figure out if at least nargs
> elements is set or if it won't be happy until all 100 elements are
> set.
I applied the attached patch but got the same warning:
clauses.c: In function ‘recheck_cast_function_args’:
clauses.c:4297:19: warning: ‘actual_arg_types’ may be used uninitialized [-Wmaybe-uninitialized]
4297 | rettype = enforce_generic_type_consistency(actual_arg_types,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4298 |
declared_arg_types,
|
~~~~~~~~~~~~~~~~~~~
4299 | nargs,
| ~~~~~~
4300 |
funcform->prorettype,
|
~~~~~~~~~~~~~~~~~~~~~
4301 | false);
| ~~~~~~
In file included from clauses.c:45:
../../../../src/include/parser/parse_coerce.h:82:17: note: by argument 1 of type ‘const Oid *’ {aka ‘const unsigned
int*’} to ‘enforce_generic_type_consistency’ declared here
82 | extern Oid enforce_generic_type_consistency(const Oid *actual_arg_types,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
clauses.c:4279:33: note: ‘actual_arg_types’ declared here
4279 | Oid actual_arg_types[FUNC_MAX_ARGS];
| ^~~~~~~~~~~~~~~~
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
Вложения
On Mon, Aug 28, 2023 at 07:10:38PM -0400, Bruce Momjian wrote:
> On Tue, Aug 29, 2023 at 07:30:15AM +0900, Michael Paquier wrote:
> > On Mon, Aug 28, 2023 at 03:37:20PM -0400, Bruce Momjian wrote:
> > > I don't see a clean way of avoiding the warning except by initializing
> > > the array, which seems wasteful.
> >
> > Or just initialize the array with a {0}?
>
> Uh, doesn't that set all elements to zero? See:
>
> https://stackoverflow.com/questions/2589749/how-to-initialize-array-to-0-in-c
FYI, that does stop the warning.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
On Tue, Aug 29, 2023 at 6:56 AM David Rowley <dgrowleyml@gmail.com> wrote:
>
> I'm just not sure if it's unable to figure out if at least nargs
> elements is set or if it won't be happy until all 100 elements are
> set.
It looks like the former, since I can silence it on gcc 13 / -O1 by doing:
/* keep compiler quiet */
actual_arg_types[0] = InvalidOid;
--
John Naylor
EDB: http://www.enterprisedb.com
actual_arg_types[0] = InvalidOid;
--
John Naylor
EDB: http://www.enterprisedb.com
On Mon Aug 28, 2023 at 2:37 PM CDT, Bruce Momjian wrote: > I don't see a clean way of avoiding the warning except by initializing > the array, which seems wasteful. For what it's worth, we recently committed a patch[0] that initialized an array due to a similar warning being generated on Fedora 38 (gcc (GCC) 13.2.1 20230728 (Red Hat 13.2.1-1)). [0]: https://github.com/postgres/postgres/commit/4a8fef0d733965c1a1836022f8a42ab1e83a721f -- Tristan Partin Neon (https://neon.tech)
On Tue, Aug 29, 2023 at 10:26:27AM +0700, John Naylor wrote: > > On Tue, Aug 29, 2023 at 6:56 AM David Rowley <dgrowleyml@gmail.com> wrote: > > > > I'm just not sure if it's unable to figure out if at least nargs > > elements is set or if it won't be happy until all 100 elements are > > set. > > It looks like the former, since I can silence it on gcc 13 / -O1 by doing: > > /* keep compiler quiet */ > actual_arg_types[0] = InvalidOid; Agreed, that fixes it for me too. In fact, assigning to only element 99 or 200 also prevents the warning, and considering the array is defined for 100 elements, the fact is accepts 200 isn't a good thing. Patch attached. I think the question is whether we add this to silence a common compiler but non-default optimization level. It is the only such case in our source code right now. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Вложения
Bruce Momjian <bruce@momjian.us> writes:
> On Tue, Aug 29, 2023 at 10:26:27AM +0700, John Naylor wrote:
>> It looks like the former, since I can silence it on gcc 13 / -O1 by doing:
>> /* keep compiler quiet */
>> actual_arg_types[0] = InvalidOid;
> Agreed, that fixes it for me too. In fact, assigning to only element 99 or
> 200 also prevents the warning, and considering the array is defined for
> 100 elements, the fact is accepts 200 isn't a good thing. Patch attached.
That seems like a pretty clear compiler bug, particularly since it just
appears in this one version. Rather than contorting our code, I'd
suggest filing a gcc bug.
regards, tom lane
On Tue, Aug 29, 2023 at 10:18:36AM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Tue, Aug 29, 2023 at 10:26:27AM +0700, John Naylor wrote: > >> It looks like the former, since I can silence it on gcc 13 / -O1 by doing: > >> /* keep compiler quiet */ > >> actual_arg_types[0] = InvalidOid; > > > Agreed, that fixes it for me too. In fact, assigning to only element 99 or > > 200 also prevents the warning, and considering the array is defined for > > 100 elements, the fact is accepts 200 isn't a good thing. Patch attached. > > That seems like a pretty clear compiler bug, particularly since it just > appears in this one version. Rather than contorting our code, I'd > suggest filing a gcc bug. I assume I have to create a test case to report this to the gcc team. I tried to create such a test case on gcc 12 but it doesn't generate the warning. Attached is my attempt. Any ideas? I assume we can't just tell them to download our software and compile it. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
Вложения
Bruce Momjian <bruce@momjian.us> writes: > On Tue, Aug 29, 2023 at 10:18:36AM -0400, Tom Lane wrote: >> That seems like a pretty clear compiler bug, particularly since it just >> appears in this one version. Rather than contorting our code, I'd >> suggest filing a gcc bug. > I assume I have to create a test case to report this to the gcc team. I > tried to create such a test case on gcc 12 but it doesn't generate the > warning. Attached is my attempt. Any ideas? I assume we can't just > tell them to download our software and compile it. IIRC, they'll accept preprocessed compiler input for the specific file; you don't need to provide a complete source tree. Per https://gcc.gnu.org/bugs/ Please include all of the following items, the first three of which can be obtained from the output of gcc -v: the exact version of GCC; the system type; the options given when GCC was configured/built; the complete command line that triggers the bug; the compiler output (error messages, warnings, etc.); and the preprocessed file (*.i*) that triggers the bug, generated by adding -save-temps to the complete compilation command,or, in the case of a bug report for the GNAT front end, a complete set of source files (see below). Obviously, if you can trim the input it's good, but it doesn't have to be a minimal reproducer. regards, tom lane
On Tue, Aug 29, 2023 at 11:30:06PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Tue, Aug 29, 2023 at 10:18:36AM -0400, Tom Lane wrote: > >> That seems like a pretty clear compiler bug, particularly since it just > >> appears in this one version. Rather than contorting our code, I'd > >> suggest filing a gcc bug. > > > I assume I have to create a test case to report this to the gcc team. I > > tried to create such a test case on gcc 12 but it doesn't generate the > > warning. Attached is my attempt. Any ideas? I assume we can't just > > tell them to download our software and compile it. > > IIRC, they'll accept preprocessed compiler input for the specific file; > you don't need to provide a complete source tree. Per > https://gcc.gnu.org/bugs/ > > Please include all of the following items, the first three of which can be obtained from the output of gcc -v: > > the exact version of GCC; > the system type; > the options given when GCC was configured/built; > the complete command line that triggers the bug; > the compiler output (error messages, warnings, etc.); and > the preprocessed file (*.i*) that triggers the bug, generated by adding -save-temps to the complete compilation command,or, in the case of a bug report for the GNAT front end, a complete set of source files (see below). > > Obviously, if you can trim the input it's good, but it doesn't > have to be a minimal reproducer. Bug submitted, thanks for th preprocessed file tip. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Wed, Aug 30, 2023 at 11:16:48AM -0400, Bruce Momjian wrote: > On Tue, Aug 29, 2023 at 11:30:06PM -0400, Tom Lane wrote: > > Bruce Momjian <bruce@momjian.us> writes: > > > On Tue, Aug 29, 2023 at 10:18:36AM -0400, Tom Lane wrote: > > >> That seems like a pretty clear compiler bug, particularly since it just > > >> appears in this one version. Rather than contorting our code, I'd > > >> suggest filing a gcc bug. > > > > > I assume I have to create a test case to report this to the gcc team. I > > > tried to create such a test case on gcc 12 but it doesn't generate the > > > warning. Attached is my attempt. Any ideas? I assume we can't just > > > tell them to download our software and compile it. > > > > IIRC, they'll accept preprocessed compiler input for the specific file; > > you don't need to provide a complete source tree. Per > > https://gcc.gnu.org/bugs/ > > > > Please include all of the following items, the first three of which can be obtained from the output of gcc -v: > > > > the exact version of GCC; > > the system type; > > the options given when GCC was configured/built; > > the complete command line that triggers the bug; > > the compiler output (error messages, warnings, etc.); and > > the preprocessed file (*.i*) that triggers the bug, generated by adding -save-temps to the complete compilation command,or, in the case of a bug report for the GNAT front end, a complete set of source files (see below). > > > > Obviously, if you can trim the input it's good, but it doesn't > > have to be a minimal reproducer. > > Bug submitted, thanks for th preprocessed file tip. Good news, I was able to prevent the bug by causing compiling of clauses.c to use -O2 by adding this to src/Makefile.custom: clauses.o : CFLAGS+=-O2 Here is my submitted bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111240 -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.
On Wed, Aug 30, 2023 at 11:34:22AM -0400, Bruce Momjian wrote: > Good news, I was able to prevent the bug by causing compiling of > clauses.c to use -O2 by adding this to src/Makefile.custom: > > clauses.o : CFLAGS+=-O2 > > Here is my submitted bug report: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111240 I got this reply on the bug report: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111240#c5 Richard Biener 2023-08-31 09:46:44 UTC Confirmed. rettype_58 = enforce_generic_type_consistency (&actual_arg_types, &declared_arg_types, 0, _56, 0); and we reach this on the args == 0 path where indeed actual_arg_types is uninitialized and our heuristic says that a const qualified pointer is an input and thus might be read. So you get a maybe-uninitialized diagnostic at the call. GCC doesn't know that the 'nargs' argument relates to the array and that at most 'nargs' (zero here) arguments are inspected. So I think it works as designed, we have some duplicate bugreports complaining about this "heuristic". We are exposing this to ourselves by optimizing the args == 0 case (skipping the initialization loop and constant propagating the nargs argument). Aka jump-threading. I think we just have to assume this incorrect warning will be around for a while. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you.