A compiling warning in jsonb_populate_record_valid

Started by Richard Guoalmost 2 years ago9 messages

guofenglinux@gmail.com

almost 2 years ago

I came across a warning when building master (a044e61f1b) on old GCC
(4.8.5).

jsonfuncs.c: In function ‘jsonb_populate_record_valid’:
../../../../src/include/nodes/miscnodes.h:53:15: warning: the comparison
will always evaluate as ‘true’ for the address of ‘escontext’ will never be
NULL [-Waddress]
((escontext) != NULL && IsA(escontext, ErrorSaveContext) && \
^
jsonfuncs.c:2481:23: note: in expansion of macro ‘SOFT_ERROR_OCCURRED’
return BoolGetDatum(!SOFT_ERROR_OCCURRED(&escontext));

This was introduced in commit 1edb3b491b, and can be observed on several
systems in the buildfarm too, such as:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=arowana&dt=2024-01-25%2004%3A54%3A38&stg=build

Thanks
Richard

Amit Langote

amitlangote09@gmail.com

almost 2 years ago

In reply to: Richard Guo (#1)

1 attachment(s)

Re: A compiling warning in jsonb_populate_record_valid

Hi,

On Thu, Jan 25, 2024 at 2:59 PM Richard Guo <guofenglinux@gmail.com> wrote:

I came across a warning when building master (a044e61f1b) on old GCC
(4.8.5).

jsonfuncs.c: In function ‘jsonb_populate_record_valid’:
../../../../src/include/nodes/miscnodes.h:53:15: warning: the comparison will always evaluate as ‘true’ for the address of ‘escontext’ will never be NULL [-Waddress]
((escontext) != NULL && IsA(escontext, ErrorSaveContext) && \
^
jsonfuncs.c:2481:23: note: in expansion of macro ‘SOFT_ERROR_OCCURRED’
return BoolGetDatum(!SOFT_ERROR_OCCURRED(&escontext));

This was introduced in commit 1edb3b491b, and can be observed on several
systems in the buildfarm too, such as:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=arowana&dt=2024-01-25%2004%3A54%3A38&stg=build

Thanks for the report.

Will apply the attached, which does this:

-   return BoolGetDatum(!SOFT_ERROR_OCCURRED(&escontext));
+   return BoolGetDatum(!escontext.error_occurred);

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Attachments:

0001-Silence-compiler-warning-introduced-in-1edb3b491b.patchapplication/octet-stream; name=0001-Silence-compiler-warning-introduced-in-1edb3b491b.patchDownload

From 2bf840ed2f009682d41bb68cf2be0b3208339d53 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlan@postgresql.org>
Date: Thu, 25 Jan 2024 15:22:47 +0900
Subject: [PATCH] Silence compiler warning introduced in 1edb3b491b

Reported-by: Richard Guo <guofenglinux@gmail.com>
Discussion: https://postgr.es/m/CAMbWs48qEoe9Du5tuUxrkGQ6VC9oy+tQOORQ6jpob14-E1Z+jg@mail.gmail.com
---
 src/backend/utils/adt/jsonfuncs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index fe1a7ec09c..54dbd7e79f 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -2478,7 +2478,7 @@ jsonb_populate_record_valid(PG_FUNCTION_ARGS)
 	(void) populate_record_worker(fcinfo, "jsonb_populate_record",
 								  false, true, (Node *) &escontext);
 
-	return BoolGetDatum(!SOFT_ERROR_OCCURRED(&escontext));
+	return BoolGetDatum(!escontext.error_occurred);
 }
 
 Datum
-- 
2.35.3

Richard Guo

guofenglinux@gmail.com

almost 2 years ago

In reply to: Amit Langote (#2)

Re: A compiling warning in jsonb_populate_record_valid

On Thu, Jan 25, 2024 at 2:28 PM Amit Langote <amitlangote09@gmail.com>
wrote:

On Thu, Jan 25, 2024 at 2:59 PM Richard Guo <guofenglinux@gmail.com>
wrote:

I came across a warning when building master (a044e61f1b) on old GCC
(4.8.5).

jsonfuncs.c: In function ‘jsonb_populate_record_valid’:
../../../../src/include/nodes/miscnodes.h:53:15: warning: the comparison

will always evaluate as ‘true’ for the address of ‘escontext’ will never be
NULL [-Waddress]

((escontext) != NULL && IsA(escontext, ErrorSaveContext) && \
^
jsonfuncs.c:2481:23: note: in expansion of macro ‘SOFT_ERROR_OCCURRED’
return BoolGetDatum(!SOFT_ERROR_OCCURRED(&escontext));

This was introduced in commit 1edb3b491b, and can be observed on several
systems in the buildfarm too, such as:

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=arowana&dt=2024-01-25%2004%3A54%3A38&stg=build

Thanks for the report.

Will apply the attached, which does this:
-   return BoolGetDatum(!SOFT_ERROR_OCCURRED(&escontext));
+   return BoolGetDatum(!escontext.error_occurred);

Looks good to me.

Thanks
Richard

Amit Langote

amitlangote09@gmail.com

almost 2 years ago

In reply to: Richard Guo (#3)

Re: A compiling warning in jsonb_populate_record_valid

On Thu, Jan 25, 2024 at 4:47 PM Richard Guo <guofenglinux@gmail.com> wrote:

On Thu, Jan 25, 2024 at 2:28 PM Amit Langote <amitlangote09@gmail.com> wrote:
On Thu, Jan 25, 2024 at 2:59 PM Richard Guo <guofenglinux@gmail.com> wrote:

I came across a warning when building master (a044e61f1b) on old GCC
(4.8.5).

jsonfuncs.c: In function ‘jsonb_populate_record_valid’:
../../../../src/include/nodes/miscnodes.h:53:15: warning: the comparison will always evaluate as ‘true’ for the address of ‘escontext’ will never be NULL [-Waddress]
((escontext) != NULL && IsA(escontext, ErrorSaveContext) && \
^
jsonfuncs.c:2481:23: note: in expansion of macro ‘SOFT_ERROR_OCCURRED’
return BoolGetDatum(!SOFT_ERROR_OCCURRED(&escontext));

This was introduced in commit 1edb3b491b, and can be observed on several
systems in the buildfarm too, such as:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=arowana&dt=2024-01-25%2004%3A54%3A38&stg=build

Thanks for the report.

Will apply the attached, which does this:
-   return BoolGetDatum(!SOFT_ERROR_OCCURRED(&escontext));
+   return BoolGetDatum(!escontext.error_occurred);
Looks good to me.

Thanks for checking, pushed.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Tom Lane

tgl@sss.pgh.pa.us

almost 2 years ago

In reply to: Amit Langote (#2)

Re: A compiling warning in jsonb_populate_record_valid

Amit Langote <amitlangote09@gmail.com> writes:

On Thu, Jan 25, 2024 at 2:59 PM Richard Guo <guofenglinux@gmail.com> wrote:

I came across a warning when building master (a044e61f1b) on old GCC
(4.8.5).

Will apply the attached, which does this:

-   return BoolGetDatum(!SOFT_ERROR_OCCURRED(&escontext));
+   return BoolGetDatum(!escontext.error_occurred);

-1 please. We should not break that abstraction for the sake
of ignorable warnings on ancient compilers.

regards, tom lane

Amit Langote

amitlangote09@gmail.com

almost 2 years ago

In reply to: Tom Lane (#5)

Re: A compiling warning in jsonb_populate_record_valid

On Thu, Jan 25, 2024 at 23:57 Tom Lane <tgl@sss.pgh.pa.us> wrote:

Amit Langote <amitlangote09@gmail.com> writes:

On Thu, Jan 25, 2024 at 2:59 PM Richard Guo <guofenglinux@gmail.com>

wrote:

I came across a warning when building master (a044e61f1b) on old GCC
(4.8.5).

Will apply the attached, which does this:
-   return BoolGetDatum(!SOFT_ERROR_OCCURRED(&escontext));
+   return BoolGetDatum(!escontext.error_occurred);
-1 please. We should not break that abstraction for the sake
of ignorable warnings on ancient compilers.

Ignoring the warning was my 1st thought too, because an old discussion I
found about the warning was too old (2011). The style I adopted in my
“fix” is used in a few other places too, so I thought I might as well
go for it.

Anyway, I’m fine with reverting.

Show quoted text

Tom Lane

tgl@sss.pgh.pa.us

almost 2 years ago

In reply to: Amit Langote (#6)

Re: A compiling warning in jsonb_populate_record_valid

Amit Langote <amitlangote09@gmail.com> writes:

On Thu, Jan 25, 2024 at 23:57 Tom Lane <tgl@sss.pgh.pa.us> wrote:

-1 please. We should not break that abstraction for the sake
of ignorable warnings on ancient compilers.

Ignoring the warning was my 1st thought too, because an old discussion I
found about the warning was too old (2011). The style I adopted in my
“fix” is used in a few other places too, so I thought I might as well
go for it.

Oh, well maybe I'm missing some context. What comparable places did
you see?

regards, tom lane

Amit Langote

amitlangote09@gmail.com

almost 2 years ago

In reply to: Tom Lane (#7)

Re: A compiling warning in jsonb_populate_record_valid

On Fri, Jan 26, 2024 at 0:15 Tom Lane <tgl@sss.pgh.pa.us> wrote:

Amit Langote <amitlangote09@gmail.com> writes:

On Thu, Jan 25, 2024 at 23:57 Tom Lane <tgl@sss.pgh.pa.us> wrote:

-1 please. We should not break that abstraction for the sake
of ignorable warnings on ancient compilers.

Ignoring the warning was my 1st thought too, because an old discussion I
found about the warning was too old (2011). The style I adopted in my
“fix” is used in a few other places too, so I thought I might as well
go for it.

Oh, well maybe I'm missing some context. What comparable places did
you see?

Sorry, not on my computer right now so can’t paste any code, but I was able
to find a couple of functions (using Google ;) that refer to error_occurred
directly for one reason or another:

process_integer_literal(),
xml_is_document()

Tom Lane

tgl@sss.pgh.pa.us

almost 2 years ago

In reply to: Amit Langote (#8)

Re: A compiling warning in jsonb_populate_record_valid

Amit Langote <amitlangote09@gmail.com> writes:

On Fri, Jan 26, 2024 at 0:15 Tom Lane <tgl@sss.pgh.pa.us> wrote:

Amit Langote <amitlangote09@gmail.com> writes:

Ignoring the warning was my 1st thought too, because an old discussion I
found about the warning was too old (2011). The style I adopted in my
“fix” is used in a few other places too, so I thought I might as well
go for it.

Oh, well maybe I'm missing some context. What comparable places did
you see?

Sorry, not on my computer right now so can’t paste any code, but I was able
to find a couple of functions (using Google ;) that refer to error_occurred
directly for one reason or another:

OK, looking around, it seems like our pattern is that direct access
to escontext.error_occurred is okay in the same function that sets up
the escontext (so that there's no possibility of a NULL pointer).
So this change is fine and I withdraw my objection. Sorry for
the noise.

regards, tom lane