new warnings with clang-21 / how const is Datum
clang-21 shows some new warnings:
../src/backend/access/common/toast_internals.c:296:33: error: variable
'chunk_data' is uninitialized when passed as a const pointer argument
here [-Werror,-Wuninitialized-const-pointer]
296 | t_values[2] = PointerGetDatum(&chunk_data);
../src/backend/access/gist/gistutil.c:207:28: error: variable 'attrsize'
is uninitialized when passed as a const pointer argument here
[-Werror,-Wuninitialized-const-pointer]
207 |
PointerGetDatum(&attrsize));
|
^~~~~~~~
../src/backend/access/gist/gistutil.c:276:27: error: variable 'dstsize'
is uninitialized when passed as a const pointer argument here
[-Werror,-Wuninitialized-const-pointer]
276 |
PointerGetDatum(&dstsize));
|
^~~~~~~
The first one is easily fixed by re-arranging the code a bit.
The other two indicate more fundamental problems. The gist API uses
these arguments to pass back information; these pointers do not in fact
point to a const object.
This possibly means that the change
commit c8b2ef05f48
Author: Peter Eisentraut <peter@eisentraut.org>
Date: Tue Sep 27 20:47:07 2022
Convert *GetDatum() and DatumGet*() macros to inline functions
that made PointerGetDatum() look like
static inline Datum
PointerGetDatum(const void *X)
was mistaken in adding the const qualifier.
We could remove that qualifier (this would propagate to several other
functions built on top of PointerGetDatum()), but then there will be
complaints from the other side:
static const TableAmRoutine heapam_methods = {
PG_RETURN_POINTER(&heapam_methods);
Then the question is, which one of these should be considered the outlier?
We could remove the const qualifications from the API and stick an
unconstify() around &heapam_methods.
Or we could maybe make a new function PointerNonConstGetDatum() that we
use for exceptional cases like the gist API.
There is a related issue that I have been researching for some time. It
seems intuitively correct that the string passed into a data type input
function should not be modified by that function. And so the relevant
functions could use const qualifiers like
extern Datum InputFunctionCall(FmgrInfo *flinfo, const char *str, ...);
which they currently do not.
There are a some places in the code where const strings get passed into
some *InputFunctionCall() functions and have their const qualifications
rudely cast away, which seems unsatisfactory.
Overall, the question to what extent fmgr functions are allowed to
modify objects pointed to by their input arguments doesn't seem to be
addressed anywhere.
Peter Eisentraut <peter@eisentraut.org> writes:
Overall, the question to what extent fmgr functions are allowed to
modify objects pointed to by their input arguments doesn't seem to be
addressed anywhere.
I think it's generally understood that an fmgr function must not
modify pass-by-reference inputs, because they could easily be
pointing directly into a heap tuple in a shared buffer. The
exception is functions that participate in custom APIs where the
presence of output argument(s) is explicitly documented.
One question that statement leaves unanswered is whether datatype
input functions qualify as a "custom API". I'd vote not (ie they
shouldn't modify their input strings) unless we find exceptions.
So that suggests that the use of non-const pointer Datums should be
the outlier.
regards, tom lane
On 01.09.25 08:47, Peter Eisentraut wrote:
clang-21 shows some new warnings:
../src/backend/access/common/toast_internals.c:296:33: error: variable
'chunk_data' is uninitialized when passed as a const pointer argument
here [-Werror,-Wuninitialized-const-pointer]
296 | t_values[2] = PointerGetDatum(&chunk_data);../src/backend/access/gist/gistutil.c:207:28: error: variable 'attrsize'
is uninitialized when passed as a const pointer argument here [-Werror,-
Wuninitialized-const-pointer]
207 | PointerGetDatum(&attrsize));
| ^~~~~~~~
../src/backend/access/gist/gistutil.c:276:27: error: variable 'dstsize'
is uninitialized when passed as a const pointer argument here [-Werror,-
Wuninitialized-const-pointer]
276 | PointerGetDatum(&dstsize));
| ^~~~~~~
Here is a quick-fix patch for this. It silences these warnings by
initializing the respective variables first. This is already done
similarly in nearby code. This can be backpatched to PG16, where these
warnings began.
The second patch is a bit of a more extensive code rearrangement to make
the need for the workaround in the first patch go away. This would be
for master only.