Thread: 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) [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. James Coleman 1: https://www.postgresql.org/message-id/Yk6UgCGlZKuxRr4n%40paquier.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
Attachment
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
Attachment
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
Attachment
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
Attachment
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.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. 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
Attachment
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
Attachment
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