From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22a.google.com (mail-oi1-x22a.google.com [IPv6:2607:f8b0:4864:20::22a]) by sourceware.org (Postfix) with ESMTPS id 090783959E5D for ; Wed, 16 Nov 2022 08:58:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 090783959E5D 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-oi1-x22a.google.com with SMTP id q83so17744929oib.10 for ; Wed, 16 Nov 2022 00:58:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=WcjGYK7ZpKV1XzAI7fPFgMdiKZCDppy2HQ7iUrKys60=; b=JxxulprQjXJlGBCDZU2r4E5ZLAu8ctjn8gylXJ0j/Ih5vGf4u+TL/fpnDEUnoA/hl1 5rMT/dgxCwSutIkacdxpC2ExlKrU+lpYmtdDfegYs/Jc85zid01r29XLPOYzean7Ru+3 cTcFs7b1VIcfKuhjF//vTxLxzzWU1qAUdLGNdCn6tkQbnySCOwi0FTQ5tFtrHibPBw3F y9X63DyOxkE+cyAqiz2uS1KlPvdGB788YuxYOCmcGY36KkJPXcGd19iMKFB00I24X3oS nT2Ser9GlGmQXjnpSGMOdAvJpvrUlLwFtrtNLiACr24x5vfpLES8xVQuil4QxmRVP4+A z1Og== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=WcjGYK7ZpKV1XzAI7fPFgMdiKZCDppy2HQ7iUrKys60=; b=BWDaaxMmzEeMeJAmVW/xuXYmmMiHY6ny/X13Wf4IvkYNSIw/9WJEDYKGa5OwQ3yJRQ luw+BfCliy0BXQcKoEYACppl5UAmCv1wifEUDSj3gVOIGQw/IAVVZWdcoF8LJExd1w1y JevteA9d8v1CvdpmSWYmaAVafs1NOL9PlNm0fA9OunYGPHHVRtfoUVY/yGKSLSUEqH7K EsD901zR0kFQ9NrVGXyeh6LOXbRlaDXsx1CekIQ4GPoldut/l/JBmB40U6Y+wld3oL/N Lk6FpyW1rnTzkr0NkJxFms1STsdEOIoSWtFDkqMVkkGGFnOy/gR+TmFLXwR+4Iph6hai fm1A== X-Gm-Message-State: ANoB5pkMhBBKNab8Pd9A4KOMFcV5L3+Dnen2PBZr9tM5xbFOqjkuW0pu 3s4wT1m/ae0A2CeE9fwWLMYTKlhdhkK5xWksIQ24xA== X-Google-Smtp-Source: AA0mqf48YJu/ZRFZ7drf1sZeo9vppX/LQcR8B8/mQXipLYI2p38gbMpXm8tPXoJ9xWXqK6bIL6FM7Kl7QjKzV2vph88= X-Received: by 2002:a05:6808:1986:b0:35a:83c1:ce7 with SMTP id bj6-20020a056808198600b0035a83c10ce7mr1069251oib.107.1668589111296; Wed, 16 Nov 2022 00:58:31 -0800 (PST) MIME-Version: 1.0 References: <20221115081455.2354987-1-zengxiao@eswincomputing.com> <20221115081455.2354987-2-zengxiao@eswincomputing.com> In-Reply-To: <20221115081455.2354987-2-zengxiao@eswincomputing.com> From: Nelson Chu Date: Wed, 16 Nov 2022 16:58:20 +0800 Message-ID: Subject: Re: [PATCH v1 1/1] RISC-V: Make R_RISCV_SUB6 conforms to riscv abi standard To: zengxiao@eswincomputing.com Cc: binutils@sourceware.org, kito.cheng@gmail.com, andrew@sifive.com, shihua@iscas.ac.cn, gaofei@eswincomputing.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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 Tue, Nov 15, 2022 at 4:15 PM wrote: > > From: zengxiao > > This patch makes R_RISCV_SUB6 conforms to riscv abi standard. > R_RISCV_SUB6 only the lower 6 bits of the code are valid. > The proposed specification which can be found in 8.5. Relocations of, > https://github.com/riscv-non-isa/riscv-elf-psabi-doc/releases/download/v1.0-rc4/riscv-abi.pdf > > bfd/ChangeLog: > > * elfxx-riscv.c (riscv_elf_add_sub_reloc): > > binutils/ChangeLog: > > * testsuite/binutils-all/riscv/dwarf-SUB6.d: New test. > * testsuite/binutils-all/riscv/dwarf-SUB6.s: New test. I tried but it seems like the testcases won't generate any R_RISCV_SET6/SUB6, so it is useless in fact. Maybe reducing your test case here is the right thing to do, https://github.com/zeng-xiao/gnu-bug-fix/tree/main/EG-769, I can reproduce the problem when testing the link. > reviewed-by: gaofei@eswincomputing.com > jinyanjiang@eswincomputing.com > > Signed-off-by: zengxiao > --- > bfd/elfxx-riscv.c | 7 +++++ > .../testsuite/binutils-all/riscv/dwarf-SUB6.d | 31 +++++++++++++++++++ > .../testsuite/binutils-all/riscv/dwarf-SUB6.s | 12 +++++++ > 3 files changed, 50 insertions(+) > create mode 100644 binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d > create mode 100644 binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s > > diff --git a/bfd/elfxx-riscv.c b/bfd/elfxx-riscv.c > index 300ccf49534..e71d4a456f2 100644 > --- a/bfd/elfxx-riscv.c > +++ b/bfd/elfxx-riscv.c > @@ -994,6 +994,13 @@ riscv_elf_add_sub_reloc (bfd *abfd, > relocation = old_value + relocation; > break; > case R_RISCV_SUB6: > + { > + bfd_vma six_bit_valid_value = old_value & howto->dst_mask; > + six_bit_valid_value -= relocation; > + relocation = (six_bit_valid_value & howto->dst_mask) | > + (old_value & ~howto->dst_mask); > + } > + break; Seems like it will cause problems if we are dumping the debug information of the object by the objdump, before the final link. There are two issues here, one is the overflow checking, and another is to have the same calculations for both riscv_elf_add_sub_reloc and riscv_elf_relocate_section. For the former, I just talked to Palmer today, and talked to Kito a few months ago, we all agree that we should have overflow checks for these kinds of relocations, including ADD/SUB/SET, even if we don't have any of them for now. I don't remember the details when I tried to add the overflow checks before, but that caused the debug information broken for the rv64 toolchains when running the gcc regressions. Since sometimes we still generate the ADD32/SUB32 relocations in rv64, but that seems dangerous though. Anyway, I'm not the expert for that, so I don't have any useful thoughts for now. For the latter, I think llvm did the things right, https://llvm.org/doxygen/RelocationResolver_8cpp_source.html#l00469. So, 1. relocation = (old_value & ~howto->dst_mask) | (((old_value & howto->dst_mask) - relocation) & howto->dst_mask); 2. We should have the same behavior in riscv_elf_relocate_section - filter to get the valid least 6-bit address for the old value, https://github.com/bminor/binutils-gdb/blob/master/bfd/elfnn-riscv.c#L2429. Btw, we don't need to do anything else when encoding, since perform_relocation already did the right thing. It would be great if we have the testcase, but the testcase in this patch doesn't seems useful, so we need a reduced case from here, https://github.com/zeng-xiao/gnu-bug-fix/tree/main/EG-769, to show something like "DW_ CFA_??? (User defined call frame op: 0x3c)" before the fix. Or if it is too difficult to reduce, then it's okay for me to ignore the testcase. Just remember to pass the riscv-gnu-toolchain regressions, that should be enough to prove we won't break something basically. Please see the regressions here, https://github.com/riscv-collab/riscv-gnu-toolchain/tree/master/regression Thanks Nelson > case R_RISCV_SUB8: > case R_RISCV_SUB16: > case R_RISCV_SUB32: > diff --git a/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d > new file mode 100644 > index 00000000000..47d5ae570d7 > --- /dev/null > +++ b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.d > @@ -0,0 +1,31 @@ > +#PROG: objcopy > +#objdump: --dwarf=frames > + > +tmpdir/riscvcopy.o: file format elf32-littleriscv > + > +Contents of the .eh_frame section: > + > + > +00000000 00000020 00000000 CIE > + Version: 3 > + Augmentation: "zR" > + Code alignment factor: 1 > + Data alignment factor: -4 > + Return address column: 1 > + Augmentation data: 1b > + DW_CFA_def_cfa_register: r2 \(sp\) > + DW_CFA_def_cfa_offset: 48 > + DW_CFA_offset: r1 \(ra\) at cfa-4 > + DW_CFA_offset: r8 \(s0\) at cfa-8 > + DW_CFA_def_cfa: r8 \(s0\) ofs 0 > + DW_CFA_restore: r1 \(ra\) > + DW_CFA_restore: r8 \(s0\) > + DW_CFA_def_cfa: r2 \(sp\) ofs 48 > + DW_CFA_def_cfa_offset: 0 > + DW_CFA_nop > + > +00000024 00000010 00000028 FDE cie=00000000 pc=0000002c..0000002c > + DW_CFA_nop > + DW_CFA_nop > + DW_CFA_nop > + > diff --git a/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s > new file mode 100644 > index 00000000000..fe959f59d9b > --- /dev/null > +++ b/binutils/testsuite/binutils-all/riscv/dwarf-SUB6.s > @@ -0,0 +1,12 @@ > + .attribute arch, "rv32i2p0_m2p0_a2p0_f2p0_c2p0" > + .cfi_startproc > + .cfi_def_cfa_offset 48 > + .cfi_offset 1, -4 > + .cfi_offset 8, -8 > + .cfi_def_cfa 8, 0 > + .cfi_restore 1 > + .cfi_restore 8 > + .cfi_def_cfa 2, 48 > + .cfi_def_cfa_offset 0 > + .cfi_endproc > + > \ No newline at end of file > -- > 2.34.1 >