[PATCH, CFI] Fix EH for coldfire-uclinux

[Maxim Kuvyrkov-2]

Hi,

The following patch fixes a problem in EH on coldfire-uclinux.  Due to
peculiarities of ColdFire uClinux it is not possible to use PC-relative
encoding for CFI.  This problem was uncovered by recent switch of GCC to
emit .cfi* directives by default.

To generate bFLT binaries for coldfire-uclinux elf2flt utility is used.
  Elf2flt does not honor PT_LOAD's from the executable.  So, despite
appearances of the ELF file, .text and .eh_frame sections will not end
up in the same segment in bFLT file.  Therefore PC-relative encoding
cannot be used for CFI on coldfire-uclinux.  Hence this patch defines
CFI_DIFF_EXPR_OK to 0 for coldfire-uclinux.

However, there is one exception to the above -- encoding of LSDA.  LSDA
encoding is explicitly specified by the compiler and can be relied on.
So the patch also adds CFI_DIFF_LSDA_OK macro to convince GAS to accept
PC-relative encoding in this one case.

Tested on coldfire-uclinux and various other architectures where this
patch is a no-op.

OK to check in?

Thanks,

--
Maxim Kuvyrkov
CodeSourcery
maxim@...
[phone] x724


2009-11-01  Daniel Jacobowitz  <dan@...>
            Maxim Kuvyrkov  <maxim@...>

        * config/tc-m68k.h (CF_DIFF_EXPR_OK): Define to 0 for uClinux.
        (CFI_DIFF_LSDA_OK): Define.
        * config/te-uclinux.h: New file.
        * configure.tgt (m68k-uclinux): Define em.
        * dw2gencfi.c (CFI_DIFF_LSDA_OK): New macro.
        (dot_cfi_lsda, output_fde): Use instead of CFI_DIFF_EXPR_OK.

Index: gas/config/tc-m68k.h
===================================================================
--- gas/config/tc-m68k.h (revision 267216)
+++ gas/config/tc-m68k.h (working copy)
@@ -178,3 +178,13 @@ extern int tc_m68k_regname_to_dw2regnum
 
 #define tc_cfi_frame_initial_instructions tc_m68k_frame_initial_instructions
 extern void tc_m68k_frame_initial_instructions (void);
+
+#ifdef TE_UCLINUX
+/* elf2flt does not honor PT_LOAD's from the executable.
+   .text and .eh_frame sections will not end up in the same segment and so
+   we cannot use PC-relative encoding for CFI.  */
+# define CFI_DIFF_EXPR_OK 0
+
+/* However, follow compiler's guidance when it specifies encoding for LSDA.  */
+# define CFI_DIFF_LSDA_OK 1
+#endif
Index: gas/config/te-uclinux.h
===================================================================
--- gas/config/te-uclinux.h (revision 0)
+++ gas/config/te-uclinux.h (revision 0)
@@ -0,0 +1,22 @@
+/* Copyright 2009 Free Software Foundation, Inc.
+
+   This file is part of GAS, the GNU Assembler.
+
+   GAS is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as
+   published by the Free Software Foundation; either version 3,
+   or (at your option) any later version.
+
+   GAS is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
+   the GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with GAS; see the file COPYING.  If not, write to the Free
+   Software Foundation, 51 Franklin Street - Fifth Floor, Boston, MA
+   02110-1301, USA.  */
+
+#define TE_UCLINUX
+
+#include "te-generic.h"
Index: gas/dw2gencfi.c
===================================================================
--- gas/dw2gencfi.c (revision 267216)
+++ gas/dw2gencfi.c (working copy)
@@ -36,6 +36,14 @@
 # endif
 #endif
 
+#ifndef CFI_DIFF_LSDA_OK
+# define CFI_DIFF_LSDA_OK CFI_DIFF_EXPR_OK
+#endif
+
+#if CFI_DIFF_EXPR_OK == 1 && CFI_DIFF_LSDA_OK == 0
+# error "CFI_DIFF_EXPR_OK should imply CFI_DIFF_LSDA_OK"
+#endif
+
 /* We re-use DWARF2_LINE_MIN_INSN_LENGTH for the code alignment field
    of the CIE.  Default to 1 if not otherwise specified.  */
 #ifndef  DWARF2_LINE_MIN_INSN_LENGTH
@@ -758,7 +766,7 @@ dot_cfi_lsda (int ignored ATTRIBUTE_UNUS
 
   if ((encoding & 0xff) != encoding
       || ((encoding & 0x70) != 0
-#if CFI_DIFF_EXPR_OK || defined tc_cfi_emit_pcrel_expr
+#if CFI_DIFF_LSDA_OK || defined tc_cfi_emit_pcrel_expr
   && (encoding & 0x70) != DW_EH_PE_pcrel
 #endif
   )
@@ -1445,7 +1453,7 @@ output_fde (struct fde_entry *fde, struc
       exp = fde->lsda;
       if ((fde->lsda_encoding & 0x70) == DW_EH_PE_pcrel)
  {
-#if CFI_DIFF_EXPR_OK
+#if CFI_DIFF_LSDA_OK
   exp.X_op = O_subtract;
   exp.X_op_symbol = symbol_temp_new_now ();
   emit_expr (&exp, augmentation_size);
Index: gas/configure.tgt
===================================================================
--- gas/configure.tgt (revision 267216)
+++ gas/configure.tgt (working copy)
@@ -267,7 +267,7 @@ case ${generic_target} in
   m68k-*-sysv4*) fmt=elf em=svr4 ;;
   m68k-*-rtems*) fmt=elf ;;
   m68k-*-linux-*) fmt=elf em=linux ;;
-  m68k-*-uclinux*) fmt=elf ;;
+  m68k-*-uclinux*) fmt=elf em=uclinux ;;
   m68k-*-gnu*) fmt=elf ;;
   m68k-*-netbsdelf*) fmt=elf em=nbsd ;;
   m68k-*-netbsd*) fmt=aout em=nbsd bfd_gas=yes ;;

[Richard Henderson-2]

On 11/03/2009 01:35 PM, Maxim Kuvyrkov wrote:
> To generate bFLT binaries for coldfire-uclinux elf2flt utility is used.
> Elf2flt does not honor PT_LOAD's from the executable. So, despite
> appearances of the ELF file, .text and .eh_frame sections will not end
> up in the same segment in bFLT file. Therefore PC-relative encoding
> cannot be used for CFI on coldfire-uclinux. Hence this patch defines
> CFI_DIFF_EXPR_OK to 0 for coldfire-uclinux.

I don't see anything wrong with the patch as far as it goes, but...

The best solution would be to fix elf2flt and get the eh data into the
read-only section, then use DW_EH_PE_datarel for any items that reside
in the data section.

The ideal relocation type for datarel relocations is something like
R_386_GOTOFF.  You don't have that one on m68k (though it wouldn't be
hard to add, and would improve all of the code generated by the compiler
for this memory model).  What you do have is R_68K_GOT32O, which is
DW_EH_PE_datarel|DW_EH_PE_indirect.  The only thing missing at this
point is the fact that unlike other targets, m68k doesn't parse the
@RELOC specifiers on .long data.

Fix that and you'll significantly reduce the number of relocations
needed in those bFLT files.


r~

[Daniel Jacobowitz-3]

On Wed, Nov 04, 2009 at 10:07:28AM -0800, Richard Henderson wrote:
> On 11/03/2009 01:35 PM, Maxim Kuvyrkov wrote:
> >To generate bFLT binaries for coldfire-uclinux elf2flt utility is used.
> >Elf2flt does not honor PT_LOAD's from the executable. So, despite
> >appearances of the ELF file, .text and .eh_frame sections will not end
> >up in the same segment in bFLT file. Therefore PC-relative encoding
> >cannot be used for CFI on coldfire-uclinux. Hence this patch defines
> >CFI_DIFF_EXPR_OK to 0 for coldfire-uclinux.
>
> I don't see anything wrong with the patch as far as it goes, but...

That's good enough for me (for now) - Maxim, the patch can go in.

> The best solution would be to fix elf2flt and get the eh data into
> the read-only section, then use DW_EH_PE_datarel for any items that
> reside in the data section.

The problem's not just with elf2flt, but also with various other bits
including ld and linker scripts (I haven't tracked down all the pieces
yet): we end up with only one PT_LOAD segment.  Then elf2flt splits
things based on BFD's section flags (argh).  So things ld thought were
in a single segment turn out not to be.

Once we get things into two segments as they are Intended To Be, then
we should be able to teach elf2flt to work based on segments.  At that
point this becomes a lot simpler; read-only eh data -> read-only
segment.

We have plenty of incentive to get this really right.  For instance,
GDB tries to relocate the binary at runtime.  It uses ptrace to get
the text and data offsets, then applies those offsets... cleverly, to
segments as seen in the ELF file.  So the mismatch messes up the
address of every global variable.

I guess we can back out the gas change at that point.

> The only thing missing
> at this point is the fact that unlike other targets, m68k doesn't
> parse the @RELOC specifiers on .long data.

Which doesn't matter if you're using CFI directives, does it?

--
Daniel Jacobowitz
CodeSourcery

[Richard Henderson-2]

On 11/04/2009 11:46 AM, Daniel Jacobowitz wrote:
> The problem's not just with elf2flt, but also with various other bits
> including ld and linker scripts (I haven't tracked down all the pieces
> yet): we end up with only one PT_LOAD segment.  Then elf2flt splits

Browsing buildroot elf2flt sources, the most obvious thing to do is to
modify the linker script to explicitly manage the phdrs:

   PHDRS {
     text PT_LOAD ;
     data PT_LOAD ;
   }

I guess I can assume you've tried this already and there are other problems?

> Which doesn't matter if you're using CFI directives, does it?

No.

But personally think it's a shame not to be able to easily generate any
appropriate relocation with data directives.  No telling how they'll be
useful for random applications.  Of course, you can always do something
gross like

        .reloc ., R_68K_GOT32O, foo
        .long 0

but... really...  ;-)


r~

[Daniel Jacobowitz-2]

On Wed, Nov 04, 2009 at 02:11:08PM -0800, Richard Henderson wrote:

> On 11/04/2009 11:46 AM, Daniel Jacobowitz wrote:
> >The problem's not just with elf2flt, but also with various other bits
> >including ld and linker scripts (I haven't tracked down all the pieces
> >yet): we end up with only one PT_LOAD segment.  Then elf2flt splits
>
> Browsing buildroot elf2flt sources, the most obvious thing to do is
> to modify the linker script to explicitly manage the phdrs:
>
>   PHDRS {
>     text PT_LOAD ;
>     data PT_LOAD ;
>   }
>
> I guess I can assume you've tried this already and there are other problems?

I don't recall :-(  That's definitely something to try though.  I
wanted to work out first why ld was making a different choice; usually
it's because some section is writable that should be read-only.

> But personally think it's a shame not to be able to easily generate
> any appropriate relocation with data directives.  No telling how
> they'll be useful for random applications.  Of course, you can always
> do something gross like
>
> .reloc ., R_68K_GOT32O, foo
> .long 0
>
> but... really...  ;-)

Still, a neat trick... I should remember to buy Alan a beer for .reloc
sometime :-)

--
Daniel Jacobowitz
CodeSourcery

[Richard Henderson-2]

On 11/04/2009 03:07 PM, Daniel Jacobowitz wrote:
> I don't recall :-(  That's definitely something to try though.  I
> wanted to work out first why ld was making a different choice; usually
> it's because some section is writable that should be read-only.

There's no page alignment between .text and .data in the linker script,
for one.


r~

[Maxim Kuvyrkov-2]

Daniel Jacobowitz wrote:

> On Wed, Nov 04, 2009 at 10:07:28AM -0800, Richard Henderson wrote:
>> On 11/03/2009 01:35 PM, Maxim Kuvyrkov wrote:
>>> To generate bFLT binaries for coldfire-uclinux elf2flt utility is used.
>>> Elf2flt does not honor PT_LOAD's from the executable. So, despite
>>> appearances of the ELF file, .text and .eh_frame sections will not end
>>> up in the same segment in bFLT file. Therefore PC-relative encoding
>>> cannot be used for CFI on coldfire-uclinux. Hence this patch defines
>>> CFI_DIFF_EXPR_OK to 0 for coldfire-uclinux.
>> I don't see anything wrong with the patch as far as it goes, but...
>
> That's good enough for me (for now) - Maxim, the patch can go in.

This patch is now checked in.

Thanks for all the review,

--
Maxim Kuvyrkov
CodeSourcery
maxim@...
[phone] x724