Several regressions and we branch soon.

  On Tue, 23 Jun 2015, Doug Evans wrote:

> On Tue, Jun 23, 2015 at 3:17 PM, Keith Seitz <keiths@redhat.com> wrote:
>> FWIW, I updated my virgin sandbox earlier, and I don't see a bunch of
>> the failures that you are/have. I've listed the ones I do see below.
>> FWIW, I have another branch that was last updated the first week of May
>> which shows none of these problems.
>>
>> I'll start looking at these failures immediately.
>>
>> Keith
>>
>> On 06/23/2015 12:03 PM, Doug Evans wrote:
>>
>>>>> FAIL: gdb.base/default.exp: info set
>>>>> FAIL: gdb.base/default.exp: show

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
"show foo" ever throw an exception?  Should the "info set"/"show" loop
expect an exception to be thrown from a show function?

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

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?
  

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
  

"Tedeschi, Walfred" <walfred.tedeschi@intel.com> writes:

> 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"

Command "show mpx bound" expects an argument, which is an address.  Is
this argument mandatory?  In other words, can gdb scan bound directory
and bound table from inferior memory and print all entries? this would
be slow, but "show mpx bound" doesn't need an argument.

After I read intel mpx doc and the patch, I have more questions in my
mind,

 - if program doesn't set mpx bounds at all, GDB attaches to the program,
   and set mpx bounds, when GDB detaches from this program, does GDB
   need to clear these mpx bounds it sets?

 - if program does set mpx bounds too (through mpx instructions or
   compiler builtins), do we expect GDB to show these mpx bounds too?

 - If program sets mpx bounds through mxp instructions and GDB sets mpx
   bounds too, does this interfere each other? or program's mxp bounds
   setting is stored in bnd0-bnd3, but GDB's mpx bound setting is bound
   directory and bound table, so this doesn't interfere each other?

> 2. Should initialization move to the validation routine?

If we do so, commands are not shown up on the target doesn't meet the
requirements of commands.  After some thinking, I prefer registering
mpx commands unconditionally even target doesn't support mpx.  The
"show" command still can tell user that this command doesn't work on
this target.  Otherwise, it should be confusing that some commands
disappear silently.
  

Hello Yao,

Thank you for your feedback!

See below:

> 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"
> Command "show mpx bound" expects an argument, which is an address.  Is this argument mandatory?  In other words, can gdb scan bound directory and bound table from inferior memory and print all entries? this would be slow, but "show mpx bound" doesn't need an argument.
Though slow i think it could be done. In case of displaying all entries 
there will be no context for the user. It means he will se a big table 
of numbers.  Therefore we decided to display only for the pointer desired.
In case you guys think that it might be also interesting we could spand 
the command work in that way.
> After I read intel mpx doc and the patch, I have more questions in my mind,
>
>   - if program doesn't set mpx bounds at all, GDB attaches to the program,
>     and set mpx bounds, when GDB detaches from this program, does GDB
>     need to clear these mpx bounds it sets?
In case program does not set bounds GDB will also not able to set 
bounds. Basically idea is to have bounds as variables.
Once user has modified its done.
>   - if program does set mpx bounds too (through mpx instructions or
>     compiler builtins), do we expect GDB to show these mpx bounds too?
No. Same as above.
>   - If program sets mpx bounds through mxp instructions and GDB sets mpx
>     bounds too, does this interfere each other? or program's mxp bounds
>     setting is stored in bnd0-bnd3, but GDB's mpx bound setting is bound
>     directory and bound table, so this doesn't interfere each other?

Yes. Like it happens with variables location dependent variables.
To be able to to set bounds also on registers and tables and make them 
sync we need debug information.
The solution by now is without. In this sense user has to know a bit of 
assembler to know where to set the bounds, for the case it being debugged.

>> 2. Should initialization move to the validation routine?
> If we do so, commands are not shown up on the target doesn't meet the requirements of commands.  After some thinking, I prefer registering mpx commands unconditionally even target doesn't support mpx.  The "show" command still can tell user that this command doesn't work on this target.  Otherwise, it should be confusing that some commands disappear silently
> --
> Yao (齐尧)

Hope I have answered your questions.

Thanks a lot and best regards,
-Fred
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
  

Walfred Tedeschi <walfred.tedeschi@intel.com> writes:

>>   - if program doesn't set mpx bounds at all, GDB attaches to the program,
>>     and set mpx bounds, when GDB detaches from this program, does GDB
>>     need to clear these mpx bounds it sets?
> In case program does not set bounds GDB will also not able to set
> bounds. Basically idea is to have bounds as variables.
> Once user has modified its done.

so GDB can only update and show the mpx bounds set in the program, is it correct?

>>   - if program does set mpx bounds too (through mpx instructions or
>>     compiler builtins), do we expect GDB to show these mpx bounds too?
> No. Same as above.

Your answer to Q1 is contradictive to it.

>>   - If program sets mpx bounds through mxp instructions and GDB sets mpx
>>     bounds too, does this interfere each other? or program's mxp bounds
>>     setting is stored in bnd0-bnd3, but GDB's mpx bound setting is bound
>>     directory and bound table, so this doesn't interfere each other?
>
> Yes. Like it happens with variables location dependent variables.

Sorry, I don't understand what does "Yes" mean to my alternative
question.  What are "variables location dependent variables"?

> To be able to to set bounds also on registers and tables and make them
> sync we need debug information.

Can you elaborate on this?  Why do we need debug information?  AFAIK,
bounds are stored in either registers and tables, GDB can read them from
registers and tables, and show them.

> The solution by now is without. In this sense user has to know a bit
> of assembler to know where to set the bounds, for the case it being
> debugged.
  

Hi Patrick,
After discussing with Walfred Tedeschi on intel mpx, I think command
"show mpx bound" still needs an input argument, otherwise, GDB
will display many bound table entries, which is less useful to
users.

I don't want to rename the command to "show-mpx-bound", because GDB
doesn't have any other "show-*" commands.  So looks your fix is the
best one I can think of.

On 23/06/15 22:45, Patrick Palka wrote:
> 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"));

The indentation looks wrong to me.

> +      return;
> +    }

Could you add the changelog entry in your patch, and post it again?
Then, it can be reviewed properly.