Обсуждение: xlc 12.1 miscompiles 32-bit ginCompareItemPointers()
In a 32-bit build, xlc 12.1 for AIX miscompiles three inline expansions of ginCompareItemPointers(), all in ginget.c (line numbers per commit 9aa6634): 739 Assert(!ItemPointerIsValid(&entry->curItem) || 740 ginCompareItemPointers(&entry->curItem, &advancePast) <= 0); 847 } while (ginCompareItemPointers(&entry->curItem, &advancePast) <= 0); 915 if (ginCompareItemPointers(&key->curItem, &advancePast) > 0) For one of the arguments, instead of computing hi << 32 | lo << 16 | posid it computes (lo << 16) << 32 | lo << 16 | posid PostgreSQL 9.4, which introduced the inline ginCompareItemPointers(), is the oldest version affected. The problem remained invisible until my recent commit 43d89a2; the quiet inline "configure" test had been failing, leaving PG_USE_INLINE unset. I tried some workarounds. Introducing intermediate variables or moving parts of the calculation down into other inline functions did not help. The code compiles fine when not inlined. Changing "hi << 32" to "hi << 33" worked (we need just 48 of the 64 bits), but it presumably reduces performance on ABIs where the current bit shifts boil down to a no-op. I propose to expand the gin_private.h "#ifdef PG_USE_INLINE" test to exclude xlc 32-bit configurations. The last 32-bit AIX kernel exited support on 2012-04-30. There's little reason to prefer 32-bit PostgreSQL under a 64-bit kernel, so that configuration can do without the latest GIN optimization. One alternative would be to distill a configure-time test detecting the bug, so fixed xlc versions reacquire the optimization. Another alternative is to change the bit layout within the uint64; for example, we could use the most-significant 48 bits instead of the least-significant 48 bits. I dislike the idea of a niche configuration driving that choice. Thanks, nm
On July 19, 2015 9:50:33 PM GMT+02:00, Noah Misch <noah@leadboat.com> wrote: >In a 32-bit build, xlc 12.1 for AIX miscompiles three inline expansions >of >ginCompareItemPointers(), all in ginget.c (line numbers per commit >9aa6634): > >739 Assert(!ItemPointerIsValid(&entry->curItem) || >740 ginCompareItemPointers(&entry->curItem, &advancePast) <= 0); > >847 } while (ginCompareItemPointers(&entry->curItem, &advancePast) <= >0); > >915 if (ginCompareItemPointers(&key->curItem, &advancePast) > 0) > >For one of the arguments, instead of computing > hi << 32 | lo << 16 | posid >it computes > (lo << 16) << 32 | lo << 16 | posid > >PostgreSQL 9.4, which introduced the inline ginCompareItemPointers(), >is the >oldest version affected. The problem remained invisible until my >recent >commit 43d89a2; the quiet inline "configure" test had been failing, >leaving >PG_USE_INLINE unset. > >I tried some workarounds. Introducing intermediate variables or moving >parts >of the calculation down into other inline functions did not help. The >code >compiles fine when not inlined. Changing "hi << 32" to "hi << 33" >worked (we >need just 48 of the 64 bits), but it presumably reduces performance on >ABIs >where the current bit shifts boil down to a no-op. > >I propose to expand the gin_private.h "#ifdef PG_USE_INLINE" test to >exclude >xlc 32-bit configurations. The last 32-bit AIX kernel exited support >on >2012-04-30. I vote to simply error out in that case then. Trying to fix individual compiler bugs in an niche OS sounds like a bad idea. Andres --- Please excuse brevity and formatting - I am writing this on my mobile phone.
Andres Freund <andres@anarazel.de> writes:
> On July 19, 2015 9:50:33 PM GMT+02:00, Noah Misch <noah@leadboat.com> wrote:
>> I propose to expand the gin_private.h "#ifdef PG_USE_INLINE" test to
>> exclude xlc 32-bit configurations. The last 32-bit AIX kernel exited
>> support on 2012-04-30.
> I vote to simply error out in that case then. Trying to fix individual compiler bugs in an niche OS sounds like a bad
idea.
I think I'm with Andres --- are there really enough users of this
configuration to justify working around such a bug?
More: if the compiler does have a bug like that, how much confidence can
we have, really, that there are no other miscompiled places and won't be
any in the future? If we are going to support this configuration, I think
what the patch ought to do is disable PG_USE_INLINE globally when this
compiler is detected. That would revert the behavior to what it was
before 43d89a2. We have absolutely no field experience justifying the
assumption that a narrower fix would be safe.
regards, tom lane
On Sun, Jul 19, 2015 at 04:22:47PM -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On July 19, 2015 9:50:33 PM GMT+02:00, Noah Misch <noah@leadboat.com> wrote: > >> I propose to expand the gin_private.h "#ifdef PG_USE_INLINE" test to > >> exclude xlc 32-bit configurations. The last 32-bit AIX kernel exited > >> support on 2012-04-30. > > > I vote to simply error out in that case then. Trying to fix individual compiler bugs in an niche OS sounds like a badidea. Fair principle. PostgreSQL 9.4.4 does work in this configuration due to the accident of -qlonglong causing a warning that in turn disables PG_USE_INLINE. I do not want 9.4.5 to break this configuration, and I also don't want to restore the works-accidentally state. Adding a few more conditions to one #if is cheap and has neither problem. > I think I'm with Andres --- are there really enough users of this > configuration to justify working around such a bug? We never have the benefit of an answer to that question. > More: if the compiler does have a bug like that, how much confidence can > we have, really, that there are no other miscompiled places and won't be > any in the future? If we are going to support this configuration, I think > what the patch ought to do is disable PG_USE_INLINE globally when this > compiler is detected. That would revert the behavior to what it was > before 43d89a2. That's a reasonable alternative.
On Sun, Jul 19, 2015 at 04:32:16PM -0400, Noah Misch wrote: > On Sun, Jul 19, 2015 at 04:22:47PM -0400, Tom Lane wrote: > > More: if the compiler does have a bug like that, how much confidence can > > we have, really, that there are no other miscompiled places and won't be > > any in the future? If we are going to support this configuration, I think > > what the patch ought to do is disable PG_USE_INLINE globally when this > > compiler is detected. That would revert the behavior to what it was > > before 43d89a2. > > That's a reasonable alternative. Here's the patch implementing it.