On 2024-08-06 11:21, Richard Sandiford wrote:
Matthieu Longo <matthieu.lo...@arm.com> writes:
This patch provides a new handler MD_ARCH_FRAME_STATE_T to hide an
architecture-specific structure containing CIE and FDE data related
to DWARF architecture extensions.
Hiding the architecture-specific attributes behind a handler has the
following benefits:
1. isolating those data from the generic ones in _Unwind_FrameState
2. avoiding casts to custom types.
3. preserving typing information when debugging with GDB, and so
facilitating their printing.
This approach required to add a new header md-unwind-def.h included at
the top of libgcc/unwind-dw2.h, and redirecting to the corresponding
architecture header via a symbolic link.
An obvious drawback is the increase in complexity with macros, and
headers. It also caused a split of architecture definitions between
md-unwind-def.h (types definitions used in unwind-dw2.h) and
md-unwind.h (local types definitions and handlers implementations).
The naming of md-unwind.h with .h extension is a bit misleading as
the file is only included in the middle of unwind-dw2.c. Changing
this naming would require modification of others backends, which I
prefered to abstain from. Overall the benefits are worth the added
complexity from my perspective.
Sorry, I should have read 2/3 before making the suggestion in the
previous review. I agree that it makes sense to separate this change
out, given that it involves a new header file.
It'd be good to update the comment in no-unwind.h:
/* Dummy header for targets without a definition of
MD_FALLBACK_FRAME_STATE_FOR. */
Done
LGTM otherwise, thanks.
On patch 3: IMO it's better to post the regenerated files as part
of the same patch, so that each patch is self-contained.
>
> Richard
>
I did it this way as it makes things easier to rebase, and regenerate
the files if there is a conflict.
I agree, this patch can be squashed with the previous when merging those
changes into master.
Matthieu
