Re: [PATCH V2] AArch64 pauth: Indicate unmasked addresses in backtrace

[Pedro Alves <palves at redhat dot com>]

On 8/8/19 5:58 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> Hmm, I had suggested considering MI in the previous iteration, but
> Pedro> I was just thinking of including the "[PAC]" text in the
> Pedro> "addr" field.  If we're adding a new field, then a few extra
> Pedro> things need to be considered:
> 
> Pedro>  #1 - documentation, both manual and NEWS should mention this new MI field.
> 
> Oops, I forgot about this.  Sorry about that.
> 
> I don't think putting this information into the "addr" field is a good
> idea.  It's better, IMO, to let MI field names provide the structure,
> rather than requiring clients to also parse the values of fields.

True, though I observe that the addr field isn't strictly numeric,
since it can contain "<unavailable>".

> 
> I realize MI isn't 100% clean on this topic, but we can still not make
> it worse.

Can't disagree with that.

> 
> Pedro>  #2 - calling the attribute "pac" makes it architecture specific. 
> 
> I don't think this is such a big deal but at the same time any
> reasonable name is fine by me.

It depends on what is the "pac" attribute intended for.  If it is strictly
about Aarch64 PAC, then when we need to expose some other attribute, either
for Aarch64 or some other architecture, it's better to not reuse "pac",
since it may conflict with whatever use IDEs may give to "pac", assuming
it really is about PAC, only.  And if that's the intended use, it seems
better to make it a boolean, since "[PAC]" is CLI's presentation, not the
datum.

If OTOH we assume that later on, other address attributes would be displayed
here, like "[PAC,FOO,BAR]", then it seems better to me to name it and
document it like that from the get go, to avoid causing confusion to
MI clients down the road if/when such attributes start showing, and
also avoid having to have a future discussion of what MI attribute
should be used to output my "[FOO]" attribute, given that "pac" is
seemingly about PAC only.

I.e. I think it's better to nail down the semantics upfront, and if
the consensus is to make it a "generic address attributes" field, then
I'd rather name it and document it as such from the get go.

> 
> Pedro>  #3 - The MI attribute is called "pac", and its content is
> Pedro>       literally " [PAC]".  I'd find that odd if I were a frontend author:
> Pedro>       the content is right aligned with a space, making doing anything with
> Pedro>       it other than appending it to the address text probably look odd,
> Pedro>       unless you bake in awareness of the attribute's text...  If I saw
> Pedro>       an attribute named "pac", I'd expect it to be a boolean?  At the
> Pedro>       least, the left space should not be part of the field, I think?
> 
> I think part of the pain here is an internal constraint, namely that the
> CLI ui-out wouldn't know to rewrite the boolean value to something else
> here.  But perhaps that's something that could just be addressed
> directly.

There's always 'uiout->is_mi_like_p()', but I'm not advocating
making this a boolean (and not advocating not making it a boolean
either).

Thanks,
Pedro Alves