diff mbox

Fix windows-nat.c for -Wnarrowing

Message ID 20180829174253.18507-1-tom@tromey.com
State New
Headers show

Commit Message

Tom Tromey Aug. 29, 2018, 5:42 p.m. UTC
Sergio pointed out that the Windows builder was failing due to the
-Wnarrowing patch, with:

../../binutils-gdb/gdb/windows-nat.c:301:27: error: narrowing conversion of '3221225477' from 'DWORD {aka long unsigned int}' to 'int' inside { } [-Wnarrowing]
   {-1, GDB_SIGNAL_UNKNOWN}};
                           ^
../../binutils-gdb/gdb/windows-nat.c:301:27: error: narrowing conversion of '3221225725' from 'DWORD {aka long unsigned int}' to 'int' inside { } [-Wnarrowing]
../../binutils-gdb/gdb/windows-nat.c:301:27: error: narrowing conversion of '2147483651' from 'DWORD {aka long unsigned int}' to 'int' inside { } [-Wnarrowing]
../../binutils-gdb/gdb/windows-nat.c:301:27: error: narrowing conversion of '2147483652' from 'DWORD {aka long unsigned int}' to 'int' inside { } [-Wnarrowing]
../../binutils-gdb/gdb/windows-nat.c:301:27: error: narrowing conversion of '3221225614' from 'DWORD {aka long unsigned int}' to 'int' inside { } [-Wnarrowing]

Looking into this, I found two things.

First, in struct xlate_exception, it is better to have "them" be of
type DWORD, as that's the type actually in use.

Second, struct xlate_exception and xlate are not used in this file,
because the code in windows_nat_target::resume is #if'd out.

This patch changes the type of "them", but also similarly #if's out
this object.

Tested by rebuilding using the mingw toolchain on x86-64 Fedora 28.

gdb/ChangeLog
2018-08-29  Tom Tromey  <tom@tromey.com>

	* windows-nat.c (struct xlate_exception) <them>: Change type to
	DWORD.
	(xlate): Fix formatting.
	(struct xlate_exception, xlate): Comment out.
---
 gdb/ChangeLog     |  7 +++++++
 gdb/windows-nat.c | 12 ++++++++----
 2 files changed, 15 insertions(+), 4 deletions(-)

Comments

Pedro Alves Aug. 29, 2018, 6:39 p.m. UTC | #1
On 08/29/2018 06:42 PM, Tom Tromey wrote:

> gdb/ChangeLog
> 2018-08-29  Tom Tromey  <tom@tromey.com>
> 
> 	* windows-nat.c (struct xlate_exception) <them>: Change type to
> 	DWORD.
> 	(xlate): Fix formatting.
> 	(struct xlate_exception, xlate): Comment out.

This is OK as is.

> -static const struct xlate_exception
> -  xlate[] =
> +static const struct xlate_exception xlate[] =
>  {
>    {EXCEPTION_ACCESS_VIOLATION, GDB_SIGNAL_SEGV},
>    {STATUS_STACK_OVERFLOW, GDB_SIGNAL_SEGV},
> @@ -298,8 +300,10 @@ static const struct xlate_exception
>    {DBG_CONTROL_C, GDB_SIGNAL_INT},
>    {EXCEPTION_SINGLE_STEP, GDB_SIGNAL_TRAP},
>    {STATUS_FLOAT_DIVIDE_BY_ZERO, GDB_SIGNAL_FPE},
> -  {-1, GDB_SIGNAL_UNKNOWN}};
> +  {-1, GDB_SIGNAL_UNKNOWN}
> +};
I guess that -1 would still cause a -Wnarrowing warning.
If so, it'd be easily fixable by removing that terminator
entry and using range-for in the other #if 0 block.
Just thinking out loud.  Please don't bother.

Thanks,
Pedro Alves
Tom Tromey Aug. 29, 2018, 6:58 p.m. UTC | #2
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> +  {-1, GDB_SIGNAL_UNKNOWN}

Pedro> I guess that -1 would still cause a -Wnarrowing warning.
Pedro> If so, it'd be easily fixable by removing that terminator
Pedro> entry and using range-for in the other #if 0 block.
Pedro> Just thinking out loud.  Please don't bother.

I didn't consider this, oops.
I rebuilt without the new #if and, yes, it failed.
So, I've added a DWORD cast there as well; then restore the #if.

I'm going to push this version.

thanks,
Tom
Tom Tromey Aug. 29, 2018, 6:59 p.m. UTC | #3
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> So, I've added a DWORD cast there as well; then restore the #if.

That's not going to work either, is it?
Since the test is

	  for (i = 0; xlate[i].them != -1; i++)

... and it should be for (DWORD) -1 ?

Tom
Pedro Alves Aug. 29, 2018, 7:09 p.m. UTC | #4
On 08/29/2018 07:59 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> So, I've added a DWORD cast there as well; then restore the #if.
> 
> That's not going to work either, is it?
> Since the test is
> 
> 	  for (i = 0; xlate[i].them != -1; i++)
> 
> ... and it should be for (DWORD) -1 ?
> 

If you're going to fix it, I'd go with my earlier suggestion:

  It'd be easily fixable by removing that terminator
  entry and using range-for in the other #if 0 block.

I.e., remote the -1 entry, and replace:

  for (i = 0; xlate[i].them != -1; i++)
    if (xlate[i].us == sig)
      {
	current_event.u.Exception.ExceptionRecord.ExceptionCode
	  = xlate[i].them;

with:

  for (const xlate_exception &x : xlate)
    if (x.us == sig)
      {
	current_event.u.Exception.ExceptionRecord.ExceptionCode
	  = x.them;

Thanks,
Pedro Alves
Eli Zaretskii Aug. 29, 2018, 7:16 p.m. UTC | #5
> From: Tom Tromey <tom@tromey.com>
> Cc: Pedro Alves <palves@redhat.com>,  gdb-patches@sourceware.org
> Date: Wed, 29 Aug 2018 12:59:52 -0600
> 
> >>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
> 
> Tom> So, I've added a DWORD cast there as well; then restore the #if.
> 
> That's not going to work either, is it?
> Since the test is
> 
> 	  for (i = 0; xlate[i].them != -1; i++)
> 
> ... and it should be for (DWORD) -1 ?

Why did you need to make .them be DWORD?  I think it should be a
'long', and then you can use -1L without any trouble.
Tom Tromey Aug. 29, 2018, 7:22 p.m. UTC | #6
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> Why did you need to make .them be DWORD?  I think it should be a
Eli> 'long', and then you can use -1L without any trouble.

Thanks for the suggestion.  I tried this just now but long doesn't work
without additional casts:

../../binutils-gdb/gdb/windows-nat.c:304:1: error: narrowing conversion of '3221225477' from 'DWORD {aka long unsigned int}' to 'long int' inside { } [-Wnarrowing]
 };
 ^
../../binutils-gdb/gdb/windows-nat.c:304:1: error: narrowing conversion of '3221225725' from 'DWORD {aka long unsigned int}' to 'long int' inside { } [-Wnarrowing]
../../binutils-gdb/gdb/windows-nat.c:304:1: error: narrowing conversion of '2147483651' from 'DWORD {aka long unsigned int}' to 'long int' inside { } [-Wnarrowing]
../../binutils-gdb/gdb/windows-nat.c:304:1: error: narrowing conversion of '2147483652' from 'DWORD {aka long unsigned int}' to 'long int' inside { } [-Wnarrowing]
../../binutils-gdb/gdb/windows-nat.c:304:1: error: narrowing conversion of '3221225614' from 'DWORD {aka long unsigned int}' to 'long int' inside { } [-Wnarrowing]

Tom
Eli Zaretskii Aug. 29, 2018, 7:28 p.m. UTC | #7
> From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>,  palves@redhat.com,  gdb-patches@sourceware.org
> Date: Wed, 29 Aug 2018 13:22:27 -0600
> 
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> Eli> Why did you need to make .them be DWORD?  I think it should be a
> Eli> 'long', and then you can use -1L without any trouble.
> 
> Thanks for the suggestion.  I tried this just now but long doesn't work
> without additional casts:
> 
> ../../binutils-gdb/gdb/windows-nat.c:304:1: error: narrowing conversion of '3221225477' from 'DWORD {aka long unsigned int}' to 'long int' inside { } [-Wnarrowing]
>  };
>  ^

I don't understand: EXCEPTION_ACCESS_VIOLATION is defined like this:

  #define STATUS_ACCESS_VIOLATION 		      0xC0000005
  #define EXCEPTION_ACCESS_VIOLATION		      STATUS_ACCESS_VIOLATION

And you've removed DWORD from the definition of xlate.  So where does
DWORD come from?
Eli Zaretskii Aug. 29, 2018, 7:32 p.m. UTC | #8
> Date: Wed, 29 Aug 2018 22:28:40 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> CC: tom@tromey.com, palves@redhat.com, gdb-patches@sourceware.org
> 
> I don't understand: EXCEPTION_ACCESS_VIOLATION is defined like this:
> 
>   #define STATUS_ACCESS_VIOLATION 		      0xC0000005
>   #define EXCEPTION_ACCESS_VIOLATION		      STATUS_ACCESS_VIOLATION
> 
> And you've removed DWORD from the definition of xlate.  So where does
> DWORD come from?

Ah, I see: it's a MinGW64 specific thing.  It defines

   #define STATUS_ACCESS_VIOLATION 		      (DWORD)0xC0000005

So I would suggest to do this:

  static const struct xlate_exception xlate[] =
  {
    {(long)EXCEPTION_ACCESS_VIOLATION, GDB_SIGNAL_SEGV},
    {(long)STATUS_STACK_OVERFLOW, GDB_SIGNAL_SEGV},
    {(long)EXCEPTION_BREAKPOINT, GDB_SIGNAL_TRAP},
    {(long)DBG_CONTROL_C, GDB_SIGNAL_INT},
    {(long)EXCEPTION_SINGLE_STEP, GDB_SIGNAL_TRAP},
    {(long)STATUS_FLOAT_DIVIDE_BY_ZERO, GDB_SIGNAL_FPE},
    {-1L, GDB_SIGNAL_UNKNOWN}
  };
Tom Tromey Aug. 29, 2018, 7:33 p.m. UTC | #9
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> I don't understand: EXCEPTION_ACCESS_VIOLATION is defined like this:

Eli>   #define STATUS_ACCESS_VIOLATION 		      0xC0000005
Eli>   #define EXCEPTION_ACCESS_VIOLATION		      STATUS_ACCESS_VIOLATION

Eli> And you've removed DWORD from the definition of xlate.  So where does
Eli> DWORD come from?

gcc -E reports:

static const struct xlate_exception xlate[] =
{
  {
# 297 "../../binutils-gdb/gdb/windows-nat.c" 3 4
  ((DWORD)0xC0000005)
# 297 "../../binutils-gdb/gdb/windows-nat.c"

etc.

So somewhere in the Fedora mingw, I guess these are defined with
explicit DWORD casts.

Tom
Tom Tromey Aug. 29, 2018, 7:34 p.m. UTC | #10
>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> So I would suggest to do this:

Please look at the newest patch and let me know what you think.
It's simpler and, IMO, more obviously correct, since there are no casts.

Tom
Eli Zaretskii Aug. 29, 2018, 7:38 p.m. UTC | #11
> From: Tom Tromey <tom@tromey.com>
> Cc: tom@tromey.com,  palves@redhat.com,  gdb-patches@sourceware.org
> Date: Wed, 29 Aug 2018 13:34:40 -0600
> 
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> Eli> So I would suggest to do this:
> 
> Please look at the newest patch and let me know what you think.

I think it's fine, thanks.
diff mbox

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index a30dacee4a3..0c4327f0637 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@ 
+2018-08-29  Tom Tromey  <tom@tromey.com>
+
+	* windows-nat.c (struct xlate_exception) <them>: Change type to
+	DWORD.
+	(xlate): Fix formatting.
+	(struct xlate_exception, xlate): Comment out.
+
 2018-08-29  Sergio Durigan Junior  <sergiodj@redhat.com>
 
 	PR gdb/23555
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index aea502638e0..404646c94b0 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -280,17 +280,19 @@  static const int *mappings;
    a segment register or not.  */
 static segment_register_p_ftype *segment_register_p;
 
+/* See windows_nat_target::resume to understand why this is commented
+   out.  */
+#if 0
 /* This vector maps the target's idea of an exception (extracted
    from the DEBUG_EVENT structure) to GDB's idea.  */
 
 struct xlate_exception
   {
-    int them;
+    DWORD them;
     enum gdb_signal us;
   };
 
-static const struct xlate_exception
-  xlate[] =
+static const struct xlate_exception xlate[] =
 {
   {EXCEPTION_ACCESS_VIOLATION, GDB_SIGNAL_SEGV},
   {STATUS_STACK_OVERFLOW, GDB_SIGNAL_SEGV},
@@ -298,8 +300,10 @@  static const struct xlate_exception
   {DBG_CONTROL_C, GDB_SIGNAL_INT},
   {EXCEPTION_SINGLE_STEP, GDB_SIGNAL_TRAP},
   {STATUS_FLOAT_DIVIDE_BY_ZERO, GDB_SIGNAL_FPE},
-  {-1, GDB_SIGNAL_UNKNOWN}};
+  {-1, GDB_SIGNAL_UNKNOWN}
+};
 
+#endif /* 0 */
 
 struct windows_nat_target final : public x86_nat_target<inf_child_target>
 {