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

Message ID CAKdteOaix2wyhp62oQrmf6TigBOBdJK1cVDJMzFqqM2b47KHOw@mail.gmail.com
State New
Headers show
Series
  • Cast "const char *" pointers to "char *" to avoid compiler warnings.
Related show

Commit Message

Christophe Lyon Oct. 1, 2018, 9:33 p.m.
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
commit 349a08c43cd4baf0d93f28ea8ca7351bf9606d50
Author: Christophe Lyon <christophe.lyon@linaro.org>
Date:   Mon Oct 1 18:53:37 2018 +0000

    Cast "const char *" pointers to "char *" to avoid compiler warnings.
    
    2018-10-01  Christophe Lyon  <christophe.lyon@linaro.org>
    
    	* 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 *".

Comments

Craig Howland Oct. 1, 2018, 10:50 p.m. | #1
On 10/01/2018 05:33 PM, Christophe Lyon wrote:
> 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

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 Oct. 2, 2018, 9:10 a.m. | #2
On Tue, 2 Oct 2018 at 00:50, Craig Howland <howland@lgsinnovations.com> wrote:
>

> On 10/01/2018 05:33 PM, Christophe Lyon wrote:

> > 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

> 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.)


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.


> Craig

>

>

>
Jeffrey Walton Oct. 2, 2018, 9:18 a.m. | #3
On Tue, Oct 2, 2018 at 5:10 AM Christophe Lyon
<christophe.lyon@linaro.org> wrote:
>...  (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.)

>

> 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.


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
Richard Earnshaw (lists) Oct. 2, 2018, 9:26 a.m. | #4
On 01/10/18 22:33, Christophe Lyon wrote:
> 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

> Author: Christophe Lyon <christophe.lyon@linaro.org>

> Date:   Mon Oct 1 18:53:37 2018 +0000

> 

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

>     

>     2018-10-01  Christophe Lyon  <christophe.lyon@linaro.org>

>     

>     	* 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.

>    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

> @@ -515,7 +515,7 @@ restart:

>      }

>  # define FAIL	goto restart

>  #else

> -  locale = new_locale;

> +  locale = (char *) new_locale;

>  # define FAIL	return NULL

>  #endif

>  

>
Christophe Lyon Oct. 5, 2018, 9:32 a.m. | #5
On Tue, 2 Oct 2018 at 11:26, Richard Earnshaw (lists)
<Richard.Earnshaw@arm.com> wrote:
>

> On 01/10/18 22:33, Christophe Lyon wrote:

> > 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

> > Author: Christophe Lyon <christophe.lyon@linaro.org>

> > Date:   Mon Oct 1 18:53:37 2018 +0000

> >

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

> >

> >     2018-10-01  Christophe Lyon  <christophe.lyon@linaro.org>

> >

> >       * 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.

>


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 ?

> R.

>

> >    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

> > @@ -515,7 +515,7 @@ restart:

> >      }

> >  # define FAIL        goto restart

> >  #else

> > -  locale = new_locale;

> > +  locale = (char *) new_locale;

> >  # define FAIL        return NULL

> >  #endif

> >

> >

>
commit 2f4f47d593d6bdbd62510afe4c3c2298615d5762
Author: Christophe Lyon <christophe.lyon@linaro.org>
Date:   Fri Oct 5 09:11:05 2018 +0000

    Declare "cs" variable as "const char *"
    
    Instead of "char *" to avoid compiler warnings.
    This is OK because "cs" is only used as input of strcmp.
    
    2018-10-01  Christophe Lyon  <christophe.lyon@linaro.org>
    
    	* newlib/libc/ctype/jp2uc.c (_jp2uc_l, _uc2jp_l): Declare "cs" as
    	const.

diff --git a/newlib/libc/ctype/jp2uc.c b/newlib/libc/ctype/jp2uc.c
index b89b5ea..5e30f09 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();
+  const char * cs = l ? __locale_charset(l) : __current_locale_charset();
   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();
+  const char * cs = l ? __locale_charset(l) : __current_locale_charset();
   if (0 == strcmp (cs, "JIS"))
     c = __uc2jp (c, JP_JIS);
   else if (0 == strcmp (cs, "SJIS"))
commit c6af2b7be0e94935c589307c14eeb9f53901a811
Author: Christophe Lyon <christophe.lyon@linaro.org>
Date:   Mon Oct 1 18:53:37 2018 +0000

    Cast "const char *" pointers to "char *" to avoid compiler warnings.
    
    2018-10-01  Christophe Lyon  <christophe.lyon@linaro.org>
    
    	* newlib/libc/ctype/ctype_.c (__set_ctype): Cast "_ctype_" 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/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
@@ -515,7 +515,7 @@ restart:
     }
 # define FAIL	goto restart
 #else
-  locale = new_locale;
+  locale = (char *) new_locale;
 # define FAIL	return NULL
 #endif
Sebastian Huber Oct. 5, 2018, 11:18 a.m. | #6
On 05/10/2018 11:32, Christophe Lyon wrote:
> commit c6af2b7be0e94935c589307c14eeb9f53901a811

> Author: Christophe Lyon<christophe.lyon@linaro.org>

> Date:   Mon Oct 1 18:53:37 2018 +0000

>

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

>      

>      2018-10-01  Christophe Lyon<christophe.lyon@linaro.org>

>      

>      	* 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  : sebastian.huber@embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
Christophe Lyon Oct. 8, 2018, 8:59 a.m. | #7
On Fri, 5 Oct 2018 at 13:18, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
>

> On 05/10/2018 11:32, Christophe Lyon wrote:

> > commit c6af2b7be0e94935c589307c14eeb9f53901a811

> > Author: Christophe Lyon<christophe.lyon@linaro.org>

> > Date:   Mon Oct 1 18:53:37 2018 +0000

> >

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

> >

> >      2018-10-01  Christophe Lyon<christophe.lyon@linaro.org>

> >

> >       * 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.

>


Hmmm, it seems it isn't used anywhere in the newlib sources, is it
really a good idea?
I'm not sure it will be clearer...

> --

> 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  : sebastian.huber@embedded-brains.de

> PGP     : Public key available on request.

>

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

>
Corinna Vinschen Oct. 10, 2018, 9:22 a.m. | #8
On Oct  2 11:10, Christophe Lyon wrote:
> On Tue, 2 Oct 2018 at 00:50, Craig Howland <howland@lgsinnovations.com> wrote:

> >

> > On 10/01/2018 05:33 PM, Christophe Lyon wrote:

> > > 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

> > 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.)

> 

> 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.


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 Oct. 10, 2018, 2:37 p.m. | #9
On Wed, 10 Oct 2018 at 11:22, Corinna Vinschen <vinschen@redhat.com> wrote:
>

> On Oct  2 11:10, Christophe Lyon wrote:

> > On Tue, 2 Oct 2018 at 00:50, Craig Howland <howland@lgsinnovations.com> wrote:

> > >

> > > On 10/01/2018 05:33 PM, Christophe Lyon wrote:

> > > > 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

> > > 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.)

> >

> > 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.

>

> 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

>

> Corinna

>

> --

> Corinna Vinschen

> Cygwin Maintainer

> Red Hat
Corinna Vinschen Oct. 10, 2018, 3:01 p.m. | #10
On Oct 10 16:37, Christophe Lyon wrote:
> On Wed, 10 Oct 2018 at 11:22, Corinna Vinschen <vinschen@redhat.com> wrote:

> >

> > On Oct  2 11:10, Christophe Lyon wrote:

> > > On Tue, 2 Oct 2018 at 00:50, Craig Howland <howland@lgsinnovations.com> wrote:

> > > >

> > > > On 10/01/2018 05:33 PM, Christophe Lyon wrote:

> > > > > 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

> > > > 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.)

> > >

> > > 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.

> >

> > 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 ?


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 Oct. 10, 2018, 8:37 p.m. | #11
On Wed, 10 Oct 2018 at 17:02, Corinna Vinschen <vinschen@redhat.com> wrote:
>

> On Oct 10 16:37, Christophe Lyon wrote:

> > On Wed, 10 Oct 2018 at 11:22, Corinna Vinschen <vinschen@redhat.com> wrote:

> > >

> > > On Oct  2 11:10, Christophe Lyon wrote:

> > > > On Tue, 2 Oct 2018 at 00:50, Craig Howland <howland@lgsinnovations.com> wrote:

> > > > >

> > > > > On 10/01/2018 05:33 PM, Christophe Lyon wrote:

> > > > > > 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

> > > > > 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.)

> > > >

> > > > 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.

> > >

> > > 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 ?

>

> I only had a look into the __loadlocal issue due to this discussion.

> For everything else, please send a new patch.

>


OK, here is the patch for jp2uc.c:

>

> Corinna

>

> --

> Corinna Vinschen

> Cygwin Maintainer

> Red Hat
commit 5d108977d84d506c0020b075775ed49f8d14a89b
Author: Christophe Lyon <christophe.lyon@linaro.org>
Date:   Fri Oct 5 09:11:05 2018 +0000

    Declare "cs" variable as "const char *"
    
    Instead of "char *" to avoid compiler warnings.
    This is OK because "cs" is only used as input of strcmp.
    
    2018-10-01  Christophe Lyon  <christophe.lyon@linaro.org>
    
    	* newlib/libc/ctype/jp2uc.c (_jp2uc_l, _uc2jp_l): Declare "cs" as
    	const.

diff --git a/newlib/libc/ctype/jp2uc.c b/newlib/libc/ctype/jp2uc.c
index b89b5ea..5e30f09 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();
+  const char * cs = l ? __locale_charset(l) : __current_locale_charset();
   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();
+  const char * cs = l ? __locale_charset(l) : __current_locale_charset();
   if (0 == strcmp (cs, "JIS"))
     c = __uc2jp (c, JP_JIS);
   else if (0 == strcmp (cs, "SJIS"))
Corinna Vinschen Oct. 11, 2018, 7:11 a.m. | #12
On Oct 10 22:37, Christophe Lyon wrote:
> On Wed, 10 Oct 2018 at 17:02, Corinna Vinschen <vinschen@redhat.com> wrote:

> >

> > On Oct 10 16:37, Christophe Lyon wrote:

> > > 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.

> >

> 

> OK, here is the patch for jp2uc.c:


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 Oct. 11, 2018, 8:45 a.m. | #13
On Thu, 11 Oct 2018 at 09:11, Corinna Vinschen <vinschen@redhat.com> wrote:
>

> On Oct 10 22:37, Christophe Lyon wrote:

> > On Wed, 10 Oct 2018 at 17:02, Corinna Vinschen <vinschen@redhat.com> wrote:

> > >

> > > On Oct 10 16:37, Christophe Lyon wrote:

> > > > 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.

> > >

> >

> > OK, here is the patch for jp2uc.c:

>

> 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?

>

> Thanks,

> Corinna

>

> --

> Corinna Vinschen

> Cygwin Maintainer

> Red Hat
From dfb509574e1cbbdf4fe9d4d3e8319bd3ffdca98c Mon Sep 17 00:00:00 2001
From: Christophe Lyon <christophe.lyon@linaro.org>
Date: Fri, 5 Oct 2018 09:11:05 +0000
Subject: [PATCH] newlib/libc/ctype/jp2uc.c: Declare "cs" variable as "const
 char *"

Instead of "char *" to avoid compiler warnings.
This is OK because "cs" is only used as input of strcmp.
---
 newlib/libc/ctype/jp2uc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/newlib/libc/ctype/jp2uc.c b/newlib/libc/ctype/jp2uc.c
index b89b5ea..5e30f09 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();
+  const char * cs = l ? __locale_charset(l) : __current_locale_charset();
   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();
+  const char * cs = l ? __locale_charset(l) : __current_locale_charset();
   if (0 == strcmp (cs, "JIS"))
     c = __uc2jp (c, JP_JIS);
   else if (0 == strcmp (cs, "SJIS"))
Corinna Vinschen Oct. 11, 2018, 2:43 p.m. | #14
On Oct 11 10:45, Christophe Lyon wrote:
> On Thu, 11 Oct 2018 at 09:11, Corinna Vinschen <vinschen@redhat.com> wrote:

> >

> > On Oct 10 22:37, Christophe Lyon wrote:

> > > On Wed, 10 Oct 2018 at 17:02, Corinna Vinschen <vinschen@redhat.com> wrote:

> > > >

> > > > On Oct 10 16:37, Christophe Lyon wrote:

> > > > > 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.

> > > >

> > >

> > > OK, here is the patch for jp2uc.c:

> >

> > 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

Patch

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();
   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
@@ -515,7 +515,7 @@  restart:
     }
 # define FAIL	goto restart
 #else
-  locale = new_locale;
+  locale = (char *) new_locale;
 # define FAIL	return NULL
 #endif