Cast "const char *" pointers to "char *" to avoid compiler warnings.

If I understand what you're saying properly, it amounts to saying that you did
not verify whether the GCC warnings about discarding const are valid or not, yet
you are suppressing them. Is this a proper understanding? If so, it seems like
these proposed patches are a bad idea, as they might be hiding a real problem,
or changing the wrong thing. (In a very quick look at locale.c, for example,
locale can definitely be written to--it is definitely not const. This implies
that the const on new_locale is what is wrong.)
Craig

Christophe Lyon

2018-10-02 09:10:20 UTC

I did have this "very quick look at locale.c" before writing the
patch, and not adding
the cast at the assignment point means removing "const" from __loadlocale()
prototype:
char *__loadlocale (struct __locale_t *loc, int category, const char
*new_locale)
which in turn has a significant impact on the callers which I hope
people familiar
with this area can confirm, or not.

It looks like several of these small patches uncover inconsistencies.

Post by Craig Howland
Craig

Jeffrey Walton

2018-10-02 09:18:41 UTC

On Tue, Oct 2, 2018 at 5:10 AM Christophe Lyon

... (In a very quick look at locale.c, for example,

Post by Craig Howland
locale can definitely be written to--it is definitely not const. This implies
that the const on new_locale is what is wrong.)

I did have this "very quick look at locale.c" before writing the
patch, and not adding
the cast at the assignment point means removing "const" from __loadlocale()
char *__loadlocale (struct __locale_t *loc, int category, const char
*new_locale)
which in turn has a significant impact on the callers which I hope
people familiar
with this area can confirm, or not.

As I understand it, you can only cast const away if the [original]
object is in fact non-const.

OK:

void foo(const char* buff)
{
((char*)buf)[0] = '\0';
}

char bar[10] = {0};
foo(bar);

Not OK:

void foo(const char* buff)
{
((char*)buf)[0] = (char)1;
}

const char bar[10] = {0};
foo(bar);

The tricky part is finding the original object.

I don't know if there is a sanitizer to help find these violations,
but I have often wondered about one.

Jeff

Corinna Vinschen

2018-10-10 09:22:33 UTC

The bug was, in fact, to define the third parameter as const, given it
gets potentially overwritten. None of the incoming values is const
anyway. I pushed a patch.

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Christophe Lyon

2018-10-10 14:37:18 UTC

The bug was, in fact, to define the third parameter as const, given it
gets potentially overwritten. None of the incoming values is const
anyway. I pushed a patch.

Thanks.

However, when building for aarch64, I'm still seeing:
newlib/libc/ctype/ctype_.c:179:16: warning: assignment discards
‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]

And I think my patch (or something similar) is still needed for jp2uc.c ?

Christophe

Post by Corinna Vinschen
Corinna
--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Corinna Vinschen

2018-10-10 15:01:56 UTC

The bug was, in fact, to define the third parameter as const, given it
gets potentially overwritten. None of the incoming values is const
anyway. I pushed a patch.

Thanks.
newlib/libc/ctype/ctype_.c:179:16: warning: assignment discards
âconstâ qualifier from pointer target type [-Wdiscarded-qualifiers]
And I think my patch (or something similar) is still needed for jp2uc.c ?

I only had a look into the __loadlocal issue due to this discussion.
For everything else, please send a new patch.

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Christophe Lyon

2018-10-10 20:37:34 UTC

The bug was, in fact, to define the third parameter as const, given it
gets potentially overwritten. None of the incoming values is const
anyway. I pushed a patch.

I only had a look into the __loadlocal issue due to this discussion.
For everything else, please send a new patch.
Corinna
--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Corinna Vinschen

2018-10-11 07:11:24 UTC

Post by Christophe Lyon
And I think my patch (or something similar) is still needed for jp2uc.c ?

I only had a look into the __loadlocal issue due to this discussion.
For everything else, please send a new patch.

Can you please send it in `git format-patch' format? The patch
doesn't apply as is. There's also no requirement anymore to add
CVS-like commit messages.

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Christophe Lyon

2018-10-11 08:45:55 UTC

Post by Christophe Lyon
And I think my patch (or something similar) is still needed for jp2uc.c ?

I only had a look into the __loadlocal issue due to this discussion.
For everything else, please send a new patch.

Can you please send it in `git format-patch' format? The patch
doesn't apply as is. There's also no requirement anymore to add
CVS-like commit messages.

Is this new version OK?

Post by Corinna Vinschen
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Corinna Vinschen

2018-10-11 14:43:40 UTC

Post by Christophe Lyon
And I think my patch (or something similar) is still needed for jp2uc.c ?

I only had a look into the __loadlocal issue due to this discussion.
For everything else, please send a new patch.

Can you please send it in `git format-patch' format? The patch
doesn't apply as is. There's also no requirement anymore to add
CVS-like commit messages.

Is this new version OK?

Yep, pushed.

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Richard Earnshaw (lists)

2018-10-02 09:26:50 UTC

Post by Christophe Lyon
Hi,
GCC complains that some assignments loose the const-ness of several
data. This small patch adds explicit (char *) casts, but I'm not
familiar enough with what newlib does with these to be sure that they
are not modified. Maybe the proper fix would be to declare the
destinations as "const"?
Christophe
newlib-4.txt
commit 349a08c43cd4baf0d93f28ea8ca7351bf9606d50
Date: Mon Oct 1 18:53:37 2018 +0000
Cast "const char *" pointers to "char *" to avoid compiler warnings.
* newlib/libc/ctype/ctype_.c (__set_ctype): Cast "_ctype_" to "char *".
* newlib/libc/ctype/jp2uc.c (_jp2uc_l, _uc2jp_l): Cast output of
"__locale_charset()" and "__current_locale_charset()" to "char *".
* newlib/libc/locale/locale.c (__loadlocale): Cast "new_locale" to
"char *".
diff --git a/newlib/libc/ctype/ctype_.c b/newlib/libc/ctype/ctype_.c
index 28727e8..851fc06 100644
--- a/newlib/libc/ctype/ctype_.c
+++ b/newlib/libc/ctype/ctype_.c
@@ -176,7 +176,7 @@ __set_ctype (struct __locale_t *loc, const char *charset)
# if defined(ALLOW_NEGATIVE_CTYPE_INDEX)
ctype_ptr = _ctype_b;
# else
- ctype_ptr = _ctype_;
+ ctype_ptr = (char *) _ctype_;
# endif
}
# if defined(ALLOW_NEGATIVE_CTYPE_INDEX)
diff --git a/newlib/libc/ctype/jp2uc.c b/newlib/libc/ctype/jp2uc.c
index b89b5ea..00272eb 100644
--- a/newlib/libc/ctype/jp2uc.c
+++ b/newlib/libc/ctype/jp2uc.c
@@ -166,7 +166,7 @@ __uc2jp (wint_t c, int type)
wint_t
_jp2uc_l (wint_t c, struct __locale_t * l)
{
- char * cs = l ? __locale_charset(l) : __current_locale_charset();
+ char * cs = l ? (char *) __locale_charset(l) : (char *) __current_locale_charset();

Why not change cs to const char *? It's only used to call strcmp.

Probably most other casts should be similarly considered.

Generally, this patch feels wrong, IMO.

R.

Post by Christophe Lyon
if (0 == strcmp (cs, "JIS"))
c = __jp2uc (c, JP_JIS);
else if (0 == strcmp (cs, "SJIS"))
@@ -186,7 +186,7 @@ _jp2uc (wint_t c)
wint_t
_uc2jp_l (wint_t c, struct __locale_t * l)
{
- char * cs = l ? __locale_charset(l) : __current_locale_charset();
+ char * cs = l ? (char *) __locale_charset(l) : (char *) __current_locale_charset();
if (0 == strcmp (cs, "JIS"))
c = __uc2jp (c, JP_JIS);
else if (0 == strcmp (cs, "SJIS"))
diff --git a/newlib/libc/locale/locale.c b/newlib/libc/locale/locale.c
index 791a775..79da35f 100644
--- a/newlib/libc/locale/locale.c
+++ b/newlib/libc/locale/locale.c
}
# define FAIL goto restart
#else
- locale = new_locale;
+ locale = (char *) new_locale;
# define FAIL return NULL
#endif

Christophe Lyon

2018-10-05 09:32:06 UTC

Post by Richard Earnshaw (lists)

On Tue, 2 Oct 2018 at 11:26, Richard Earnshaw (lists)

Why not change cs to const char *? It's only used to call strcmp.
Probably most other casts should be similarly considered.
Generally, this patch feels wrong, IMO.

OK, I've split the patch in two parts, then:
- one for jp2uc.c following Richard's suggestion
- one for the other two files.

In locale.c, I add a (char *) cast for new_locale just like what is
already done a few lines above for the Cygwin case.
In ctype_.c, it's similar in that it adds the cast to effectively
behave like in the Cygwin case.
I've noticed the comment saying that with Cygwin the values maybe
overwritten hence the absence of 'const', but
I don't know what happens for non-Cygwin environments: maybe the
proper fix would be to remove 'const' when
declaring _ctype_ and new_locale ?

Post by Richard Earnshaw (lists)
R.

Sebastian Huber

2018-10-05 11:18:15 UTC

commit c6af2b7be0e94935c589307c14eeb9f53901a811
Date: Mon Oct 1 18:53:37 2018 +0000
Cast "const char *" pointers to "char *" to avoid compiler warnings.
* newlib/libc/ctype/ctype_.c (__set_ctype): Cast "_ctype_" to "char *".
* newlib/libc/locale/locale.c (__loadlocale): Cast "new_locale" to
"char *".

There is a __DECONST() macro in <sys/cdefs.h>. I think this better
documents the purpose of these casts. It silences also -Wcast-qual warnings.
--
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail : ***@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Christophe Lyon

2018-10-08 08:59:25 UTC