Thread: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)
Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)
From
Ranier Vilela
Date:
Hi,
The pg_leftmost_one_pos32 function with msvc compiler,
relies on Assert to guarantee the correct result.
But msvc documentation [1] says clearly that
undefined behavior can occur.
Fix by checking the result of the function to avoid
a possible undefined behavior.
patch attached.
best regards,
Ranier Vilela
Attachment
Re: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)
From
John Naylor
Date:
On Sat, Jul 29, 2023 at 7:37 PM Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Hi,
>
> The pg_leftmost_one_pos32 function with msvc compiler,
> relies on Assert to guarantee the correct result.
>
> But msvc documentation [1] says clearly that
> undefined behavior can occur.
It seems that we should have "Assert(word != 0);" at the top, which matches the other platforms anyway, so I'll add that.
> Fix by checking the result of the function to avoid
> a possible undefined behavior.
No other platform does this, and that is by design. I'm also not particularly impressed by the unrelated cosmetic changes.
--
John Naylor
EDB: http://www.enterprisedb.com
Re: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)
From
Tom Lane
Date:
John Naylor <john.naylor@enterprisedb.com> writes:
> It seems that we should have "Assert(word != 0);" at the top, which matches
> the other platforms anyway, so I'll add that.
That's basically equivalent to the existing Assert(non_zero).
I think it'd be okay to drop that one and instead have
the same Assert condition as other platforms, but having both
would be redundant.
I agree that adding the non-Assert test that Ranier wants
is entirely pointless. If a caller did manage to violate
the asserted-for condition (and we don't have asserts on),
returning zero is not better than returning an unspecified
value. If anything it might be worse, since it might not
lead to obvious misbehavior.
On the whole, there's not anything wrong with the code as-is.
A case could be made for making the MSVC asserts more like
other platforms, but it's quite cosmetic.
regards, tom lane
Re: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)
From
John Naylor
Date:
On Sun, Jul 30, 2023 at 9:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> John Naylor <john.naylor@enterprisedb.com> writes:
> > It seems that we should have "Assert(word != 0);" at the top, which matches
> > the other platforms anyway, so I'll add that.
>
> That's basically equivalent to the existing Assert(non_zero).
> I think it'd be okay to drop that one and instead have
> the same Assert condition as other platforms, but having both
> would be redundant.
Works for me, so done that way for both forward and reverse variants. Since the return value is no longer checked in any builds, I thought about removing the variable containing it, but it seems best to leave it behind for clarity since these are not our functions.
--
John Naylor
EDB: http://www.enterprisedb.com
Re: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)
From
Tom Lane
Date:
John Naylor <john.naylor@enterprisedb.com> writes:
> On Sun, Jul 30, 2023 at 9:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> That's basically equivalent to the existing Assert(non_zero).
>> I think it'd be okay to drop that one and instead have
>> the same Assert condition as other platforms, but having both
>> would be redundant.
> Works for me, so done that way for both forward and reverse variants. Since
> the return value is no longer checked in any builds, I thought about
> removing the variable containing it, but it seems best to leave it behind
> for clarity since these are not our functions.
Hmm, aren't you risking "variable is set but not used" warnings?
Personally I'd have made these like
(void) _BitScanReverse(&result, word);
Anybody trying to understand this code is going to have to look up
the man page for _BitScanReverse anyway, so I'm not sure that
keeping the variable adds much readability.
However, if no version of MSVC actually issues such a warning,
it's moot.
regards, tom lane
Re: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)
From
John Naylor
Date:
On Mon, Jul 31, 2023 at 5:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> John Naylor <john.naylor@enterprisedb.com> writes:
> > Works for me, so done that way for both forward and reverse variants. Since
> > the return value is no longer checked in any builds, I thought about
> > removing the variable containing it, but it seems best to leave it behind
> > for clarity since these are not our functions.
>
> Hmm, aren't you risking "variable is set but not used" warnings?
> Personally I'd have made these like
>
> (void) _BitScanReverse(&result, word);
I'd reasoned that such a warning would have showed up in non-assert builds already, but I neglected to consider that those could have different warning settings. For the time being drongo shows green, at least, but we'll see.
> > the return value is no longer checked in any builds, I thought about
> > removing the variable containing it, but it seems best to leave it behind
> > for clarity since these are not our functions.
>
> Hmm, aren't you risking "variable is set but not used" warnings?
> Personally I'd have made these like
>
> (void) _BitScanReverse(&result, word);
I'd reasoned that such a warning would have showed up in non-assert builds already, but I neglected to consider that those could have different warning settings. For the time being drongo shows green, at least, but we'll see.