xlc 12.1 miscompiles 32-bit ginCompareItemPointers()

From: Noah Misch <noah(at)leadboat(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: xlc 12.1 miscompiles 32-bit ginCompareItemPointers()
Date: 2015-07-19 19:50:33
Message-ID: 20150719195033.GB1300191@tornado.leadboat.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2015-07-19 20:07:37 Re: xlc 12.1 miscompiles 32-bit ginCompareItemPointers()
Previous Message Josh Berkus 2015-07-19 19:46:00 Re: Support for N synchronous standby servers - take 2