Noah Misch <noah@leadboat.com> writes:
> On Mon, Oct 07, 2019 at 03:06:35PM -0400, Tom Lane wrote:
>> This still fails on Apple's compilers. ...
> Thanks for testing. That error boils down to "need to use some other
> register". The second operand of addi is one of the ppc instruction operands
> that can hold a constant zero or a register number[1], so the proper
> constraint is "b". I've made it so and added a comment.
Ah-hah. This version does compile and pass check-world for me.
> I should probably
> update s_lock.h, too, in a later patch. I don't know how it has
> mostly-avoided this failure mode, but its choice of constraint could explain
> https://postgr.es/m/flat/36E70B06-2C5C-11D8-A096-0005024EF27F%40ifrance.com
Indeed. It's a bit astonishing that more people haven't hit that.
This should be back-patched.
Now that the patch passes mechanical checks, I took a closer look,
and there are some things I don't like:
* I still think that the added configure test is a waste of build cycles.
It'd be sufficient to test "#ifdef HAVE__BUILTIN_CONSTANT_P" where you
are testing HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P, because our previous
buildfarm go-round with this showed that all supported compilers
interpret "i" this way.
* I really dislike building the asm calls with macros as you've done
here. The macros violate project style, and are not remotely general-
purpose, because they have hard-wired references to variables that are
not in their argument lists. While that could be fixed with more
arguments, I don't think that the approach is readable or maintainable
--- it's impossible for example to understand the register constraints
without looking simultaneously at the calls and the macro definition.
And, as we've seen in this "b" issue, the interactions between the chosen
instruction types and the constraints are subtle enough to make me wonder
whether you won't need even more arguments to allow some of the other
constraints to be variable. I think it'd be far better just to write out
the asm in-line and accept the not-very-large amount of code duplication
you'd get.
* src/tools/pginclude/headerscheck needs the same adjustment as you
made in cpluspluscheck.
regards, tom lane
