On Mon, Oct 7, 2013 at 4:47 PM, Andres Freund <andres@2ndquadrant.com> wrote: > On 2013-10-07 13:25:17 -0400, Robert Haas wrote: >> On Fri, Oct 4, 2013 at 8:19 AM, Andres Freund <andres@2ndquadrant.com> wrote: >> > Could it be that MAXALIGN/TYPEALIGN doesn't really work for values >> > bigger than 32bit? >> > >> > #define MAXALIGN(LEN) TYPEALIGN(MAXIMUM_ALIGNOF, (LEN)) >> > #define TYPEALIGN(ALIGNVAL,LEN) \ >> > (((intptr_t) (LEN) + ((ALIGNVAL) - 1)) & ~((intptr_t) ((ALIGNVAL) - 1))) >> >> Isn't the problem, more specifically, that it doesn't work for values >> larger than an intptr_t? > > Well, yes. And intptr_t is 32bit wide on a 32bit platform. > >> 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). > 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. > > Maybe we should add a StaticAssert ensuring the TYPEALIGN macro only > gets passed 32bit types?
I think having two macros that behave identically on all platforms anyone cares about[1] but which can cause difficult-to-find corner-case-bugs on other platforms is just a recipe for disaster. I pledge to screw that up at least once.
The only improvement I thought of during writing the patch was to rename MAXALIGN to something more related to RAM, like perhaps RAMMAXALIGN or MEMORYMAXALIGN, so callers might think twice if they're using it for anything apart from memory addresses. I didn't really come up with a name I thought was good enough to warrant the change, so I left it as it was.
I also don't think it is perfect that both the marcos do the same thing on a 64bit compilation, but I think I was in the same boat as you... couldn't think of anything better.
If you can think of a name that will confuse users less then maybe it's worth making the change.
