Re: [HACKERS] [COMMITTERS] packing/alignment annotation for ItemPointerData redux

2016-10-19 Thread Tom Lane 
Greg Stark  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] packing/alignment annotation for ItemPointerData redux

2016-10-19 Thread Greg Stark 
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();
   ^~
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();
   ^~
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.
#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__)
#define pg_attribute_aligned(a) __attribute__((aligned(a)))
#define pg_attribute_packed() __attribute__((packed))
#endif

typedef unsigned BlockNumber;
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;
}
/* If compiler understands packed and aligned pragmas, use those */
#if defined(pg_attribute_packed) && defined(pg_attribute_aligned)
pg_attribute_packed()
pg_attribute_aligned(2)
#endif
ItemPointerData;

typedef ItemPointerData *ItemPointer;



/*
 * BlockIdGetBlockNumber
 *		Retrieve the block number from a block identifier.
 */
#define BlockIdGetBlockNumber(blockId) \
( \
	(BlockNumber) (((blockId)->bi_hi << 16) | ((uint16) (blockId)->bi_lo)) \
)


/*
 * ItemPointerGetBlockNumber
 *		Returns the block number of a disk item pointer.
 */
#define ItemPointerGetBlockNumber(pointer) \
( \
	BlockIdGetBlockNumber(&(pointer)->ip_blkid) \
)

int main() {
	ItemPointerData ip;

	return ItemPointerGetBlockNumber();
}

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] packing/alignment annotation for ItemPointerData redux

2016-10-19 Thread Greg Stark 
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();
   ^
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 "" 1
# 1 "" 3
# 317 "" 3
# 1 "" 1
# 1 "" 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_blkid)->bi_hi << 16) |
((uint16) (&()->ip_blkid)->bi_lo)) ) );
}
#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__)
#define pg_attribute_aligned(a) __attribute__((aligned(a)))
#define pg_attribute_packed() __attribute__((packed))
#endif

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;
}
/* If compiler understands packed and aligned pragmas, use those */
#if defined(pg_attribute_packed) && defined(pg_attribute_aligned)
pg_attribute_packed()
pg_attribute_aligned(2)
#endif
ItemPointerData;

typedef ItemPointerData *ItemPointer;



/*
 * BlockIdGetBlockNumber
 *		Retrieve the block number from a block identifier.
 */
#define BlockIdGetBlockNumber(blockId) \
( \
	(BlockNumber) (((blockId)->bi_hi << 16) | ((uint16) (blockId)->bi_lo)) \
)


/*
 * ItemPointerGetBlockNumber
 *		Returns the block number of a disk item pointer.
 */
#define ItemPointerGetBlockNumber(pointer) \
( \
	BlockIdGetBlockNumber(&(pointer)->ip_blkid) \
)

int main() {
	ItemPointerData ip;

	return ItemPointerGetBlockNumber();
}

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] packing/alignment annotation for ItemPointerData redux

2016-10-19 Thread Greg Stark 
On Wed, Oct 19, 2016 at 5:20 PM, Tom Lane  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [COMMITTERS] packing/alignment annotation for ItemPointerData redux

2016-10-19 Thread Tom Lane 
[ moved to -hackers ]

Greg Stark  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers