This is the mail archive of the mailing list for the libc-ports project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Add CFI information for MIPS assembly sources

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]