public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v1 2/2] Make sure published XIDs are persistent
10+ messages / 4 participants
[nested] [flat]
* [PATCH v1 2/2] Make sure published XIDs are persistent
@ 2021-03-08 06:43 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Kyotaro Horiguchi @ 2021-03-08 06:43 UTC (permalink / raw)
pg_xact_status() premises that XIDs obtained by
pg_current_xact_id(_if_assigned)() are persistent beyond a crash. But
XIDs are not guaranteed to go beyond WAL buffers before commit and
thus XIDs may vanish if server crashes before commit. This patch
guarantees the XID shown by the functions to be flushed out to disk.
---
src/backend/access/transam/xact.c | 55 +++++++++++++++++++++++++------
src/backend/access/transam/xlog.c | 2 +-
src/backend/utils/adt/xid8funcs.c | 12 ++++++-
src/include/access/xact.h | 3 +-
4 files changed, 59 insertions(+), 13 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 4e6a3df6b8..85b1c21ee0 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -201,7 +201,7 @@ typedef struct TransactionStateData
int prevSecContext; /* previous SecurityRestrictionContext */
bool prevXactReadOnly; /* entry-time xact r/o state */
bool startedInRecovery; /* did we start in recovery? */
- bool didLogXid; /* has xid been included in WAL record? */
+ XLogRecPtr minLSN; /* LSN needed to reach to record the xid */
int parallelModeLevel; /* Enter/ExitParallelMode counter */
bool chain; /* start a new block after this one */
bool assigned; /* assigned to top-level XID */
@@ -520,14 +520,46 @@ GetCurrentFullTransactionIdIfAny(void)
* MarkCurrentTransactionIdLoggedIfAny
*
* Remember that the current xid - if it is assigned - now has been wal logged.
+ *
+ * upto is the LSN up to which we need to flush WAL to ensure the current xid
+ * to be persistent. See EnsureCurrentTransactionIdLogged().
*/
void
-MarkCurrentTransactionIdLoggedIfAny(void)
+MarkCurrentTransactionIdLoggedIfAny(XLogRecPtr upto)
{
- if (FullTransactionIdIsValid(CurrentTransactionState->fullTransactionId))
- CurrentTransactionState->didLogXid = true;
+ if (FullTransactionIdIsValid(CurrentTransactionState->fullTransactionId) &&
+ XLogRecPtrIsInvalid(CurrentTransactionState->minLSN))
+ CurrentTransactionState->minLSN = upto;
}
+/*
+ * EnsureCurrentTransactionIdLogged
+ *
+ * Make sure that the current top XID is WAL-logged.
+ */
+void
+EnsureTopTransactionIdLogged(void)
+{
+ /*
+ * We need at least one WAL record for the current top transaction to be
+ * flushed out. Write one if we don't have one yet.
+ */
+ if (XLogRecPtrIsInvalid(TopTransactionStateData.minLSN))
+ {
+ xl_xact_assignment xlrec;
+
+ xlrec.xtop = XidFromFullTransactionId(XactTopFullTransactionId);
+ Assert(TransactionIdIsValid(xlrec.xtop));
+ xlrec.nsubxacts = 0;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, MinSizeOfXactAssignment);
+ TopTransactionStateData.minLSN =
+ XLogInsert(RM_XACT_ID, XLOG_XACT_ASSIGNMENT);
+ }
+
+ XLogFlush(TopTransactionStateData.minLSN);
+}
/*
* GetStableLatestTransactionId
@@ -616,14 +648,14 @@ AssignTransactionId(TransactionState s)
* When wal_level=logical, guarantee that a subtransaction's xid can only
* be seen in the WAL stream if its toplevel xid has been logged before.
* If necessary we log an xact_assignment record with fewer than
- * PGPROC_MAX_CACHED_SUBXIDS. Note that it is fine if didLogXid isn't set
+ * PGPROC_MAX_CACHED_SUBXIDS. Note that it is fine if minLSN isn't set
* for a transaction even though it appears in a WAL record, we just might
* superfluously log something. That can happen when an xid is included
* somewhere inside a wal record, but not in XLogRecord->xl_xid, like in
* xl_standby_locks.
*/
if (isSubXact && XLogLogicalInfoActive() &&
- !TopTransactionStateData.didLogXid)
+ XLogRecPtrIsInvalid(TopTransactionStateData.minLSN))
log_unknown_top = true;
/*
@@ -693,6 +725,7 @@ AssignTransactionId(TransactionState s)
log_unknown_top)
{
xl_xact_assignment xlrec;
+ XLogRecPtr endptr;
/*
* xtop is always set by now because we recurse up transaction
@@ -707,11 +740,13 @@ AssignTransactionId(TransactionState s)
XLogRegisterData((char *) unreportedXids,
nUnreportedXids * sizeof(TransactionId));
- (void) XLogInsert(RM_XACT_ID, XLOG_XACT_ASSIGNMENT);
+ endptr = XLogInsert(RM_XACT_ID, XLOG_XACT_ASSIGNMENT);
nUnreportedXids = 0;
- /* mark top, not current xact as having been logged */
- TopTransactionStateData.didLogXid = true;
+
+ /* set minLSN of top, not of current xact if not yet */
+ if (XLogRecPtrIsInvalid(TopTransactionStateData.minLSN))
+ TopTransactionStateData.minLSN = endptr;
}
}
}
@@ -1996,7 +2031,7 @@ StartTransaction(void)
* initialize reported xid accounting
*/
nUnreportedXids = 0;
- s->didLogXid = false;
+ s->minLSN = InvalidXLogRecPtr;
/*
* must initialize resource-management stuff first
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 377afb8732..1a503c1447 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1159,7 +1159,7 @@ XLogInsertRecord(XLogRecData *rdata,
*/
WALInsertLockRelease();
- MarkCurrentTransactionIdLoggedIfAny();
+ MarkCurrentTransactionIdLoggedIfAny(EndPos);
END_CRIT_SECTION();
diff --git a/src/backend/utils/adt/xid8funcs.c b/src/backend/utils/adt/xid8funcs.c
index cc2b4ac797..992482f8c8 100644
--- a/src/backend/utils/adt/xid8funcs.c
+++ b/src/backend/utils/adt/xid8funcs.c
@@ -357,6 +357,8 @@ bad_format:
Datum
pg_current_xact_id(PG_FUNCTION_ARGS)
{
+ FullTransactionId xid;
+
/*
* Must prevent during recovery because if an xid is not assigned we try
* to assign one, which would fail. Programs already rely on this function
@@ -365,7 +367,12 @@ pg_current_xact_id(PG_FUNCTION_ARGS)
*/
PreventCommandDuringRecovery("pg_current_xact_id()");
- PG_RETURN_FULLTRANSACTIONID(GetTopFullTransactionId());
+ xid = GetTopFullTransactionId();
+
+ /* the XID is going to be published, make sure it is psersistent */
+ EnsureTopTransactionIdLogged();
+
+ PG_RETURN_FULLTRANSACTIONID(xid);
}
/*
@@ -380,6 +387,9 @@ pg_current_xact_id_if_assigned(PG_FUNCTION_ARGS)
if (!FullTransactionIdIsValid(topfxid))
PG_RETURN_NULL();
+ /* the XID is going to be published, make sure it is psersistent */
+ EnsureTopTransactionIdLogged();
+
PG_RETURN_FULLTRANSACTIONID(topfxid);
}
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index f49a57b35e..536a5e653b 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -386,7 +386,8 @@ extern FullTransactionId GetTopFullTransactionId(void);
extern FullTransactionId GetTopFullTransactionIdIfAny(void);
extern FullTransactionId GetCurrentFullTransactionId(void);
extern FullTransactionId GetCurrentFullTransactionIdIfAny(void);
-extern void MarkCurrentTransactionIdLoggedIfAny(void);
+extern void MarkCurrentTransactionIdLoggedIfAny(XLogRecPtr upto);
+extern void EnsureTopTransactionIdLogged(void);
extern bool SubTransactionIsActive(SubTransactionId subxid);
extern CommandId GetCurrentCommandId(bool used);
extern void SetParallelStartTimestamps(TimestampTz xact_ts, TimestampTz stmt_ts);
--
2.27.0
----Next_Part(Mon_Mar__8_17_32_42_2021_572)----
^ permalink raw reply [nested|flat] 10+ messages in thread
* warning: dereferencing type-punned pointer
@ 2024-07-24 06:55 Tatsuo Ishii <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Tatsuo Ishii @ 2024-07-24 06:55 UTC (permalink / raw)
To: pgsql-hackers
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#bd93089182d13c79b74593ec70...
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
[text/x-patch] fix_ExecEvalJsonExprPath.patch (1.1K, ../../[email protected]/2-fix_ExecEvalJsonExprPath.patch)
download | inline diff:
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index d8735286c4..387311fdfb 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -4381,6 +4381,7 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op,
if (!*op->resnull && jsexpr->use_io_coercion)
{
FunctionCallInfo fcinfo;
+ Node *node;
Assert(jump_eval_coercion == -1);
fcinfo = jsestate->input_fcinfo;
@@ -4396,7 +4397,8 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op,
fcinfo->isnull = false;
*op->resvalue = FunctionCallInvoke(fcinfo);
- if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
+ node = (Node *) &jsestate->escontext;
+ if (SOFT_ERROR_OCCURRED(node))
error = true;
}
@@ -4552,8 +4554,9 @@ void
ExecEvalJsonCoercionFinish(ExprState *state, ExprEvalStep *op)
{
JsonExprState *jsestate = op->d.jsonexpr.jsestate;
+ Node *node = (Node *) &jsestate->escontext;
- if (SOFT_ERROR_OCCURRED(&jsestate->escontext))
+ if (SOFT_ERROR_OCCURRED(node))
{
*op->resvalue = (Datum) 0;
*op->resnull = true;
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: warning: dereferencing type-punned pointer
@ 2024-07-24 14:05 Tom Lane <[email protected]>
parent: Tatsuo Ishii <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Tom Lane @ 2024-07-24 14:05 UTC (permalink / raw)
To: Tatsuo Ishii <[email protected]>; +Cc: pgsql-hackers
Tatsuo Ishii <[email protected]> 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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: warning: dereferencing type-punned pointer
@ 2024-07-24 17:53 Peter Eisentraut <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Peter Eisentraut @ 2024-07-24 17:53 UTC (permalink / raw)
To: Tom Lane <[email protected]>; Tatsuo Ishii <[email protected]>; +Cc: pgsql-hackers
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)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: warning: dereferencing type-punned pointer
@ 2024-07-24 18:09 Tom Lane <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 2 replies; 10+ messages in thread
From: Tom Lane @ 2024-07-24 18:09 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Tatsuo Ishii <[email protected]>; pgsql-hackers
Peter Eisentraut <[email protected]> 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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: warning: dereferencing type-punned pointer
@ 2024-07-24 18:26 Peter Eisentraut <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 2 replies; 10+ messages in thread
From: Peter Eisentraut @ 2024-07-24 18:26 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Tatsuo Ishii <[email protected]>; pgsql-hackers
On 24.07.24 20:09, Tom Lane wrote:
> Peter Eisentraut<[email protected]> 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.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: warning: dereferencing type-punned pointer
@ 2024-07-24 18:29 Tom Lane <[email protected]>
parent: Tom Lane <[email protected]>
1 sibling, 0 replies; 10+ messages in thread
From: Tom Lane @ 2024-07-24 18:29 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Tatsuo Ishii <[email protected]>; pgsql-hackers
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: warning: dereferencing type-punned pointer
@ 2024-07-24 18:31 Tom Lane <[email protected]>
parent: Peter Eisentraut <[email protected]>
1 sibling, 0 replies; 10+ messages in thread
From: Tom Lane @ 2024-07-24 18:31 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Tatsuo Ishii <[email protected]>; pgsql-hackers
Peter Eisentraut <[email protected]> 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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: warning: dereferencing type-punned pointer
@ 2024-07-26 07:11 Tatsuo Ishii <[email protected]>
parent: Peter Eisentraut <[email protected]>
1 sibling, 1 reply; 10+ messages in thread
From: Tatsuo Ishii @ 2024-07-26 07:11 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; pgsql-hackers
> On 24.07.24 20:09, Tom Lane wrote:
>> Peter Eisentraut<[email protected]> 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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: warning: dereferencing type-punned pointer
@ 2024-07-26 07:49 Tatsuo Ishii <[email protected]>
parent: Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: Tatsuo Ishii @ 2024-07-26 07:49 UTC (permalink / raw)
To: [email protected]; [email protected]; +Cc: pgsql-hackers
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
/*
* Minimum definitions copied from PostgreSQL to make the
* test self-contained.
*/
#define nodeTag(nodeptr) (((const Node*)(nodeptr))->type)
#define IsA(nodeptr,_type_) (nodeTag(nodeptr) == T_##_type_)
typedef enum NodeTag
{
T_Invalid = 0,
T_List = 1
} NodeTag;
typedef struct Node
{
NodeTag type;
} Node;
/* Home brew node */
typedef struct Foo
{
NodeTag type;
int d;
} Foo;
static int sub(void)
{
Foo f;
Foo *fp = &f;
f.type = T_List;
/* strict aliasing rule error */
if (IsA(&f, List))
return 1;
/* This is ok */
if (IsA(fp, List))
return 1;
return 0;
}
Attachments:
[text/plain] strict2.c (594B, ../../[email protected]/2-strict2.c)
download | inline:
/*
* Minimum definitions copied from PostgreSQL to make the
* test self-contained.
*/
#define nodeTag(nodeptr) (((const Node*)(nodeptr))->type)
#define IsA(nodeptr,_type_) (nodeTag(nodeptr) == T_##_type_)
typedef enum NodeTag
{
T_Invalid = 0,
T_List = 1
} NodeTag;
typedef struct Node
{
NodeTag type;
} Node;
/* Home brew node */
typedef struct Foo
{
NodeTag type;
int d;
} Foo;
static int sub(void)
{
Foo f;
Foo *fp = &f;
f.type = T_List;
/* strict aliasing rule error */
if (IsA(&f, List))
return 1;
/* This is ok */
if (IsA(fp, List))
return 1;
return 0;
}
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2024-07-26 07:49 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-08 06:43 [PATCH v1 2/2] Make sure published XIDs are persistent Kyotaro Horiguchi <[email protected]>
2024-07-24 06:55 warning: dereferencing type-punned pointer Tatsuo Ishii <[email protected]>
2024-07-24 14:05 ` Re: warning: dereferencing type-punned pointer Tom Lane <[email protected]>
2024-07-24 17:53 ` Re: warning: dereferencing type-punned pointer Peter Eisentraut <[email protected]>
2024-07-24 18:09 ` Re: warning: dereferencing type-punned pointer Tom Lane <[email protected]>
2024-07-24 18:26 ` Re: warning: dereferencing type-punned pointer Peter Eisentraut <[email protected]>
2024-07-24 18:31 ` Re: warning: dereferencing type-punned pointer Tom Lane <[email protected]>
2024-07-26 07:11 ` Re: warning: dereferencing type-punned pointer Tatsuo Ishii <[email protected]>
2024-07-26 07:49 ` Re: warning: dereferencing type-punned pointer Tatsuo Ishii <[email protected]>
2024-07-24 18:29 ` Re: warning: dereferencing type-punned pointer Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox