This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] Fix hpux_major_release variable setting
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Pierre Muller <pierre dot muller at ics-cnrs dot unistra dot fr>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 14 Jan 2011 11:30:35 -0500
- Subject: Re: [RFC] Fix hpux_major_release variable setting
- References: <000f01cbb401$1093cdc0$31bb6940$@muller@ics-cnrs.unistra.fr>
> The patch below fixes that problem by moving the call to uname to
> hppa-hpux-nat.c
>
> Is this patch OK?
I think that for a platform such as pa-hpux, this is fine (MarkK,
who just informed us that he has access to HP-UX hardware as well,
might want to coment as well). But I've since learnt that the correct
way of doing this is to use target_xfer_partial routine. The reason
why I think it's OK this way is that the fix seems contains to pa/hp-ux
and I don't see a lot of benefit in spending time and energy into trying
to support HP-UX 10.x.
> I am unsure about the way to move it to the header... Would it be
> better to declare it without external in the header?
I don't think that this would work. You'd end up with multiple definitions
of the same global variable.
> This would then mean that I should also move
> DEFAULT_HPUX_MAJOR_RELEASE macro to the header, but it would have
> the advantage of allowing to not use hard-coded 11 in
> _initialize_hppa_hpux_nat.
You can just leave the variable untouched instead of setting it to 11...
> 2011-01-14 Pierre Muller <muller@ics.u-strasbg.fr>
>
> * solib-som.h (hpux_major_release): Declare variable here.
> * solib-som.c: Remove <sys/utsname.h> header.
> (DEFAULT_HPUX_MAJOR_RELEASE): New macro.
> (hpux_major_release): Make global, change default value to
> DEFAULT_HPUX_MAJOR_RELEASE.
> (get_hpux_major_release): Simply return HPUX_MAJOR_RELEASE.
> * hppa-hpux-nat.c: Add <sys/utsname.h> include.
> Add "solib-som.h" header.
> (_initialize_hppa_hpux_nat): Set hpux_major_release variable by
> analyzis of uname call return.
> +/* Variable storing HP-UX major release number.
> + On non native system, simply suppose that the major release number is
^^^^^^^^
assume
> 11.
> + hppa-hpux-nat.c initialization code sets this number to
> + the real one on startup in native conditions. */
Suggest the following formatting (following the 70 characters guideline,
as well as an empty line for better clarity (personal opinion, can be
ignored). I'd also add a comment explaining why this value is computed
elsewhere. So here is a suggested rewordin:
/* Variable storing HP-UX major release number.
On non-native system, simply assume that the major release number
is 11. On native systems, hppa-hpux-nat.c initialization code
sets this number to the real one on startup in native conditions.
We cannot compute this value here, because we need to make a native
call to "uname". We are are not allowed to do that from here, as
this file is used for both native and cross debugging. */
> +#define DEFAULT_HPUX_MAJOR_RELEASE 11
> +int
> +hpux_major_release = DEFAULT_HPUX_MAJOR_RELEASE;
Formatting:
int hpux_major_release = DEFAULT_HPUX_MAJOR_RELEASE;
(I'm wondering if the DEFAULT_HPUX_MAJOR_RELEASE macro really brings much).
> static int
> get_hpux_major_release (void)
Let's get rid of this function at the same time, WDYT? It should be
a simple search-and-replace...
> + struct utsname x;
> + char *p;
>
> + uname (&x);
> + p = strchr (x.release, '.');
> + hpux_major_release = p ? atoi (p + 1) : 11;
Let's make that a function, and call the function from the
initialization code. I think it'll be easier to understand.
Otherwise, a small comment, and a new-line after the code block
would help visually separate this operation from the rest.
--
Joel