Re: pg_verify_checksums and -fno-strict-aliasing

On Thu, Aug 30, 2018 at 9:35 PM, Magnus Hagander <magnus(at)hagander(dot)net>
wrote:

> 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)
>

PFA some *very* basic tests for pg_verify_checksums, which should at least
be enough to catch the kind of errors we had now in the tool itself.

Does not at this point try to solve the fact that the backend checksum code
is not tested.

(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/>