On Thu, Jan 12, 2023 at 12:05:45PM +0000, Richard Sandiford wrote:
> Jakub Jelinek <ja...@redhat.com> writes:
> > On Wed, Jan 11, 2023 at 11:59:27AM +0000, Wilco Dijkstra wrote:
> >> Hi,
> >>
> >> > On 1/10/23 19:12, Jakub Jelinek via Gcc-patches wrote:
> >> >> Anyway, the sooner this makes it into gcc trunk, the better, it breaks
> >> >> quite
> >> >> a lot of stuff.
> >> >
> >> > Yep, please, we're also waiting for this patch for pushing to our gcc13
> >> > package.
> >>
> >> Well I'm waiting for an OK from a maintainer... I believe Jakub can
> >> approve it as well.
> >
> > Yes, I can, but am not sure it is appropriate. While I'm familiar with the
> > unwinder, I'm not familiar with the pointer signing, and AArch64 has active
> > maintainers, so I'd prefer to defer the review to them.
>
> I think my main question is: how clean vs hacky is it to use
> REG_UNDEFINED as effectively a toggle bit, rather than using
> REG_UNDEFINED for its intended purpose?
>
> In the review for earlier (May) patch, I'd asked whether it would
> make sense to add a new enum. Would that be OK from a target-independent
> point of view? E.g. maybe REG_TOGGLE_ON.
>
> Although we don't AFAIK support using DW_CFA_undefined with RA signing,
> the failure mode would be non-obvious: it would effectively toggle the
> bit on.
We don't install unwind-dw2.h nor give user code access to the how array
(and it just lives on the stack of __frame_state_for/uw_init_context_1
functions and address of it is passed to functions called from it),
so I hope all this is private to the libgcc unwinder. After all, otherwise
e.g. the change how "how" is represented couldn't be done.
That said, if new enum entries are added in the generic code, then
I think uw_update_context_1 will warn about unhandled case in a switch,
unless we e.g. change
case REG_UNSAVED:
case REG_UNDEFINED:
break;
to
default:
break;
(and provided that the new enums would want such handling).
Another option is to just define the arch dependent value for how field
in the arch code, right now it is unsigned char type, so using say
(unsigned char) ~0 or (unsigned char) ~0 and (unsigned char) ~1 as arch
specific values might be ok too.
> It would be good to remove the definition of RA_SIGNED_BIT as well,
> so that people don't accidentally use it in future.
Jakub
