Support pg_attribute_aligned and noreturn in MSVC
Over in the "Add last commit LSN to pg_last_committed_xact()" [1]
thread this patch had been added as a precursor, but Michael Paquier
suggested it be broken out separately, so I'm doing that here.
It turns out that MSVC supports both noreturn [2] [3] and alignment
[4]: [5] attributes, so this patch adds support for those. MSVC also supports a form of packing, but the implementation as I can tell requires wrapping the entire struct (with a push/pop declaration set)
supports a form of packing, but the implementation as I can tell
requires wrapping the entire struct (with a push/pop declaration set)
[6]: , which doesn't seem to match the style of macros we're using for packing in other compilers, so I opted not to implement that attribute.
packing in other compilers, so I opted not to implement that
attribute.
James Coleman
1: /messages/by-id/Yk6UgCGlZKuxRr4n@paquier.xyz
2: 2008+ https://learn.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2008/k6ktzx3s(v=vs.90)
3. 2015+ https://learn.microsoft.com/en-us/cpp/c-language/noreturn?view=msvc-140
4. 2008+ https://learn.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2008/dabb5z75(v=vs.90)
5. 2015+ https://learn.microsoft.com/en-us/cpp/cpp/align-cpp?view=msvc-170
6. https://learn.microsoft.com/en-us/cpp/preprocessor/pack?view=msvc-170
Attachments:
v1-0001-Support-pg_attribute_aligned-and-noretur-in-MSVC.patchapplication/octet-stream; name=v1-0001-Support-pg_attribute_aligned-and-noretur-in-MSVC.patchDownload
From cc66db9ace7a9ed3d91f499a76022dc58505edf2 Mon Sep 17 00:00:00 2001
From: jcoleman <jtc331@gmail.com>
Date: Sat, 29 Jan 2022 11:28:45 -0500
Subject: [PATCH v1] Support pg_attribute_aligned and noretur in MSVC
---
config/c-compiler.m4 | 2 ++
configure | 2 ++
src/include/c.h | 8 ++++++++
3 files changed, 12 insertions(+)
diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index 69efc5bb10..000b075312 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -139,6 +139,8 @@ if test x"$pgac_cv__128bit_int" = xyes ; then
/* This must match the corresponding code in c.h: */
#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__)
#define pg_attribute_aligned(a) __attribute__((aligned(a)))
+#elif defined(_MSC_VER)
+#define pg_attribute_aligned(a) __declspec(align(a))
#endif
typedef __int128 int128a
#if defined(pg_attribute_aligned)
diff --git a/configure b/configure
index f325bd85b8..5488574c02 100755
--- a/configure
+++ b/configure
@@ -17643,6 +17643,8 @@ else
/* This must match the corresponding code in c.h: */
#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__)
#define pg_attribute_aligned(a) __attribute__((aligned(a)))
+#elif defined(_MSC_VER)
+#define pg_attribute_aligned(a) __declspec(align(a))
#endif
typedef __int128 int128a
#if defined(pg_attribute_aligned)
diff --git a/src/include/c.h b/src/include/c.h
index 101ba41331..551edb0d70 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -181,6 +181,14 @@
#define pg_attribute_noreturn() __attribute__((noreturn))
#define pg_attribute_packed() __attribute__((packed))
#define HAVE_PG_ATTRIBUTE_NORETURN 1
+/*
+ * MSVC supports aligned and noreturn
+ * Packing is also possible but only by wrapping the entire struct definition
+ * which doesn't fit into our current macro declarations.
+ */
+#elif defined(_MSC_VER)
+#define pg_attribute_aligned(a) __declspec(align(a))
+#define pg_attribute_noreturn() __declspec(noreturn)
#else
/*
* NB: aligned and packed are not given default definitions because they
--
2.32.1 (Apple Git-133)
On Mon, Sep 19, 2022 at 06:21:58PM -0400, James Coleman wrote:
It turns out that MSVC supports both noreturn [2] [3] and alignment
[4] [5] attributes, so this patch adds support for those. MSVC also
supports a form of packing, but the implementation as I can tell
requires wrapping the entire struct (with a push/pop declaration set)
[6], which doesn't seem to match the style of macros we're using for
packing in other compilers, so I opted not to implement that
attribute.
Interesting. Thanks for the investigation.
+/*
+ * MSVC supports aligned and noreturn
+ * Packing is also possible but only by wrapping the entire struct definition
+ * which doesn't fit into our current macro declarations.
+ */
+#elif defined(_MSC_VER)
+#define pg_attribute_aligned(a) __declspec(align(a))
+#define pg_attribute_noreturn() __declspec(noreturn)
#else
Nit: I think that the comment should be in the elif block for Visual.pg_attribute_aligned() could be used in generic-msvc.h's
pg_atomic_uint64 as it uses now align.
Shouldn't HAVE_PG_ATTRIBUTE_NORETURN be set for the MSVC case as well?
--
Michael
On Mon, Sep 19, 2022 at 8:21 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Sep 19, 2022 at 06:21:58PM -0400, James Coleman wrote:
It turns out that MSVC supports both noreturn [2] [3] and alignment
[4] [5] attributes, so this patch adds support for those. MSVC also
supports a form of packing, but the implementation as I can tell
requires wrapping the entire struct (with a push/pop declaration set)
[6], which doesn't seem to match the style of macros we're using for
packing in other compilers, so I opted not to implement that
attribute.Interesting. Thanks for the investigation.
+/* + * MSVC supports aligned and noreturn + * Packing is also possible but only by wrapping the entire struct definition + * which doesn't fit into our current macro declarations. + */ +#elif defined(_MSC_VER) +#define pg_attribute_aligned(a) __declspec(align(a)) +#define pg_attribute_noreturn() __declspec(noreturn) #else Nit: I think that the comment should be in the elif block for Visual.
I was following the style of the comment outside the "if", but I'm not
attached to that style, so changed in this version.
pg_attribute_aligned() could be used in generic-msvc.h's
pg_atomic_uint64 as it uses now align.
Added.
Shouldn't HAVE_PG_ATTRIBUTE_NORETURN be set for the MSVC case as well?
Yes, fixed.
James Coleman
Attachments:
v2-0001-Support-pg_attribute_aligned-and-noreturn-in-MSVC.patchapplication/octet-stream; name=v2-0001-Support-pg_attribute_aligned-and-noreturn-in-MSVC.patchDownload
From dff11d4589a3eed6a36fbef1c4df2a07b231d0f3 Mon Sep 17 00:00:00 2001
From: jcoleman <jtc331@gmail.com>
Date: Sat, 29 Jan 2022 11:28:45 -0500
Subject: [PATCH v2] Support pg_attribute_aligned and noreturn in MSVC
---
config/c-compiler.m4 | 2 ++
configure | 2 ++
src/include/c.h | 9 +++++++++
src/include/port/atomics/generic-msvc.h | 2 +-
4 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index 69efc5bb10..000b075312 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -139,6 +139,8 @@ if test x"$pgac_cv__128bit_int" = xyes ; then
/* This must match the corresponding code in c.h: */
#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__)
#define pg_attribute_aligned(a) __attribute__((aligned(a)))
+#elif defined(_MSC_VER)
+#define pg_attribute_aligned(a) __declspec(align(a))
#endif
typedef __int128 int128a
#if defined(pg_attribute_aligned)
diff --git a/configure b/configure
index f325bd85b8..5488574c02 100755
--- a/configure
+++ b/configure
@@ -17643,6 +17643,8 @@ else
/* This must match the corresponding code in c.h: */
#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__)
#define pg_attribute_aligned(a) __attribute__((aligned(a)))
+#elif defined(_MSC_VER)
+#define pg_attribute_aligned(a) __declspec(align(a))
#endif
typedef __int128 int128a
#if defined(pg_attribute_aligned)
diff --git a/src/include/c.h b/src/include/c.h
index 101ba41331..f4568345ef 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -181,6 +181,15 @@
#define pg_attribute_noreturn() __attribute__((noreturn))
#define pg_attribute_packed() __attribute__((packed))
#define HAVE_PG_ATTRIBUTE_NORETURN 1
+#elif defined(_MSC_VER)
+/*
+ * MSVC supports aligned and noreturn
+ * Packing is also possible but only by wrapping the entire struct definition
+ * which doesn't fit into our current macro declarations.
+ */
+#define pg_attribute_aligned(a) __declspec(align(a))
+#define pg_attribute_noreturn() __declspec(noreturn)
+#define HAVE_PG_ATTRIBUTE_NORETURN 1
#else
/*
* NB: aligned and packed are not given default definitions because they
diff --git a/src/include/port/atomics/generic-msvc.h b/src/include/port/atomics/generic-msvc.h
index 1a4adfde68..f3091b9731 100644
--- a/src/include/port/atomics/generic-msvc.h
+++ b/src/include/port/atomics/generic-msvc.h
@@ -39,7 +39,7 @@ typedef struct pg_atomic_uint32
} pg_atomic_uint32;
#define PG_HAVE_ATOMIC_U64_SUPPORT
-typedef struct __declspec(align(8)) pg_atomic_uint64
+typedef struct pg_attribute_aligned(8) pg_atomic_uint64
{
volatile uint64 value;
} pg_atomic_uint64;
--
2.32.1 (Apple Git-133)
On Mon, Sep 19, 2022 at 08:51:37PM -0400, James Coleman wrote:
Yes, fixed.
The CF bot is failing compilation on Windows:
http://commitfest.cputube.org/james-coleman.html
https://api.cirrus-ci.com/v1/task/5376566577332224/logs/build.log
There is something going on with noreturn() after applying it to
elog.h:
01:11:00.468] c:\cirrus\src\include\utils\elog.h(410,45): error C2085:
'ThrowErrorData': not in formal parameter list (compiling source file
src/common/hashfn.c) [c:\cirrus\libpgcommon.vcxproj]
[01:11:00.468] c:\cirrus\src\include\mb\pg_wchar.h(701,80): error
C2085: 'pgwin32_message_to_UTF16': not in formal parameter list
(compiling source file src/common/encnames.c)
[c:\cirrus\libpgcommon.vcxproj]
[01:11:00.468] c:\cirrus\src\include\utils\elog.h(411,54): error
C2085: 'pg_re_throw': not in formal parameter list (compiling source
file src/common/hashfn.c) [c:\cirrus\libpgcommon.vcxproj]
align() seems to look fine, at least. I'd be tempted to apply the
align part first, as that's independently useful for itemptr.h.
--
Michael
On Mon, Sep 19, 2022 at 11:21 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Sep 19, 2022 at 08:51:37PM -0400, James Coleman wrote:
Yes, fixed.
The CF bot is failing compilation on Windows:
http://commitfest.cputube.org/james-coleman.html
https://api.cirrus-ci.com/v1/task/5376566577332224/logs/build.logThere is something going on with noreturn() after applying it to
elog.h:
01:11:00.468] c:\cirrus\src\include\utils\elog.h(410,45): error C2085:
'ThrowErrorData': not in formal parameter list (compiling source file
src/common/hashfn.c) [c:\cirrus\libpgcommon.vcxproj]
[01:11:00.468] c:\cirrus\src\include\mb\pg_wchar.h(701,80): error
C2085: 'pgwin32_message_to_UTF16': not in formal parameter list
(compiling source file src/common/encnames.c)
[c:\cirrus\libpgcommon.vcxproj]
[01:11:00.468] c:\cirrus\src\include\utils\elog.h(411,54): error
C2085: 'pg_re_throw': not in formal parameter list (compiling source
file src/common/hashfn.c) [c:\cirrus\libpgcommon.vcxproj]align() seems to look fine, at least. I'd be tempted to apply the
align part first, as that's independently useful for itemptr.h.
I don't have access to a Windows machine for testing, but re-reading
the documentation it looks like the issue is that our noreturn macro
is used after the definition while the MSVC equivalent is used before.
I've removed that for now (and commented about it); it's not as
valuable anyway since it's mostly an indicator for code analysis
(human and machine).
James Coleman
Attachments:
v3-0001-Support-pg_attribute_aligned-in-MSVC.patchapplication/octet-stream; name=v3-0001-Support-pg_attribute_aligned-in-MSVC.patchDownload
From 327597738ae4315a0af858c14b7d0e8d1764ea9a Mon Sep 17 00:00:00 2001
From: jcoleman <jtc331@gmail.com>
Date: Sat, 29 Jan 2022 11:28:45 -0500
Subject: [PATCH v3] Support pg_attribute_aligned in MSVC
---
config/c-compiler.m4 | 2 ++
configure | 2 ++
src/include/c.h | 12 ++++++++++++
src/include/port/atomics/generic-msvc.h | 2 +-
4 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index 69efc5bb10..000b075312 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -139,6 +139,8 @@ if test x"$pgac_cv__128bit_int" = xyes ; then
/* This must match the corresponding code in c.h: */
#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__)
#define pg_attribute_aligned(a) __attribute__((aligned(a)))
+#elif defined(_MSC_VER)
+#define pg_attribute_aligned(a) __declspec(align(a))
#endif
typedef __int128 int128a
#if defined(pg_attribute_aligned)
diff --git a/configure b/configure
index f325bd85b8..5488574c02 100755
--- a/configure
+++ b/configure
@@ -17643,6 +17643,8 @@ else
/* This must match the corresponding code in c.h: */
#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__)
#define pg_attribute_aligned(a) __attribute__((aligned(a)))
+#elif defined(_MSC_VER)
+#define pg_attribute_aligned(a) __declspec(align(a))
#endif
typedef __int128 int128a
#if defined(pg_attribute_aligned)
diff --git a/src/include/c.h b/src/include/c.h
index 101ba41331..9f3122b4ab 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -181,6 +181,18 @@
#define pg_attribute_noreturn() __attribute__((noreturn))
#define pg_attribute_packed() __attribute__((packed))
#define HAVE_PG_ATTRIBUTE_NORETURN 1
+#elif defined(_MSC_VER)
+/*
+ * MSVC supports aligned
+ *
+ * noreturn is also possible but in MSVC is declared before the definition
+ * while our pg_attribute_noreturn() macro is currently used after
+ * the definition.
+ *
+ * Packing is also possible but only by wrapping the entire struct definition
+ * which doesn't fit into our current macro declarations.
+ */
+#define pg_attribute_aligned(a) __declspec(align(a))
#else
/*
* NB: aligned and packed are not given default definitions because they
diff --git a/src/include/port/atomics/generic-msvc.h b/src/include/port/atomics/generic-msvc.h
index 1a4adfde68..f3091b9731 100644
--- a/src/include/port/atomics/generic-msvc.h
+++ b/src/include/port/atomics/generic-msvc.h
@@ -39,7 +39,7 @@ typedef struct pg_atomic_uint32
} pg_atomic_uint32;
#define PG_HAVE_ATOMIC_U64_SUPPORT
-typedef struct __declspec(align(8)) pg_atomic_uint64
+typedef struct pg_attribute_aligned(8) pg_atomic_uint64
{
volatile uint64 value;
} pg_atomic_uint64;
--
2.32.1 (Apple Git-133)
On Tue, Sep 20, 2022 at 08:01:20AM -0400, James Coleman wrote:
I don't have access to a Windows machine for testing, but re-reading
the documentation it looks like the issue is that our noreturn macro
is used after the definition while the MSVC equivalent is used before.
A CI setup would do the job for example, see src/tools/ci/README that
explains how to set up things.
I've removed that for now (and commented about it); it's not as
valuable anyway since it's mostly an indicator for code analysis
(human and machine).
Except for the fact that the patch missed to define
pg_attribute_noreturn() in the MSVC branch, this looks fine to me. I
have been looking at what you meant with packing, and I can see the
business with PACK(), something actually doable with gcc.
That's a first step, at least, and I see no reason not to do it, so
applied.
--
Michael
On Tue, Sep 20, 2022 at 9:18 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Sep 20, 2022 at 08:01:20AM -0400, James Coleman wrote:
I don't have access to a Windows machine for testing, but re-reading
the documentation it looks like the issue is that our noreturn macro
is used after the definition while the MSVC equivalent is used before.A CI setup would do the job for example, see src/tools/ci/README that
explains how to set up things.
That's a good reminder; I've been meaning to set that up but haven't
taken the time yet.
I've removed that for now (and commented about it); it's not as
valuable anyway since it's mostly an indicator for code analysis
(human and machine).Except for the fact that the patch missed to define
pg_attribute_noreturn() in the MSVC branch, this looks fine to me. I
have been looking at what you meant with packing, and I can see the
business with PACK(), something actually doable with gcc.That's a first step, at least, and I see no reason not to do it, so
applied.
Thanks!
James Coleman