Обсуждение: Windows build warnings
Hi, I'm seeing the following annoying build warnings on Windows (without asserts, latest Postgres source): pruneheap.c(858): warning C4101: 'htup': unreferenced local variable pruneheap.c(870): warning C4101: 'tolp': unreferenced local variable I notice that these are also reported here: [1] I've attached a patch to fix these warnings. (Note that currently PG_USED_FOR_ASSERTS_ONLY is defined as the unused attribute, which is only supported by GCC) [1]: https://www.postgresql.org/message-id/CAH2-WznwWU+9on9nZCnZtk7uA238MCTgPxYr1Ty7U_Msn5ZGwQ@mail.gmail.com Regards, Greg Nancarrow Fujitsu Australia
Вложения
> On 22 Nov 2021, at 12:10, Greg Nancarrow <gregn4422@gmail.com> wrote: > I've attached a patch to fix these warnings. LGTM. -- Daniel Gustafsson https://vmware.com/
On 2021-Nov-22, Daniel Gustafsson wrote: > > On 22 Nov 2021, at 12:10, Greg Nancarrow <gregn4422@gmail.com> wrote: > > > I've attached a patch to fix these warnings. > > LGTM. .. but see https://postgr.es/m/CAH2-WznwWU+9on9nZCnZtk7uA238MCTgPxYr1Ty7U_Msn5ZGwQ@mail.gmail.com where this was already discussed. I think if we're going to workaround PG_USED_FOR_ASSERTS_ONLY not actually working, we may as well get rid of it entirely. My preference would be to fix it so that it works on more platforms (at least Windows in addition to GCC). -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ "In Europe they call me Niklaus Wirth; in the US they call me Nickel's worth. That's because in Europe they call me by name, and in the US by value!"
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > .. but see > https://postgr.es/m/CAH2-WznwWU+9on9nZCnZtk7uA238MCTgPxYr1Ty7U_Msn5ZGwQ@mail.gmail.com > where this was already discussed. I think if we're going to workaround > PG_USED_FOR_ASSERTS_ONLY not actually working, we may as well get rid of > it entirely. My preference would be to fix it so that it works on more > platforms (at least Windows in addition to GCC). Yeah, I do not think there is a reason to change the code if it's using PG_USED_FOR_ASSERTS_ONLY properly. We should either make that macro work on $compiler, or ignore such warnings from $compiler. regards, tom lane
> On 22 Nov 2021, at 16:06, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: >> .. but see >> https://postgr.es/m/CAH2-WznwWU+9on9nZCnZtk7uA238MCTgPxYr1Ty7U_Msn5ZGwQ@mail.gmail.com >> where this was already discussed. I think if we're going to workaround >> PG_USED_FOR_ASSERTS_ONLY not actually working, we may as well get rid of >> it entirely. My preference would be to fix it so that it works on more >> platforms (at least Windows in addition to GCC). > > Yeah, I do not think there is a reason to change the code if it's using > PG_USED_FOR_ASSERTS_ONLY properly. We should either make that macro > work on $compiler, or ignore such warnings from $compiler. Fair enough. Looking at where we use PG_USED_FOR_ASSERTS_ONLY (and where it works), these two warnings are the only places where we apply it to a pointer typedef (apart from one place where the variable is indeed used outside of asserts). Since it clearly works in all other cases, I wonder if something like the below sketch could make MSVC handle the attribute? diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 5c0b60319d..9701c9eba0 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -855,7 +855,7 @@ heap_page_prune_execute(Buffer buffer, { Page page = (Page) BufferGetPage(buffer); OffsetNumber *offnum; - HeapTupleHeader htup PG_USED_FOR_ASSERTS_ONLY; + HeapTupleHeaderData *htup PG_USED_FOR_ASSERTS_ONLY; /* Shouldn't be called unless there's something to do */ Assert(nredirected > 0 || ndead > 0 || nunused > 0); @@ -867,7 +867,7 @@ heap_page_prune_execute(Buffer buffer, OffsetNumber fromoff = *offnum++; OffsetNumber tooff = *offnum++; ItemId fromlp = PageGetItemId(page, fromoff); - ItemId tolp PG_USED_FOR_ASSERTS_ONLY; + ItemIdData *tolp PG_USED_FOR_ASSERTS_ONLY; #ifdef USE_ASSERT_CHECKING -- Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
> Fair enough. Looking at where we use PG_USED_FOR_ASSERTS_ONLY (and where it
> works), these two warnings are the only places where we apply it to a pointer
> typedef (apart from one place where the variable is indeed used outside of
> asserts). Since it clearly works in all other cases, I wonder if something
> like the below sketch could make MSVC handle the attribute?
Ugh. If we're changing the code anyway, I think I prefer Greg's original
patch; it's at least not randomly inconsistent with everything else.
However ... I question your assumption that it works everywhere else.
I can't find anything that is providing a non-empty definition of
PG_USED_FOR_ASSERTS_ONLY (a/k/a pg_attribute_unused) for anything
except GCC. It seems likely to me that MSVC simply fails to produce
such warnings in most places, but it's woken up and done so here.
regards, tom lane
> On 22 Nov 2021, at 16:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> Fair enough. Looking at where we use PG_USED_FOR_ASSERTS_ONLY (and where it
>> works), these two warnings are the only places where we apply it to a pointer
>> typedef (apart from one place where the variable is indeed used outside of
>> asserts). Since it clearly works in all other cases, I wonder if something
>> like the below sketch could make MSVC handle the attribute?
>
> Ugh. If we're changing the code anyway, I think I prefer Greg's original
> patch; it's at least not randomly inconsistent with everything else.
Totally agree.
> However ... I question your assumption that it works everywhere else.
Right, I wasn't expressing myself well uncaffeinated; I meant that it doesn't
emit a warning for the other instances, which I agree isn't the same thing as
the warning avoidance working.
> I can't find anything that is providing a non-empty definition of
> PG_USED_FOR_ASSERTS_ONLY (a/k/a pg_attribute_unused) for anything
> except GCC.
It's supported in clang as well per the documentation [0] in at least some
configurations or distributions:
"The [[maybe_unused]] (or __attribute__((unused))) attribute can be
used to silence such diagnostics when the entity cannot be removed.
For instance, a local variable may exist solely for use in an assert()
statement, which makes the local variable unused when NDEBUG is
defined."
> It seems likely to me that MSVC simply fails to produce
> such warnings in most places, but it's woken up and done so here.
Looking at the instances of PG_USED_FOR_ASSERTS_ONLY in the tree, every such
variable is set (although not read) either at instantiation or later in the
codepath *outside* of the Assertion. The documentation for C4101 [1] isn't
exactly verbose, but the examples therein show it for variables not set at all
so it might be that it simply only warns for pruneheap.c since all other cases
are at least set. Setting the variables in question to NULL as a test, instead
of marking them unused, the build pass through MSVC without any warnings. We
might still want to use the original patch in this thread, but it seems that
this might at least hint at explaining why MSVC only emitted warnings for those
two.
While skimming the variables marked as unused, I noticed that two of them seems
to be marked as such while being used in non-assert codepaths, see the attached
diff. The one in postgres_fdw.c was added in 8998e3cafa2 with 1ec7fca8592
using the variable; in lsyscache.c it was introduced in 2a6368343ff and then
promptly used in the fix commit 7e041603904 shortly thereafter. Any objections
to applying that?
--
Daniel Gustafsson https://vmware.com/
[0] https://clang.llvm.org/docs/AttributeReference.html#maybe-unused-unused
[1] https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4101
Вложения
On Tue, Nov 23, 2021 at 2:11 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> On 22 Nov 2021, at 16:40, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I can't find anything that is providing a non-empty definition of
> PG_USED_FOR_ASSERTS_ONLY (a/k/a pg_attribute_unused) for anything
> except GCC.
It's supported in clang as well per the documentation [0] in at least some
configurations or distributions:
"The [[maybe_unused]] (or __attribute__((unused))) attribute can be
used to silence such diagnostics when the entity cannot be removed.
For instance, a local variable may exist solely for use in an assert()
statement, which makes the local variable unused when NDEBUG is
defined."
[[maybe_unused]] is also recognized from Visual Studio 2017 onwards [1].
Regards,
Juan José Santamaría Flecha
On 2021-Nov-23, Juan José Santamaría Flecha wrote: > On Tue, Nov 23, 2021 at 2:11 PM Daniel Gustafsson <daniel@yesql.se> wrote: > > It's supported in clang as well per the documentation [0] in at least some > > configurations or distributions: > [[maybe_unused]] is also recognized from Visual Studio 2017 onwards [1]. > > [1] https://docs.microsoft.com/en-us/cpp/cpp/attributes?view=msvc-170 Right ... the problem, as I understand, is that the syntax for [[maybe_unused]] is different from what we can do with the current pg_attribute_unused -- [[maybe_unused]] goes before the variable name. We would need to define pg_attribute_unused macro (maybe have it take the variable name and initializator value as arguments?), and also define PG_USED_FOR_ASSERTS_ONLY in the same style. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Right ... the problem, as I understand, is that the syntax for
> [[maybe_unused]] is different from what we can do with the current
> pg_attribute_unused -- [[maybe_unused]] goes before the variable name.
> We would need to define pg_attribute_unused macro (maybe have it take
> the variable name and initializator value as arguments?), and also
> define PG_USED_FOR_ASSERTS_ONLY in the same style.
I've thought all along that PG_USED_FOR_ASSERTS_ONLY was making
unwarranted assumptions about what the underlying syntax would be,
and it seems I was right. Anyone want to look into what it'd take
to change this?
(It might be an idea to introduce a new macro with a slightly
different name, so we don't have to touch every usage site
right away.)
regards, tom lane
On Wed, Nov 24, 2021 at 1:41 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > On 2021-Nov-23, Juan José Santamaría Flecha wrote: > > > On Tue, Nov 23, 2021 at 2:11 PM Daniel Gustafsson <daniel@yesql.se> wrote: > > > > It's supported in clang as well per the documentation [0] in at least some > > > configurations or distributions: > > > [[maybe_unused]] is also recognized from Visual Studio 2017 onwards [1]. > > > > [1] https://docs.microsoft.com/en-us/cpp/cpp/attributes?view=msvc-170 > > Right ... the problem, as I understand, is that the syntax for > [[maybe_unused]] is different from what we can do with the current > pg_attribute_unused -- [[maybe_unused]] goes before the variable name. > We would need to define pg_attribute_unused macro (maybe have it take > the variable name and initializator value as arguments?), and also > define PG_USED_FOR_ASSERTS_ONLY in the same style. > Isn't "[[maybe_unused]]" only supported for MS C++ (not C)? I'm using Visual Studio 17, and I get nothing but a syntax error if trying to use it in C code, whereas it works if I rename the same source file to have a ".cpp" extension (but even then I need to use the "/std:c++17" compiler flag) Regards, Greg Nancarrow Fujitsu Australia
Daniel Gustafsson <daniel@yesql.se> writes: > On 22 Nov 2021, at 16:40, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> I can't find anything that is providing a non-empty definition of >> PG_USED_FOR_ASSERTS_ONLY (a/k/a pg_attribute_unused) for anything >> except GCC. > > It's supported in clang as well per the documentation [0] in at least some > configurations or distributions: > > "The [[maybe_unused]] (or __attribute__((unused))) attribute can be > used to silence such diagnostics when the entity cannot be removed. > For instance, a local variable may exist solely for use in an assert() > statement, which makes the local variable unused when NDEBUG is > defined." Should we change the compiler checks for attributes in c.h to include `|| __has_attribute(…)`, so that we automatically get them on compilers that support that (particularly clang)? - ilmari
=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> Should we change the compiler checks for attributes in c.h to include
> `|| __has_attribute(…)`, so that we automatically get them on compilers
> that support that (particularly clang)?
clang already #defines GCC, no?
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes: >> Should we change the compiler checks for attributes in c.h to include >> `|| __has_attribute(…)`, so that we automatically get them on compilers >> that support that (particularly clang)? > > clang already #defines GCC, no? __GNUC__, but yes, I didn't realise that. Clang 11 seems to claim to be GCC 4.2 by default, but that can be overridden usng the -fgnuc-version (and turned off by setting it to zero). Do any other compilers support __has_attribute()? > regards, tom lane - ilmari
> On 22 Nov 2021, at 16:06, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: >> .. but see >> https://postgr.es/m/CAH2-WznwWU+9on9nZCnZtk7uA238MCTgPxYr1Ty7U_Msn5ZGwQ@mail.gmail.com >> where this was already discussed. I think if we're going to workaround >> PG_USED_FOR_ASSERTS_ONLY not actually working, we may as well get rid of >> it entirely. My preference would be to fix it so that it works on more >> platforms (at least Windows in addition to GCC). > > Yeah, I do not think there is a reason to change the code if it's using > PG_USED_FOR_ASSERTS_ONLY properly. We should either make that macro > work on $compiler, or ignore such warnings from $compiler. So, to reach some conclusion on this thread; it seems the code is using PG_USED_FOR_ASSERTS_ONLY - as it's currently implemented - properly, but it doesn't work on MSVC and isn't likely to work in the shape it is today. Reworking to support a wider range of compilers will also likely mean net new code. To silence the warnings in the meantime (if the rework at all happens) we should either apply the patch from Greg or add C4101 to disablewarnings in src/tools/msvc/Project.pm as mentioned above. On top of that, we should apply the patch I proposed downthread to remove PG_USED_FOR_ASSERTS_ONLY where it's no longer applicable. Personally I'm fine with either, and am happy to make it happen, once we agree on what it should be. Thoughts? -- Daniel Gustafsson https://vmware.com/
On Thu, Nov 25, 2021 at 11:03 PM Daniel Gustafsson <daniel@yesql.se> wrote: > > To silence the warnings in the meantime (if the rework at all happens) we > should either apply the patch from Greg or add C4101 to disablewarnings in > src/tools/msvc/Project.pm as mentioned above. On top of that, we should apply > the patch I proposed downthread to remove PG_USED_FOR_ASSERTS_ONLY where it's > no longer applicable. Personally I'm fine with either, and am happy to make it > happen, once we agree on what it should be. > AFAICS, the fundamental difference here seems to be that the GCC compiler still regards a variable as "unused" if it is never read, whereas if the variable is set (but not necessarily read) that's enough for the Windows C compiler to regard it as "used". This is why, at least for the majority of cases, why we're not seeing the C4101 warnings on Windows where PG_USED_FOR_ASSERTS_ONLY has been used in the Postgres source, because in those cases the variable has been set prior its use in an Assert or "#ifdef USE_ASSERT_CHECKING" block. IMO, for the case in point, it's best to fix it by either setting the variables to NULL, prior to their use in the "#ifdef USE_ASSERT_CHECKING" block, or by applying my patch. Of course, this doesn't address fixing the PG_USED_ONLY_FOR_ASSERTS macro to work on Windows, but I don't see an easy way forward on that if it's to remain in its "variable attribute" form, and in any case the Windows C compiler doesn't seem to support any annotation to mark a variable as potentially unused. Personally I'm not really in favour of outright disabling the C4101 warning on Windows, because I think it is a useful warning for Postgres developers on Windows for cases unrelated to the use of PG_USED_FOR_ASSERTS_ONLY. I agree with your proposal to apply your patch to remove PG_USED_FOR_ASSERTS_ONLY where it's no longer applicable. Regards, Greg Nancarrow Fujitsu Australia
Greg Nancarrow <gregn4422@gmail.com> writes:
> AFAICS, the fundamental difference here seems to be that the GCC
> compiler still regards a variable as "unused" if it is never read,
> whereas if the variable is set (but not necessarily read) that's
> enough for the Windows C compiler to regard it as "used".
It depends. Older gcc versions don't complain about set-but-not-read
variables, but clang has done so for awhile (with a specific warning
message about the case), and I think recent gcc follows suit.
> Personally I'm not really in favour of outright disabling the C4101
> warning on Windows, because I think it is a useful warning for
> Postgres developers on Windows for cases unrelated to the use of
> PG_USED_FOR_ASSERTS_ONLY.
IMO we should either do that or do whatever's necessary to make the
macro work properly on MSVC. I'm not very much in favor of jumping
through hoops to satisfy a compiler that has a randomly-different-
but-still-demonstrably-inadequate version of this warning.
regards, tom lane
> On 26 Nov 2021, at 05:45, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Personally I'm not really in favour of outright disabling the C4101 >> warning on Windows, because I think it is a useful warning for >> Postgres developers on Windows for cases unrelated to the use of >> PG_USED_FOR_ASSERTS_ONLY. I'm not sure I find it useful, as the only reason I *think* I know what it's doing is through trial and error. The only warnings we get from a tree where PG_USED_FOR_ASSERTS_ONLY clearly does nothing, are quite uninteresting and fixing them only amounts to silencing the compiler and not improving the code. > IMO we should either do that or do whatever's necessary to make the > macro work properly on MSVC. I'm not very much in favor of jumping > through hoops to satisfy a compiler that has a randomly-different- > but-still-demonstrably-inadequate version of this warning. Since there is no equivalent attribute in MSVC ([[maybe_unused]] being a C++17 feature) I propose that we silence the warning. If someone comes along with an implementation of PG_USED_FOR_ASSERTS_ONLY we can always revert that then. -- Daniel Gustafsson https://vmware.com/
On 11/26/21 04:12, Daniel Gustafsson wrote: >> On 26 Nov 2021, at 05:45, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Personally I'm not really in favour of outright disabling the C4101 >>> warning on Windows, because I think it is a useful warning for >>> Postgres developers on Windows for cases unrelated to the use of >>> PG_USED_FOR_ASSERTS_ONLY. > I'm not sure I find it useful, as the only reason I *think* I know what it's > doing is through trial and error. The only warnings we get from a tree where > PG_USED_FOR_ASSERTS_ONLY clearly does nothing, are quite uninteresting and > fixing them only amounts to silencing the compiler and not improving the code. > I agree with Tom. I don't think we should disable the warning. If we can't come up with a reasonable implementation of PG_USED_FOR_ASSERTS_ONLY that works with MSVC we should just live with the warnings. It's not like we get flooded with them. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 11/26/21 04:12, Daniel Gustafsson wrote:
>> On 26 Nov 2021, at 05:45, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> Personally I'm not really in favour of outright disabling the C4101
>>>> warning on Windows, because I think it is a useful warning for
>>>> Postgres developers on Windows for cases unrelated to the use of
>>>> PG_USED_FOR_ASSERTS_ONLY.
[ FTR, that text is not mine; somebody messed up the attribution ]
> I agree with Tom. I don't think we should disable the warning. If we
> can't come up with a reasonable implementation of
> PG_USED_FOR_ASSERTS_ONLY that works with MSVC we should just live with
> the warnings. It's not like we get flooded with them.
I think our policy is to suppress unused-variable warnings if they
appear on current mainstream compilers; and it feels a little churlish
to deem MSVC non-mainstream. So I stick with my previous suggestion,
which basically was to disable C4101 until such time as somebody can
make PG_USED_FOR_ASSERTS_ONLY work correctly on MSVC. In the worst
case, that might lead a Windows-based developer to submit a patch that
draws warnings elsewhere ... but the cfbot, other developers, or the
buildfarm will find such problems soon enough.
regards, tom lane
> On 26 Nov 2021, at 20:33, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Andrew Dunstan <andrew@dunslane.net> writes: >> On 11/26/21 04:12, Daniel Gustafsson wrote: >>> On 26 Nov 2021, at 05:45, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>>> Personally I'm not really in favour of outright disabling the C4101 >>>>> warning on Windows, because I think it is a useful warning for >>>>> Postgres developers on Windows for cases unrelated to the use of >>>>> PG_USED_FOR_ASSERTS_ONLY. > > [ FTR, that text is not mine; somebody messed up the attribution ] That was probably me fat-fingering it, sorry. >> I agree with Tom. I don't think we should disable the warning. If we >> can't come up with a reasonable implementation of >> PG_USED_FOR_ASSERTS_ONLY that works with MSVC we should just live with >> the warnings. It's not like we get flooded with them. > > I think our policy is to suppress unused-variable warnings if they > appear on current mainstream compilers; and it feels a little churlish > to deem MSVC non-mainstream. So I stick with my previous suggestion, > which basically was to disable C4101 until such time as somebody can > make PG_USED_FOR_ASSERTS_ONLY work correctly on MSVC. In the worst > case, that might lead a Windows-based developer to submit a patch that > draws warnings elsewhere ... but the cfbot, other developers, or the > buildfarm will find such problems soon enough. I agree with that, and can go make that happen. -- Daniel Gustafsson https://vmware.com/
On 11/26/21 15:14, Daniel Gustafsson wrote: >> On 26 Nov 2021, at 20:33, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >> I think our policy is to suppress unused-variable warnings if they >> appear on current mainstream compilers; and it feels a little churlish >> to deem MSVC non-mainstream. So I stick with my previous suggestion, >> which basically was to disable C4101 until such time as somebody can >> make PG_USED_FOR_ASSERTS_ONLY work correctly on MSVC. In the worst >> case, that might lead a Windows-based developer to submit a patch that >> draws warnings elsewhere ... but the cfbot, other developers, or the >> buildfarm will find such problems soon enough. > I agree with that, and can go make that happen. > [trust I have attributions right] ISTM the worst case is that there will be undetected unused variables in Windows-only code. I guess that would mostly be detected by Msys systems running gcc. Anyway I don't think it's worth arguing a lot about. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
> On 27 Nov 2021, at 14:55, Andrew Dunstan <andrew@dunslane.net> wrote: > ISTM the worst case is that there will be undetected unused variables in > Windows-only code. I guess that would mostly be detected by Msys systems > running gcc. Yes, that should be caught there. I've applied this now together with the removal of PG_USED_FOR_ASSERTS_ONLY on those variables where it was set on variables in general use. -- Daniel Gustafsson https://vmware.com/