On Wed, Oct 09, 2019 at 01:15:29PM -0400, Tom Lane wrote:
> 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.
I may as well do that first, so there's no time when s_lock.h disagrees with
arch-ppc.h about the constraint to use. I'm attaching that patch, too.
> * 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.
xlc does not interpret "i" that way:
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=hornet&dt=2019-09-14%2016%3A42%3A32&stg=config
> * 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.
For a macro local to one C file, I think readability is the relevant metric.
In particular, it would be wrong to add arguments to make these more like
header file macros. I think the macros make the code somewhat more readable,
and you think they make the code less readable. I have removed the macros.
> * src/tools/pginclude/headerscheck needs the same adjustment as you
> made in cpluspluscheck.
Done.