Thread: Re: [COMMITTERS] packing/alignment annotation for ItemPointerData redux
[ moved to -hackers ]
Greg Stark <stark@mit.edu> writes:
> Back in 2001 a hack to add __attribute__((packed)) to ItemPtr was
> added with a comment "Appropriate whack upside the head for ARM"
> (dcbbdb1b3ee). I don't know if this is still a factor in 2016 or not
> but it has already resulted in some collateral damage in 2015 when
> some compiler took that as license to align the whole struct on single
> byte alignment when it was buried inside another struct
> (d4b538ea367de).
> I just tried compiling with Clang 3.8.0 and got tons of warnings about
> this because:
> 'ItemPointerData' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
> ...ItemPointerGetBlockNumber(&(xlrec->target_tid)),
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../../src/include/storage/itemptr.h:69:25: note: expanded from macro
> 'ItemPointerGetBlockNumber'
> BlockIdGetBlockNumber(&(pointer)->ip_blkid) \
> ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
> ../../../src/include/storage/block.h:118:19: note: expanded from macro
> 'BlockIdGetBlockNumber'
> (BlockNumber) (((blockId)->bi_hi << 16) | ((uint16) (blockId)->bi_lo)) \
> ^~~~~~~
> Which seems to indicate that clang may not understand the
> "pg_attribute_aligned(2)" or perhaps it does and just doesn't take it
> into account when generating these warnings.
Ick. Can you look to see how those macros are expanding on your clang?
> I'm sure there are other people testing clang -- isn't it the default
> on MacOS? Do they not see these warnings?
I've never seen this on MacOS. The current compiler version (on Sierra)
is
$ gcc -v
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr
--with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/include/c++/4.2.1
Apple LLVM version 8.0.0 (clang-800.0.38)
Target: x86_64-apple-darwin16.0.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
Don't know how that version number compares to "3.8.0".
regards, tom lane
On Wed, Oct 19, 2016 at 5:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Don't know how that version number compares to "3.8.0". Argh. Just to further confuse matters apparently the warnings are from clang 4.0. I had been testing with 3.8.0 earlier but updated at some point. And I'm not being able to reproduce them with a minimal test case yet. -- greg
Ah. Here we go:
$ /usr/bin/clang-4.0 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -Wno-unused-command-line-argument -Wall -c clang-bug.c
clang-bug.c:54:9: error: use of undeclared identifier 'BlockNumber'
return ItemPointerGetBlockNumber(&ip);
^
clang-bug.c:48:2: note: expanded from macro 'ItemPointerGetBlockNumber'
BlockIdGetBlockNumber(&(pointer)->ip_blkid) \
^
clang-bug.c:38:3: note: expanded from macro 'BlockIdGetBlockNumber'
(BlockNumber) (((blockId)->bi_hi << 16) | ((uint16) (blockId)->bi_lo)) \
^
1 error generated.
Preprocessor output:
# 1 "clang-bug.c"
# 1 "<built-in>" 1
# 1 "<built-in>" 3
# 317 "<built-in>" 3
# 1 "<command line>" 1
# 1 "<built-in>" 2
# 1 "clang-bug.c" 2
typedef unsigned short uint16;
typedef uint16 OffsetNumber;
typedef struct BlockIdData
{
uint16 bi_hi;
uint16 bi_lo;
} BlockIdData;
typedef struct ItemPointerData
{
BlockIdData ip_blkid;
OffsetNumber ip_posid;
}
__attribute__((packed))
__attribute__((aligned(2)))
ItemPointerData;
typedef ItemPointerData *ItemPointer;
# 51 "clang-bug.c"
int main() {
ItemPointerData ip;
return ( ( (BlockNumber) (((&(&ip)->ip_blkid)->bi_hi << 16) |
((uint16) (&(&ip)->ip_blkid)->bi_lo)) ) );
}
Attachment
Sorry -- with the obvious error fixed:
$ /usr/bin/clang-4.0 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -Wno-unused-command-line-argument -Wall -c clang-bug.c
clang-bug.c:55:9: warning: taking address of packed member 'ip_blkid'
of class or structure
'ItemPointerData' may result in an unaligned pointer value
[-Waddress-of-packed-member]
return ItemPointerGetBlockNumber(&ip);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
clang-bug.c:49:25: note: expanded from macro 'ItemPointerGetBlockNumber'
BlockIdGetBlockNumber(&(pointer)->ip_blkid) \
~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
clang-bug.c:39:19: note: expanded from macro 'BlockIdGetBlockNumber'
(BlockNumber) (((blockId)->bi_hi << 16) | ((uint16) (blockId)->bi_lo)) \
^~~~~~~
clang-bug.c:55:9: warning: taking address of packed member 'ip_blkid'
of class or structure
'ItemPointerData' may result in an unaligned pointer value
[-Waddress-of-packed-member]
return ItemPointerGetBlockNumber(&ip);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
clang-bug.c:49:25: note: expanded from macro 'ItemPointerGetBlockNumber'
BlockIdGetBlockNumber(&(pointer)->ip_blkid) \
~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~
clang-bug.c:39:55: note: expanded from macro 'BlockIdGetBlockNumber'
(BlockNumber) (((blockId)->bi_hi << 16) | ((uint16) (blockId)->bi_lo)) \
^~~~~~~
2 warnings generated.
Attachment
Greg Stark <stark@mit.edu> writes:
> Sorry -- with the obvious error fixed:
You didn't show -E output from this version, but the other one had
> __attribute__((packed))
> __attribute__((aligned(2)))
so it appears that clang 4.0 does accept these attributes but then
produces the warning anyway. I suggest filing this as a bug in clang 4.0,
and marking it as a regression from older versions which did not produce
such a warning.
If you get pushback claiming it's intentional, I'd be inclined to hack
our macro definitions so that we don't believe clang understands
attribute(aligned), because it evidently doesn't. But let's see
their response first.
regards, tom lane