Re: [RFC] nm header removal: Get rid of config/i386/nm-i386gnu.h header

[Thomas Schwinge <thomas at schwinge dot name>]

Hallo!

On Mon, 21 Mar 2011 16:40:00 +0100, "Pierre Muller" <pierre.muller@ics-cnrs.unistra.fr> wrote:
>   I started to work again on a final removal of remaining native header
> macros.
> 
>   The mist of remaining macros is on 
> http://sourceware.org/gdb/current/ari/
> Under the sub-title Macro.
> 
>   I tried to get rid of all the THREAD_STATE_XXX macros,
> which allows to remove config/i386/nm-i386gnu.h
> 
>   The problems are:
> 1) I don't know GNU Hurd :(

We're here to help.

> 2) I have no access to GNU Hurd machine
> (I read that it is possible to ask for one to Thomas, but if
> GDB is not usable anyhow, I am not sure it is worthwhile...)

Access to such a machine is possible, yes.  Apart from the issue 12222,
the port is in reasonable shape.

> 3) I tried to use the cross-tools from Thomas Schwinge on gcc45 from
> CompilerFarm
> but got problems to compile GDB...

Likely you're missing the libraries that GDB depends on?

> 4) Saw that there are unfixed problems related to GNU Hurd and GDB.
> http://sourceware.org/bugzilla/show_bug.cgi?id=12222

I will (be able to) continue with these issues (12222, as well as yours)
as soon as one Hurd-internal issue has been sorted out, which inhibits me
From running the testsuite:
<http://lists.gnu.org/archive/html/bug-hurd/2011-03/msg00081.html>.


> All I did was to replace the macros in config/i386/nm-i386gnu.h
> by variables and functions declared in gnu-nat.h and implemented in
> i386gnu-nat.c

Seems reasonable.

A bit of a review -- continue toi work on it if you like, otherwise I'll
pick it up later on.

> -#define THREAD_STATE_FLAVOR		i386_REGS_SEGS_STATE
> replaced by
> +extern unsigned int thread_state_flavor;

Plain int.

> -#define THREAD_STATE_SIZE		i386_THREAD_STATE_COUNT
> replaced by
> +extern unsigned int thread_state_size;

I wondered, should thread_state_flavor and thread_state_size be added to
struct proc in gnu-nat.h instead?  But then they'd need to be initialized
From i386gnu-nat.c which is currently not done (but could easily be, of
course).  As far as I can tell, `struct proc's are only instantiated in
gnu-nat.c:make_proc.  Anyway, we can do such cleanups later on.  I don't
know enough about GDB yet in order to suggest ``the GDB way'' of doing
this.  What you have done looks sufficient for now.

> -#define THREAD_STATE_SET_TRACED(state) \
> -  	((struct i386_thread_state *) (state))->efl |= 0x100
> replaced by
> +extern void thread_state_set_traced (thread_state_t *state);
> -#define THREAD_STATE_CLEAR_TRACED(state) \
> -  	((((struct i386_thread_state *) (state))->efl &= ~0x100), 1)
> replaced by
> +extern void thread_state_clear_traced (thread_state_t *state);

>   2) the TRACED macros seems to be ment to return a value
> the new flag value for THREAD_STATE_SET_TRACED and 1 for
> THREAD_STATE_CLEAR_TRACED
> but the return value is unused, so I declared the substitute functions
> as returning void, is this OK?

Seems correct.

> 2011-03-21  Pierre Muller  <muller@ics.u-strasbg.fr>
> 
> 	Get rid of i386 GNU hurd native header.
> 	config/i386/i386nu.mh (NAT_FILE): Remove.

Missing `*'s in front of the file names.

> diff --git a/gdb/config/i386/i386gnu.mh b/gdb/config/i386/i386gnu.mh
> index d4e09d9..bc4e221 100644
> --- a/gdb/config/i386/i386gnu.mh
> +++ b/gdb/config/i386/i386gnu.mh
> @@ -3,7 +3,7 @@ NATDEPFILES= i386gnu-nat.o gnu-nat.o corelow.o core-regset.o
> fork-child.o \
>  	     notify_S.o process_reply_S.o msg_reply_S.o \
>  	     msg_U.o exc_request_U.o exc_request_S.o
>  
> -NAT_FILE= nm-i386gnu.h
> +# Removed NATFILE nm-i386gnu.h

Is it worthwhile to keep this comment?  (I don't think so.)

> diff --git a/gdb/config/i386/nm-i386gnu.h b/gdb/config/i386/nm-i386gnu.h
> deleted file mode 100644
> index 8ce6c79..0000000
> --- a/gdb/config/i386/nm-i386gnu.h
> +++ /dev/null

> -extern char *gnu_target_pid_to_str (int pid);

(This indeed isn't used / defined anywhere.)

> diff --git a/gdb/gnu-nat.h b/gdb/gnu-nat.h
> index a213991..8af5965 100644
> --- a/gdb/gnu-nat.h
> +++ b/gdb/gnu-nat.h
> @@ -103,4 +103,12 @@ extern int gnu_debug_flag;
>     override it with local methods.  */
>  struct target_ops *gnu_target (void);
>  
> +/* Thread flavors used in re-setting the T bit.
> +   Thiese variables and functions are only defined in i386gnu-nat.c
> +   which is the only supported architecture for now.  */
> +extern unsigned int thread_state_flavor;
> +extern unsigned int thread_state_size;
> +extern void thread_state_set_traced (thread_state_t *state);
> +extern void thread_state_clear_traced (thread_state_t *state);
> +
>  #endif /* __GNU_NAT_H__ */

> diff --git a/gdb/i386gnu-nat.c b/gdb/i386gnu-nat.c
> index 89354dc..4ddf708 100644
> --- a/gdb/i386gnu-nat.c
> +++ b/gdb/i386gnu-nat.c
> @@ -297,6 +297,29 @@ gnu_store_registers (struct target_ops *ops,
>      }
>  }
>  
> +/* Thread flavors used in re-setting the T bit.  */
> +/* Replacement of define THREAD_STATE_FLAVOR macro.  */
> +unsigned int thread_state_flavor = i386_REGS_SEGS_STATE;
> +/* Remplacement of define THREAD_STATE_SIZE macro.  */
> +unsigned int thread_state_size = i386_THREAD_STATE_COUNT;

As above, I don't think this is the most beautiful solution (for anyone
listening, the i386* macros come from the kernel's header files, and may
be different per architecture), but it should do for now.

> +/* Replacement of define THREAD_STATE_SET_TRACED(state) macro.  */
> +void
> +thread_state_set_traced (thread_state_t *state)
> +{
> +  ((struct i386_thread_state *) (state))->efl |= 0x100;
> +  return;
> +}

Can get rid of some parens (no longer needed to protect from macro
expansions), can get rid of return statement.

> +/* Replacement of define THREAD_STATE_CLEAR_TRACED(state) macro.  */
> +void
> +thread_state_clear_traced (thread_state_t *state)
> +{
> +  ((struct i386_thread_state *) (state))->efl &= ~0x100;
> +  return;
> +}

Likewise.


GrÃÃe,
 Thomas

Attachment: pgp00000.pgp
Description: PGP signature