Bug 752905

Created attachment 532907 [details]
original source code, edited to remove extraneous functions

Created attachment 532908 [details]
-E output, so you don't need a postgresql source tree to compile

Might be the leaf attribute that was recently added to glibc triggering the problem.

Yup.  The __leaf__ attribute on the sigsetjmp prototype is causing FRE to decide it's OK to remove the "redundant" assignment to PLy_curr_procedure.

setjmp/__sigsetjmp/_setjmp definitely must be __THROWNL rather than __THROW.
Similarly setcontext and swapcontext.  They all have side-effects which can modify module static variables that don't have address taken.

(In reply to comment #5)
> setjmp/__sigsetjmp/_setjmp definitely must be __THROWNL rather than __THROW.

Oh ... perhaps that explains why it seemed that the behavioral change was triggered by the most recent glibc update.  Could it be that they had them correctly declared previously, and reverted the change?  See my previous complaint about this issue at bug #752883.

AFAIK for F16 the solution was just to revert the git commit that added leaf attribute to __THROW macro (glibc-2.14.90-14).
Guess -15 (the .1 should be removed for -16 I'd say) this revert has been removed
and instead the upstream patch from Andreas is in, but that doesn't handle setjmp etc. side-effects correctly.

(In reply to comment #4)
> Yup.  The __leaf__ attribute on the sigsetjmp prototype is causing FRE to
> decide it's OK to remove the "redundant" assignment to PLy_curr_procedure.

I don't pretend to understand this in any detail, but the function that's called between the two PLy_curr_procedure assignments doesn't have any such decoration.  So isn't the optimization still wrong, regardless of how sigsetjmp is marked?

glibc-2.14.90-15.2 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/FEDORA-2011-15723

The glibc 15.2 update does fix this problem for me ... but I'm still wondering why the leaf attribute on sigsetjmp should have allowed removal of an assignment that was *after* that call.

One of the side-effects of the leaf attribute is that static CU local variables that don't have address taken can be considered unchanged during the call to a leaf function (changing such a variable can be done only from the same compilation unit and leaf attribute says that the function doesn't call anything in the current function).  So the compiler can e.g. optimize:
static int var_with_address_not_taken;
...
  var_with_address_not_taken = 5;
  leaf_function (); // where leaf_function () isn't defined in the current CU
  return var_with_address_not_taken;
into
...
  var_with_address_not_taken = 5;
  leaf_function ();
  return 5;
because the leaf function couldn't have changed the var_with_address_not_taken.
Now, if you use leaf attribute on setjmp and do:
  var_with_address_not_taken = 5;
  if (!setjmp (&jmpbuf))
    {
      ..
      var_with_address_not_taken = x;
    }
  return var_with_address_not_taken;
gcc might optimize it similarly, thinking that var_with_address_not_taken couldn't have changed during the call.  Now, certainly gcc could just ignore leaf attribute on functions with returns_twice attribute (or those that get it as builtins automatically), but GCC 4.6 (which introduced leaf attribute) wasn't doing that, nobody thought that somebody would mark functions with such side-effects as leaf.  So it is just better to keep this on the glibc side where it has been introduced a few days ago.

(In reply to comment #11)
> One of the side-effects of the leaf attribute is that static CU local variables
> that don't have address taken can be considered unchanged during the call to a
> leaf function (changing such a variable can be done only from the same
> compilation unit and leaf attribute says that the function doesn't call
> anything in the current function).

Right, makes sense.  But the case I was dealing with here was essentially

  if (setjmp (&jmpbuf))
    {
      ..
      var_with_address_not_taken = x;
      non_leaf_function();
    }
  var_with_address_not_taken = y;

ISTM that removing the first assignment is wrong, because of the presence of the non-leaf function, no matter how setjmp() is marked.  I still think there's an additional gcc bug lurking here.

No.
You are dealing essentially with:
  localvar = var_with_address_not_taken;
  if (setjmp (&jmpbuf))
    {
      var_with_address_not_taken = localvar;
      non_leaf_noreturn_function ();
    }
  else
    {
      ...
    }
And, when you promise to the compiler that the setjmp call will not change
var_with_address_not_taken by using leaf attribute for it, then the store to
var_with_address_not_taken can be optimized away, because it is dead.  The compiler has the right to assume that it already contains the value that the store would set it to (remember, var_with_address_not_taken isn't volatile, doesn't have address taken and the setjmp call with leaf attribute isn't supposed to call, even indirectly, anything in the current CU.

Oh, it's dependent on the value being stored.  Got it.  Thanks for the explanation.

Package glibc-2.14.90-15.2:
* should fix your issue,
* was pushed to the Fedora 16 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing glibc-2.14.90-15.2'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2011-15723
then log in and leave karma (feedback).

This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.

Package glibc-2.14.90-16:
* should fix your issue,
* was pushed to the Fedora 16 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing glibc-2.14.90-16'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2011-15723
then log in and leave karma (feedback).

This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.

glibc-2.14.90-18 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/glibc-2.14.90-18

Bogus updates which caused this problem have been reverted/unpushed (-15, -16).