Thread: Re: [BUGS] Compile fails on AIX 6.1
A year ago we had a thread about assembler syntax errors on AIX, but it
died off when the original complainant stopped responding. I was recently
contacted off-list by Steve Underwood, who was seeing the same symptoms
on AIX 7.1. After some investigation, we found that the problem is that
IBM's assembler doesn't understand the "local symbol" notation supported
by the GNU assembler ("bne 1f" referencing the next occurrence of "1:").
So s_lock.h's PowerPC assembly code works if you have gcc configured to
use gas as backend, but not if it's configured to use the native AIX
assembler. Steve says the latter configuration is pretty common.
Curiously, our build goes through just fine if you use the gcc 4.2.0
build available from IBM's website. But it turns out it's using the
"generic AIX" stanza of s_lock.h, ie _check_lock(), rather than the
lwarx assembly code. AFAICS this must mean that that gcc build does
not define __ppc__ --- very weird. But a bit offtopic.
So now that we know what is happening, what do we want to do about it?
AFAICS there are two plausible ways to fix it:
1. Add a configure-time test to see if the assembler supports local
symbols. If not, don't try to use the lwarx assembly stanza, but let
it fall through to using _check_lock(). This would be simple but
there would presumably be some performance hit.
2. Don't rely on local symbols in the PPC spinlock assembly code. This
is a bit ugly, because the only way to do that is to hard-code branch
offsets, as in the attached draft patch. If there were any likelihood
that we'd be changing the PPC spinlock code in future, I would regard
this as unmaintainable ... but really, that code is pretty static.
So I think this is a viable alternative.
Comments?
regards, tom lane
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index ef66644..9e709f8 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -447,6 +447,12 @@ typedef unsigned int slock_t;
* NOTE: per the Enhanced PowerPC Architecture manual, v1.0 dated 7-May-2002,
* an isync is a sufficient synchronization barrier after a lwarx/stwcx loop.
* On newer machines, we can use lwsync instead for better performance.
+ *
+ * Ordinarily, we'd code the branches here using GNU-style local symbols, that
+ * is "1f" referencing "1:" and so on. But some people run gcc on AIX with
+ * IBM's assembler as backend, and IBM's assembler doesn't do local symbols.
+ * So hand-code the branch offsets; fortunately, all PPC instructions are
+ * exactly 4 bytes each, so it's not too hard to count.
*/
static __inline__ int
tas(volatile slock_t *lock)
@@ -461,20 +467,18 @@ tas(volatile slock_t *lock)
" lwarx %0,0,%3 \n"
#endif
" cmpwi %0,0 \n"
-" bne 1f \n"
+" bne .+16 \n" /* branch to li %1,1 */
" addi %0,%0,1 \n"
" stwcx. %0,0,%3 \n"
-" beq 2f \n"
-"1: li %1,1 \n"
-" b 3f \n"
-"2: \n"
+" beq .+12 \n" /* branch to lwsync/isync */
+" li %1,1 \n"
+" b .+12 \n" /* branch to end of asm sequence */
#ifdef USE_PPC_LWSYNC
" lwsync \n"
#else
" isync \n"
#endif
" li %1,0 \n"
-"3: \n"
: "=&r"(_t), "=r"(_res), "+m"(*lock)
: "r"(lock)
On Thu, Aug 27, 2015 at 10:36:46AM -0400, Tom Lane wrote:
> the problem is that
> IBM's assembler doesn't understand the "local symbol" notation supported
> by the GNU assembler ("bne 1f" referencing the next occurrence of "1:").
> So s_lock.h's PowerPC assembly code works if you have gcc configured to
> use gas as backend, but not if it's configured to use the native AIX
> assembler. Steve says the latter configuration is pretty common.
These days, the latter configuration is all but universal. Per the GCC
installation instructions, "The GNU Assembler has not been updated to support
AIX 6 or AIX 7."
> So now that we know what is happening, what do we want to do about it?
> AFAICS there are two plausible ways to fix it:
>
> 1. Add a configure-time test to see if the assembler supports local
> symbols. If not, don't try to use the lwarx assembly stanza, but let
> it fall through to using _check_lock(). This would be simple but
> there would presumably be some performance hit.
>
> 2. Don't rely on local symbols in the PPC spinlock assembly code. This
> is a bit ugly, because the only way to do that is to hard-code branch
> offsets, as in the attached draft patch. If there were any likelihood
> that we'd be changing the PPC spinlock code in future, I would regard
> this as unmaintainable ... but really, that code is pretty static.
> So I think this is a viable alternative.
A third option is to use __sync intrinsics, like we do on ARM. I like (2).
Noah Misch <noah@leadboat.com> writes:
> On Thu, Aug 27, 2015 at 10:36:46AM -0400, Tom Lane wrote:
>> the problem is that
>> IBM's assembler doesn't understand the "local symbol" notation supported
>> by the GNU assembler ("bne 1f" referencing the next occurrence of "1:").
>> So s_lock.h's PowerPC assembly code works if you have gcc configured to
>> use gas as backend, but not if it's configured to use the native AIX
>> assembler. Steve says the latter configuration is pretty common.
> These days, the latter configuration is all but universal. Per the GCC
> installation instructions, "The GNU Assembler has not been updated to support
> AIX 6 or AIX 7."
Ouch. I'm surprised we've not gotten more complaints.
>> 2. Don't rely on local symbols in the PPC spinlock assembly code. This
>> is a bit ugly, because the only way to do that is to hard-code branch
>> offsets, as in the attached draft patch. If there were any likelihood
>> that we'd be changing the PPC spinlock code in future, I would regard
>> this as unmaintainable ... but really, that code is pretty static.
>> So I think this is a viable alternative.
> A third option is to use __sync intrinsics, like we do on ARM. I like (2).
I've been waiting to hear confirmation from Steve that the proposed patch
works with IBM's assembler. (For all I know, it uses "*" rather than ".",
or some other randomness.) He's not responded yet though. Are you in
a position to test the patch?
regards, tom lane
I wrote:
> Noah Misch <noah@leadboat.com> writes:
>> On Thu, Aug 27, 2015 at 10:36:46AM -0400, Tom Lane wrote:
>>> 2. Don't rely on local symbols in the PPC spinlock assembly code.
>> A third option is to use __sync intrinsics, like we do on ARM. I like (2).
> I've been waiting to hear confirmation from Steve that the proposed patch
> works with IBM's assembler. (For all I know, it uses "*" rather than ".",
> or some other randomness.) He's not responded yet though. Are you in
> a position to test the patch?
Steve got back to me with the news that AIX's assembler thinks that "."
is an ordinary symbol, not the current location. Some googling says that
that assembler likes "$" for current location. I did a quick check on my
oldest OS X PPC box, and it seems to be happy with "$" as well, so maybe
we can use that --- though I see nothing about "$" in the GNU Assembler
manual, which makes me a bit worried about whether it works on all PPC
systems.
regards, tom lane
I wrote:
> ... that assembler likes "$" for current location. I did a quick check on my
> oldest OS X PPC box, and it seems to be happy with "$" as well, so maybe
> we can use that --- though I see nothing about "$" in the GNU Assembler
> manual, which makes me a bit worried about whether it works on all PPC
> systems.
A look into the current gas sources finds this in config/tc-ppc.h:
/* $ is used to refer to the current location. */
#define DOLLAR_DOT
so apparently this is indeed standard behavior for gas on PPC. There's
no indication that you could turn it off without manually hacking this
config header.
Source code access to one's tools is so pleasant ...
regards, tom lane
On Fri, Aug 28, 2015 at 09:58:46AM -0400, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > On Thu, Aug 27, 2015 at 10:36:46AM -0400, Tom Lane wrote: > >> So s_lock.h's PowerPC assembly code works if you have gcc configured to > >> use gas as backend, but not if it's configured to use the native AIX > >> assembler. Steve says the latter configuration is pretty common. > > > These days, the latter configuration is all but universal. Per the GCC > > installation instructions, "The GNU Assembler has not been updated to support > > AIX 6 or AIX 7." > > Ouch. I'm surprised we've not gotten more complaints. That surprised me, too. Perhaps almost everyone has used either xlc or that IBM-provided gcc you wrote about. > >> 2. Don't rely on local symbols in the PPC spinlock assembly code. > > A third option is to use __sync intrinsics, like we do on ARM. I like (2). > > I've been waiting to hear confirmation from Steve that the proposed patch > works with IBM's assembler. (For all I know, it uses "*" rather than ".", > or some other randomness.) He's not responded yet though. Are you in > a position to test the patch? I tested a gcc 64-bit build. Consistent with your followup, "b .+12" doesn't build, but "b $+12" builds and passes "make check". I am attaching the exact diff I tested. On GNU/Linux ppc, I get the same opcodes before and after the change.
Attachment
Noah Misch <noah@leadboat.com> writes:
> I tested a gcc 64-bit build. Consistent with your followup, "b .+12" doesn't
> build, but "b $+12" builds and passes "make check". I am attaching the exact
> diff I tested.
> On GNU/Linux ppc, I get the same opcodes before and after the change.
Thanks for checking! I have some other business to attend to today, but
I'll get this committed soon.
Please consider spinning up a gcc buildfarm critter on the machine running
hornet & mandrill.
regards, tom lane