On 07.10.2013 23:47, Andres Freund wrote:
> On 2013-10-07 13:25:17 -0400, Robert Haas wrote:
>> And does that indicate that intptr_t is the wrong type to be using here?
>
> No, I don't think so. intptr_t is defined to be a integer type to which
> you can cast a pointer, cast it back and still get the old value. On
> 32bit platforms it usually will be 32bit wide.
> All that's fine for the classic usages of TYPEALIGN where it's used on
> pointers or lengths of stuff stored in memory. Those will always fit in
> 32bit on a 32bit platform. But here we're using it on explicit 64bit
> types (XLogRecPtr).
Yep.
> Now, you could argue that we should make it use 64bit math everywhere -
> but I think that might incur quite the price on some 32bit
> platforms. It's used in the tuple decoding stuff, that's quite the hot
> path in some workloads.
>
> So I guess it's either a separate macro, or we rewrite that piece of
> code to work slightly differently and work directly on the lenght or
> such.
Committed what is pretty much David's original patch.
> Maybe we should add a StaticAssert ensuring the TYPEALIGN macro only
> gets passed 32bit types?
I tried that, like this:
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -532,7 +532,7 @@ typedef NameData *Name; */
#define TYPEALIGN(ALIGNVAL,LEN) \
- (((intptr_t) (LEN) + ((ALIGNVAL) - 1)) & ~((intptr_t) ((ALIGNVAL) - 1)))
+ ( StaticAssertExpr(sizeof(LEN) <= 4, "type is too wide"), ((intptr_t)
(LEN) + ((ALIGNVAL) - 1)) & ~((intptr_t) ((ALIGNVAL) - 1)))
#define SHORTALIGN(LEN) TYPEALIGN(ALIGNOF_SHORT, (LEN)) #define INTALIGN(LEN)
TYPEALIGN(ALIGNOF_INT,(LEN))
However, it gave a lot of errors from places where we have something
like "char buf[MaxHeapTuplesPerPage]". MaxHeapTuplesPerPage uses
MAXALIGN, and gcc doesn't like it when StaticAssertExpr is used in such
a context (to determine the size of an array).
I temporarily changed places where that was a problem to use a copy of
TYPEALIGN with no assertion, to verify that there are no more genuine
bugs like this lurking. It was worth it as a one-off check, but I don't
think we want to commit that.
Thanks for the report, and analysis!
- Heikki
