diff mbox

Warnings in native MinGW32 build of GDB 7.8

Message ID 83r40plpp3.fsf@gnu.org
State New
Headers show

Commit Message

Eli Zaretskii Aug. 9, 2014, 2:09 p.m. UTC
I've built today a native MinGW32 GDB 7.8, and saw warnings about
incomplete argument types:

     In file included from defs.h:631,
		      from gdb.c:19:
     gdbarch.h:429: warning: parameter has incomplete type
     gdbarch.h:430: warning: parameter has incomplete type

     In file included from target-dcache.h:21,
		      from target-dcache.c:19:
     dcache.h:42: warning: parameter has incomplete type

I fixed that as below, but I wonder why no one else saw this.  is this
because I use an ancient version of GCC?

OK to commit the below (master and 7.8 branch), with suitable
ChangeLog entries?

Comments

Yao Qi Aug. 13, 2014, 4:07 a.m. UTC | #1
On 08/09/2014 10:09 PM, Eli Zaretskii wrote:
> I fixed that as below, but I wonder why no one else saw this.  is this
> because I use an ancient version of GCC?

I don't see any warning in my mingw32 build.  I am using
i686-w64-mingw32-gcc 4.8.2.

> 
> OK to commit the below (master and 7.8 branch), with suitable
> ChangeLog entries?
> 
> --- gdb/dcache.c~0	2014-07-29 15:37:42.000000000 +0300
> +++ gdb/dcache.c	2014-08-09 16:17:31.823000000 +0300
> @@ -18,6 +18,7 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  #include "defs.h"
> +#include "target.h"	/* for 'enum target_xfer_status' */

I can see enum target_xfer_status is used in this c file.  This is good
to me.

>  #include "dcache.h"
>  #include "gdbcmd.h"
>  #include <string.h>
> 
> --- gdb/defs.h~0	2014-07-29 15:37:42.000000000 +0300
> +++ gdb/defs.h	2014-08-09 15:33:59.666750000 +0300
> @@ -628,6 +628,7 @@
>  #endif /* alloca not defined */
>  
>  /* Dynamic target-system-dependent parameters for GDB.  */
> +#include "frame.h"	/* for 'struct frame_id' */

It is unclear to me why do we need this include?

>  #include "gdbarch.h"
>  
>  /* * Maximum size of a register.  Something small, but large enough for
> 
> --- gdb/target-dcache.c~0	2014-06-11 19:34:41.000000000 +0300
> +++ gdb/target-dcache.c	2014-08-09 16:17:42.244875000 +0300
> @@ -16,6 +16,7 @@
>     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>  
>  #include "defs.h"
> +#include "target.h"	/* for 'enum target_xfer_status' */

enum target_xfer_status isn't used in target-dcache.c.  Do we really
need this?
Eli Zaretskii Aug. 13, 2014, 3:21 p.m. UTC | #2
> Date: Wed, 13 Aug 2014 12:07:22 +0800
> From: Yao Qi <yao@codesourcery.com>
> 
> On 08/09/2014 10:09 PM, Eli Zaretskii wrote:
> > I fixed that as below, but I wonder why no one else saw this.  is this
> > because I use an ancient version of GCC?
> 
> I don't see any warning in my mingw32 build.  I am using
> i686-w64-mingw32-gcc 4.8.2.

Your GCC version is eons ahead of mine, and from a different distro on
top of that.

> > OK to commit the below (master and 7.8 branch), with suitable
> > ChangeLog entries?
> > 
> > --- gdb/dcache.c~0	2014-07-29 15:37:42.000000000 +0300
> > +++ gdb/dcache.c	2014-08-09 16:17:31.823000000 +0300
> > @@ -18,6 +18,7 @@
> >     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> >  
> >  #include "defs.h"
> > +#include "target.h"	/* for 'enum target_xfer_status' */
> 
> I can see enum target_xfer_status is used in this c file.  This is good
> to me.

Not sure what you mean here.  Do you agree with this change?  If not,
why not?

> >  #include "dcache.h"
> >  #include "gdbcmd.h"
> >  #include <string.h>
> > 
> > --- gdb/defs.h~0	2014-07-29 15:37:42.000000000 +0300
> > +++ gdb/defs.h	2014-08-09 15:33:59.666750000 +0300
> > @@ -628,6 +628,7 @@
> >  #endif /* alloca not defined */
> >  
> >  /* Dynamic target-system-dependent parameters for GDB.  */
> > +#include "frame.h"	/* for 'struct frame_id' */
> 
> It is unclear to me why do we need this include?

Because 'struct frame_id' is otherwise not defined, and I get warnings
like this one:

     In file included from defs.h:631,
		      from gdb.c:19:
     gdbarch.h:429: warning: parameter has incomplete type
     gdbarch.h:430: warning: parameter has incomplete type

> > --- gdb/target-dcache.c~0	2014-06-11 19:34:41.000000000 +0300
> > +++ gdb/target-dcache.c	2014-08-09 16:17:42.244875000 +0300
> > @@ -16,6 +16,7 @@
> >     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> >  
> >  #include "defs.h"
> > +#include "target.h"	/* for 'enum target_xfer_status' */
> 
> enum target_xfer_status isn't used in target-dcache.c.  Do we really
> need this?

It is used in dcache.h which target-dcache.c includes:

     In file included from target-dcache.h:21,
		      from target-dcache.c:19:
     dcache.h:42: warning: parameter has incomplete type

Thanks for reviewing the patch.
Joel Brobecker Aug. 13, 2014, 5:42 p.m. UTC | #3
> > >  #include "defs.h"
> > > +#include "target.h"	/* for 'enum target_xfer_status' */
> > 
> > I can see enum target_xfer_status is used in this c file.  This is good
> > to me.
> 
> Not sure what you mean here.  Do you agree with this change?  If not,
> why not?

I think Yao was agreeing with the patch. I agree with the change
as well, so you can commit that part right away.

> > >  /* Dynamic target-system-dependent parameters for GDB.  */
> > > +#include "frame.h"	/* for 'struct frame_id' */
> > 
> > It is unclear to me why do we need this include?
> 
> Because 'struct frame_id' is otherwise not defined, and I get warnings
> like this one:
> 
>      In file included from defs.h:631,
> 		      from gdb.c:19:
>      gdbarch.h:429: warning: parameter has incomplete type
>      gdbarch.h:430: warning: parameter has incomplete type

I think we should fix gdbarch.h to include frame.h instead, which
effectively means adjusting gdbarch.sh. The solution you chose seems
to be relying on an indirect include, which we really really try
to avoid. Yao's question is a good example of one of the reasons
why we avoid that; but there is also the fact that not everyone
will need frame.h's declarations.

> > > --- gdb/target-dcache.c~0	2014-06-11 19:34:41.000000000 +0300
> > > +++ gdb/target-dcache.c	2014-08-09 16:17:42.244875000 +0300
> > > @@ -16,6 +16,7 @@
> > >     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> > >  
> > >  #include "defs.h"
> > > +#include "target.h"	/* for 'enum target_xfer_status' */
> > 
> > enum target_xfer_status isn't used in target-dcache.c.  Do we really
> > need this?
> 
> It is used in dcache.h which target-dcache.c includes:
> 
>      In file included from target-dcache.h:21,
> 		      from target-dcache.c:19:
>      dcache.h:42: warning: parameter has incomplete type

Same here, dcache.h should be the one including target.h.
Eli Zaretskii Aug. 13, 2014, 6:02 p.m. UTC | #4
> Date: Wed, 13 Aug 2014 10:42:27 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Yao Qi <yao@codesourcery.com>, gdb-patches@sourceware.org
> 
> > > >  #include "defs.h"
> > > > +#include "target.h"	/* for 'enum target_xfer_status' */
> > > 
> > > I can see enum target_xfer_status is used in this c file.  This is good
> > > to me.
> > 
> > Not sure what you mean here.  Do you agree with this change?  If not,
> > why not?
> 
> I think Yao was agreeing with the patch. I agree with the change
> as well, so you can commit that part right away.
> 
> > > >  /* Dynamic target-system-dependent parameters for GDB.  */
> > > > +#include "frame.h"	/* for 'struct frame_id' */
> > > 
> > > It is unclear to me why do we need this include?
> > 
> > Because 'struct frame_id' is otherwise not defined, and I get warnings
> > like this one:
> > 
> >      In file included from defs.h:631,
> > 		      from gdb.c:19:
> >      gdbarch.h:429: warning: parameter has incomplete type
> >      gdbarch.h:430: warning: parameter has incomplete type
> 
> I think we should fix gdbarch.h to include frame.h instead, which
> effectively means adjusting gdbarch.sh. The solution you chose seems
> to be relying on an indirect include, which we really really try
> to avoid. Yao's question is a good example of one of the reasons
> why we avoid that; but there is also the fact that not everyone
> will need frame.h's declarations.
> 
> > > > --- gdb/target-dcache.c~0	2014-06-11 19:34:41.000000000 +0300
> > > > +++ gdb/target-dcache.c	2014-08-09 16:17:42.244875000 +0300
> > > > @@ -16,6 +16,7 @@
> > > >     along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> > > >  
> > > >  #include "defs.h"
> > > > +#include "target.h"	/* for 'enum target_xfer_status' */
> > > 
> > > enum target_xfer_status isn't used in target-dcache.c.  Do we really
> > > need this?
> > 
> > It is used in dcache.h which target-dcache.c includes:
> > 
> >      In file included from target-dcache.h:21,
> > 		      from target-dcache.c:19:
> >      dcache.h:42: warning: parameter has incomplete type
> 
> Same here, dcache.h should be the one including target.h.

I can do the 1st and the 3rd parts, but I'd prefer not to touch
gdbarch.sh.  Could one of you please do that?

TIA
Joel Brobecker Aug. 13, 2014, 6:10 p.m. UTC | #5
> I can do the 1st and the 3rd parts, but I'd prefer not to touch
> gdbarch.sh.  Could one of you please do that?

Let me do it now...
diff mbox

Patch

--- gdb/dcache.c~0	2014-07-29 15:37:42.000000000 +0300
+++ gdb/dcache.c	2014-08-09 16:17:31.823000000 +0300
@@ -18,6 +18,7 @@ 
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "target.h"	/* for 'enum target_xfer_status' */
 #include "dcache.h"
 #include "gdbcmd.h"
 #include <string.h>

--- gdb/defs.h~0	2014-07-29 15:37:42.000000000 +0300
+++ gdb/defs.h	2014-08-09 15:33:59.666750000 +0300
@@ -628,6 +628,7 @@ 
 #endif /* alloca not defined */
 
 /* Dynamic target-system-dependent parameters for GDB.  */
+#include "frame.h"	/* for 'struct frame_id' */
 #include "gdbarch.h"
 
 /* * Maximum size of a register.  Something small, but large enough for

--- gdb/target-dcache.c~0	2014-06-11 19:34:41.000000000 +0300
+++ gdb/target-dcache.c	2014-08-09 16:17:42.244875000 +0300
@@ -16,6 +16,7 @@ 
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "target.h"	/* for 'enum target_xfer_status' */
 #include "target-dcache.h"
 #include "gdbcmd.h"
 #include "progspace.h"