Extend usage of const char*

Extend usage of const char*

[Jörg Sommer]

The compiler option `-Wwrite-strings` brings up many places where a string
literal (`const char*`) gets assigned to a variable whose type (`char*`)
allows modifications of the string content. If the DATA segment of the ELF
binary is mapped as read-only, this leads to a segmentation violation and
crashes the process. But even without a crash, the compiler could do better
checks and optimizations, if the variables have the appropriate type.

These patches are the beginning of a series of many commits—if you like. So,
please tell, if I should merge commits, reorder or rewrite them. Many of
them are trivial changes of adding `const` to the variable declaration. But
some of them require code rearrangements or rewrites. Hence, I plan to
commit one file at a time, if possible.

[PATCH 1/6] zle.textobjects: Mark variables as const
[PATCH 2/6] lex: Mark variables with const init as const
[PATCH 3/6] zle_vi: Mark variables with const init as const
[PATCH 4/6] zle_main: mark statusline as const
[PATCH 5/6] module: Mark name argument of some functions const
[PATCH 6/6] zsh: mark hookdef.name as const

 Src/Zle/textobjects.c |  6 +++---
 Src/Zle/zle_main.c    |  2 +-
 Src/Zle/zle_vi.c      |  2 +-
 Src/lex.c             |  6 +++---
 Src/module.c          | 15 ++++++++-------
 Src/zsh.h             |  2 +-
 6 files changed, 17 insertions(+), 16 deletions(-)

[PATCH 1/6] zle.textobjects: Mark variables as const

[Jörg Sommer]

Because these variables are initialized with as constant string, they should
be marked as *const* to make the compiler running with `-Wwrite-strings`
more happy.
---
 Src/Zle/textobjects.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Src/Zle/textobjects.c b/Src/Zle/textobjects.c
index c93777b65..a68c5296e 100644
--- a/Src/Zle/textobjects.c
+++ b/Src/Zle/textobjects.c
@@ -283,9 +283,9 @@ selectargument(UNUSED(char **args))
     free(linein);
 
     if (IS_THINGY(bindk, selectinshellword)) {
-	ZLE_CHAR_T *match = ZWS("`\'\"");
-	ZLE_CHAR_T *lmatch = ZWS("\'({"), *rmatch = ZWS("\')}");
-	ZLE_CHAR_T *ematch = match, *found;
+	const ZLE_CHAR_T *match = ZWS("`\'\"");
+	const ZLE_CHAR_T *lmatch = ZWS("\'({"), *rmatch = ZWS("\')}");
+	const ZLE_CHAR_T *ematch = match, *found;
 	int start, end = zlecs;
 	/* for 'in' widget, don't include initial blanks ... */
 	while (mark < zlecs && ZC_iblank(zleline[mark]))
-- 
2.43.0

[PATCH 2/6] lex: Mark variables with const init as const

[Jörg Sommer]

Because these variables are initialized with as constant string, they should
be marked as *const* to make the compiler running with `-Wwrite-strings`
more happy.
---
 Src/lex.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Src/lex.c b/Src/lex.c
index 31b130b07..e2aa059cd 100644
--- a/Src/lex.c
+++ b/Src/lex.c
@@ -168,7 +168,7 @@ static struct lexbufstate lexbuf_raw;
 /* text of punctuation tokens */
 
 /**/
-mod_export char *tokstrings[WHILE + 1] = {
+mod_export const char *tokstrings[WHILE + 1] = {
     NULL,	/* NULLTOK	  0  */
     ";",	/* SEPER	     */
     "\\n",	/* NEWLIN	     */
@@ -410,8 +410,8 @@ void
 initlextabs(void)
 {
     int t0;
-    static char *lx1 = "\\q\n;!&|(){}[]<>";
-    static char *lx2 = ";)|$[]~({}><=\\\'\"`,-!";
+    static const char *lx1 = "\\q\n;!&|(){}[]<>";
+    static const char *lx2 = ";)|$[]~({}><=\\\'\"`,-!";
 
     for (t0 = 0; t0 != 256; t0++) {
        lexact1[t0] = LX1_OTHER;
-- 
2.43.0

Re: [PATCH 2/6] lex: Mark variables with const init as const

[Oliver Kiddle]

On 1 Jan, Jörg Sommer wrote:
> Because these variables are initialized with as constant string, they should
> be marked as *const* to make the compiler running with `-Wwrite-strings`
> more happy.

Applying this patch series causes a couple of fresh warnings:

zle_thingy.c: In function 'bin_zle_refresh':
zle_thingy.c:420:15: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
  420 |     char *s = statusline;
      |

lex.c: In function 'exalias':
lex.c:1964:20: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
 1964 |         zshlextext = tokstrings[tok];
      |

clang describes these as "initializing 'char *' with an expression of
type 'const char *' discards qualifiers
[-Wincompatible-pointer-types-discards-qualifiers]".

I have no objection to adding const in more places where possible. You
may find we have code that is not really modifying its parameter but
relies on being able to temporarily swap a nul in for a character as
part of parsing. I also tend to find the old issue that you can't
safely cast a char ** to const char** tends to get in the way.

Oliver

Re: [PATCH 2/6] lex: Mark variables with const init as const

[Jörg Sommer]

[-- Attachment #1: Type: text/plain, Size: 1997 bytes --]

Oliver Kiddle schrieb am Fr 26. Jan, 08:39 (+0100):
> On 1 Jan, Jörg Sommer wrote:
> > Because these variables are initialized with as constant string, they should
> > be marked as *const* to make the compiler running with `-Wwrite-strings`
> > more happy.
> 
> Applying this patch series causes a couple of fresh warnings:
> 
> zle_thingy.c: In function 'bin_zle_refresh':
> zle_thingy.c:420:15: warning: initialization discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>   420 |     char *s = statusline;
>       |
> 
> lex.c: In function 'exalias':
> lex.c:1964:20: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
>  1964 |         zshlextext = tokstrings[tok];
>       |
> 
> clang describes these as "initializing 'char *' with an expression of
> type 'const char *' discards qualifiers
> [-Wincompatible-pointer-types-discards-qualifiers]".

Yes, it's a kind of chicken egg problem. Either it gets a really big patch
or you have steps that introduce new warnings and solve them with later
patches. I don't know what you prefer for review.

> I have no objection to adding const in more places where possible. You may
> find we have code that is not really modifying its parameter but relies on
> being able to temporarily swap a nul in for a character as part of
> parsing.

Unfortunately, in the sense of *const* this is a modification. And I think I
saw a function that expects to have modifiable memory, if it contains a
separator character. If it not, because its not user input and hence
pre-splitted, it doesn't modify the memory (and is const). Sorry, I don't
remember the name after that time.


My main question is how this patch set should be organized? One commit per
file which reduces the number of warnings in this file to the cost of new
warnings in other files?


Regards Jörg

-- 
Wenn du nur einen Hammer hast, sieht jedes Problem aus wie ein Nagel.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 269 bytes --]

Re: [PATCH 2/6] lex: Mark variables with const init as const

[Oliver Kiddle]

Jörg Sommer wrote:
> Yes, it's a kind of chicken egg problem. Either it gets a really big patch
> or you have steps that introduce new warnings and solve them with later
> patches. I don't know what you prefer for review.

For now, I've applied those four (of the six) patches that don't
introduce new warnings.

> My main question is how this patch set should be organized? One commit per
> file which reduces the number of warnings in this file to the cost of new
> warnings in other files?

Separating patches by source file is probably not the most helpful. I'd
prefer if at least each patch series doesn't add new warnings so that
we're leaving the source tree in a decent state when pushing. There
might be some things that initially look like they clearly should be
marked const but as the const percolates through you get stuck.

Perhaps start with something like statusline, resolve the knock-on
effects of marking that const and at that point finalise it as a single
patch. If a subset of the changes can be applied standalone, consider
separating that out. But really, whatever works for you is fine.

Oliver

[PATCH 3/6] zle_vi: Mark variables with const init as const

[Jörg Sommer]

Because these variables are initialized with as constant string, they should
be marked as *const* to make the compiler running with `-Wwrite-strings`
more happy.
---
 Src/Zle/zle_vi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/Zle/zle_vi.c b/Src/Zle/zle_vi.c
index 24d9de6ea..6692df830 100644
--- a/Src/Zle/zle_vi.c
+++ b/Src/Zle/zle_vi.c
@@ -1014,7 +1014,7 @@ int
 visetbuffer(char **args)
 {
     ZLE_INT_T ch;
-    ZLE_CHAR_T *match = ZWS("_*+");
+    const ZLE_CHAR_T *match = ZWS("_*+");
     int registermod[] = { MOD_NULL, MOD_PRI, MOD_CLIP };
     ZLE_CHAR_T *found;
 
-- 
2.43.0

[PATCH 4/6] zle_main: mark statusline as const

[Jörg Sommer]

The statusline points to a const string which content isn't editable.
---
 Src/Zle/zle_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 1afb1bf58..de8a05191 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -154,7 +154,7 @@ mod_export Widget compwidget;
 /* the status line, a null-terminated metafied string */
 
 /**/
-mod_export char *statusline;
+mod_export const char *statusline;
 
 /* The current history line and cursor position for the top line *
  * on the buffer stack.                                          */
-- 
2.43.0

[PATCH 5/6] module: Mark name argument of some functions const

[Jörg Sommer]

---
 Src/module.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/Src/module.c b/Src/module.c
index a6005f30b..b4b5d0a2c 100644
--- a/Src/module.c
+++ b/Src/module.c
@@ -356,7 +356,7 @@ finish_(UNUSED(Module m))
 
 /**/
 void
-register_module(char *n, Module_void_func setup,
+register_module(const char *n, Module_void_func setup,
 		Module_features_func features,
 		Module_enables_func enables,
 		Module_void_func boot,
@@ -846,7 +846,7 @@ Hookdef hooktab;
 
 /**/
 Hookdef
-gethookdef(char *n)
+gethookdef(const char *n)
 {
     Hookdef p;
 
@@ -974,7 +974,7 @@ deletehookdeffunc(Hookdef h, Hookfn f)
 
 /**/
 mod_export int
-deletehookfunc(char *n, Hookfn f)
+deletehookfunc(const char *n, Hookfn f)
 {
     Hookdef h = gethookdef(n);
 
@@ -1766,7 +1766,7 @@ dyn_finish_module(Module m)
 #else
 
 static Module_generic_func
-module_func(Module m, char *name)
+module_func(Module m, const char *name)
 {
 #ifdef DYNAMIC_NAME_CLASH_OK
     return (Module_generic_func) dlsym(m->u.handle, name);
@@ -2443,7 +2443,7 @@ bin_zmodload(char *nam, char **args, Options ops, UNUSED(int func))
     int ops_au = OPT_ISSET(ops,'a') || OPT_ISSET(ops,'u');
     int ret = 1, autoopts;
     /* options only allowed with -F */
-    char *fonly = "lP", *fp;
+    const char *fonly = "lP", *fp;
 
     if (ops_bcpf && !ops_au) {
 	zwarnnam(nam, "-b, -c, -f, and -p must be combined with -a or -u");
@@ -3182,7 +3182,7 @@ bin_zmodload_features(const char *nam, char **args, Options ops)
 	} else if (OPT_ISSET(ops, 'L'))
 	    printf("zmodload -F %s ", m->node.nam);
 	for (fp = features, ep = enables; *fp; fp++, ep++) {
-	    char *onoff;
+	    const char *onoff;
 	    int term;
 	    if (*args) {
 		char **argp;
@@ -3452,7 +3452,8 @@ autofeatures(const char *cmdnam, const char *module, char **features,
 	defm = NULL;
 
     for (; *features; features++) {
-	char *fnam, *typnam, *feature;
+	char *fnam, *feature;
+	const char *typnam;
 	int add, fchar, flags = defflags;
 	autofeaturefn_t fn;
 
-- 
2.43.0

[PATCH 6/6] zsh: mark hookdef.name as const

[Jörg Sommer]

At least *zle_main* uses const strings to initialize its
structure *zlehooks*.
---
 Src/zsh.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/zsh.h b/Src/zsh.h
index a0243e98e..fae62b8d0 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1578,7 +1578,7 @@ typedef int (*Hookfn) _((Hookdef, void *));
 
 struct hookdef {
     Hookdef next;
-    char *name;
+    const char *name;
     Hookfn def;
     int flags;
     LinkList funcs;
-- 
2.43.0