RE: Several regressions and we branch soon.

["Tedeschi, Walfred" <walfred dot tedeschi at intel dot com>]

Hi Patrick,

This command was proposed initially as an standalone like:

Set-mpx-bound and show-mpx-bound.

Recommendation was to introduce it in the sets and shows, which I have agreed.
Also because "set" is also used to set values of variables when used alone.
Which is similar to what "set mpx bound" is doing, In this sense it can be considered as the right category to have it, as Joel indicated.

About the show, well that is the natural counterpart of the set, right?

Also, I agree with Yao patch. I would use a "warning" instead.

Initialization of the command can be placed in a different location. I could think of adding them at the validation of the tdesc, i.e.
I386_validate_tdesc_p and amd64_validate_tdesc_p.  Would you agree with that?

Open questions are:
1. Command call. Should they still be called "set mpx bound" / "show mpx bound"
2. Should initialization move to the validation routine?


Thanks a lot and regards,
-Fred

-----Original Message-----
From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Yao Qi
Sent: Wednesday, June 24, 2015 1:55 PM
To: Patrick Palka
Cc: Doug Evans; Keith Seitz; gdb-patches
Subject: Re: Several regressions and we branch soon.

Patrick Palka <patrick@parcs.ath.cx> writes:

> The problem here seems to be that the "show" function corresponding to 
> the "mpx bound" command calls error("Intel(R) Memory Protection 
> Extensions not supported on this target."); which throws an exception 
> that causes the entire "info set"/"show" loop to exit early.  Should

Yes, I think your analysis is correct.

> "show foo" ever throw an exception?  Should the "info set"/"show" loop 
> expect an exception to be thrown from a show function?

IMO, "show foo" shouldn't throw an exception.

>
> This diff fixes the default.exp FAILs as far as I can tell:
>

Thanks for the fix, but I feel that the original code has some drawbacks,

> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index 42d0346..d11efa1 
> 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -8777,11 +8777,17 @@ i386_mpx_info_bounds (char *args, int from_tty)
>    struct type *data_ptr_type = builtin_type 
> (gdbarch)->builtin_data_ptr;
>
>    if (!i386_mpx_enabled ())
> -    error (_("Intel(R) Memory Protection Extensions not\
> - supported on this target."));
> +    {
> +      printf_unfiltered (_("Intel(R) Memory Protection Extensions not "
> +			   "supported on this target.\n"));
> +      return;
> +    }
>
>    if (args == NULL)
> -    error (_("Address of pointer variable expected."));
> +    {
> +      printf_unfiltered (_("Address of pointer variable expected.\n"));
> +      return;
> +    }
>
>    addr = parse_and_eval_address (args);

It is odd that command "info mpx bounds" requires an argument, which is an address.  The set/show command pair works in a way that command set modify or update some state of GDB, and command info just display the state of GDB.  That is to say, "set mpx bounds ADDRESS" should set address, which is stored somewhere in GDB (in a global variable or a per-inferior variable), and "info mpx bounds" shows the current state or setting in GDB, no argument is needed.

If this is a right direction, I can give a patch.

b.t.w, is it a good (or bad) idea to register this command if mpx is supported on the target?

--
Yao (éå)
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052