This is the mail archive of the
libc-ports@sources.redhat.com
mailing list for the libc-ports project.
Re: Add CFI information for MIPS assembly sources
- From: "Maciej W. Rozycki" <macro at codesourcery dot com>
- To: "Joseph S. Myers" <joseph at codesourcery dot com>
- Cc: <libc-ports at sourceware dot org>
- Date: Fri, 8 Feb 2013 14:57:31 +0000
- Subject: Re: Add CFI information for MIPS assembly sources
- References: <Pine.LNX.4.64.1302072250540.17419@digraph.polyomino.org.uk>
On Thu, 7 Feb 2013, Joseph S. Myers wrote:
> This patch adds CFI information to many of the assembly sources in
> MIPS glibc, similar to that for various other architectures, to
> support better backtracing through such code by various tools.
>
> This is a start on adding such CFI information rather than a patch
> intended to add complete CFI information everywhere in the library.
> Specifically, the following are not covered but could be covered by a
> separate patch:
>
> * Assembly sources used through inline asm in .c files (including some
> complete functions generated that way through inline asm).
>
> * The SETUP_GPX* macros.
>
> * The bit of setcontext.S after sp is set from the saved context and
> before the jump to that saved context.
>
> * sysdeps/mips/start.S.
>
> * sysdeps/unix/mips/sysdep.S.
>
> For SETUP_GP64 and RESTORE_GP64, the cfi_* directives are in the
> individual .S files rather than the macro definitions because the
> macros can be used to save the old gp value in either a register or a
> stack slot, and those need different CFI directives, so a single macro
> SETUP_GP64 can't handle both cases and generate CFI at the same time.
> This does mean that various callers need to condition the use of CFI
> with these macro so no such CFI is generated in the o32 case when the
> GP64 macros expand to empty (this is of course only needed for those
> callers in .S files actually shared by o32 and non-o32 ABIs).
> Although the RESTORE_GP64 doesn't actually depend on where the value
> was saved, it seems better to be consistent and have the directives in
> the .S files in both cases, rather than just for SETUP_GP64.
>
> Quite a few of the changes are simply making functions consistently
> use macros such as ENTRY and END at the start and end of the functions
> to generate the cfi_startproc / cfi_endproc directives. Some changes
> to use ENTRY may have the effect of changing the .align directives
> (which were .align 4 in some cases, now more consistently .align 2),
> but I see no particular reason for the variation in those directives
> in the cases where macros were previously not used at
> start-of-function.
>
> I plan to commit this patch subject to the results of testing that is
> currently running, but comments or corrections are welcome.
Just three quick notes:
1. If I were the maintainer, I'd expect the change to convert sources to
use ENTRY/END/etc. properly to be done separately, preferably before
making changes to add CFI pieces so as not to obfuscate the actual
matter of this change. It would make people easier to review this
change too. Of course it's you who are the MIPS maintainer, so it's
just my opinion, you're free to do whatever you like there.
2. Given the implementation I fail to see the point of modifying
<sys/asm.h> at all. Yes, it saves a line here or there, but at the
cost of non-standard semantics, some obfuscation, and then you need to
place other CFI ops manually throughout functions concerned anyway.
Not worth the complication in my opinion, I'd suggest placing all the
CFI ops outside these macros in the functions handled. It's a one-off
effort anyway, we can require further assembly contributions to copy
the ops from existing examples -- which is much easier once there are
some actually in place.
3. On the other hand it may be worthwhile to avoid obfuscation like:
# if _MIPS_SIM != _ABIO32
cfi_rel_offset (gp, GPOFF)
# endif
and define a macro like mips_cfi_rel_offset64 (a better name may be
possible, perhaps just cfi_rel_offset64) that'll handle the ABI
condition in the definition, pretty much like SETUP_GP64 does.
Likewise with complementing cfi_restore.
I've had a look through individual changes too, and with the concerns
above put aside they look good to me. Thanks for your work on this
problem.
Maciej