Horton

Thread

Commits

  1. Suppress compiler warning in relptr_store().

  1. Pointer subtraction with a null pointer

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-26T16:04:54Z

    Several of Andres' buildfarm animals have recently started to whine
that "performing pointer subtraction with a null pointer has undefined
behavior" for assorted places in freepage.c.

From a mathematical standpoint, this astonishes me: "x - 0 = x" is a
tautology.  So I'm a bit inclined to say "you're full of it" and disable
-Wnull-pointer-subtraction.  On the other hand, all of the occurrences
are in calls of relptr_store with a constant-NULL third argument.
So we could silence them without too much pain by adjusting that macro
to special-case NULL.  Or maybe we should change these call sites to do
something different, because this is surely abusing the intent of
relptr_store.

Thoughts?

			regards, tom lane

  2. Re: Pointer subtraction with a null pointer

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-26T16:13:12Z

    I wrote:
> Several of Andres' buildfarm animals have recently started to whine
> that "performing pointer subtraction with a null pointer has undefined
> behavior" for assorted places in freepage.c.

Ah, sorry, I meant to include a link:

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=mylodon&dt=2022-03-26%2000%3A02%3A10&stg=make

This code is old, but mylodon wasn't doing that a week ago, so
Andres must've updated the compiler and/or changed its options.
kestrel and olingo are reporting it too, but they're new.

			regards, tom lane

  3. Re: Pointer subtraction with a null pointer

    Andres Freund <andres@anarazel.de> — 2022-03-26T16:24:18Z

    Hi,

On 2022-03-26 12:04:54 -0400, Tom Lane wrote:
> Several of Andres' buildfarm animals have recently started to whine
> that "performing pointer subtraction with a null pointer has undefined
> behavior" for assorted places in freepage.c.
>
> From a mathematical standpoint, this astonishes me: "x - 0 = x" is a
> tautology.

I don't think that's quite what the warning is warning about. The C standard
doesn't allow pointer arithmetic between arbitrary pointers, they have to be
to the same "object" (plus a trailing array element).

http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf 6.5.6 Additive
operators, 8/9

  When two pointers are subtracted, both shall point to elements of the same array object,
  or one past the last element of the array object; the result is the difference of the
  subscripts of the two array elements.

NULL can never be part of the same "array object" or one past past the last
element as the pointer it is subtracted from. Hence the undefined beaviour.


> Or maybe we should change these call sites to do something different,
> because this is surely abusing the intent of relptr_store.

I think a relptr_zero(), relptr_setnull() or such would make sense. That'd get
rid of the need for the cast as well.

Greetings,

Andres Freund

  4. Re: Pointer subtraction with a null pointer

    Isaac Morland <isaac.morland@gmail.com> — 2022-03-26T16:34:10Z

    On Sat, 26 Mar 2022 at 12:24, Andres Freund <andres@anarazel.de> wrote:


> NULL can never be part of the same "array object" or one past past the last
> element as the pointer it is subtracted from. Hence the undefined beaviour.
>

Even more fundamentally, NULL is not 0 in any ordinary mathematical sense,
even though it can be written 0 in source code and is often (but not
always) represented in memory as an all-0s bit pattern. I'm not at all
surprised to learn that arithmetic involving NULL is undefined.

  5. Re: Pointer subtraction with a null pointer

    Andres Freund <andres@anarazel.de> — 2022-03-26T16:34:41Z

    Hi,

On 2022-03-26 12:13:12 -0400, Tom Lane wrote:
> This code is old, but mylodon wasn't doing that a week ago, so
> Andres must've updated the compiler and/or changed its options.

Yep, updated it to clang 13. It's a warning present in 13, but not in 12.

I'll update it to 14 soon, now that that's released. It still has that
warning, so it's not going to help us avoid the warning.

Greetings,

Andres Freund

  6. Re: Pointer subtraction with a null pointer

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-26T16:37:44Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-03-26 12:13:12 -0400, Tom Lane wrote:
>> This code is old, but mylodon wasn't doing that a week ago, so
>> Andres must've updated the compiler and/or changed its options.

> Yep, updated it to clang 13. It's a warning present in 13, but not in 12.

OK, that answers that.

After more thought I agree that replacing these relptr_store calls
with something else would be the better solution.  I'll prepare a
patch.

			regards, tom lane

  7. Re: Pointer subtraction with a null pointer

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-26T17:23:34Z

    I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2022-03-26 12:13:12 -0400, Tom Lane wrote:
>>> This code is old, but mylodon wasn't doing that a week ago, so
>>> Andres must've updated the compiler and/or changed its options.

>> Yep, updated it to clang 13. It's a warning present in 13, but not in 12.

> OK, that answers that.

... Actually, after looking closer, I misread what our code is doing.
These call sites are trying to set the relptr value to "null" (zero),
and AFAICS it should be allowed:

freepage.c:188:2: warning: performing pointer subtraction with a null pointer has undefined behavior [-Wnull-pointer-subtraction]
        relptr_store(base, fpm->btree_root, (FreePageBtree *) NULL);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../src/include/utils/relptr.h:63:59: note: expanded from macro 'relptr_store'
         (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
                                                ~~~~~~~~~~~~~~~~ ^

clang is complaining about the subtraction despite it being inside
a conditional arm that cannot be reached when val is null.  It's hard
to see how that isn't a flat-out compiler bug.

However, granting that it isn't going to get fixed right away,
we could replace these call sites with "relptr_store_null()",
and maybe get rid of the conditional in relptr_store().

			regards, tom lane

  8. Re: Pointer subtraction with a null pointer

    Andres Freund <andres@anarazel.de> — 2022-03-26T17:49:53Z

    Hi,

On 2022-03-26 13:23:34 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> On 2022-03-26 12:13:12 -0400, Tom Lane wrote:
> >>> This code is old, but mylodon wasn't doing that a week ago, so
> >>> Andres must've updated the compiler and/or changed its options.
>
> >> Yep, updated it to clang 13. It's a warning present in 13, but not in 12.
>
> > OK, that answers that.
>
> ... Actually, after looking closer, I misread what our code is doing.
> These call sites are trying to set the relptr value to "null" (zero),
> and AFAICS it should be allowed:
>
> freepage.c:188:2: warning: performing pointer subtraction with a null pointer has undefined behavior [-Wnull-pointer-subtraction]
>         relptr_store(base, fpm->btree_root, (FreePageBtree *) NULL);
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../../../src/include/utils/relptr.h:63:59: note: expanded from macro 'relptr_store'
>          (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
>                                                 ~~~~~~~~~~~~~~~~ ^
>
> clang is complaining about the subtraction despite it being inside
> a conditional arm that cannot be reached when val is null.

Huh, yea. I somehow read the conditional branch as guarding against a an
uninitialized base pointer or such.


> It's hard to see how that isn't a flat-out compiler bug.

It only happens if the NULL is directly passed as an argument to the macro,
not if there's an intermediary variable. Argh.


#include <stddef.h>

#define relptr_store(base, rp, val) \
	 ((rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))

typedef union { struct foo *relptr_type; size_t relptr_off; } relptr;

void
problem_not_present(relptr *rp, char *base)
{
    struct foo *val = NULL;

    relptr_store(base, *rp, val);
}

void
problem_present(relptr *rp, char *base)
{
    relptr_store(base, *rp, NULL);
}


Looks like that warning is uttered whenever there's a subtraction from a
pointer with NULL, even if the code isn't reachable. Which I guess makes
*some* sense, outside of macros it's not something that'd ever be reasonable.


Wonder if we should try to get rid of the problem by also fixing the double
evaluation of val? I think something like

static inline void
relptr_store_internal(size_t *off, char *base, char *val)
{
    if (val == NULL)
        *off = 0;
    else
        *off = val - base;
}

#ifdef HAVE__BUILTIN_TYPES_COMPATIBLE_P
#define relptr_store(base, rp, val) \
	(AssertVariableIsOfTypeMacro(base, char *), \
	 AssertVariableIsOfTypeMacro(val, __typeof__((rp).relptr_type)), \
         relptr_store_internal(&(rp).relptr_off, base, (char *) val))
#else
...

should do the trick?

Might also be worth adding an assertion that base < val.


> However, granting that it isn't going to get fixed right away,
> we could replace these call sites with "relptr_store_null()",
> and maybe get rid of the conditional in relptr_store().

Also would be good with that.

Greetings,

Andres Freund

  9. Re: Pointer subtraction with a null pointer

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-26T17:51:07Z

    I wrote:
> clang is complaining about the subtraction despite it being inside
> a conditional arm that cannot be reached when val is null.  It's hard
> to see how that isn't a flat-out compiler bug.
> However, granting that it isn't going to get fixed right away,
> we could replace these call sites with "relptr_store_null()",
> and maybe get rid of the conditional in relptr_store().

I've confirmed that the attached silences the warning with clang
13.0.0 (on Fedora 35).  The store_null notation is not awful, perhaps;
it makes those lines shorter and more readable.

I'm a bit less enthused about removing the conditional in relptr_store,
as that forces re-introducing it at a couple of call sites.  Perhaps
we should leave relptr_store alone ... but then the reason for
relptr_store_null is hard to explain except as a workaround for a
broken compiler.

I changed the comment suggesting that you could use relptrs with the
"process address space" as a base, because that would presumably mean
base == NULL which is going to draw the same warning.

			regards, tom lane

  10. Re: Pointer subtraction with a null pointer

    Andres Freund <andres@anarazel.de> — 2022-03-26T18:08:59Z

    Hi,

On 2022-03-26 10:49:53 -0700, Andres Freund wrote:
> > It's hard to see how that isn't a flat-out compiler bug.
> 
> It only happens if the NULL is directly passed as an argument to the macro,
> not if there's an intermediary variable. Argh.
> 
> 
> #include <stddef.h>
> 
> #define relptr_store(base, rp, val) \
> 	 ((rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
> 
> typedef union { struct foo *relptr_type; size_t relptr_off; } relptr;
> 
> void
> problem_not_present(relptr *rp, char *base)
> {
>     struct foo *val = NULL;
> 
>     relptr_store(base, *rp, val);
> }
> 
> void
> problem_present(relptr *rp, char *base)
> {
>     relptr_store(base, *rp, NULL);
> }
> 
> 
> Looks like that warning is uttered whenever there's a subtraction from a
> pointer with NULL, even if the code isn't reachable. Which I guess makes
> *some* sense, outside of macros it's not something that'd ever be reasonable.

Reported as https://github.com/llvm/llvm-project/issues/54570

Greetings,

Andres Freund

  11. Re: Pointer subtraction with a null pointer

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-26T18:13:56Z

    Andres Freund <andres@anarazel.de> writes:
> Wonder if we should try to get rid of the problem by also fixing the double
> evaluation of val? I think something like

Good idea.  The attached also silences the warning, and getting rid
of the double-eval hazard seems like a net win.

> Might also be worth adding an assertion that base < val.

Did that too.  On the whole I like this better.

			regards, tom lane

  12. Re: Pointer subtraction with a null pointer

    Andres Freund <andres@anarazel.de> — 2022-03-26T18:41:47Z

    Hi,

On 2022-03-26 14:13:56 -0400, Tom Lane wrote:
> The attached also silences the warning, and getting rid of the double-eval
> hazard seems like a net win.

Looks good wrt relptr_store. Maybe we should fix the double-eval hazard in
relptr_access too, think that'd be only one left over...


> > Might also be worth adding an assertion that base < val.
> 
> Did that too.  On the whole I like this better.

Better than the relptr_store_null() approach I assume? Agreed, if so.

Greetings,

Andres Freund

  13. Re: Pointer subtraction with a null pointer

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-26T18:49:44Z

    Andres Freund <andres@anarazel.de> writes:
> Looks good wrt relptr_store. Maybe we should fix the double-eval hazard in
> relptr_access too, think that'd be only one left over...

Hm.  Probably not worth the trouble, because it's hard to envision
a situation where rp is not a plain lvalue.

			regards, tom lane

  14. Re: Pointer subtraction with a null pointer

    Andres Freund <andres@anarazel.de> — 2022-06-03T16:01:17Z

    On 2022-03-26 11:08:59 -0700, Andres Freund wrote:
> On 2022-03-26 10:49:53 -0700, Andres Freund wrote:
> > > It's hard to see how that isn't a flat-out compiler bug.
> >
> > It only happens if the NULL is directly passed as an argument to the macro,
> > not if there's an intermediary variable. Argh.
> >
> >
> > #include <stddef.h>
> >
> > #define relptr_store(base, rp, val) \
> > 	 ((rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
> >
> > typedef union { struct foo *relptr_type; size_t relptr_off; } relptr;
> >
> > void
> > problem_not_present(relptr *rp, char *base)
> > {
> >     struct foo *val = NULL;
> >
> >     relptr_store(base, *rp, val);
> > }
> >
> > void
> > problem_present(relptr *rp, char *base)
> > {
> >     relptr_store(base, *rp, NULL);
> > }
> >
> >
> > Looks like that warning is uttered whenever there's a subtraction from a
> > pointer with NULL, even if the code isn't reachable. Which I guess makes
> > *some* sense, outside of macros it's not something that'd ever be reasonable.
>
> Reported as https://github.com/llvm/llvm-project/issues/54570

And it now got fixed. Will obviously be a bit till it reaches a compiler near
you...