Re: [PATCH] Make various variables read-only (const) - Mailing list pgsql-hackers

Oskari Saarenmaa <os@ohmu.fi> writes:
> On Sun, Dec 22, 2013 at 09:43:57PM -0500, Robert Haas wrote:
>> -#define DEF_ENC2NAME(name, codepage) { #name, PG_##name }
>> +/* The extra NUL-terminator will make sure a warning is raised if the
>> + * storage space for name is too small, otherwise when strlen(name) ==
>> + * sizeof(pg_enc2name.name) the NUL-terminator would be silently dropped.
>> + */
>> +#define DEF_ENC2NAME(name, codepage) { #name "\0", PG_##name }
>> 
>> - The above hunk is not related to the primary purpose of this patch.

> It sort-of is.  Without fixed size char-arrays it's not possible to move
> everything to .rodata, but fixed size char-arrays come with the drawback of
> silently dropping the NUL-terminator when strlen(str) == sizeof(array), by
> forcing a NUL-terminator in we always get a warning if it would've been
> dropped and the size of the array can then be increased.

AFAICT, this change and some similar ones are based on a false assumption.
It is *not* necessary to replace pointers by fixed-length arrays in order
to get things into .rodata.  For example, in chklocale.c where we already
had this coding:

struct encoding_match
{enum pg_enc pg_enc_code;const char *system_enc_name;
};

static const struct encoding_match encoding_match_list[] = {{PG_EUC_JP, "EUC-JP"},{PG_EUC_JP, "eucJP"},...

what I see in gcc -S (on x86_64 Linux) is:
.section    .rodata.align 32.type    encoding_match_list, @object.size    encoding_match_list, 1776
encoding_match_list:.long    1.zero    4.quad    .LC5.long    1.zero    4.quad    .LC6
....section    .rodata.str1.1
.LC5:.string    "EUC-JP"
.LC6:.string    "eucJP"
...

So it's all read-only data anyway, as can be confirmed with "size"
which shows that the .o file contains *no* data outside the text
segment.  There might be some platforms where this isn't the case,
but I don't think they're mainstream.

Converting the pointer arrangement to a fixed-length struct member
as per the patch has the following implications:

* We save the pointer, and possibly some alignment padding, at the cost
that now every string has to be padded to the maximal length.  This
might produce a net space savings if all the strings in the table
are of similar length, but it's hardly a guaranteed win.  Note also
that we no longer get to share storage with any other uses of the
same strings as plain literals.

* Possibly there's some savings in executable startup time as a
consequence of eliminating pointer-relocation work.  This would be
platform dependent, and in any case I've never heard any complaints
suggesting that this is a metric we need to care about at all.

* We now have a risk of possible silent breakage if any string reaches
the maximum length.  This is exacerbated by the temptation to not leave
a lot of daylight in the chosen maximum length, so as to minimize the
amount of bloat created.

The hunk Robert is complaining about is attempting to deal with that last
drawback; but it seems like a mighty ugly technique to me, and there are
other places where you've replaced pointers by fixed-length arrays without
adding any such protection (because it's notationally impractical).

TBH I don't find this to be an improvement, especially in cases where
it forces code changes beyond just const-ifying the data declarations.
I don't have a lot of confidence for instance that you found all the
places that were checking for a NULL pointer as an array terminator.
Having to change existing code greatly weakens the argument that this
is a correctness-improving patch, because now you have to factor in a
risk of actively breaking something.

So I think the fixed-length-array changes in this patch are a bad
idea; it's sufficient to make sure we've const-ified both the pointers
and the containing structs, as in the existing chklocale.c coding.
        regards, tom lane