* Re: [PATCH] Make out of range type conversions explicit
2020-06-30 13:58 [PATCH] Make out of range type conversions explicit Gary Benson
@ 2020-06-30 17:52 ` Luis Machado
2020-07-02 20:23 ` Pedro Alves
1 sibling, 0 replies; 8+ messages in thread
From: Luis Machado @ 2020-06-30 17:52 UTC (permalink / raw)
To: Gary Benson, gdb-patches
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;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] Make out of range type conversions explicit
2020-06-30 13:58 [PATCH] Make out of range type conversions explicit Gary Benson
2020-06-30 17:52 ` Luis Machado
@ 2020-07-02 20:23 ` Pedro Alves
2020-07-03 9:30 ` Gary Benson
1 sibling, 1 reply; 8+ messages in thread
From: Pedro Alves @ 2020-07-02 20:23 UTC (permalink / raw)
To: Gary Benson, gdb-patches
[-- Attachment #1: Type: text/plain, Size: 3113 bytes --]
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.
[-- Attachment #2: 0001-Fix-gdb.base-charset.exp-with-Clang.patch --]
[-- Type: text/x-patch, Size: 2192 bytes --]
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
[-- Attachment #3: 0002-Fix-gdb.base-structs2.exp-with-Clang.patch --]
[-- Type: text/x-patch, Size: 1972 bytes --]
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
^ permalink raw reply [flat|nested] 8+ messages in thread