Re: [PATCH 1/2] gdb: updates to gdbarch.py algorithm

[Simon Marchi <simark@simark.ca>]

On 3/2/23 05:13, Andrew Burgess wrote:
> Simon Marchi <simark@simark.ca> writes:
> 
>> On 2/28/23 11:51, Andrew Burgess via Gdb-patches wrote:
>>> Restructure how gdbarch.py generates the verify_gdbarch function.
>>> Previously the postdefault handling was bundled together with the
>>> validation.  This means that a field can't have both a postdefault,
>>> and set its invalid attribute to a string.
>>>
>>> This doesn't seem reasonable to me, I see no reason why a field can't
>>> have both a postdefault (used when the tdep doesn't set the field),
>>> and an invalid expression, which can be used to validate the value
>>> that a tdep might set.
>>>
>>> In this commit I restructure the verify_gdbarch generation code to
>>> allow the above, there is no change in the actual generated code in
>>> this commit, that will come in later commit.
>>>
>>> I did end up having to remove the "invalid" attribute (where the
>>> attribute was set to True) from a number of fields in this commit.
>>> This invalid attribute was never having an effect as these components
>>> all have a postdefault.  Consider; the "postdefault" is applied if the
>>> field still has its initial value, while an "invalid" attribute set to
>>> True means error if the field still has its default value.  But the
>>> field never will have its default value, it will always have its
>>> postdefault value.
>>> ---
>>>  gdb/gdbarch.py            | 31 ++++++++++++++++---------
>>>  gdb/gdbarch_components.py | 49 ++++++++++++++-------------------------
>>>  2 files changed, 37 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/gdb/gdbarch.py b/gdb/gdbarch.py
>>> index 93b1e8bf84e..7fea41c9572 100755
>>> --- a/gdb/gdbarch.py
>>> +++ b/gdb/gdbarch.py
>>> @@ -203,35 +203,44 @@ with open("gdbarch.c", "w") as f:
>>>          file=f,
>>>      )
>>>      for c in filter(not_info, components):
>>> -        if c.invalid is False:
>>> -            print(f"  /* Skip verify of {c.name}, invalid_p == 0 */", file=f)
>>> -        elif c.predicate:
>>> -            print(f"  /* Skip verify of {c.name}, has predicate.  */", file=f)
>>> -        elif isinstance(c.invalid, str) and c.postdefault is not None:
>>> -            print(f"  if ({c.invalid})", file=f)
>>> -            print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>>> -        elif c.predefault is not None and c.postdefault is not None:
>>> +        # An opportunity to write in the 'postdefault' value.
>>> +        if c.postdefault is not None and c.predefault is not None:
>>>              print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
>>>              print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>>>          elif c.postdefault is not None:
>>>              print(f"  if (gdbarch->{c.name} == 0)", file=f)
>>>              print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>>
>> I would find this postdefault snippet easier to read like this, with a
>> single "if c.postdefault is not None", and then another condition inside
>> to decide what we should compare against:
>>
>>         if c.postdefault is not None:
>>             if c.predefault is not None:
>>                 print(f"  if (gdbarch->{c.name} == {c.predefault})", file=f)
>>                 print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>>             else:
>>                 print(f"  if (gdbarch->{c.name} == 0)", file=f)
>>                 print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
>>
>> or even
>>
>>         if c.postdefault is not None:
>>             predefault = c.predefault or "0"
>>             print(f"  if (gdbarch->{c.name} == {predefault})", file=f)
>>             print(f"    gdbarch->{c.name} = {c.postdefault};", file=f)
> 
> I went with the second approach, I like removing the duplicate print
> calls.
> 
>>
>>> +
>>> +        # Now validate the value.
>>> +        if c.invalid is False:
>>> +            print(f"  /* Skip verify of {c.name}, invalid_p == 0 */", file=f)
>>> +        elif c.predicate:
>>> +            print(f"  /* Skip verify of {c.name}, has predicate.  */", file=f)
>>> +        elif c.invalid is None:
>>
>> I think it's confusing for the "invalid" parameter to be able to be
>> None, that it's one to many state versus what we need to be able to
>> represent.  I think we can get by with string, True and False, where
>> True means "auto", where the validity check is generated if it makes
>> sense to.  Having one less state would help simplify things.  I hacked
>> this locally and it seems to work.  I can post this as a cleanup before
>> or on top of your patch, as you prefer.
>>
>> Another cleanup that would help me understand what is going on would be
>> to change this long list of if/elif to something that looks more like a
>> decision tree.  On top of your patch, and on top of my suggestion to get
>> rid of the invalid=None state, this is what I made looks like:
>>
>>         predefault = c.predefault or "0"
>>
>>         # Now validate the value.
>>         if type(c.invalid) is str:
>>             print(f"  if ({c.invalid})", file=f)
>>             print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
>>         elif c.invalid:
>>             if c.predicate:
>>                 print(f"  /* Skip verify of {c.name}, has predicate.  */", file=f)
>>             elif c.postdefault:
>>                 # We currently don't print anything, but we could print:
>>                 # print(f"  /* Skip verify of {c.name}, has predicate.  */", file=f)
>>                 pass
>>             else:
>>                 print(f"  if (gdbarch->{c.name} == {predefault})", file=f)
>>                 print(f"""    log.puts ("\\n\\t{c.name}");""", file=f)
>>         else:
>>             print(f"  /* Skip verify of {c.name}, invalid_p == 0 */", file=f)
>>
>> That structure is clearer to me.  We see clearly the portions handling
>> the three states of "invalid" (str, True and False).  Inside invalid ==
>> True (which really means "auto"), we see that we skip generating the
>> check when either predicate or postdefault is set, the two situations
>> where generating the check doesn't make sense.
>>
>> Another nice thing about this is that there isn't the "unhandled case
>> when generating gdbarch validation" case at the end.  Each branch of the
>> decision tree has an outcome.
>>
>> Again, if you agree with this cleanup, we could do it before or after
>> your patch, as you wish.
> 
> Yeah, I think what you have above would be a great improvement.  I like
> that we can (with what you propose) now have an "invalid" string and
> also have a predicate, that feels like an improvement.

I'm not sure if it was intended, but I don't mind.  It would have been
useful for my return_value patch (which we will not use in the end).  In
there, I wanted an "invalid" expression to validate that return_value
was not set at the same time as return_value_as_value.  But it was also
possible to set neither, so I wanted a predicate as well (which would
just check != 0).

> I've included an update of my patch below.   I haven't included the
> "invalid" refactor that you describe above as it sounds like you already
> have a patch for this work, and it would end up as a separate patch
> anyway.  I'm fine with it going in after my work of before, whatever
> works best.
> 
> Let me know what you think.

Thanks, I think this looks good, and we can make further improvements
from there.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon