Re: [PATCH 1/2] gdb: Restore a settings previous value on error during change

[Pedro Alves <palves at redhat dot com>]

Hi Andrew,

On 11/15/2016 11:02 PM, Andrew Burgess wrote:

> I've folded the whole restore or clean-up the previous value into a
> single cleanup function.  I'm not 100% convinced that the new code is
> better than the previous, but I don't think it's any worse.

I'm not sure the "revert if set throws" is the best design, but
I do prefer this version with a single cleanup over the first, because
it should be easier to convert the new cleanup to a RAII C++
class (we're trying to get rid of cleanups).

> 
> gdb: Restore a settings previous value on error during change
> 
> When changing the value of a setting in do_set_command, there are three
> phases:
> 
>  1. The old value is destroyed, and the new value is placed into the
>  settings variable.
> 
>  2. The set-hook, the 'func' field of the setting is called.
> 
>  3. A change notification is issued.
> 
> There are two problems that I try to address in this commit.
> 
>  1. If the set-hook does not like the new value then either the setting
>  is restored to a default value, or we have to have a complex system
>  within the set-hook to remember the previous value and restore it
>  manually when an invalid change is made.
> 
>  2. If the set-hook throws an error then the change notification is
>  skipped, even though the setting might still have changed (say, back to
>  some default value).
> 
> The solution I propose here is to delay destroying the old value of the
> setting until after the set-hook has been called.  If the set-hook runs
> successfully then the old value will be destroyed when do_set_command
> returns, the new value will be left in place, and the change
> notification will be sent sent out exactly as before.
> 
> However, if the set-hook throws an error this is now caught in
> do_set_command, the new value of the setting is removed, and the old
> value is restored.
> 
> After this change, it is no longer possible in a set-hook to change a
> setting to a default value _and_ throw an error.  If you throw an error
> you should assume that gdb will restore the previous value of the
> setting.  If you want to change a setting in the set-hook and inform the
> user, then a warning, or just a standard message should be fine.

I have a few questions on this design.

Take a look at solib.c:gdb_sysroot_changed, the "set sysroot"
set hook.  reload_shared_libraries does a lot of work, and it
can easily trip on some error trying to fetch DSOs out of the
new sysroot.  Should the sysroot setting change back to what it was before,
when such an error happens?  What happens to the shared libraries that might
have already been loaded in the new sysroot?  Should "set" hooks
handle cases like this themselves, by wrapping all but command variable
validation with TRY/CATCH?

I'm also concerned about ctrl-c/QUIT while inside the set hook.
E.g., if the user presses ctrl-c while the set hook is working,
and a QUIT is thrown out of the set hook, should the setting's value
be reverted or not?  Maybe the simplest to imagine is if the set hook
prints something that paginates, and the user presses ctrl-c to
abort the pagination.  Maybe whether to revert depends on whether
the setting's new value was already validated or not?

I'm wondering whether a new "validate" hook, called before
the "set" hook is called, would be a better design.  Maybe not.
I'd like to have the design around those points clarified, so
that we have some agreed direction on how to handle such scenarios.

> +
> +  /* Should we revert the setting to the previous value?  If not then we
> +     should clean up the previous value.  */
> +  int revert_p;

"bool".

> -  c->func (c, NULL, from_tty);
> +
> +  TRY
> +    {
> +      c->func (c, NULL, from_tty);
> +    }
> +  CATCH (ex, RETURN_MASK_ERROR)
> +    {
> +      /* Set the REVERT_P flag based on OPTION_CHANGED.  If this leaves
> +	 REVERT_P as false then the cleanup SET_CMD_CLEANUP_OR_REVERT will
> +	 try to cleanup the previous value, but as OPTION_CHANGED was
> +	 false, there is no previous value, and so nothing is done.  If
> +	 REVERT_P is changed to TRUE here then there was a previous value,
> +	 and so the SET_CMD_CLEANUP_OR_REVERT will restore the previous
> +	 value, and cleanup the new value that we no longer need.  */
> +      set_cmd_cleanup_data.revert_p = option_changed;
> +      throw_exception (ex);
> +    }
> +  END_CATCH

Why wrap with TRY/CATCH instead of:

      set_cmd_cleanup_data.revert_p = option_changed;
      c->func (c, NULL, from_tty);
      set_cmd_cleanup_data.revert_p = false;

Note how ctrl-c inside the "set" hook ends up not reverting,
of RETURN_MASK_ERROR instead of RETURN_MASK_ALL.
If that was the reason for the TRY/CATCH, then I'd expect the
comment to mention it.



> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +
> +# Read the current dcache line-size, check default is still 64.
> +set default_line_size 0
> +set testname "read default value for dcache line-size"
> +send_gdb "show dcache line-size\n"
> +gdb_expect {

Use gdb_test_multiple.

> +    -re "Dcache line size is (\[0-9\]+\)\.\r\n$gdb_prompt $" {
> +	set default_line_size $expect_out(1,string)
> +	pass $testname
> +    }
> +    -re ".*$gdb_prompt $"   { fail $testname }
> +    timeout	            { fail "$testname (timeout)" }
> +}
> +
> +if { $default_line_size == 0 } then {
> +    unsupported "dcache-line-set-error"

untested.

> +# Try to change to some invalid (non power of 2 value)

Please write complete sentences in comments.  I.e., add a period.

> +set invalid_line_size [expr $default_line_size - 1]
> +gdb_test "set dcache line-size $invalid_line_size" \
> +    "Invalid dcache line size: $invalid_line_size .*" \
> +    "First attempt to set invalid dcache line size"

Lowercase test messages:

    "first attempt to set invalid dcache line size"

(throughout) 

> +
> +# Check the default is still 64

Period.

> +gdb_test "show dcache line-size" \
> +    "Dcache line size is $new_line_size\." \
> +    "Check that the new value took"

"took effect" ?  Or is that idiomatic English perhaps? (I had never
seen it).

Thanks,
Pedro Alves