Re: pg_verify_checksums and -fno-strict-aliasing

On Thu, Aug 30, 2018 at 4:39 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> writes:
> >> If I add -fno-strict-aliasing to $CFLAGS, the problem goes away.
> >> Is this something to worry about, or just pilot error cause I am not
> >> using the same $CFLAGS as for the rest of the build? I originally
> >> noticed this problem with my external fork of pg_verify_checksums.
>
> > It looks like the code is using aliasing where the standard says it
> should
> > not, which breaks compiler optimization, and the added options tells the
> > compiler to not assume that the code conforms to the standard...
>
> Actually, this code is just plain broken:
>
> char buf[BLCKSZ];
> PageHeader header = (PageHeader) buf;
>
> There is no guarantee that a local char[] array is aligned on anything
> stronger than a byte boundary, and if it isn't, word-sized accesses to
> *header are going to fail on machines with strict alignment rules.
> I suppose that that is really what Michael's compiler is moaning about.
>
> I rather suspect that this hasn't been tested on anything but Intel
> hardware, which is famously misalignment-tolerant. The lack of any
> apparent regression test infrastructure for it isn't leaving a warm
> feeling about how much the buildfarm is testing it.
>

I know I certainly didn't test it on non-intel.

We did have that in the online verify checksum patch, but it came out as
part of the revert of that part.

Given that we also recently found bugs in the actual hash index
implementation that would've gotten caught by this, perhaps we should add a
TAP test for this.

Should we make it a separate test in pg_verify_checksums, or should we
piggyback on the pg_basebackup tests (which AFAICT is the only ones that
create a cluster with checksums enabled at all, and thus is the only
codepath that actually uses the backend checksum code at all, which I think
is an even worse thing to have no tests of)

(The right fix, of course, is to malloc the work buffer rather than
> put it on the stack.)

So if I get you right, you're saying the attached patch should be all
that's needed?

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/>
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>