On Wed, May 03, 2023 at 04:33:32PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> Here's a new version of the patch. Besides adding comments and a commit
>> message, I made sure to decrement the reference count for pltargs in the
>> PG_CATCH block (which means that pltargs likely needs to be volatile).
>
> Hmm, actually I think these changes should allow you to *remove* some
> volatile markers. IIUC, you need volatile for variables that are declared
> outside PG_TRY but modified within it. That is the case for these
> pointers as the code stands, but your patch is changing them to the
> non-risky case where they are assigned once before entering PG_TRY.
>
> (My mental model of this is that without "volatile", the compiler
> may keep the variable in a register, creating the hazard that longjmp
> will revert the variable's value to what it was at setjmp time thanks
> to the register save/restore that those functions do. But if it hasn't
> changed value since entering PG_TRY, then that doesn't matter.)
Ah, I think you are right. elog.h states as follows:
* Note: if a local variable of the function containing PG_TRY is modified
* in the PG_TRY section and used in the PG_CATCH section, that variable
* must be declared "volatile" for POSIX compliance. This is not mere
* pedantry; we have seen bugs from compilers improperly optimizing code
* away when such a variable was not marked. Beware that gcc's -Wclobbered
* warnings are just about entirely useless for catching such oversights.
With this change, pltdata isn't modified in the PG_TRY section, and the
only modification of pltargs happens after all elogs. It might be worth
keeping pltargs volatile in case someone decides to add an elog() in the
future, but I see no need to keep it for pltdata.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com