From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id EE83D3896C3A for ; Fri, 21 Jun 2024 14:38:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EE83D3896C3A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EE83D3896C3A Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1718980723; cv=none; b=kZhUHl71SnwanzDSJgroVVJxxIuxEHxDiF/rzTQ614mpOZUusPHqAC11OuIVoO2BMn0lpyY7E0In0INvJUhwMhYGNh3eXbfu4ItD05rWIchcL6MnBJ3otFYRJci24rTmzTDIHfs3qQGHd56nSjGG+FvyA2Yd0qdkpEd4EFYo8o0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1718980723; c=relaxed/simple; bh=VYU4w7L/V+YtQS5hQGfpcbdSGHWMJpkcQKSNP/pIgVE=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=P3Kb/i3UfQnR/uTosiwghcwpGTBVRnkjBTN5Wr192QHQJEutmz4g4JCoe5ZXMXffc3MYjBZR+FjS0lYv+qE2NUkqGxgPbc4SSCZsVd3c4k0sCFwu9iFIs2qTG5KAVaYUfO+Cld1zVxSDPcjO8iZhiNDoNCLH70o79wngZo/rTAQ= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 7955BDA7; Fri, 21 Jun 2024 07:39:03 -0700 (PDT) Received: from [10.2.78.57] (e120077-lin.cambridge.arm.com [10.2.78.57]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EC4C13F6A8; Fri, 21 Jun 2024 07:38:37 -0700 (PDT) Message-ID: <47098783-b051-4b8e-a4f9-071096d2a392@arm.com> Date: Fri, 21 Jun 2024 15:38:36 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] arm: Do not insert stubs needing Arm code on Thumb-only cores. To: Christophe Lyon , binutils@sourceware.org, andre.simoesdiasvieira@arm.com References: <20240619143547.31460-1-christophe.lyon@linaro.org> From: "Richard Earnshaw (lists)" Content-Language: en-GB In-Reply-To: <20240619143547.31460-1-christophe.lyon@linaro.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3497.1 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 19/06/2024 15:35, Christophe Lyon wrote: > We recently fixed a bug in libgcc > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115360) > where a symbol was missing a %function .type decoration. > > This meant the linker would silently pick the wrong type of 'farcall > stub', involving Arm-mode instructions on Thumb-only CPUs. > > This patch emits an error instead, and warns in some other cases, to > encourage users to add the missing '.type foo,%function' directive. > > In practice: in arm_type_of_stub() we no longer try to infer which > stub to use if the destination is of unknown type and the CPU is > Thumb-only; so we won't lie to elf32_arm_size_stubs() which does not > check branch_type. > > If branch_type is ST_BRANCH_TO_ARM but the CPU is Thumb-only, we now > convert it to ST_BRANCH_TO_THUMB only if the destination is of > absolute type. This is to support the case where the destination of > the branch is defined by the linker script (see thumb-b-lks-sym.s and > thumb-bl-lks-sym.s testcases for instance). > > The motivating case is covered by the new farcall-missing-type > testcase, where we now emit an error message. We do not emit an error > when branch_type is ST_BRANCH_UNKNOWN and the CPU supports Arm-mode: a > lot of legacy code (e.g. newlib's crt0.S) lacks the corresponding > '.type foo, %function' directives and even a (too verbose) warning > could be perceived as a nuisance. > > Existing testcases where such a warning would trigger: > arm-static-app.s (app_func, app_func2) > arm-rel32.s (foo) > arm-app.s (app_func) > rel32-reject.s () main) > fix-arm1176.s (func_to_branch_to) > but this list is not exhaustive. > > For the sake of clarity, the patch replaces occurrences of > sym.st_target_internal = 0; with > sym.st_target_internal = ST_BRANCH_TO_ARM; > > enum arm_st_branch_type is defined in include/elf/arm.h, and relies on > ST_BRANCH_TO_ARM==0, as sym.st_target_internal is also initialized to > 0 in other target-independent parts of BFD code. (For instance, > swapping the ST_BRANCH_TO_ARM and ST_BRANCH_TO_THUMB entries in the > enum definition leads to 'interesting' results...) > > Regarding the testsuite: > * new expected warning for thumb-b-lks-sym and thumb-bl-lks-sym > * new testcase farcall-missing-type to check the new error case > * attr-merge-arch-2b.s, branch-futures (and bfs-1.s) updated to avoid > a diagnostic > This is OK thanks, but I think that, given this might have some unexpected fallout, it might be wise to delay pushing it until after the 2.43 branch has been cut. That way we get plenty of time to assess whether or not this is too agressive. R. > Tested on arm-eabi and arm-pe with no regression. > --- > bfd/elf32-arm.c | 99 ++++++++++++++++--- > ld/testsuite/ld-arm/arm-elf.exp | 5 +- > ld/testsuite/ld-arm/attr-merge-arch-2b.s | 1 + > ld/testsuite/ld-arm/bfs-1.s | 1 + > ld/testsuite/ld-arm/branch-futures.d | 10 +- > .../ld-arm/farcall-missing-type-bad.s | 7 ++ > .../ld-arm/farcall-missing-type-main.s | 9 ++ > ld/testsuite/ld-arm/farcall-missing-type.d | 5 + > ld/testsuite/ld-arm/farcall-missing-type.ld | 23 +++++ > ld/testsuite/ld-arm/thumb-b-lks-sym.output | 1 + > ld/testsuite/ld-arm/thumb-bl-lks-sym.output | 1 + > 11 files changed, 143 insertions(+), 19 deletions(-) > create mode 100644 ld/testsuite/ld-arm/farcall-missing-type-bad.s > create mode 100644 ld/testsuite/ld-arm/farcall-missing-type-main.s > create mode 100644 ld/testsuite/ld-arm/farcall-missing-type.d > create mode 100644 ld/testsuite/ld-arm/farcall-missing-type.ld > create mode 100644 ld/testsuite/ld-arm/thumb-b-lks-sym.output > create mode 100644 ld/testsuite/ld-arm/thumb-bl-lks-sym.output > > diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c > index ca76bee6adc..7441ee2cc38 100644 > --- a/bfd/elf32-arm.c > +++ b/bfd/elf32-arm.c > @@ -4226,12 +4226,33 @@ arm_type_of_stub (struct bfd_link_info *info, > > r_type = ELF32_R_TYPE (rel->r_info); > > + /* Don't pretend we know what stub to use (if any) when we target a > + Thumb-only target and we don't know the actual destination > + type. */ > + if (branch_type == ST_BRANCH_UNKNOWN && thumb_only) > + return stub_type; > + > /* ST_BRANCH_TO_ARM is nonsense to thumb-only targets when we > are considering a function call relocation. */ > if (thumb_only && (r_type == R_ARM_THM_CALL || r_type == R_ARM_THM_JUMP24 > || r_type == R_ARM_THM_JUMP19) > && branch_type == ST_BRANCH_TO_ARM) > - branch_type = ST_BRANCH_TO_THUMB; > + { > + if (sym_sec == bfd_abs_section_ptr) > + /* As an exception, assume that absolute symbols are of the > + right kind (Thumb). They are presumably defined in the > + linker script, where it is not possible to declare them as > + Thumb (and thus are seen as Arm mode). We'll inform the > + user with a warning, though, in > + elf32_arm_final_link_relocate. */ > + branch_type = ST_BRANCH_TO_THUMB; > + else > + /* Otherwise do not silently build a stub, and let the users > + know they have to fix their code. Indeed, we could decide > + to insert a stub involving Arm code and/or BLX, leading to > + a run-time crash. */ > + return stub_type; > + } > > /* For TLS call relocs, it is the caller's responsibility to provide > the address of the appropriate trampoline. */ > @@ -10382,14 +10403,6 @@ elf32_arm_final_link_relocate (reloc_howto_type * howto, > else > addend = signed_addend = rel->r_addend; > > - /* ST_BRANCH_TO_ARM is nonsense to thumb-only targets when we > - are resolving a function call relocation. */ > - if (using_thumb_only (globals) > - && (r_type == R_ARM_THM_CALL > - || r_type == R_ARM_THM_JUMP24) > - && branch_type == ST_BRANCH_TO_ARM) > - branch_type = ST_BRANCH_TO_THUMB; > - > /* Record the symbol information that should be used in dynamic > relocations. */ > dynreloc_st_type = st_type; > @@ -10452,6 +10465,68 @@ elf32_arm_final_link_relocate (reloc_howto_type * howto, > gotplt_offset = (bfd_vma) -1; > } > > + /* ST_BRANCH_TO_ARM is nonsense to thumb-only targets when we are > + resolving a function call relocation. We want to inform the user > + that something is wrong. */ > + if (using_thumb_only (globals) > + && (r_type == R_ARM_THM_CALL > + || r_type == R_ARM_THM_JUMP24) > + && branch_type == ST_BRANCH_TO_ARM > + /* Calls through a PLT are special: the assembly source code > + cannot be annotated with '.type foo(PLT), %function', and > + they handled specifically below anyway. */ > + && splt == NULL) > + { > + if (sym_sec == bfd_abs_section_ptr) > + { > + /* As an exception, assume that absolute symbols are of the > + right kind (Thumb). They are presumably defined in the > + linker script, where it is not possible to declare them as > + Thumb (and thus are seen as Arm mode). Inform the user with > + a warning, though. */ > + branch_type = ST_BRANCH_TO_THUMB; > + > + if (sym_sec->owner) > + _bfd_error_handler > + (_("warning: %pB(%s): Forcing bramch to absolute symbol in Thumb mode (Thumb-only CPU)" > + " in %pB"), > + sym_sec->owner, sym_name, input_bfd); > + else > + _bfd_error_handler > + (_("warning: (%s): Forcing branch to absolute symbol in Thumb mode (Thumb-only CPU)" > + " in %pB"), > + sym_name, input_bfd); > + } > + else > + /* Otherwise do not silently build a stub, and let the users > + know they have to fix their code. Indeed, we could decide > + to insert a stub involving Arm code and/or BLX, leading to > + a run-time crash. */ > + branch_type = ST_BRANCH_UNKNOWN; > + } > + > + /* Fail early if branch_type is ST_BRANCH_UNKNOWN and we target a > + Thumb-only CPU. We could emit a warning on Arm-capable targets > + too, but that would be too verbose (a lot of legacy code does not > + use the .type foo, %function directive). */ > + if (using_thumb_only (globals) > + && (r_type == R_ARM_THM_CALL > + || r_type == R_ARM_THM_JUMP24) > + && branch_type == ST_BRANCH_UNKNOWN) > + { > + if (sym_sec != NULL > + && sym_sec->owner != NULL) > + _bfd_error_handler > + (_("%pB(%s): Unknown destination type (ARM/Thumb) in %pB"), > + sym_sec->owner, sym_name, input_bfd); > + else > + _bfd_error_handler > + (_("(%s): Unknown destination type (ARM/Thumb) in %pB"), > + sym_name, input_bfd); > + > + return bfd_reloc_notsupported; > + } > + > resolved_to_zero = (h != NULL > && UNDEFWEAK_NO_DYNAMIC_RELOC (info, h)); > > @@ -17838,7 +17913,7 @@ elf32_arm_output_map_sym (output_arch_syminfo *osi, > sym.st_other = 0; > sym.st_info = ELF_ST_INFO (STB_LOCAL, STT_NOTYPE); > sym.st_shndx = osi->sec_shndx; > - sym.st_target_internal = 0; > + sym.st_target_internal = ST_BRANCH_TO_ARM; > elf32_arm_section_map_add (osi->sec, names[type][1], offset); > return osi->func (osi->flaginfo, names[type], &sym, osi->sec, NULL) == 1; > } > @@ -17995,7 +18070,7 @@ elf32_arm_output_stub_sym (output_arch_syminfo *osi, const char *name, > sym.st_other = 0; > sym.st_info = ELF_ST_INFO (STB_LOCAL, STT_FUNC); > sym.st_shndx = osi->sec_shndx; > - sym.st_target_internal = 0; > + sym.st_target_internal = ST_BRANCH_TO_ARM; > return osi->func (osi->flaginfo, name, &sym, osi->sec, NULL) == 1; > } > > @@ -19743,7 +19818,7 @@ elf32_arm_swap_symbol_in (bfd * abfd, > { > if (!bfd_elf32_swap_symbol_in (abfd, psrc, pshn, dst)) > return false; > - dst->st_target_internal = 0; > + dst->st_target_internal = ST_BRANCH_TO_ARM; > > /* New EABI objects mark thumb function symbols by setting the low bit of > the address. */ > diff --git a/ld/testsuite/ld-arm/arm-elf.exp b/ld/testsuite/ld-arm/arm-elf.exp > index 272d518c4d4..5f380e383d0 100644 > --- a/ld/testsuite/ld-arm/arm-elf.exp > +++ b/ld/testsuite/ld-arm/arm-elf.exp > @@ -653,10 +653,10 @@ set armeabitests_nonacl { > {{objdump -d thumb2-bl-bad.d}} > "thumb2-bl-bad"} > {"Branch to linker script symbol with BL for thumb-only target" "-T branch-lks-sym.ld" "" "" {thumb-bl-lks-sym.s} > - {{objdump -d thumb-bl-lks-sym.d}} > + {{ld thumb-bl-lks-sym.output} {objdump -d thumb-bl-lks-sym.d}} > "thumb-bl-lks-sym"} > {"Branch to linker script symbol with B for thumb-only target" "-T branch-lks-sym.ld" "" "" {thumb-b-lks-sym.s} > - {{objdump -d thumb-b-lks-sym.d}} > + {{ld thumb-b-lks-sym.output} {objdump -d thumb-b-lks-sym.d}} > "thumb-b-lks-sym"} > > {"erratum 760522 fix (default for v6z)" "--section-start=.foo=0x2001014" "" > @@ -1207,6 +1207,7 @@ run_dump_test "attr-merge-wchar-40-nowarn" > run_dump_test "attr-merge-wchar-42-nowarn" > run_dump_test "attr-merge-wchar-44-nowarn" > run_dump_test "farcall-section" > +run_dump_test "farcall-missing-type" > run_dump_test "attr-merge-unknown-1" > run_dump_test "attr-merge-unknown-2" > run_dump_test "attr-merge-unknown-2r" > diff --git a/ld/testsuite/ld-arm/attr-merge-arch-2b.s b/ld/testsuite/ld-arm/attr-merge-arch-2b.s > index 57718354145..f20522f81d2 100644 > --- a/ld/testsuite/ld-arm/attr-merge-arch-2b.s > +++ b/ld/testsuite/ld-arm/attr-merge-arch-2b.s > @@ -4,5 +4,6 @@ > .align 2 > .global foo > .type foo, %function > + .thumb_func > foo: > bx lr > diff --git a/ld/testsuite/ld-arm/bfs-1.s b/ld/testsuite/ld-arm/bfs-1.s > index 2b72819598e..1e31d440cc2 100644 > --- a/ld/testsuite/ld-arm/bfs-1.s > +++ b/ld/testsuite/ld-arm/bfs-1.s > @@ -4,6 +4,7 @@ > .thumb > .global _start > .global target > +.type target, %function > _start: > target: > add r0, r0, r1 > diff --git a/ld/testsuite/ld-arm/branch-futures.d b/ld/testsuite/ld-arm/branch-futures.d > index 427ecce62a4..bc9ae8eddf7 100644 > --- a/ld/testsuite/ld-arm/branch-futures.d > +++ b/ld/testsuite/ld-arm/branch-futures.d > @@ -5,13 +5,13 @@ > Disassembly of section .text: > > 0[0-9a-f]+ : > - [0-9a-f]+: f2c0 e807 bf a, 8012 <_start> > - [0-9a-f]+: f182 e805 bfcsel 6, 8012 <_start>, a, eq > - [0-9a-f]+: f080 c803 bfl 2, 8012 <_start> > + [0-9a-f]+: f2c0 e807 bf a, 8012 > + [0-9a-f]+: f182 e805 bfcsel 6, 8012 , a, eq > + [0-9a-f]+: f080 c803 bfl 2, 8012 > [0-9a-f]+: 4408 add r0, r1 > > 0[0-9a-f]+ : > - [0-9a-f]+: f000 b800 b.w 8012 <_start> > + [0-9a-f]+: f000 b800 b.w 8012 > > -0[0-9a-f]+ <_start>: > +0[0-9a-f]+ : > [0-9a-f]+: 4408 add r0, r1 > diff --git a/ld/testsuite/ld-arm/farcall-missing-type-bad.s b/ld/testsuite/ld-arm/farcall-missing-type-bad.s > new file mode 100644 > index 00000000000..e087992b33f > --- /dev/null > +++ b/ld/testsuite/ld-arm/farcall-missing-type-bad.s > @@ -0,0 +1,7 @@ > + .thumb > + .cpu cortex-m33 > + .syntax unified > + .section .text.bar > + .global bad @ untyped > +# .type bad, function > +bad: bx lr > diff --git a/ld/testsuite/ld-arm/farcall-missing-type-main.s b/ld/testsuite/ld-arm/farcall-missing-type-main.s > new file mode 100644 > index 00000000000..c97df0cf459 > --- /dev/null > +++ b/ld/testsuite/ld-arm/farcall-missing-type-main.s > @@ -0,0 +1,9 @@ > + .thumb > + .cpu cortex-m33 > + .syntax unified > + .global __start > + .type __start, function > +__start: > + push {r4, lr} > + bl bad > + pop {r4, pc} > diff --git a/ld/testsuite/ld-arm/farcall-missing-type.d b/ld/testsuite/ld-arm/farcall-missing-type.d > new file mode 100644 > index 00000000000..9766bf1e10c > --- /dev/null > +++ b/ld/testsuite/ld-arm/farcall-missing-type.d > @@ -0,0 +1,5 @@ > +#source: farcall-missing-type-main.s > +#source: farcall-missing-type-bad.s > +#as: > +#ld:-T farcall-missing-type.ld > +#error: .* .*/farcall-missing-type-bad.o\(bad\): Unknown destination type \(ARM/Thumb\) in .*/farcall-missing-type-main.o > diff --git a/ld/testsuite/ld-arm/farcall-missing-type.ld b/ld/testsuite/ld-arm/farcall-missing-type.ld > new file mode 100644 > index 00000000000..b1136de93ba > --- /dev/null > +++ b/ld/testsuite/ld-arm/farcall-missing-type.ld > @@ -0,0 +1,23 @@ > +/* Linker script to configure memory regions. */ > +MEMORY > +{ > + FLASH (rx) : ORIGIN = 0x00000000, LENGTH = 0x40000 /* 256k */ > + FLASH2 (rx) : ORIGIN = 0x200001e0, LENGTH = 0x4000 > +} > + > +ENTRY(__start) > + > +SECTIONS > +{ > + .far_away_section : > + { > + *(.text.bar) > + } > FLASH2 > + > + .text : > + { > + *(.text*) > + > + } > FLASH > + > +} > diff --git a/ld/testsuite/ld-arm/thumb-b-lks-sym.output b/ld/testsuite/ld-arm/thumb-b-lks-sym.output > new file mode 100644 > index 00000000000..49612042910 > --- /dev/null > +++ b/ld/testsuite/ld-arm/thumb-b-lks-sym.output > @@ -0,0 +1 @@ > +.* \(extFunc\): Forcing branch to absolute symbol in Thumb mode \(Thumb-only CPU\) in tmpdir/thumb-b-lks-sym.o > diff --git a/ld/testsuite/ld-arm/thumb-bl-lks-sym.output b/ld/testsuite/ld-arm/thumb-bl-lks-sym.output > new file mode 100644 > index 00000000000..7c5e3543d84 > --- /dev/null > +++ b/ld/testsuite/ld-arm/thumb-bl-lks-sym.output > @@ -0,0 +1 @@ > +.* \(extFunc\): Forcing branch to absolute symbol in Thumb mode \(Thumb-only CPU\) in tmpdir/thumb-bl-lks-sym.o