[patch] Fix Sun compiler compat. for empty structs (PR build/14003)

[patch] Fix Sun compiler compat. for empty structs (PR build/14003)

[Jan Kratochvil]

Hi,

build fails on solaris in infrun.c because of empty inferior_suspend_state struct
http://sourceware.org/bugzilla/show_bug.cgi?id=14003

I do not think the struct should be completely removed as it may suggest where
is appropriate to put some possible new fields.

Tom suggested putting there 'char dummy;' but this degrade normal systems
because of some worse systems.

Providing autoconf check for it seems excessive for me.

Any style issues with this fix?


Thanks,
Jan


2012-06-12  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR build/14003
	* inferior.h (struct inferior_suspend_state): Comment out.
	(struct inferior): Comment out the field suspend.
	* infrun.c (struct infcall_suspend_state): Comment out the field
	inferior_suspend.
	(save_infcall_suspend_state, restore_infcall_suspend_state): Comment
	out its assignment.

diff --git a/gdb/inferior.h b/gdb/inferior.h
index dfcbda4..c4ad434 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -395,11 +395,13 @@ struct inferior_control_state
 
 /* Inferior process specific part of `struct infcall_suspend_state'.
 
-   Inferior thread counterpart is `struct thread_suspend_state'.  */
+   Inferior thread counterpart is `struct thread_suspend_state'.
 
 struct inferior_suspend_state
 {
+  Currently unused and some compilers do not accept empty structures.
 };
+ */
 
 /* GDB represents the state of each program execution with an object
    called an inferior.  An inferior typically corresponds to a process
@@ -430,8 +432,11 @@ struct inferior
   struct inferior_control_state control;
 
   /* State of inferior process to restore after GDB is done with an inferior
-     call.  See `struct inferior_suspend_state'.  */
+     call.  See `struct inferior_suspend_state'.
+
+     Currently unused and some compilers do not accept empty structures.
   struct inferior_suspend_state suspend;
+   */
 
   /* True if this was an auto-created inferior, e.g. created from
      following a fork; false, if this inferior was manually added by
diff --git a/gdb/infrun.c b/gdb/infrun.c
index b98e379..81182c2 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6639,7 +6639,10 @@ siginfo_make_value (struct gdbarch *gdbarch, struct internalvar *var,
 struct infcall_suspend_state
 {
   struct thread_suspend_state thread_suspend;
+
+  /* Currently unused and some compilers do not accept empty structures.
   struct inferior_suspend_state inferior_suspend;
+   */
 
   /* Other fields:  */
   CORE_ADDR stop_pc;
@@ -6693,7 +6696,10 @@ save_infcall_suspend_state (void)
     }
 
   inf_state->thread_suspend = tp->suspend;
+
+  /* Currently unused and some compilers do not accept empty structures.
   inf_state->inferior_suspend = inf->suspend;
+   */
 
   /* run_inferior_call will not use the signal due to its `proceed' call with
      GDB_SIGNAL_0 anyway.  */
@@ -6717,7 +6723,10 @@ restore_infcall_suspend_state (struct infcall_suspend_state *inf_state)
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
 
   tp->suspend = inf_state->thread_suspend;
+
+  /* Currently unused and some compilers do not accept empty structures.
   inf->suspend = inf_state->inferior_suspend;
+   */
 
   stop_pc = inf_state->stop_pc;
 

Re: [patch] Fix Sun compiler compat. for empty structs (PR build/14003)

[Joel Brobecker]

> Tom suggested putting there 'char dummy;' but this degrade normal systems
> because of some worse systems.

Actually, the compiler is somewhat correct in this case.  In C89,
it's really fuzzy whether empty structs are allowed or not, but
my understanding is that it is forbidden with C99:

        A structure type describes a sequentially allocated nonempty
        set of member objects [...]

So I think that we shouldn't be defining empty structs anyway
(to my dismay - I don't know what would be wrong with empty
structures, but ISTR seeing a discussion about something similar
with Ada, whereby two distinct objects must have distinct addresses,
and thus must have a non-zero size).

> Providing autoconf check for it seems excessive for me.

In light of the above, agreed.

> 2012-06-12  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	PR build/14003
> 	* inferior.h (struct inferior_suspend_state): Comment out.
> 	(struct inferior): Comment out the field suspend.
> 	* infrun.c (struct infcall_suspend_state): Comment out the field
> 	inferior_suspend.
> 	(save_infcall_suspend_state, restore_infcall_suspend_state): Comment
> 	out its assignment.

Just my 2 cents: Given the number of such structs being allocated,
I'd rather waste the few bytes than making the code uglier.  But
it's not a strong opinion, and I'd accept this patch too. Perhaps
using #if 0 instead might make it looks like a little less unusual,
not sure...

-- 
Joel

Re: [patch] Fix Sun compiler compat. for empty structs (PR build/14003)

[Jan Kratochvil]

On Tue, 12 Jun 2012 23:24:13 +0200, Joel Brobecker wrote:
>         A structure type describes a sequentially allocated nonempty
>         set of member objects [...]

oops, OK.


> Perhaps using #if 0 instead might make it looks like a little less unusual,
> not sure...

While #if 0 is IIRC forbidden in general it would at least permit ctags to
work.


Thanks,
Jan

[patchv2] Fix Sun compiler compat. for empty structs (PR build/14003)

[Jan Kratochvil]

On Tue, 12 Jun 2012 23:51:16 +0200, Jan Kratochvil wrote:
> On Tue, 12 Jun 2012 23:24:13 +0200, Joel Brobecker wrote:
> >         A structure type describes a sequentially allocated nonempty
> >         set of member objects [...]
> 
> oops, OK.

emptystruct.c:1:8: warning: struct has no members [-pedantic]


> > Perhaps using #if 0 instead might make it looks like a little less unusual,
> > not sure...
> 
> While #if 0 is IIRC forbidden in general it would at least permit ctags to
> work.


Jan


2012-06-13  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR build/14003
	* inferior.h (struct inferior_suspend_state): Comment out.
	(struct inferior): Comment out the field suspend.
	* infrun.c (struct infcall_suspend_state): Comment out the field
	inferior_suspend.
	(save_infcall_suspend_state, restore_infcall_suspend_state): Comment
	out its assignment.

diff --git a/gdb/inferior.h b/gdb/inferior.h
index dfcbda4..3945962 100644
--- a/gdb/inferior.h
+++ b/gdb/inferior.h
@@ -397,9 +397,11 @@ struct inferior_control_state
 
    Inferior thread counterpart is `struct thread_suspend_state'.  */
 
+#if 0 /* Currently unused and empty structures are not valid C.  */
 struct inferior_suspend_state
 {
 };
+#endif
 
 /* GDB represents the state of each program execution with an object
    called an inferior.  An inferior typically corresponds to a process
@@ -431,7 +433,9 @@ struct inferior
 
   /* State of inferior process to restore after GDB is done with an inferior
      call.  See `struct inferior_suspend_state'.  */
+#if 0 /* Currently unused and empty structures are not valid C.  */
   struct inferior_suspend_state suspend;
+#endif
 
   /* True if this was an auto-created inferior, e.g. created from
      following a fork; false, if this inferior was manually added by
diff --git a/gdb/infrun.c b/gdb/infrun.c
index b98e379..e6b7a5e 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -6639,7 +6639,9 @@ siginfo_make_value (struct gdbarch *gdbarch, struct internalvar *var,
 struct infcall_suspend_state
 {
   struct thread_suspend_state thread_suspend;
+#if 0 /* Currently unused and empty structures are not valid C.  */
   struct inferior_suspend_state inferior_suspend;
+#endif
 
   /* Other fields:  */
   CORE_ADDR stop_pc;
@@ -6693,7 +6695,9 @@ save_infcall_suspend_state (void)
     }
 
   inf_state->thread_suspend = tp->suspend;
+#if 0 /* Currently unused and empty structures are not valid C.  */
   inf_state->inferior_suspend = inf->suspend;
+#endif
 
   /* run_inferior_call will not use the signal due to its `proceed' call with
      GDB_SIGNAL_0 anyway.  */
@@ -6717,7 +6721,9 @@ restore_infcall_suspend_state (struct infcall_suspend_state *inf_state)
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
 
   tp->suspend = inf_state->thread_suspend;
+#if 0 /* Currently unused and empty structures are not valid C.  */
   inf->suspend = inf_state->inferior_suspend;
+#endif
 
   stop_pc = inf_state->stop_pc;
 

Re: [patchv2] Fix Sun compiler compat. for empty structs (PR build/14003)

[Mark Kettenis]

> Date: Wed, 13 Jun 2012 08:50:59 +0200
> From: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> On Tue, 12 Jun 2012 23:51:16 +0200, Jan Kratochvil wrote:
> > On Tue, 12 Jun 2012 23:24:13 +0200, Joel Brobecker wrote:
> > >         A structure type describes a sequentially allocated nonempty
> > >         set of member objects [...]
> > 
> > oops, OK.
> 
> emptystruct.c:1:8: warning: struct has no members [-pedantic]
> 
> 
> > > Perhaps using #if 0 instead might make it looks like a little less unusual,
> > > not sure...
> > 
> > While #if 0 is IIRC forbidden in general it would at least permit ctags to
> > work.
> 
> 
> Jan
> 
> 
> 2012-06-13  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	PR build/14003
> 	* inferior.h (struct inferior_suspend_state): Comment out.
> 	(struct inferior): Comment out the field suspend.
> 	* infrun.c (struct infcall_suspend_state): Comment out the field
> 	inferior_suspend.
> 	(save_infcall_suspend_state, restore_infcall_suspend_state): Comment
> 	out its assignment.
> 
> diff --git a/gdb/inferior.h b/gdb/inferior.h
> index dfcbda4..3945962 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -397,9 +397,11 @@ struct inferior_control_state
>  
>     Inferior thread counterpart is `struct thread_suspend_state'.  */
>  
> +#if 0 /* Currently unused and empty structures are not valid C.  */
>  struct inferior_suspend_state
>  {
>  };
> +#endif

At the risk of turning this into a bikeshed:

Isn't it simpler to just put a dummy member inside that struct, and forget about the #if 0 stuff?

struct inferior_suspend_state
{
	/* Empty structures are not valid C.  */
	int unused;
};

>  /* GDB represents the state of each program execution with an object
>     called an inferior.  An inferior typically corresponds to a process
> @@ -431,7 +433,9 @@ struct inferior
>  
>    /* State of inferior process to restore after GDB is done with an inferior
>       call.  See `struct inferior_suspend_state'.  */
> +#if 0 /* Currently unused and empty structures are not valid C.  */
>    struct inferior_suspend_state suspend;
> +#endif
>  
>    /* True if this was an auto-created inferior, e.g. created from
>       following a fork; false, if this inferior was manually added by
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index b98e379..e6b7a5e 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -6639,7 +6639,9 @@ siginfo_make_value (struct gdbarch *gdbarch, struct internalvar *var,
>  struct infcall_suspend_state
>  {
>    struct thread_suspend_state thread_suspend;
> +#if 0 /* Currently unused and empty structures are not valid C.  */
>    struct inferior_suspend_state inferior_suspend;
> +#endif
>  
>    /* Other fields:  */
>    CORE_ADDR stop_pc;
> @@ -6693,7 +6695,9 @@ save_infcall_suspend_state (void)
>      }
>  
>    inf_state->thread_suspend = tp->suspend;
> +#if 0 /* Currently unused and empty structures are not valid C.  */
>    inf_state->inferior_suspend = inf->suspend;
> +#endif
>  
>    /* run_inferior_call will not use the signal due to its `proceed' call with
>       GDB_SIGNAL_0 anyway.  */
> @@ -6717,7 +6721,9 @@ restore_infcall_suspend_state (struct infcall_suspend_state *inf_state)
>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
>  
>    tp->suspend = inf_state->thread_suspend;
> +#if 0 /* Currently unused and empty structures are not valid C.  */
>    inf->suspend = inf_state->inferior_suspend;
> +#endif
>  
>    stop_pc = inf_state->stop_pc;
>  
> 

Re: [patchv2] Fix Sun compiler compat. for empty structs (PR build/14003)

[Jan Kratochvil]

On Wed, 13 Jun 2012 10:30:11 +0200, Mark Kettenis wrote:
> At the risk of turning this into a bikeshed:
> 
> Isn't it simpler to just put a dummy member inside that struct, and forget
> about the #if 0 stuff?

I believe in a rule runtime on GNU system should not be negatively affected by
non-GNU systems defects compatibility.

The really correct way in this case is to autoconf compiler support for empty
structs and put there that dummy member but only on those non-GNU systems.

I can do so if anybody does not like the #if 0 construct.


Thanks,
Jan

Re: [patchv2] Fix Sun compiler compat. for empty structs (PR build/14003)

[Joel Brobecker]

> I believe in a rule runtime on GNU system should not be negatively affected by
> non-GNU systems defects compatibility.

But I don't understand why you would say that, since it's not a GNU vs
non-GNU. It's a correctness issue. We're just lucky that GCC accepts it.

How many instances of this struct are we talking about? Just a few,
right? So it's just a few wasted bytes... That's now 3 people who
suggested it, I think it's a good compromise.

> I can do so if anybody does not like the #if 0 construct.

Just repeating what I said before: I am OK with that, just not my
prefered solution. At the risk of looking like a despot, I would
not spend too much time arguing about this. This is one of these
issues that is just too minor to be wasting emails and time. Unless
there are real objections, you are the one working on the change,
so go with that you prefer.

-- 
Joel

[commit] [patchv2] Fix Sun compiler compat. for empty structs (PR build/14003)

[Jan Kratochvil]

Hi Joel,

On Wed, 13 Jun 2012 16:47:16 +0200, Joel Brobecker wrote:
> > I believe in a rule runtime on GNU system should not be negatively affected by
> > non-GNU systems defects compatibility.
> 
> But I don't understand why you would say that, since it's not a GNU vs
> non-GNU. It's a correctness issue. We're just lucky that GCC accepts it.

Because if GNU components use only the lowest common denominator of all the
other OSes, they can barely ever catch up those.


> How many instances of this struct are we talking about?

That's about principles.


> > I can do so if anybody does not like the #if 0 construct.
> 
> Just repeating what I said before: I am OK with that,

I have tried today to install Solaris 11 but I failed to install Solaris
Studio (the compiler) into it, it wants include files not present anywhere.
I wanted to (write and) test the autoconf variant there.

Therefore checked in the #if 0 variant:
	http://sourceware.org/ml/gdb-cvs/2012-06/msg00098.html


Thanks,
Jan