Postgres Pro
Downloads
Home   >   mailing lists

Thread: Re: Warning -Wclobbered in PG_TRY(...)

Re: Warning -Wclobbered in PG_TRY(...)

From
Tom Lane
Date:
m.litsarev@postgrespro.ru writes:
> Does it makes sense to add volatile attribute to the _do_rethrow or 
> should we just ignore that -Wclobbered warning?

-Wclobbered would be really useful if it worked --- but sadly,
on practically all gcc and clang versions, those warnings have
nearly nothing to do with reality.  We typically ignore them.
See eg:

https://www.postgresql.org/message-id/4001633.1722625252@sss.pgh.pa.us

            regards, tom lane

Re: Warning -Wclobbered in PG_TRY(...)

From
Maxim Orlov
Date:

On Thu, 29 May 2025 at 14:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:
-Wclobbered would be really useful if it worked --- but sadly,
on practically all gcc and clang versions, those warnings have
nearly nothing to do with reality.  We typically ignore them.

Yes, in the vast majority of cases. But I believe this is not one of them. Actually, we do 
not adhere to the C language standard for a setjump/logjump. Notice how the 
non-volatile local variable "bool dorethrow##" utilized in the setjump/logjump block. 
This topic reminds me of [0].

In fact, we do not receive a warning for a C-lang build because the compiler does not 
appear to be "smart" enough to put this variable in a register. I ran a synthetic test 
equivalent to PG_TRY using godbolt.org, and the final code was the same whether 
the volatile qualifier was used or not. This is not the case with the C++ build. If the 
volatile qualifier is not used, the compiler optimizes this line and puts it in a register, 
and we get a warning. And to get rid of this warning, you need to either turn it off via 
the pragma directive, write your own version of PG_TRY with a volatile type qualifier, 
or patch Postgres. None of these solutions sound good to me. You may be wondering 
who will write Postgres extensions in C++. Having two different runtimes is absolute 
misery, and I completely agree. However, such extensions already exist; for example, 
pg_duckdb. AFAICS, it is quite popular in our area.

So, to be safe, we should follow the standard and include a volatile qualifier here. It 
really costs us nothing; the resulting code will be identical for C, and we'll avoid any 
future issues. In the case of C++, it will also resolve the warning.

Don't consider me as something of a formalist or someone who wants to make 
mountains out of molehills. I just want to make things better. PFA trivial patch.

 
--
Best regards,
Maxim Orlov.
Attachment

Re: Warning -Wclobbered in PG_TRY(...)

From
Tom Lane
Date:
Maxim Orlov <orlovmg@gmail.com> writes:
> On Thu, 29 May 2025 at 14:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> -Wclobbered would be really useful if it worked --- but sadly,
>> on practically all gcc and clang versions, those warnings have
>> nearly nothing to do with reality.  We typically ignore them.

> Yes, in the vast majority of cases. But I believe this is not one of them.
> Actually, we do
> not adhere to the C language standard for a setjump/logjump.

Really?  I don't have a current C standard at hand, but POSIX-2024 [1]
claims to be aligned with the C standard for this, and what it says
under "longjmp" is

    All accessible objects have values, and all other components of
    the abstract machine have state (for example, floating-point
    status flags and open files), as of the time longjmp() was called,
    except that the values of objects of automatic storage duration
    are unspecified if they meet all the following conditions:

        They are local to the function containing the corresponding
        setjmp() invocation.

        They do not have volatile-qualified type.

        They are changed between the setjmp() invocation and longjmp() call.

That is pretty black-and-white, and the PG_TRY macros do not
change _do_rethrow between setjmp() and longjmp().  PG_FINALLY
will change it after return from longjmp(), but at that point
it doesn't matter.  So if we need a volatile qualifier here,
it means the implementation fails to adhere to the C standard.

I'm not clear what conditions provoke the -Wclobbered warning,
but many cases seem to amount to "the value of the variable
gets changed before reaching PG_TRY".  That again is not a
case that the implementation is allowed to call undefined.

            regards, tom lane

Re: Warning -Wclobbered in PG_TRY(...)

From
Tom Lane
Date:
I wrote:
> Really?  I don't have a current C standard at hand, but POSIX-2024 [1]
> claims to be aligned with the C standard for this,

Ooops, forgot to attach the URL:

[1] https://pubs.opengroup.org/onlinepubs/9799919799/

Go there and use the "keyword search" for "longjmp" --- they
don't seem to make it possible to bookmark individual pages.

            regards, tom lane

Re: Warning -Wclobbered in PG_TRY(...)

From
Maxim Orlov
Date:

On Thu, 29 May 2025 at 18:26, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Go there and use the "keyword search" for "longjmp" --- they
don't seem to make it possible to bookmark individual pages.

Indeed, you are fully correct. I didn't analyse the code well enough. The equivalent code 
for PG_TRY/PG_FINALLY/PG_END_TRY will appear like the following:

do {
    jmp_buf *_save_exception_stack = PG_exception_stack;
    jmp_buf _local_sigjmp_buf;
    bool _do_rethrow = false;

    if (setjmp(_local_sigjmp_buf) == 0)
    {
        PG_exception_stack = &_local_sigjmp_buf;
        /* try here */
        longjmp(_local_sigjmp_buf, 1); /* elog ERROR */
    }
    else
        _do_rethrow = true;

    {
        PG_exception_stack = _save_exception_stack;
        /* catch */
    }

    if (_do_rethrow)
        pg_re_throw();

    PG_exception_stack = _save_exception_stack;
    /* finally */
} while (0);

There is no problem here. We only change _do_rethrow after longjmp. Apparently, we're 
dealing with a false positive warning here. Not sure, but it could be something like [0]. The 
correct solution in this case would be to disable clobbered warning.


--
Best regards,
Maxim Orlov.