From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x430.google.com (mail-pf1-x430.google.com [IPv6:2607:f8b0:4864:20::430]) by sourceware.org (Postfix) with ESMTPS id D74DD3858D20 for ; Fri, 28 Jul 2023 15:03:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D74DD3858D20 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=rivosinc.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rivosinc.com Received: by mail-pf1-x430.google.com with SMTP id d2e1a72fcca58-686ba29ccb1so1401753b3a.1 for ; Fri, 28 Jul 2023 08:03:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20221208.gappssmtp.com; s=20221208; t=1690556600; x=1691161400; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:from:to:cc:subject:date:message-id :reply-to; bh=LGnZBIimueOgUeOiZ+XOn+RJqpaleGqMCaLLdtYKED8=; b=Az41pPXnAFRYU0+/40b84wU+yfvPFdw4rKTW7zKG/YoFuo1GoA2x9fuxuR/tDsSAND enerJuuzayzxggL+iw/b++UQ5omMutoXh9gmoyoLDJM1vNI1tlanSOpCRC6X9Yp6Ysgh L7usPIyl0kspjBTMwpiNDg53LANe9PNMJhwuxDq0eB0EuMD5A5wRUYFWNu61bUmwhkRt M1qBbSTUKy7Mm2MWl4Frnp0GPN7j8gnfpOlbICHLeLe+NeJJdXMJ0c+/YyV+T+eBJlm7 OOYAbwUVMcyT82sfZ3Pp4BncErD2Je7I4NAvq7ManPQW5ubDUQjMXinaZZyc8wnoay4R 7l+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690556600; x=1691161400; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=LGnZBIimueOgUeOiZ+XOn+RJqpaleGqMCaLLdtYKED8=; b=jhVnL+ynwFY6xkQGRBJDPfdd+YePk5UBvhSHzCDATVCVIZiD2/OHrRoTn9fVQpCy24 1ONMVwZDPqNvwnpnjOdOuePrmXGJMJmLS3iYYMWa/PXaERjEq40LkKdx4W9aXqsrVMEi KS0rHtdV2LfdAztlnmZSMuvx0Jk1lPVI2pBr/8X9vsZ6x3XQmg97Zf3mTa6OZo4fI1/x ZHzYRQ0DLPMrCqmsn2lGSvAqRV6GeJOQLij3wSXUinhRoPndLDasdB7CAHd/zSo34Ixp RnU+t+b9xqVY/uTXozbN7Vfi/czuhSTBY2zAHDIxD33B2w/yHvB9ivZoEhyNdZBifVRt sOWg== X-Gm-Message-State: ABy/qLY4NnDk5/Os6ej8yJKO80gDeqQubS76WPvf6l/4+4SsIPoRo0Jh ++Ga01ne7SnKw4KkU3KyvwfZtQ== X-Google-Smtp-Source: APBJJlHICH2U/uwPN7zFHHc5k/fx2Ypk8ZLp+UXJYmW7u1L+uRX1s0a8/LGuhM0JEEgBRkFwG9JFqA== X-Received: by 2002:a05:6a20:1587:b0:12e:3394:e2b6 with SMTP id h7-20020a056a20158700b0012e3394e2b6mr2681292pzj.8.1690556599954; Fri, 28 Jul 2023 08:03:19 -0700 (PDT) Received: from localhost ([135.180.227.0]) by smtp.gmail.com with ESMTPSA id x17-20020aa79191000000b006827c26f148sm3292165pfa.195.2023.07.28.08.03.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 28 Jul 2023 08:03:19 -0700 (PDT) Date: Fri, 28 Jul 2023 08:03:19 -0700 (PDT) X-Google-Original-Date: Fri, 28 Jul 2023 08:03:17 PDT (-0700) Subject: RE: [EXTERNAL] Re: [PATCH] RISC-V: Do not gp relax against an ABS symbol if it is far away. In-Reply-To: CC: nelson@rivosinc.com, binutils@sourceware.org From: Palmer Dabbelt To: Joseph.Faulls@imgtec.com Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE 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 Fri, 28 Jul 2023 02:18:49 PDT (-0700), Joseph.Faulls@imgtec.com wrote: > Thanks for the replies! > > > * Do you have a reduced test that shows we still have a chance to get the relax error by using the default linker script? > Yes, you can re-produce this with the default linkerscript, but it’s a little harder. If you take the same assembly files as the test cases in this patch (gp-relax-abs-sym.s and gp-relax-abs.s), assemble and link an elf and see the value __global_pointer$. Then adjust the absolute symbol to be this + 0x800. Re-assemble + link, and you should see a truncation. > > I came across this issue when using a custom linker script that provided some symbols (which are absolute) and linking a program with quite a few relaxations. > > * I'm not quite sure what you mean by GP moving -- GP's the register, so it doesn't have a value until runtime. GP is meant to equal __global_pointer$, but that's usually a data symbol and thus doesn't move as a result of relaxation. > Perhaps I’m using the terms incorrectly, but if the __global_pointer is set in the linkerscript at the end of some section (I think this is most cases), then this value will change if the size of any section before it changes, as would be in the case of relaxation. Could we not describe this as the GP moving? I wouldn't, but I think it's just different meanings of the words here -- it shouldn't really matter for the purposes of fixing the bug, I think I understand what you're saying ;) I think Nelson's point about cross-section GP relaxation is probably the root cause here, we've had some other bugs like that crop up recently. Assuming that's the case then SHN_ABS is just a red herring: maybe it makes triggering the bug easier, but we can probably come up with a similar failure in other ways. Assuming that's the case, both SHN_ABS and /2 are just a hueristic here. We've got some tricky cases floating around for the cross-section stuff where we don't have a great answer yet, I wouldn't be surprised if this is another case where we just can't relax safely. Can you post a reduced test case with a custom linker script that triggers the problem? That should help us a bit more concrete. > From: Nelson Chu > Sent: Friday, July 28, 2023 2:16 AM > To: Palmer Dabbelt > Cc: Joseph Faulls ; binutils@sourceware.org > Subject: [EXTERNAL] Re: [PATCH] RISC-V: Do not gp relax against an ABS symbol if it is far away. > > Maybe, if the shrink code is more than the data segment alignment in the default linker script (0x1000), then probably still have a chance to get the truncated gp by using default linker script... I will suggest if that is so, then we just have a linker option to disallow/allow the gp relax for any abs symbol, that should be more intuitive. > > On Fri, Jul 28, 2023 at 9:02 AM Nelson Chu > wrote: > Relaxation should shrink the text, but since the default linker script has huge data segment alignment, data after the segment alignment shouldn't be affected and shrinked, so ideally it should be safe by using default linker script. > > Do you have a reduced test that shows we still have a chance to get the relax error by using the default linker script? > > On Fri, Jul 28, 2023 at 5:08 AM Palmer Dabbelt > wrote: > On Thu, 27 Jul 2023 09:48:24 PDT (-0700), Joseph.Faulls@imgtec.com wrote: >> Relaxations can cause the gp to move after it has been decided to gp >> relax. Against an absolute symbol, the distance may change such that the >> offset can no longer fit in the 12-bit immediate field. >> >> bfd/ >> * elfnn-riscv.c (_bfd_riscv_relax_pc) Do not gp relax against and ABS >> symbol if it is far away. >> ld/ >> * ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp: Updated. >> * ld/testsuite/ld-riscv-elf/gp-relax-abs*: New testcases. >> --- >> bfd/elfnn-riscv.c | 12 ++++++++++++ >> ld/testsuite/ld-riscv-elf/gp-relax-abs-sym.s | 3 +++ >> ld/testsuite/ld-riscv-elf/gp-relax-abs.d | 14 ++++++++++++++ >> ld/testsuite/ld-riscv-elf/gp-relax-abs.s | 5 +++++ >> ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp | 1 + >> 5 files changed, 35 insertions(+) >> create mode 100644 ld/testsuite/ld-riscv-elf/gp-relax-abs-sym.s >> create mode 100644 ld/testsuite/ld-riscv-elf/gp-relax-abs.d >> create mode 100644 ld/testsuite/ld-riscv-elf/gp-relax-abs.s >> >> diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c >> index 09aa7be225e..79e29e8b272 100644 >> --- a/bfd/elfnn-riscv.c >> +++ b/bfd/elfnn-riscv.c >> @@ -4885,6 +4885,18 @@ _bfd_riscv_relax_pc (bfd *abfd ATTRIBUTE_UNUSED, >> max_alignment = _bfd_riscv_get_max_alignment (sec, gp); >> htab->max_alignment_for_gp = max_alignment; >> } >> + /* If the symbol is in the abs section, relaxation could cause the gp >> + * to move such that the gp relocation is no longer possible. > > I'm not quite sure what you mean by GP moving -- GP's the register, so > it doesn't have a value until runtime. GP is meant to equal > __global_pointer$, but that's usually a data symbol and thus doesn't > move as a result of relaxation. > > We do have some other issues related to SHN_ABS (for example position > independent vs PC relative stuff), so it's possible there's some other > bug here? > >> + * Conservatively half the allowed distance, as it cannot be that >> + * gp moves more than this, i.e. more than half the instructions be >> + * deleted due to relaxation. Do this by adjusting reserve_size. */ >> + if (sym_sec->output_section == bfd_abs_section_ptr) >> + { >> + if (symval >= gp) >> + reserve_size += (symval - gp) / 2; >> + else >> + reserve_size += (gp - symval) / 2; > > We can have more than a factor of two from relaxation, for example from > alignment. If a hueristic is all we can do then I guess we'll have to > live with it, but I think I'd want to understand the movement > > An extreme case is that all the instruction can be relaxed without any huge alignments between text and data, so I guess that's probably why the half of distance comes from. If the default linker script may cause the problem, then we will need to know more details and try to resolve it. But if this only occurs without the default linker script, then I will suggest users disable that relax patterns by the .norelax directive, or related options of compiler/assembler/linker. Or maybe we can have an option for users to reject gp relax when meeting abs symbols, if without using default linker script. However, please give a reduced case by using default linker script that will be broken in detailed, thanks a lot. > > Nelson > >> + } >> } >> } >> >> diff --git a/ld/testsuite/ld-riscv-elf/gp-relax-abs-sym.s b/ld/testsuite/ld-riscv-elf/gp-relax-abs-sym.s >> new file mode 100644 >> index 00000000000..a018bb3a50a >> --- /dev/null >> +++ b/ld/testsuite/ld-riscv-elf/gp-relax-abs-sym.s >> @@ -0,0 +1,3 @@ >> +.section .data >> +.globl sym >> +.set sym,0x10804 >> diff --git a/ld/testsuite/ld-riscv-elf/gp-relax-abs.d b/ld/testsuite/ld-riscv-elf/gp-relax-abs.d >> new file mode 100644 >> index 00000000000..2c7ab3a2579 >> --- /dev/null >> +++ b/ld/testsuite/ld-riscv-elf/gp-relax-abs.d >> @@ -0,0 +1,14 @@ >> +#source: gp-relax-abs.s >> +#source: gp-relax-abs-sym.s >> +#as: -march=rv64ic -mabi=lp64 >> +#ld: -Tcode-model-01.ld -melf64lriscv >> +#objdump: -d >> + >> +.*:[ ]+file format .* >> + >> +Disassembly of section \.text: >> + >> +[0-9a-f]+ <_start>: >> +.*auipc.* >> +.*lw.*# [0-9a-f]* >> +#pass >> diff --git a/ld/testsuite/ld-riscv-elf/gp-relax-abs.s b/ld/testsuite/ld-riscv-elf/gp-relax-abs.s >> new file mode 100644 >> index 00000000000..db2103bafd1 >> --- /dev/null >> +++ b/ld/testsuite/ld-riscv-elf/gp-relax-abs.s >> @@ -0,0 +1,5 @@ >> +.text >> +.global _start >> +_start: >> + auipc t0, %pcrel_hi(sym) >> + lw t0, %pcrel_lo(_start)(t0) >> diff --git a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp >> index 947a266ba72..a53a2758991 100644 >> --- a/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp >> +++ b/ld/testsuite/ld-riscv-elf/ld-riscv-elf.exp >> @@ -172,6 +172,7 @@ if [istarget "riscv*-*-*"] { >> run_dump_test "attr-merge-priv-spec-failed-06" >> run_dump_test "attr-phdr" >> run_dump_test "relax-max-align-gp" >> + run_dump_test "gp-relax-abs" >> run_dump_test "uleb128" >> run_ld_link_tests [list \ >> [list "Weak reference 32" "-T weakref.ld -m[riscv_choose_ilp32_emul]" "" \