Re: LLVM miscompiles numeric.c access to short numeric var headers

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Greg Stark <stark(at)mit(dot)edu>
Cc: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: LLVM miscompiles numeric.c access to short numeric var headers
Date: 2015-11-12 15:51:24
Message-ID: 18280.1447343484@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Greg Stark <stark(at)mit(dot)edu> writes:
> On Thu, Nov 12, 2015 at 2:39 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Either that's a reportable compiler bug, or someplace nearby we've
>> casted the pointer to something that would require a 4-byte struct.
>> I'm not sure which code you're looking at exactly, but maybe we're
>> using "union NumericChoice" prematurely?

> It triggers on the line with the NUMERIC_WEIGHT() macro call in
> init_var_from_num():

Hmm. I'd argue that that is a compiler bug, but I dunno if the LLVM
guys would see it that way. The existence of other union members in
the declaration shouldn't license them to assume that the particular
instance we're accessing is large enough for any possible member.

> I think it wouldn't be too hard to just give up on the structs
> and unions and use a char * as the underlying type. We could access
> the meta information directly using byte accesses and memcpy the
> digits to an aligned array of digits when setting up the var.

Meh. The palloc to create an aligned array of digits would eat up
any possible performance win --- it'd be just about as expensive
as the existing unpack operation.

I think we could fix the immediate issue by redeclaring numeric
headers as arrays of (u)int16 rather than structs. I'm not
very excited about the packed-header case.

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Greg Stark 2015-11-12 16:06:39 Re: LLVM miscompiles numeric.c access to short numeric var headers
Previous Message Andres Freund 2015-11-12 15:37:00 Re: checkpointer continuous flushing