Re: [PATCH v3 3/3] S390: Fix gdbserver support for TDB

[Andreas Arnez <arnez at linux dot vnet dot ibm dot com>]

On Wed, Dec 10 2014, Ulrich Weigand wrote:

> Andreas Arnez wrote:
>
>> @@ -4256,7 +4256,7 @@ regsets_fetch_inferior_registers (struct regsets_info *regsets_info,
>>  		 this process mode.  */
>>  	      disable_regset (regsets_info, regset);
>>  	    }
>> -	  else
>> +	  else if (errno != ENODATA)
>>  	    {
>>  	      char s[256];
>>  	      sprintf (s, "ptrace(regsets_fetch_inferior_registers) PID=%d",
>
> It would be better to keep the comment explaining in what situations the kernel
> can return ENODATA that you had in a previous iteration of the patch set.

OK, I will add this comment in the next version:

	  else if (errno == ENODATA)
	    {
	      /* ENODATA may be returned if the regset is currently
		 not "active".  This can happen in normal operation,
		 so suppress the warning in this case.  */
	    }
	  else ...

>
>> @@ -4293,7 +4293,8 @@ regsets_store_inferior_registers (struct regsets_info *regsets_info,
>>        void *buf, *data;
>>        int nt_type, res;
>>  
>> -      if (regset->size == 0 || regset_disabled (regsets_info, regset))
>> +      if (regset->size == 0 || regset_disabled (regsets_info, regset)
>> +	  || regset->fill_function == NULL)
>>  	continue;
>
> This (and the related s390_fill_last_break change) is really an independent
> change; maybe do it as a separate patch?  For consistency, we might likewise
> want to allow regsets with NULL store_function (in regsets_fetch_inferior_registers).

OK, I will split the change to allow regsets with a NULL store_function
and the exploitation for last_break out into a separate patch
("gdbserver: Support read-only regsets").

And for consistency I can also allow regsets with a NULL store_function.
In that case I *think* we should suppress the invocation of ptrace with
the regset's get_request in regsets_store_inferior_registers().  In
other words, instead of read-modify-write we would then only do the
write.  Agreed?

In the original patch I omitted support for write-only regsets because I
do not see a use case for this; and if there will be a use case in the
future, I am not sure that the approach described above will really be
appropriate.