Horton

Thread

Commits

  1. Bring some MSVC asserts in line with other platforms

  1. Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)

    Ranier Vilela <ranier.vf@gmail.com> — 2023-07-29T12:37:19Z

    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

[1]
https://learn.microsoft.com/en-us/cpp/intrinsics/bitscanreverse-bitscanreverse64?view=msvc-170

  2. Re: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)

    John Naylor <john.naylor@enterprisedb.com> — 2023-07-30T08:07:25Z

    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

  3. Re: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-07-30T14:45:43Z

    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

  4. Re: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)

    John Naylor <john.naylor@enterprisedb.com> — 2023-07-31T07:56:15Z

    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

  5. Re: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-07-31T10:57:48Z

    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

  6. Re: Avoid undefined behavior with msvc compiler (src/include/port/pg_bitutils.h)

    John Naylor <john.naylor@enterprisedb.com> — 2023-07-31T11:55:16Z

    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.

--
John Naylor
EDB: http://www.enterprisedb.com