Noah Misch <n...@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
