Make out of range type conversions explicit

Message ID 1593525480-5685-1-git-send-email-gbenson@redhat.com
State New
Headers show
Series
  • Make out of range type conversions explicit
Related show

Commit Message

Shahab Vahedi via Gdb-patches June 30, 2020, 1:58 p.m.
HI all,

Clang fails to compile two testcases with the following warning:
implicit conversion from 'X' to 'Y' changes value from x to y
[-Wconstant-conversion].  This patch adds casts that make the
value-changing conversions explicit.

Checked on Fedora 31 x86_64, GCC and clang.  Ok to commit?

Cheers,
Gary

--
gdb/testsuite/ChangeLog:

	* gdb.base/charset.c (main): Explicitly cast values which are
	out of range of their destination types.
	* gdb.base/structs2.c (main): Likewise.
---
 gdb/testsuite/ChangeLog           | 6 ++++++
 gdb/testsuite/gdb.base/charset.c  | 6 +++---
 gdb/testsuite/gdb.base/structs2.c | 2 +-
 3 files changed, 10 insertions(+), 4 deletions(-)

-- 
1.8.3.1

Comments

Shahab Vahedi via Gdb-patches June 30, 2020, 5:52 p.m. | #1
If the approach is acceptable, we should at least make it clear, through 
comments, that this is being done for clang. Having these casts in the 
code, without an explanation, is a bit cryptic.

On 6/30/20 10:58 AM, Gary Benson via Gdb-patches wrote:
> HI all,

> 

> Clang fails to compile two testcases with the following warning:

> implicit conversion from 'X' to 'Y' changes value from x to y

> [-Wconstant-conversion].  This patch adds casts that make the

> value-changing conversions explicit.

> 

> Checked on Fedora 31 x86_64, GCC and clang.  Ok to commit?

> 

> Cheers,

> Gary

> 

> --

> gdb/testsuite/ChangeLog:

> 

> 	* gdb.base/charset.c (main): Explicitly cast values which are

> 	out of range of their destination types.

> 	* gdb.base/structs2.c (main): Likewise.

> ---

>   gdb/testsuite/ChangeLog           | 6 ++++++

>   gdb/testsuite/gdb.base/charset.c  | 6 +++---

>   gdb/testsuite/gdb.base/structs2.c | 2 +-

>   3 files changed, 10 insertions(+), 4 deletions(-)

> 

> diff --git a/gdb/testsuite/gdb.base/charset.c b/gdb/testsuite/gdb.base/charset.c

> index ec4927d..54bd2dd 100644

> --- a/gdb/testsuite/gdb.base/charset.c

> +++ b/gdb/testsuite/gdb.base/charset.c

> @@ -141,14 +141,14 @@ int main ()

>                  120,

>                  7, 8, 12,

>                  10, 13, 9,

> -               11, 162, 17);

> +               11, (char) 162, 17);

>     fill_run (iso_8859_1_string, 7, 26, 65);

>     fill_run (iso_8859_1_string, 33, 26, 97);

>     fill_run (iso_8859_1_string, 59, 10, 48);

>   

>     /* Initialize ebcdic_us_string.  */

>     init_string (ebcdic_us_string,

> -               167,

> +               (char) 167,

>                  47, 22, 12,

>                  37, 13, 5,

>                  11, 74, 17);

> @@ -165,7 +165,7 @@ int main ()

>   

>     /* Initialize ibm1047_string.  */

>     init_string (ibm1047_string,

> -               167,

> +               (char) 167,

>                  47, 22, 12,

>                  37, 13, 5,

>                  11, 74, 17);

> diff --git a/gdb/testsuite/gdb.base/structs2.c b/gdb/testsuite/gdb.base/structs2.c

> index 7c8be03..2847cd6 100644

> --- a/gdb/testsuite/gdb.base/structs2.c

> +++ b/gdb/testsuite/gdb.base/structs2.c

> @@ -13,7 +13,7 @@ static void param_reg (register signed char pr_char,

>   

>     bkpt = 0;

>     param_reg (120, 130, 32000, 33000);

> -  param_reg (130, 120, 33000, 32000);

> +  param_reg ((signed char) 130, 120, (short) 33000, 32000);

>   

>     return 0;

>   }

>
Pedro Alves July 2, 2020, 8:23 p.m. | #2
On 6/30/20 2:58 PM, Gary Benson via Gdb-patches wrote:
> HI all,

> 

> Clang fails to compile two testcases with the following warning:

> implicit conversion from 'X' to 'Y' changes value from x to y

> [-Wconstant-conversion].  This patch adds casts that make the

> value-changing conversions explicit.


It's helpful if you show the full error.  Like:

gdb compile failed, /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/charset.c:144:20: warning: 
      implicit conversion from 'int' to 'char' changes value from 162 to -94
      [-Wconstant-conversion]
               11, 162, 17);
                   ^~~
/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/charset.c:151:16: warning: 
      implicit conversion from 'int' to 'char' changes value from 167 to -89
      [-Wconstant-conversion]
               167,
               ^~~
/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/charset.c:168:16: warning: 
      implicit conversion from 'int' to 'char' changes value from 167 to -89
      [-Wconstant-conversion]
               167,
               ^~~
3 warnings generated.

                === gdb Summary ===

# of untested testcases         1

Above, I think a better fix would be to change init_string to take
unsigned char parameters, since we're really passing down raw bytes.

The other one is:

~~~~~~
gdb compile failed, /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/structs2.c:16:14: warning: 
      implicit conversion from 'int' to 'signed char' changes value from 130 to
      -126 [-Wconstant-conversion]
  param_reg (130, 120, 33000, 32000);
  ~~~~~~~~~  ^~~
/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/structs2.c:16:24: warning: 
      implicit conversion from 'int' to 'short' changes value from 33000 to
      -32536 [-Wconstant-conversion]
  param_reg (130, 120, 33000, 32000);
  ~~~~~~~~~            ^~~~~
2 warnings generated.
WARNING: Prototypes not supported, rebuilding with -DNO_PROTOTYPES
gdb compile failed, /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/structs2.c:16:14: warning: 
      implicit conversion from 'int' to 'signed char' changes value from 130 to
      -126 [-Wconstant-conversion]
  param_reg (130, 120, 33000, 32000);
  ~~~~~~~~~  ^~~
/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/structs2.c:16:24: warning: 
      implicit conversion from 'int' to 'short' changes value from 33000 to
      -32536 [-Wconstant-conversion]
  param_reg (130, 120, 33000, 32000);
  ~~~~~~~~~            ^~~~~
2 warnings generated.

                === gdb Summary ===

# of untested testcases         1
~~~~~~

Here, param_reg's prototype is:

 static void param_reg (register signed char pr_char,
 		       register unsigned char pr_uchar,
 		       register short pr_short,
 		       register unsigned short pr_ushort);
 
pr_char and pr_short are signed, so how about just passing
down negative numbers.  That's what the testcase expects
GDB will show:

gdb_test "continue" \
    ".*pr_char=-126.*pr_uchar=120.*pr_short=-32536.*pr_ushort=32000.*bkpt = 1.*" \
    "structs2 continue2"

I think it's best to push fix each testcase in its own commit.

See patches attached.
From c28e13dcfba073df759db149c70f3512d47b287a Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Thu, 2 Jul 2020 15:54:36 +0100
Subject: [PATCH 1/2] Fix gdb.base/charset.exp with Clang

gdb.base/charset.exp fails to run with Clang, because of:

 gdb compile failed, /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/charset.c:144:20: warning:
       implicit conversion from 'int' to 'char' changes value from 162 to -94
       [-Wconstant-conversion]
		11, 162, 17);
		    ^~~
 /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/charset.c:151:16: warning:
       implicit conversion from 'int' to 'char' changes value from 167 to -89
       [-Wconstant-conversion]
		167,
		^~~
 /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/charset.c:168:16: warning:
       implicit conversion from 'int' to 'char' changes value from 167 to -89
       [-Wconstant-conversion]
		167,
		^~~
 3 warnings generated.

		 === gdb Summary ===

 # of untested testcases         1

Fix it by changing init_string to take unsigned char parameters.
---
 gdb/testsuite/gdb.base/charset.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/gdb.base/charset.c b/gdb/testsuite/gdb.base/charset.c
index ec4927da515..20d548b1928 100644
--- a/gdb/testsuite/gdb.base/charset.c
+++ b/gdb/testsuite/gdb.base/charset.c
@@ -77,12 +77,21 @@ short short_array[3];
 int int_array[3];
 long long_array[3];
 
+/* These are unsigned char so we can pass down characters >127 without
+   explicit casts or warnings.  */
+
 void
 init_string (char string[],
-             char x,
-             char alert, char backspace, char form_feed,
-             char line_feed, char carriage_return, char horizontal_tab,
-             char vertical_tab, char cent, char misc_ctrl)
+	     unsigned char x,
+	     unsigned char alert,
+	     unsigned char backspace,
+	     unsigned char form_feed,
+	     unsigned char line_feed,
+	     unsigned char carriage_return,
+	     unsigned char horizontal_tab,
+	     unsigned char vertical_tab,
+	     unsigned char cent,
+	     unsigned char misc_ctrl)
 {
   int i;
 

base-commit: c2ecccb33c307faa21f4d2f47348e7346b032d94
-- 
2.14.5
From f10f83231bcf196cf80c8214f6460484e330f97d Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>

Date: Thu, 2 Jul 2020 19:32:40 +0100
Subject: [PATCH 2/2] Fix gdb.base/structs2.exp with Clang

gdb.base/structs2.exp fails to run with Clang, because of:

 gdb compile failed, /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/structs2.c:16:14: warning:
       implicit conversion from 'int' to 'signed char' changes value from 130 to
       -126 [-Wconstant-conversion]
   param_reg (130, 120, 33000, 32000);
   ~~~~~~~~~  ^~~
 /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/structs2.c:16:24: warning:
       implicit conversion from 'int' to 'short' changes value from 33000 to
       -32536 [-Wconstant-conversion]
   param_reg (130, 120, 33000, 32000);
   ~~~~~~~~~            ^~~~~
 2 warnings generated.

		 === gdb Summary ===

 # of untested testcases         1

Fix it by passing actual negative numbers.
---
 gdb/testsuite/gdb.base/structs2.c   | 2 +-
 gdb/testsuite/gdb.base/structs2.exp | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/testsuite/gdb.base/structs2.c b/gdb/testsuite/gdb.base/structs2.c
index 7c8be035221..aac7bce8c15 100644
--- a/gdb/testsuite/gdb.base/structs2.c
+++ b/gdb/testsuite/gdb.base/structs2.c
@@ -13,7 +13,7 @@ main ()
 
   bkpt = 0;
   param_reg (120, 130, 32000, 33000);
-  param_reg (130, 120, 33000, 32000);
+  param_reg (-120, 130, -32000, 33000);
 
   return 0;
 }
diff --git a/gdb/testsuite/gdb.base/structs2.exp b/gdb/testsuite/gdb.base/structs2.exp
index 8a7d9c69378..5722be3109c 100644
--- a/gdb/testsuite/gdb.base/structs2.exp
+++ b/gdb/testsuite/gdb.base/structs2.exp
@@ -49,5 +49,5 @@ if [test_compiler_info gcc-3-*] {
   setup_xfail hppa*-* gcc/15860
 }
 gdb_test "continue" \
-    ".*pr_char=-126.*pr_uchar=120.*pr_short=-32536.*pr_ushort=32000.*bkpt = 1.*" \
+    ".*pr_char=-120.*pr_uchar=130.*pr_short=-32000.*pr_ushort=33000.*bkpt = 1.*" \
     "structs2 continue2"
-- 
2.14.5
Shahab Vahedi via Gdb-patches July 3, 2020, 9:30 a.m. | #3
Pedro Alves wrote:
> diff --git a/gdb/testsuite/gdb.base/structs2.c b/gdb/testsuite/gdb.base/structs2.c

> index 7c8be035221..aac7bce8c15 100644

> --- a/gdb/testsuite/gdb.base/structs2.c

> +++ b/gdb/testsuite/gdb.base/structs2.c

> @@ -13,7 +13,7 @@ main ()

>  

>    bkpt = 0;

>    param_reg (120, 130, 32000, 33000);

> -  param_reg (130, 120, 33000, 32000);

> +  param_reg (-120, 130, -32000, 33000);

>  

>    return 0;

>  }


On first glance I thought this was a copy-paste error.  I don't think
it is, but, just in case, can you confirm the change above is what you
intended?

> diff --git a/gdb/testsuite/gdb.base/structs2.exp b/gdb/testsuite/gdb.base/structs2.exp

> index 8a7d9c69378..5722be3109c 100644

> --- a/gdb/testsuite/gdb.base/structs2.exp

> +++ b/gdb/testsuite/gdb.base/structs2.exp

> @@ -49,5 +49,5 @@ if [test_compiler_info gcc-3-*] {

>    setup_xfail hppa*-* gcc/15860

>  }

>  gdb_test "continue" \

> -    ".*pr_char=-126.*pr_uchar=120.*pr_short=-32536.*pr_ushort=32000.*bkpt = 1.*" \

> +    ".*pr_char=-120.*pr_uchar=130.*pr_short=-32000.*pr_ushort=33000.*bkpt = 1.*" \

>      "structs2 continue2"


-- 
Gary Benson - he / him / his
Principal Software Engineer, Red Hat
Pedro Alves July 3, 2020, 10:42 a.m. | #4
On 7/3/20 10:30 AM, Gary Benson wrote:
> Pedro Alves wrote:

>> diff --git a/gdb/testsuite/gdb.base/structs2.c b/gdb/testsuite/gdb.base/structs2.c

>> index 7c8be035221..aac7bce8c15 100644

>> --- a/gdb/testsuite/gdb.base/structs2.c

>> +++ b/gdb/testsuite/gdb.base/structs2.c

>> @@ -13,7 +13,7 @@ main ()

>>  

>>    bkpt = 0;

>>    param_reg (120, 130, 32000, 33000);

>> -  param_reg (130, 120, 33000, 32000);

>> +  param_reg (-120, 130, -32000, 33000);

>>  

>>    return 0;

>>  }

> 

> On first glance I thought this was a copy-paste error.  I don't think

> it is, but, just in case, can you confirm the change above is what you

> intended?


What do you mean by "this"?  If you mean, the proposed change instead of:

    param_reg (120, 130, 32000, 33000);
 -  param_reg (130, 120, 33000, 32000);
 +  param_reg (-130, 120, -33000, 32000);

It's just that -130 and -33000 overflows, so I swapped the
numbers back.
Shahab Vahedi via Gdb-patches July 3, 2020, 1:09 p.m. | #5
Pedro Alves wrote:
> On 7/3/20 10:30 AM, Gary Benson wrote:

> > Pedro Alves wrote:

> > > diff --git a/gdb/testsuite/gdb.base/structs2.c b/gdb/testsuite/gdb.base/structs2.c

> > > index 7c8be035221..aac7bce8c15 100644

> > > --- a/gdb/testsuite/gdb.base/structs2.c

> > > +++ b/gdb/testsuite/gdb.base/structs2.c

> > > @@ -13,7 +13,7 @@ main ()

> > >  

> > >    bkpt = 0;

> > >    param_reg (120, 130, 32000, 33000);

> > > -  param_reg (130, 120, 33000, 32000);

> > > +  param_reg (-120, 130, -32000, 33000);

> > >  

> > >    return 0;

> > >  }

> > 

> > On first glance I thought this was a copy-paste error.  I don't

> > think it is, but, just in case, can you confirm the change above

> > is what you intended?

> 

> What do you mean by "this"?  If you mean, the proposed change

> instead of:

> 

>     param_reg (120, 130, 32000, 33000);

>  -  param_reg (130, 120, 33000, 32000);

>  +  param_reg (-130, 120, -33000, 32000);

> 

> It's just that -130 and -33000 overflows, so I swapped the

> numbers back.


Cool, thank you.

Cheers,
Gary

-- 
Gary Benson - he / him / his
Principal Software Engineer, Red Hat
Pedro Alves July 3, 2020, 2:05 p.m. | #6
On 7/3/20 2:09 PM, Gary Benson wrote:
> Pedro Alves wrote:

>> On 7/3/20 10:30 AM, Gary Benson wrote:

>>> Pedro Alves wrote:

>>>> diff --git a/gdb/testsuite/gdb.base/structs2.c b/gdb/testsuite/gdb.base/structs2.c

>>>> index 7c8be035221..aac7bce8c15 100644

>>>> --- a/gdb/testsuite/gdb.base/structs2.c

>>>> +++ b/gdb/testsuite/gdb.base/structs2.c

>>>> @@ -13,7 +13,7 @@ main ()

>>>>  

>>>>    bkpt = 0;

>>>>    param_reg (120, 130, 32000, 33000);

>>>> -  param_reg (130, 120, 33000, 32000);

>>>> +  param_reg (-120, 130, -32000, 33000);

>>>>  

>>>>    return 0;

>>>>  }

>>>

>>> On first glance I thought this was a copy-paste error.  I don't

>>> think it is, but, just in case, can you confirm the change above

>>> is what you intended?

>>

>> What do you mean by "this"?  If you mean, the proposed change

>> instead of:

>>

>>     param_reg (120, 130, 32000, 33000);

>>  -  param_reg (130, 120, 33000, 32000);

>>  +  param_reg (-130, 120, -33000, 32000);

>>

>> It's just that -130 and -33000 overflows, so I swapped the

>> numbers back.

> 

> Cool, thank you.

> 


I've merged both patches.

Thanks,
Pedro Alves
Shahab Vahedi via Gdb-patches July 7, 2020, 11:37 a.m. | #7
Pedro Alves wrote:
> I've merged both patches.


Thanks Pedro.

Cheers,
Gary

-- 
Gary Benson - he / him / his
Principal Software Engineer, Red Hat

Patch

diff --git a/gdb/testsuite/gdb.base/charset.c b/gdb/testsuite/gdb.base/charset.c
index ec4927d..54bd2dd 100644
--- a/gdb/testsuite/gdb.base/charset.c
+++ b/gdb/testsuite/gdb.base/charset.c
@@ -141,14 +141,14 @@  int main ()
                120,
                7, 8, 12,
                10, 13, 9,
-               11, 162, 17);
+               11, (char) 162, 17);
   fill_run (iso_8859_1_string, 7, 26, 65);
   fill_run (iso_8859_1_string, 33, 26, 97);
   fill_run (iso_8859_1_string, 59, 10, 48);
 
   /* Initialize ebcdic_us_string.  */
   init_string (ebcdic_us_string,
-               167,
+               (char) 167,
                47, 22, 12,
                37, 13, 5,
                11, 74, 17);
@@ -165,7 +165,7 @@  int main ()
 
   /* Initialize ibm1047_string.  */
   init_string (ibm1047_string,
-               167,
+               (char) 167,
                47, 22, 12,
                37, 13, 5,
                11, 74, 17);
diff --git a/gdb/testsuite/gdb.base/structs2.c b/gdb/testsuite/gdb.base/structs2.c
index 7c8be03..2847cd6 100644
--- a/gdb/testsuite/gdb.base/structs2.c
+++ b/gdb/testsuite/gdb.base/structs2.c
@@ -13,7 +13,7 @@  static void param_reg (register signed char pr_char,
 
   bkpt = 0;
   param_reg (120, 130, 32000, 33000);
-  param_reg (130, 120, 33000, 32000);
+  param_reg ((signed char) 130, 120, (short) 33000, 32000);
 
   return 0;
 }