Thread: A compiling warning in jsonb_populate_record_valid

A compiling warning in jsonb_populate_record_valid

From
Richard Guo
Date:
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

Re: A compiling warning in jsonb_populate_record_valid

From
Amit Langote
Date:
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
addressof ‘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

Attachment

Re: A compiling warning in jsonb_populate_record_valid

From
Richard Guo
Date:

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

Re: A compiling warning in jsonb_populate_record_valid

From
Amit Langote
Date:
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
addressof ‘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



Re: A compiling warning in jsonb_populate_record_valid

From
Tom Lane
Date:
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



Re: A compiling warning in jsonb_populate_record_valid

From
Amit Langote
Date:
 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.

Re: A compiling warning in jsonb_populate_record_valid

From
Tom Lane
Date:
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



Re: A compiling warning in jsonb_populate_record_valid

From
Amit Langote
Date:
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()

Re: A compiling warning in jsonb_populate_record_valid

From
Tom Lane
Date:
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