Horton

Thread

Commits

  1. Fix a strict aliasing violation

  2. Add input function for data type pg_ndistinct

  1. warning: dereferencing type-punned pointer

    Tatsuo Ishii <ishii@postgresql.org> — 2024-07-24T06:55:25Z

    Today I compiled PostgreSQL master branch with -fno-strict-aliasing
compile option removed (previous discussions on the $subject [1]). gcc
version is 9.4.0.

There are a few places where $subject warning printed.

In file included from ../../../src/include/nodes/pg_list.h:42,
                 from ../../../src/include/access/tupdesc.h:19,
                 from ../../../src/include/access/htup_details.h:19,
                 from ../../../src/include/access/heaptoast.h:16,
                 from execExprInterp.c:59:
execExprInterp.c: In function ‘ExecEvalJsonExprPath’:
../../../src/include/nodes/nodes.h:133:29: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  133 | #define nodeTag(nodeptr)  (((const Node*)(nodeptr))->type)
      |                            ~^~~~~~~~~~~~~~~~~~~~~~~
../../../src/include/nodes/nodes.h:158:31: note: in expansion of macro ‘nodeTag’
  158 | #define IsA(nodeptr,_type_)  (nodeTag(nodeptr) == T_##_type_)
      |                               ^~~~~~~
../../../src/include/nodes/miscnodes.h:53:26: note: in expansion of macro ‘IsA’
   53 |  ((escontext) != NULL && IsA(escontext, ErrorSaveContext) && \
      |                          ^~~
execExprInterp.c:4399:7: note: in expansion of macro ‘SOFT_ERROR_OCCURRED’
 4399 |   if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
      |       ^~~~~~~~~~~~~~~~~~~
execExprInterp.c: In function ‘ExecEvalJsonCoercionFinish’:
../../../src/include/nodes/nodes.h:133:29: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  133 | #define nodeTag(nodeptr)  (((const Node*)(nodeptr))->type)
      |                            ~^~~~~~~~~~~~~~~~~~~~~~~
../../../src/include/nodes/nodes.h:158:31: note: in expansion of macro ‘nodeTag’
  158 | #define IsA(nodeptr,_type_)  (nodeTag(nodeptr) == T_##_type_)
      |                               ^~~~~~~
../../../src/include/nodes/miscnodes.h:53:26: note: in expansion of macro ‘IsA’
   53 |  ((escontext) != NULL && IsA(escontext, ErrorSaveContext) && \
      |                          ^~~
execExprInterp.c:4556:6: note: in expansion of macro ‘SOFT_ERROR_OCCURRED’
 4556 |  if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
      |      ^~~~~~~~~~~~~~~~~~~
origin.c: In function ‘StartupReplicationOrigin’:
origin.c:773:16: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
  773 |    file_crc = *(pg_crc32c *) &disk_state;
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~

In my understanding from the discussion [1], it would be better to fix
our code to avoid the warning because it *might* point out that there
is something wrong with our code. However the consensus at the time
was, we will not remove -fno-strict-aliasing option for now. It will
take long time before it would happen...

So I think the warnings in ExecEvalJsonExprPath are better fixed
because these are the only places where IsA (nodeTag) macro are used
and the warning is printed. Patch attached.

I am not so sure about StartupReplicationOrigin. Should we fix it now?
For me the code looks sane as long as we keep -fno-strict-aliasing
option. Or maybe better to fix so that someday we could remove the
compiler option?

[1] https://www.postgresql.org/message-id/flat/366.1535731324%40sss.pgh.pa.us#bd93089182d13c79b74593ec70bac435

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

  2. Re: warning: dereferencing type-punned pointer

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-24T14:05:45Z

    Tatsuo Ishii <ishii@postgresql.org> writes:
> So I think the warnings in ExecEvalJsonExprPath are better fixed
> because these are the only places where IsA (nodeTag) macro are used
> and the warning is printed. Patch attached.

I'm not very thrilled with these changes.  It's not apparent why
your compiler is warning about these usages of IsA and not any other
ones, nor is it apparent why these changes suppress the warnings.
(The code's not fundamentally different, so I'd think the underlying
problem is still there, if there really is one at all.)
I'm afraid we'd be playing whack-a-mole to suppress similar warnings
on various compiler versions, with no end result other than making
the code uglier and less consistent.

If we can figure out why the warning is appearing, maybe it'd be
possible to adjust the definition of IsA() to prevent it.

			regards, tom lane

  3. Re: warning: dereferencing type-punned pointer

    Peter Eisentraut <peter@eisentraut.org> — 2024-07-24T17:41:13Z

    On 24.07.24 08:55, Tatsuo Ishii wrote:
> origin.c: In function ‘StartupReplicationOrigin’:
> origin.c:773:16: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
>    773 |    file_crc = *(pg_crc32c *) &disk_state;
>        |                ^~~~~~~~~~~~~~~~~~~~~~~~~

> I am not so sure about StartupReplicationOrigin. Should we fix it now?
> For me the code looks sane as long as we keep -fno-strict-aliasing
> option. Or maybe better to fix so that someday we could remove the
> compiler option?

This is basically the textbook example of aliasing violation, isn't it? 
Wouldn't it be just as simple to do

memcpy(&file_crc, &disk_state, sizeof(file_crc));

  4. Re: warning: dereferencing type-punned pointer

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-24T17:48:08Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> This is basically the textbook example of aliasing violation, isn't it? 
> Wouldn't it be just as simple to do

> memcpy(&file_crc, &disk_state, sizeof(file_crc));

+1.  Also, it seems thoroughly bizarre to me that this case is handled
before checking for read failure.  I'd move the stanza to after the
"if (readBytes < 0)" one.

			regards, tom lane

  5. Re: warning: dereferencing type-punned pointer

    Peter Eisentraut <peter@eisentraut.org> — 2024-07-24T17:53:47Z

    On 24.07.24 16:05, Tom Lane wrote:
> I'm not very thrilled with these changes.  It's not apparent why
> your compiler is warning about these usages of IsA and not any other
> ones,

I think one difference is that normally IsA is called on a Node * (since 
you call IsA to decide what to cast it to), but in this case it's called 
on a pointer that is already of type ErrorSaveContext *.  You wouldn't 
normally need to call IsA on that, but it comes with the 
SOFT_ERROR_OCCURRED macro.  Another difference is that most nodes are 
dynamically allocated but in this case the ErrorSaveContext object (not 
a pointer to it) is part of another struct, and so I think some 
different aliasing rules might apply, but I'm not sure.

I think here you could just bypass the SOFT_ERROR_OCCURRED macro:

-       if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
+       if (jsestate->escontext.error_occurred)

  6. Re: warning: dereferencing type-punned pointer

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-24T18:09:49Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> On 24.07.24 16:05, Tom Lane wrote:
>> I'm not very thrilled with these changes.  It's not apparent why
>> your compiler is warning about these usages of IsA and not any other
>> ones,

> I think one difference is that normally IsA is called on a Node * (since 
> you call IsA to decide what to cast it to), but in this case it's called 
> on a pointer that is already of type ErrorSaveContext *.

Hmm.  But there are boatloads of places where we call IsA on a
pointer of type Expr *, or sometimes other things.  Why aren't
those triggering the same warning?

> I think here you could just bypass the SOFT_ERROR_OCCURRED macro:
> -       if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
> +       if (jsestate->escontext.error_occurred)

Perhaps.  That's a bit sad because it's piercing a layer of
abstraction.  I do not like compiler warnings that can't be
gotten rid of without making the code objectively worse.

			regards, tom lane

  7. Re: warning: dereferencing type-punned pointer

    Peter Eisentraut <peter@eisentraut.org> — 2024-07-24T18:26:55Z

    On 24.07.24 20:09, Tom Lane wrote:
> Peter Eisentraut<peter@eisentraut.org>  writes:
>> On 24.07.24 16:05, Tom Lane wrote:
>>> I'm not very thrilled with these changes.  It's not apparent why
>>> your compiler is warning about these usages of IsA and not any other
>>> ones,
>> I think one difference is that normally IsA is called on a Node * (since
>> you call IsA to decide what to cast it to), but in this case it's called
>> on a pointer that is already of type ErrorSaveContext *.
> Hmm.  But there are boatloads of places where we call IsA on a
> pointer of type Expr *, or sometimes other things.  Why aren't
> those triggering the same warning?

It must have to do with the fact that the escontext field in 
JsonExprState has the object inline, not as a pointer.  AIUI, with 
dynamically allocated objects you have more liberties about what type to 
interpret them as than with actually declared objects.

If you change the member to a pointer

-   ErrorSaveContext escontext;
+   ErrorSaveContext *escontext;
  } JsonExprState;

and make the required adjustments elsewhere in the code, the warning 
goes away.

This arrangement would also appear to be more consistent with other 
executor nodes (e.g., ExprState, ExprEvalStep), so it might be worth it 
for consistency in any case.

  8. Re: warning: dereferencing type-punned pointer

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-24T18:29:45Z

    BTW, I tried the same experiment of building without
-fno-strict-aliasing using gcc 11.4.1 (from RHEL9).
I see one more warning than Tatsuo-san did:

In file included from verify_heapam.c:18:
verify_heapam.c: In function ‘check_tuple_attribute’:
../../src/include/access/toast_internals.h:37:11: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
   37 |         (((toast_compress_header *) (ptr))->tcinfo >> VARLENA_EXTSIZE_BITS)
      |          ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
verify_heapam.c:1693:24: note: in expansion of macro ‘TOAST_COMPRESS_METHOD’
 1693 |                 cmid = TOAST_COMPRESS_METHOD(&toast_pointer);
      |                        ^~~~~~~~~~~~~~~~~~~~~

This looks a bit messy to fix: we surely don't want to pierce
the abstraction TOAST_COMPRESS_METHOD provides.  Perhaps
the toast_pointer local variable could be turned into a union
of struct varatt_external and toast_compress_header, but that
would impose a good deal of notational overhead on the rest
of this function.

The good news is that we get through check-world (although
I didn't try very many build options).

			regards, tom lane

  9. Re: warning: dereferencing type-punned pointer

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-24T18:31:41Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> If you change the member to a pointer

> -   ErrorSaveContext escontext;
> +   ErrorSaveContext *escontext;
>   } JsonExprState;

> and make the required adjustments elsewhere in the code, the warning 
> goes away.

> This arrangement would also appear to be more consistent with other 
> executor nodes (e.g., ExprState, ExprEvalStep), so it might be worth it 
> for consistency in any case.

+1, makes sense to me.

			regards, tom lane

  10. Re: warning: dereferencing type-punned pointer

    Tatsuo Ishii <ishii@postgresql.org> — 2024-07-26T07:11:32Z

    > On 24.07.24 20:09, Tom Lane wrote:
>> Peter Eisentraut<peter@eisentraut.org>  writes:
>>> On 24.07.24 16:05, Tom Lane wrote:
>>>> I'm not very thrilled with these changes.  It's not apparent why
>>>> your compiler is warning about these usages of IsA and not any other
>>>> ones,
>>> I think one difference is that normally IsA is called on a Node *
>>> (since
>>> you call IsA to decide what to cast it to), but in this case it's
>>> called
>>> on a pointer that is already of type ErrorSaveContext *.
>> Hmm.  But there are boatloads of places where we call IsA on a
>> pointer of type Expr *, or sometimes other things.  Why aren't
>> those triggering the same warning?
> 
> It must have to do with the fact that the escontext field in
> JsonExprState has the object inline, not as a pointer.  AIUI, with
> dynamically allocated objects you have more liberties about what type
> to interpret them as than with actually declared objects.

I don't agree. I think the compiler just dislike that nodeTag macro's
argument is a pointer created by '&' operator in this case:

#define nodeTag(nodeptr)  (((const Node*)(nodeptr))->type)

If we just give a pointer variable either it's type is Node * or
ErrorSaveContext * to nodeTag macro, the compiler becomes happy.

Moreover I think whether the object is inline or not is
irrelevant. Attached is a self contained test case. In the program:

	if (IsA(&f, List))

produces the strict aliasing rule violation but

	if (IsA(fp, List))

does not. Here "f" is an object defined as:

typedef struct Foo
{
	NodeTag		type;
	int		d;
} Foo;

Foo f;

and fp is defined as:

	Foo	*fp = &f;

$ gcc -Wall -O2 -c strict2.c
strict2.c: In function ‘sub’:
strict2.c:1:29: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
    1 | #define nodeTag(nodeptr)  (((const Node*)(nodeptr))->type)
      |                            ~^~~~~~~~~~~~~~~~~~~~~~~
strict2.c:2:31: note: in expansion of macro ‘nodeTag’
    2 | #define IsA(nodeptr,_type_)  (nodeTag(nodeptr) == T_##_type_)
      |                               ^~~~~~~
strict2.c:26:6: note: in expansion of macro ‘IsA’
   26 |  if (IsA(&f, List))
      |      ^~~
At top level:
strict2.c:21:12: warning: ‘sub’ defined but not used [-Wunused-function]
   21 | static int sub(void)
      |            ^~~

> If you change the member to a pointer
> 
> -   ErrorSaveContext escontext;
> +   ErrorSaveContext *escontext;
>  } JsonExprState;
> 
> and make the required adjustments elsewhere in the code, the warning
> goes away.

I think this is not necessary. Just my patch in the upthread is enough.

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

  11. Re: warning: dereferencing type-punned pointer

    Tatsuo Ishii <ishii@postgresql.org> — 2024-07-26T07:49:10Z

    Sorry, I forgot to attach the file...

> I don't agree. I think the compiler just dislike that nodeTag macro's
> argument is a pointer created by '&' operator in this case:
> 
> #define nodeTag(nodeptr)  (((const Node*)(nodeptr))->type)
> 
> If we just give a pointer variable either it's type is Node * or
> ErrorSaveContext * to nodeTag macro, the compiler becomes happy.
> 
> Moreover I think whether the object is inline or not is
> irrelevant. Attached is a self contained test case. In the program:
> 
> 	if (IsA(&f, List))
> 
> produces the strict aliasing rule violation but
> 
> 	if (IsA(fp, List))
> 
> does not. Here "f" is an object defined as:
> 
> typedef struct Foo
> {
> 	NodeTag		type;
> 	int		d;
> } Foo;
> 
> Foo f;
> 
> and fp is defined as:
> 
> 	Foo	*fp = &f;
> 
> $ gcc -Wall -O2 -c strict2.c
> strict2.c: In function ‘sub’:
> strict2.c:1:29: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
>     1 | #define nodeTag(nodeptr)  (((const Node*)(nodeptr))->type)
>       |                            ~^~~~~~~~~~~~~~~~~~~~~~~
> strict2.c:2:31: note: in expansion of macro ‘nodeTag’
>     2 | #define IsA(nodeptr,_type_)  (nodeTag(nodeptr) == T_##_type_)
>       |                               ^~~~~~~
> strict2.c:26:6: note: in expansion of macro ‘IsA’
>    26 |  if (IsA(&f, List))
>       |      ^~~
> At top level:
> strict2.c:21:12: warning: ‘sub’ defined but not used [-Wunused-function]
>    21 | static int sub(void)
>       |            ^~~
> 
>> If you change the member to a pointer
>> 
>> -   ErrorSaveContext escontext;
>> +   ErrorSaveContext *escontext;
>>  } JsonExprState;
>> 
>> and make the required adjustments elsewhere in the code, the warning
>> goes away.
> 
> I think this is not necessary. Just my patch in the upthread is enough.
> 
> Best reagards,
> --
> Tatsuo Ishii
> SRA OSS LLC
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp

  12. Re: warning: dereferencing type-punned pointer

    Peter Eisentraut <peter@eisentraut.org> — 2025-12-01T16:08:15Z

    On 24.07.24 19:48, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> This is basically the textbook example of aliasing violation, isn't it?
>> Wouldn't it be just as simple to do
> 
>> memcpy(&file_crc, &disk_state, sizeof(file_crc));
> 
> +1.  Also, it seems thoroughly bizarre to me that this case is handled
> before checking for read failure.  I'd move the stanza to after the
> "if (readBytes < 0)" one.

(older thread)

I committed this fix, since it was simple and obvious.

I also researched again the other warnings mentioned in this thread. 
There are also a few new similar ones from commit 44eba8f06e55.

My conclusion is that casting Node * to a different pointer-to-struct is 
always an aliasing violation.  The reason why the compiler only warns in 
a few cases is probably that those are fully within the same translation 
unit, while most other ones are across file boundaries that the compiler 
cannot analyze reliably.  If you dial up the warning verbosity with 
-Wstrict-aliasing=1/2 (lower = more warnings but less reliable), you get 
more warnings about this.

Unlike what PostgreSQL code appears to assume, there is no rule in C (or 
C++) that overlaying similar structs is a valid aliasing.  So this was 
never correct, but compilers have only gotten more aggressive about this 
over time.

One correct way would be to embed the Node struct, like

typedef struct ErrorSaveContext
{
     Node        node;
     bool        error_occurred;
     bool        details_wanted;
     ErrorData  *error_data;
} ErrorSaveContext;

because you can cast between a struct and one of its field types.

We already do this node embedding for inheritance for other nodes, so 
this approach would probably be somewhat natural, but it would obviously 
require a bit of adjustment here and there (but maybe not that much).

Another approach is to annotate some fundamental types like Node and 
Expr with __attribute__((may_alias)).

Similarly with struct varlena and all its variants and related types.

Anyway, this is a battle for another day.  We're definitely going to 
have to stick with -fno-strict-aliasing for the foreseeable time.

  13. Re: warning: dereferencing type-punned pointer

    Andres Freund <andres@anarazel.de> — 2026-01-22T19:46:37Z

    Hi,

On 2025-12-01 17:08:15 +0100, Peter Eisentraut wrote:
> My conclusion is that casting Node * to a different pointer-to-struct is
> always an aliasing violation.  The reason why the compiler only warns in a
> few cases is probably that those are fully within the same translation unit,
> while most other ones are across file boundaries that the compiler cannot
> analyze reliably.  If you dial up the warning verbosity with
> -Wstrict-aliasing=1/2 (lower = more warnings but less reliable), you get
> more warnings about this.
> 
> Unlike what PostgreSQL code appears to assume, there is no rule in C (or
> C++) that overlaying similar structs is a valid aliasing.  So this was never
> correct, but compilers have only gotten more aggressive about this over
> time.

It's not clear to me that this is true.

C23's §6.7.3.2 contains (and earlier versions have similar language:

  A pointer to a structure object, suitably converted, points to its initial
  member (or if that member is a bit-field, then to the unit in which it
  resides),

Which afaict means that it'd be legal to cast an ErrorSaveContext * to a
NodeTag*.  Of course, IsA(), via nodeTag(), doesn't actually cast to NodeTag *,
but to Node *.

However, you qualified your answer with "to a different pointer-to-struct",
but afaict the rules would be the same if the "initial member" of two
different structs were a struct.


There's also C23's §6.5 7):
  An object shall have its stored value accessed only by an lvalue expression that has one of
  the following types:
  ...
  — an aggregate or union type that includes one of the aforementioned types among its
  members (including, recursively, a member of a subaggregate or contained union), or

which afaict means that if we *can* cast between different equivalent structs,
as long as they have the same initial sequence?


The purpose of the aliasing rules is to allow for type based alias analysis,
to figure out things like whether two pointers could potentially point to the
same memory.  For that aggregate types don't really matter, it's just scalars
(and I guess bitfields, however you classify them) that do. And thus whether
you have overlayed structs with the same types doesn't really matter either [1].



The language around this is near impenetrable though, and hasn't improved
meaningfully since at least C99, so it's really hard to say.

Greetings,

Andres Freund


[1] There are some nasty cases where not being careful could lead to alignment
    problems, but I don't think that's typically a problem in postgres.

  14. Re: warning: dereferencing type-punned pointer

    Peter Eisentraut <peter@eisentraut.org> — 2026-01-23T08:20:29Z

    On 22.01.26 20:46, Andres Freund wrote:
> However, you qualified your answer with "to a different pointer-to-struct",
> but afaict the rules would be the same if the "initial member" of two
> different structs were a struct.
> 
> 
> There's also C23's §6.5 7):
>    An object shall have its stored value accessed only by an lvalue expression that has one of
>    the following types:
>    ...
>    — an aggregate or union type that includes one of the aforementioned types among its
>    members (including, recursively, a member of a subaggregate or contained union), or
> 
> which afaict means that if we *can* cast between different equivalent structs,
> as long as they have the same initial sequence?

I think what this means is that if you have

typedef struct Append
{
     Plan        plan;
     ...
}

and you have an object of type Plan, then you can access that object via 
a pointer of type Append.

Now that I see this again, this is the opposite the direction of what we 
would need (have object of type Append, access via pointer to Plan, or 
pointer to Node).  Also note that it doesn't require that member to be 
the first member.  So this consideration seems to be unrelated to what 
we are looking for.