[ARM/FDPIC,v6,00/24] FDPIC ABI for ARM
mbox series

Message ID 20190909154526.11630-1-christophe.lyon@st.com
Headers show
Series
  • FDPIC ABI for ARM
Related show

Message

Christophe Lyon Sept. 9, 2019, 3:44 p.m. UTC
Hello,

Since all patches of v5 have now been approved, I'm posting v6 to
share the actual patches I'm about to commit (some had minor changes
compared to v5).

Thanks to the reviewers,

Christophe

Changes between v5 and v6:
- rebased on top of recent gcc-10 master (September 9th, 2019)
- fixed libitm support
- addressed feedback received about v5
- there are 3 more patches (skip tests that use -static, libitm fixes,
  split of libstdc++ configury)

Changes between v4 and v5:
- rebased on top of recent gcc-10 master (April 26th, 2019)
- fixed handling of stack-protector combined patterns in FDPIC mode

Changes between v3 and v4:

- improved documentation (patch 1)
- emit an error message (sorry) if the target architecture does not
  support arm nor thumb-2 modes (patch 4)
- handle Richard's comments on patch 4 (comments, unspec)
- added .align directive (patch 5)
- fixed use of kernel helpers (__kernel_cmpxchg, __kernel_dmb) (patch 6)
- code factorization in patch 7
- typos/internal function name in patch 8
- improved patch 12
- dropped patch 16
- patch 20 introduces arm_arch*_thumb_ok effective targets to help
  skip some tests
- I tested patch 2 on xtensa-buildroot-uclinux-uclibc, it adds many
  new tests, but a few regressions
  (https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00713.html)
- I compiled and executed several LTP tests to exercise pthreads and signals
- I wrote and executed a simple testcase to change the interaction
  with __kernel_cmpxchg (ie. call the kernel helper rather than use an
  implementation in libgcc as requested by Richard)

Changes between v2 and v3:
- added doc entry for -mfdpic new option
- took Kyrill's comments into account (use "Armv7" instead of "7",
  code factorization, use preprocessor instead of hard-coding "r9",
  remove leftover code for thumb1 support, fixed comments)
- rebase over recent trunk
- patches with changes: 1, 2 (commit message), 3 (rebase), 4, 6, 7, 9,
  14 (rebase), 19 (rebase)

Changes between v1 and v2:
- fix GNU coding style
- exit with an error for pre-Armv7
- use ACLE __ARM_ARCH and remove dead code for pre-Armv4
- remove unsupported attempts of pre-Armv7/thumb1 support
- add instructions in comments next to opcodes
- merge patches 11 and 13
- fixed protected visibility handling in patch 8
- merged legitimize_tls_address_fdpic and
  legitimize_tls_address_not_fdpic as requested



This patch series implements the GCC contribution of the FDPIC ABI for
ARM targets.

This ABI enables to run Linux on ARM MMU-less cores and supports
shared libraries to reduce the memory footprint.

Without MMU, text and data segments relative distances are different
from one process to another, hence the need for a dedicated FDPIC
register holding the start address of the data segment. One of the
side effects is that function pointers require two words to be
represented: the address of the code, and the data segment start
address. These two words are designated as "Function Descriptor",
hence the "FD PIC" name.

On ARM, the FDPIC register is r9 [1], and the target name is
arm-uclinuxfdpiceabi. Note that arm-uclinux exists, but uses another
ABI and the BFLAT file format; it does not support code sharing.
The -mfdpic option is enabled by default, and -mno-fdpic should be
used to build the Linux kernel.

This work was developed some time ago by STMicroelectronics, and was
presented during Linaro Connect SFO15 (September 2015). You can watch
the discussion and read the slides [2].
This presentation was related to the toolchain published on github [3],
which is based on binutils-2.22, gcc-4.7, uclibc-0.9.33.2, gdb-7.5.1
and qemu-2.3.0, and for which pre-built binaries are available [3].

The ABI itself is described in details in [1].

Our Linux kernel patches have been updated and committed by Nicolas
Pitre (Linaro) in July 2017. They are required so that the loader is
able to handle this new file type. Indeed, the ELF files are tagged
with ELFOSABI_ARM_FDPIC. This new tag has been allocated by ARM, as
well as the new relocations involved.

The binutils, QEMU and uclibc-ng patch series have been merged a few
months ago. [4][5][6]

This series provides support for architectures that support ARM and/or
Thumb-2 and has been tested on arm-linux-gnueabi without regression,
as well as arm-uclinuxfdpiceabi, using QEMU. arm-uclinuxfdpiceabi has
a few more failures than arm-linux-gnueabi, but is quite functional.

I have also booted an STM32 board (stm32f469) which uses a cortex-m4
with linux-4.20.17 and ran successfully several tools.

Thanks,

Christophe.


[1] https://github.com/mickael-guene/fdpic_doc/blob/master/abi.txt
[2] http://connect.linaro.org/resource/sfo15/sfo15-406-arm-fdpic-toolset-kernel-libraries-for-cortex-m-cortex-r-mmuless-cores/
[3] https://github.com/mickael-guene/fdpic_manifest
[4] https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=f1ac0afe481e83c9a33f247b81fa7de789edc4d9
[5] https://git.qemu.org/?p=qemu.git;a=commit;h=e8fa72957419c11984608062c7dcb204a6003a06
[6] https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/commit/?id=13c46fbc1e5a021f2b9ed32d83aecc93ae5e655d

Christophe Lyon (24):
  [ARM] FDPIC: Add -mfdpic option support
  [ARM] FDPIC: Handle arm*-*-uclinuxfdpiceabi in configure scripts
  [ARM] FDPIC: Force FDPIC related options unless -mno-fdpic is provided
  [ARM] FDPIC: Add support for FDPIC for arm architecture
  [ARM] FDPIC: Fix __do_global_dtors_aux and frame_dummy generation
  [ARM] FDPIC: Add support for c++ exceptions
  [ARM] FDPIC: Avoid saving/restoring r9 on stack since it is read-only
  [ARM] FDPIC: Enforce local/global binding for function descriptors
  [ARM] FDPIC: Add support for taking address of nested function
  [ARM] FDPIC: Implement TLS support.
  [ARM] FDPIC: Add support to unwind FDPIC signal frame
  [ARM] FDPIC: Restore r9 after we call __aeabi_read_tp
  [ARM] FDPIC: Force LSB bit for PC in Cortex-M architecture
  [ARM][testsuite] FDPIC: Skip unsupported tests
  [ARM][testsuite] FDPIC: Adjust scan-assembler patterns.
  [ARM][testsuite] FDPIC: Skip tests that don't work in PIC mode
  [ARM][testsuite] FDPIC: Handle *-*-uclinux*
  [ARM][testsuite] FDPIC: Enable tests on pie_enabled targets
  [ARM][testsuite] FDPIC: Adjust pr43698.c to avoid clash with uclibc.
  [ARM][testsuite] FDPIC: Skip tests using architectures unsupported by
    FDPIC
  [ARM] FDPIC: Handle stack-protector combined patterns
  [ARM][testsuite] FDPIC: Skip tests that require -static support
  [ARM] FDPIC: Implement libitm support.
  [ARM] FDPIC: Handle arm*-*-uclinuxfdpiceabi in libstdc++ configure
    scripts

 config/futex.m4                                    |   2 +-
 config/tls.m4                                      |   2 +-
 gcc/config.gcc                                     |  11 +-
 gcc/config/arm/arm-c.c                             |   2 +
 gcc/config/arm/arm-protos.h                        |   1 +
 gcc/config/arm/arm.c                               | 484 ++++++++++++++++++---
 gcc/config/arm/arm.h                               |  16 +-
 gcc/config/arm/arm.md                              |  83 +++-
 gcc/config/arm/arm.opt                             |   4 +
 gcc/config/arm/bpabi.h                             |   5 +-
 gcc/config/arm/linux-eabi.h                        |   7 +-
 gcc/config/arm/uclinuxfdpiceabi.h                  |  54 +++
 gcc/config/arm/unspecs.md                          |   1 +
 gcc/doc/invoke.texi                                |  24 +-
 gcc/ginclude/unwind-arm-common.h                   |   2 +-
 gcc/testsuite/g++.dg/abi/forced.C                  |   2 +-
 gcc/testsuite/g++.dg/abi/guard2.C                  |   2 +-
 gcc/testsuite/g++.dg/cpp0x/noexcept03.C            |   2 +-
 gcc/testsuite/g++.dg/ext/cleanup-10.C              |   2 +-
 gcc/testsuite/g++.dg/ext/cleanup-11.C              |   2 +-
 gcc/testsuite/g++.dg/ext/cleanup-8.C               |   2 +-
 gcc/testsuite/g++.dg/ext/cleanup-9.C               |   2 +-
 gcc/testsuite/g++.dg/ext/sync-4.C                  |   2 +-
 gcc/testsuite/g++.dg/ipa/comdat.C                  |   2 +-
 gcc/testsuite/g++.dg/ipa/devirt-c-7.C              |   3 +-
 gcc/testsuite/g++.dg/ipa/ivinline-1.C              |   2 +-
 gcc/testsuite/g++.dg/ipa/ivinline-2.C              |   2 +-
 gcc/testsuite/g++.dg/ipa/ivinline-3.C              |   2 +-
 gcc/testsuite/g++.dg/ipa/ivinline-4.C              |   2 +-
 gcc/testsuite/g++.dg/ipa/ivinline-5.C              |   2 +-
 gcc/testsuite/g++.dg/ipa/ivinline-7.C              |   2 +-
 gcc/testsuite/g++.dg/ipa/ivinline-8.C              |   2 +-
 gcc/testsuite/g++.dg/ipa/ivinline-9.C              |   2 +-
 gcc/testsuite/g++.dg/other/anon5.C                 |   1 +
 gcc/testsuite/g++.dg/tls/pr79288.C                 |   2 +-
 gcc/testsuite/gcc.c-torture/compile/pr82096.c      |   2 +-
 gcc/testsuite/gcc.dg/20020312-2.c                  |   1 +
 gcc/testsuite/gcc.dg/20041106-1.c                  |   2 +-
 gcc/testsuite/gcc.dg/addr_equal-1.c                |   3 +-
 gcc/testsuite/gcc.dg/cleanup-10.c                  |   2 +-
 gcc/testsuite/gcc.dg/cleanup-11.c                  |   2 +-
 gcc/testsuite/gcc.dg/cleanup-8.c                   |   2 +-
 gcc/testsuite/gcc.dg/cleanup-9.c                   |   2 +-
 gcc/testsuite/gcc.dg/const-1.c                     |   2 +-
 gcc/testsuite/gcc.dg/fdata-sections-1.c            |   2 +-
 gcc/testsuite/gcc.dg/fdata-sections-2.c            |   2 +-
 gcc/testsuite/gcc.dg/ipa/pure-const-1.c            |   2 +-
 gcc/testsuite/gcc.dg/noreturn-8.c                  |   2 +-
 gcc/testsuite/gcc.dg/pr33826.c                     |   3 +-
 gcc/testsuite/gcc.dg/pr39323-1.c                   |   2 +-
 gcc/testsuite/gcc.dg/pr39323-2.c                   |   2 +-
 gcc/testsuite/gcc.dg/pr39323-3.c                   |   2 +-
 gcc/testsuite/gcc.dg/pr65780-1.c                   |   2 +-
 gcc/testsuite/gcc.dg/pr65780-2.c                   |   2 +-
 gcc/testsuite/gcc.dg/pr67338.c                     |   2 +-
 gcc/testsuite/gcc.dg/pr78185.c                     |   2 +-
 gcc/testsuite/gcc.dg/pr83100-1.c                   |   2 +-
 gcc/testsuite/gcc.dg/pr83100-4.c                   |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-12g.c               |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-14g.c               |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-14gf.c              |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-16g.c               |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-17g.c               |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-18g.c               |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-1f.c                |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-22g.c               |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-2f.c                |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-31g.c               |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-33g.c               |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-4g.c                |   2 +-
 gcc/testsuite/gcc.dg/strlenopt-4gf.c               |   2 +-
 gcc/testsuite/gcc.dg/strncmp-2.c                   |   2 +-
 gcc/testsuite/gcc.dg/struct-ret-3.c                |   2 +-
 gcc/testsuite/gcc.dg/torture/ipa-pta-1.c           |   2 +-
 gcc/testsuite/gcc.dg/torture/pr69760.c             |   2 +-
 gcc/testsuite/gcc.dg/tree-ssa/alias-2.c            |   2 +-
 gcc/testsuite/gcc.dg/tree-ssa/ipa-split-5.c        |   2 +-
 gcc/testsuite/gcc.dg/tree-ssa/loadpre6.c           |   2 +-
 gcc/testsuite/gcc.target/arm/20051215-1.c          |   1 +
 .../gcc.target/arm/armv6-unaligned-load-ice.c      |   1 +
 .../gcc.target/arm/attr-unaligned-load-ice.c       |   1 +
 gcc/testsuite/gcc.target/arm/attr_arm-err.c        |   2 +-
 gcc/testsuite/gcc.target/arm/data-rel-2.c          |   1 +
 gcc/testsuite/gcc.target/arm/data-rel-3.c          |   1 +
 .../gcc.target/arm/di-longlong64-sync-withldrexd.c |   3 +-
 gcc/testsuite/gcc.target/arm/div64-unwinding.c     |   2 +-
 gcc/testsuite/gcc.target/arm/eliminate.c           |   2 +-
 gcc/testsuite/gcc.target/arm/fp16-aapcs-2.c        |   2 +-
 gcc/testsuite/gcc.target/arm/fp16-aapcs-4.c        |   2 +-
 gcc/testsuite/gcc.target/arm/ftest-armv4-arm.c     |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv4t-arm.c    |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv4t-thumb.c  |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv5t-arm.c    |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv5t-thumb.c  |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv5te-arm.c   |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv5te-thumb.c |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv6-arm.c     |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv6-thumb.c   |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv6k-arm.c    |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv6k-thumb.c  |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv6m-thumb.c  |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv6t2-arm.c   |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv6t2-thumb.c |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv6z-arm.c    |   1 +
 gcc/testsuite/gcc.target/arm/ftest-armv6z-thumb.c  |   1 +
 gcc/testsuite/gcc.target/arm/g2.c                  |   1 +
 gcc/testsuite/gcc.target/arm/interrupt-1.c         |   6 +-
 gcc/testsuite/gcc.target/arm/interrupt-2.c         |   6 +-
 gcc/testsuite/gcc.target/arm/ivopts-2.c            |   2 +-
 gcc/testsuite/gcc.target/arm/ivopts-3.c            |   2 +-
 gcc/testsuite/gcc.target/arm/ivopts-4.c            |   2 +-
 gcc/testsuite/gcc.target/arm/ivopts-5.c            |   2 +-
 gcc/testsuite/gcc.target/arm/macro_defs1.c         |   1 +
 gcc/testsuite/gcc.target/arm/mmx-1.c               |   1 +
 gcc/testsuite/gcc.target/arm/pr19599.c             |   1 +
 gcc/testsuite/gcc.target/arm/pr40887.c             |   1 +
 gcc/testsuite/gcc.target/arm/pr43597.c             |   2 +-
 gcc/testsuite/gcc.target/arm/pr43698.c             |   4 +-
 gcc/testsuite/gcc.target/arm/pr43920-2.c           |   2 +-
 gcc/testsuite/gcc.target/arm/pr45701-1.c           |   4 +-
 gcc/testsuite/gcc.target/arm/pr45701-2.c           |   4 +-
 gcc/testsuite/gcc.target/arm/pr59858.c             |   1 +
 gcc/testsuite/gcc.target/arm/pr61948.c             |   1 +
 gcc/testsuite/gcc.target/arm/pr65647-2.c           |   1 +
 gcc/testsuite/gcc.target/arm/pr66912.c             |   2 +-
 gcc/testsuite/gcc.target/arm/pr70830.c             |   3 +-
 gcc/testsuite/gcc.target/arm/pr77933-1.c           |   1 +
 gcc/testsuite/gcc.target/arm/pr77933-2.c           |   1 +
 gcc/testsuite/gcc.target/arm/pr79058.c             |   1 +
 gcc/testsuite/gcc.target/arm/pr83712.c             |   1 +
 .../gcc.target/arm/pragma_arch_switch_2.c          |   1 +
 gcc/testsuite/gcc.target/arm/scd42-1.c             |   1 +
 gcc/testsuite/gcc.target/arm/scd42-2.c             |   1 +
 gcc/testsuite/gcc.target/arm/scd42-3.c             |   1 +
 gcc/testsuite/gcc.target/arm/sibcall-1.c           |   1 +
 gcc/testsuite/gcc.target/arm/stack-checking.c      |   2 +-
 gcc/testsuite/gcc.target/arm/stack-red-zone.c      |   2 +-
 gcc/testsuite/gcc.target/arm/synchronize.c         |   2 +-
 gcc/testsuite/gcc.target/arm/tail-long-call.c      |   1 +
 gcc/testsuite/gcc.target/arm/tlscall.c             |   1 +
 gcc/testsuite/gcc.target/arm/vfp-longcall-apcs.c   |   1 +
 gcc/testsuite/lib/target-supports.exp              |  35 +-
 libatomic/configure                                |  11 +-
 libatomic/configure.tgt                            |   2 +-
 libgcc/config.host                                 |   4 +-
 libgcc/config/arm/linux-atomic.c                   |  55 ++-
 libgcc/config/arm/unwind-arm.c                     |   5 +
 libgcc/config/arm/unwind-arm.h                     |  31 +-
 libgcc/crtstuff.c                                  |  16 +
 libgcc/unwind-arm-common.inc                       | 216 +++++++++
 libgcc/unwind-pe.h                                 |  17 +
 libitm/config/arm/sjlj.S                           |  11 +-
 libitm/configure                                   |  22 +-
 libitm/configure.tgt                               |   2 +-
 libsanitizer/configure.tgt                         |   3 +
 libstdc++-v3/acinclude.m4                          |   9 +-
 libstdc++-v3/configure                             |  35 +-
 libstdc++-v3/configure.host                        |   6 +-
 libstdc++-v3/libsupc++/eh_personality.cc           |  10 +-
 libtool.m4                                         |  11 +-
 160 files changed, 1200 insertions(+), 227 deletions(-)
 create mode 100644 gcc/config/arm/uclinuxfdpiceabi.h
 mode change 100644 => 100755 libitm/configure

Comments

Wilco Dijkstra Sept. 17, 2019, 11:38 a.m. UTC | #1
Hi Christophe,

Can you explain this in more detail - it doesn't make sense to me to force the
Thumb bit during unwinding since it should already be correct, even on a
Thumb-only CPU. Perhaps the kernel code that pushes an incorrect address on
the stack could be fixed instead?

> Without this, when we are unwinding across a signal frame we can jump
> to an even address which leads to an exception.
>
> This is needed in __gnu_persnality_sigframe_fdpic() when restoring the
> PC from the signal frame since the PC saved by the kernel has the LSB
> bit set to zero.

Wilco
Christophe Lyon Sept. 17, 2019, 12:08 p.m. UTC | #2
On 17/09/2019 13:38, Wilco Dijkstra wrote:
> Hi Christophe,
> 
> Can you explain this in more detail - it doesn't make sense to me to force the
> Thumb bit during unwinding since it should already be correct, even on a
> Thumb-only CPU. Perhaps the kernel code that pushes an incorrect address on
> the stack could be fixed instead?
> 
>> Without this, when we are unwinding across a signal frame we can jump
>> to an even address which leads to an exception.
>>
>> This is needed in __gnu_persnality_sigframe_fdpic() when restoring the
>> PC from the signal frame since the PC saved by the kernel has the LSB
>> bit set to zero.
> 
> Wilco
> .
> 

Indeed, I've noticed the problem mentioned by Matthew since I committed that patch.

I was about to propose a fix, replacing #if (__thumb__) with #if (!__ARM_ARCH_ISA_ARM), but you are right: maybe the kernel code should be fixed instead.

So far I haven't managed to reproduce a failure in FDPIC mode without this patch though...

Thanks and sorry for the breakage.

Christophe
Christophe Lyon Sept. 19, 2019, 3:13 p.m. UTC | #3
On Tue, 17 Sep 2019 at 14:08, Christophe Lyon <christophe.lyon@st.com> wrote:
>
> On 17/09/2019 13:38, Wilco Dijkstra wrote:
> > Hi Christophe,
> >
> > Can you explain this in more detail - it doesn't make sense to me to force the
> > Thumb bit during unwinding since it should already be correct, even on a
> > Thumb-only CPU. Perhaps the kernel code that pushes an incorrect address on
> > the stack could be fixed instead?
> >
> >> Without this, when we are unwinding across a signal frame we can jump
> >> to an even address which leads to an exception.
> >>
> >> This is needed in __gnu_persnality_sigframe_fdpic() when restoring the
> >> PC from the signal frame since the PC saved by the kernel has the LSB
> >> bit set to zero.
> >
> > Wilco
> > .
> >
>
> Indeed, I've noticed the problem mentioned by Matthew since I committed that patch.
>
> I was about to propose a fix, replacing #if (__thumb__) with #if (!__ARM_ARCH_ISA_ARM), but you are right: maybe the kernel code should be fixed instead.
>
> So far I haven't managed to reproduce a failure in FDPIC mode without this patch though...
>
> Thanks and sorry for the breakage.
>

I'm having problems with the board I use for testing, so I propose to
revert that patch until I have a better description of the problem it
fixed.
OK?

Christophe

> Christophe
>
Kyrill Tkachov Sept. 19, 2019, 3:14 p.m. UTC | #4
On 9/19/19 4:13 PM, Christophe Lyon wrote:
> On Tue, 17 Sep 2019 at 14:08, Christophe Lyon <christophe.lyon@st.com> 
> wrote:
> >
> > On 17/09/2019 13:38, Wilco Dijkstra wrote:
> > > Hi Christophe,
> > >
> > > Can you explain this in more detail - it doesn't make sense to me 
> to force the
> > > Thumb bit during unwinding since it should already be correct, 
> even on a
> > > Thumb-only CPU. Perhaps the kernel code that pushes an incorrect 
> address on
> > > the stack could be fixed instead?
> > >
> > >> Without this, when we are unwinding across a signal frame we can jump
> > >> to an even address which leads to an exception.
> > >>
> > >> This is needed in __gnu_persnality_sigframe_fdpic() when 
> restoring the
> > >> PC from the signal frame since the PC saved by the kernel has the LSB
> > >> bit set to zero.
> > >
> > > Wilco
> > > .
> > >
> >
> > Indeed, I've noticed the problem mentioned by Matthew since I 
> committed that patch.
> >
> > I was about to propose a fix, replacing #if (__thumb__) with #if 
> (!__ARM_ARCH_ISA_ARM), but you are right: maybe the kernel code should 
> be fixed instead.
> >
> > So far I haven't managed to reproduce a failure in FDPIC mode 
> without this patch though...
> >
> > Thanks and sorry for the breakage.
> >
>
> I'm having problems with the board I use for testing, so I propose to
> revert that patch until I have a better description of the problem it
> fixed.
> OK?

Ok by me as long as lives the fdpic toolchain in a usable state (barring 
the potential issue here)

Thanks,

Kyrill


>
> Christophe
>
> > Christophe
> >
Matthew Malcomson Sept. 20, 2019, 12:51 p.m. UTC | #5
On 19/09/19 16:14, Kyrill Tkachov wrote:
> 
> On 9/19/19 4:13 PM, Christophe Lyon wrote:
>> On Tue, 17 Sep 2019 at 14:08, Christophe Lyon <christophe.lyon@st.com> 
>> wrote:
>> >
>> > On 17/09/2019 13:38, Wilco Dijkstra wrote:
>> > > Hi Christophe,
>> > >
>> > > Can you explain this in more detail - it doesn't make sense to me 
>> to force the
>> > > Thumb bit during unwinding since it should already be correct, 
>> even on a
>> > > Thumb-only CPU. Perhaps the kernel code that pushes an incorrect 
>> address on
>> > > the stack could be fixed instead?
>> > >
>> > >> Without this, when we are unwinding across a signal frame we can 
>> jump
>> > >> to an even address which leads to an exception.
>> > >>
>> > >> This is needed in __gnu_persnality_sigframe_fdpic() when 
>> restoring the
>> > >> PC from the signal frame since the PC saved by the kernel has the 
>> LSB
>> > >> bit set to zero.
>> > >
>> > > Wilco
>> > > .
>> > >
>> >
>> > Indeed, I've noticed the problem mentioned by Matthew since I 
>> committed that patch.
>> >
>> > I was about to propose a fix, replacing #if (__thumb__) with #if 
>> (!__ARM_ARCH_ISA_ARM), but you are right: maybe the kernel code should 
>> be fixed instead.
>> >
>> > So far I haven't managed to reproduce a failure in FDPIC mode 
>> without this patch though...
>> >
>> > Thanks and sorry for the breakage.
>> >
>>
>> I'm having problems with the board I use for testing, so I propose to
>> revert that patch until I have a better description of the problem it
>> fixed.
>> OK?
> 
> Ok by me as long as lives the fdpic toolchain in a usable state (barring 
> the potential issue here)

Thanks Christophe -- reverting that patch would help our internal 
testing a lot!
MM

> 
> Thanks,
> 
> Kyrill
> 
> 
>>
>> Christophe
>>
>> > Christophe
>> >
Christophe Lyon Sept. 20, 2019, 2:23 p.m. UTC | #6
On Fri, 20 Sep 2019 at 14:51, Matthew Malcomson
<Matthew.Malcomson@arm.com> wrote:
>
> On 19/09/19 16:14, Kyrill Tkachov wrote:
> >
> > On 9/19/19 4:13 PM, Christophe Lyon wrote:
> >> On Tue, 17 Sep 2019 at 14:08, Christophe Lyon <christophe.lyon@st.com>
> >> wrote:
> >> >
> >> > On 17/09/2019 13:38, Wilco Dijkstra wrote:
> >> > > Hi Christophe,
> >> > >
> >> > > Can you explain this in more detail - it doesn't make sense to me
> >> to force the
> >> > > Thumb bit during unwinding since it should already be correct,
> >> even on a
> >> > > Thumb-only CPU. Perhaps the kernel code that pushes an incorrect
> >> address on
> >> > > the stack could be fixed instead?
> >> > >
> >> > >> Without this, when we are unwinding across a signal frame we can
> >> jump
> >> > >> to an even address which leads to an exception.
> >> > >>
> >> > >> This is needed in __gnu_persnality_sigframe_fdpic() when
> >> restoring the
> >> > >> PC from the signal frame since the PC saved by the kernel has the
> >> LSB
> >> > >> bit set to zero.
> >> > >
> >> > > Wilco
> >> > > .
> >> > >
> >> >
> >> > Indeed, I've noticed the problem mentioned by Matthew since I
> >> committed that patch.
> >> >
> >> > I was about to propose a fix, replacing #if (__thumb__) with #if
> >> (!__ARM_ARCH_ISA_ARM), but you are right: maybe the kernel code should
> >> be fixed instead.
> >> >
> >> > So far I haven't managed to reproduce a failure in FDPIC mode
> >> without this patch though...
> >> >
> >> > Thanks and sorry for the breakage.
> >> >
> >>
> >> I'm having problems with the board I use for testing, so I propose to
> >> revert that patch until I have a better description of the problem it
> >> fixed.
> >> OK?
> >
> > Ok by me as long as lives the fdpic toolchain in a usable state (barring
> > the potential issue here)
>
> Thanks Christophe -- reverting that patch would help our internal
> testing a lot!
> MM
>
OK, I've reverted it.

Christophe

> >
> > Thanks,
> >
> > Kyrill
> >
> >
> >>
> >> Christophe
> >>
> >> > Christophe
> >> >
>