From: Fangrui Song <i@maskray.me>
To: Nelson Chu <nelson@rivosinc.com>
Cc: Jan Beulich <jbeulich@suse.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Jim Wilson <jim.wilson.gcc@gmail.com>,
binutils@sourceware.org, Andrew Waterman <andrew@sifive.com>,
Kito Cheng <kito.cheng@sifive.com>
Subject: Re: [PATCH 2/3] RISC-V: correct alignment directive handling for text sections
Date: Wed, 25 Sep 2024 23:44:26 -0700 [thread overview]
Message-ID: <SN4PR01MB7440092DC814B0AF354C71DACB6A2@SN4PR01MB7440.prod.exchangelabs.com> (raw)
In-Reply-To: <CAPpQWtDvagUrK7E64dxb6QjWdXt-3h6RnJe=gMJkBmFNi242nw@mail.gmail.com>
On Wed, Sep 25, 2024 at 10:44 PM Nelson Chu <nelson@rivosinc.com> wrote:
>
> Hi Jan,
>
> I think this should be the correct way to do it for mixed data/text cases, although the current compiler, both gcc and llvm, won't generate this kind of code, it should be worth trying and do something in advance. Since there still isn't any spec that gives a rule for mixed data/text cases (https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/210#issuecomment-929769772), I think it should be fine to go ahead and try :-)
>
> I am not sure who should need to know the changes, so I at least cc Kito and Maskray. Please feel free to cc or reference this information to anyone who will need to know.
>
> Thanks
> Nelson
>
> On Tue, Sep 24, 2024 at 4:31 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 04.09.2024 01:02, Palmer Dabbelt wrote:
>> > On Mon, 12 Aug 2024 08:08:04 PDT (-0700), jbeulich@suse.com wrote:
>> >> .insn or data emitted inside text sections can lead to positions not
>> >> being at insn granularity. In such situations using alignment
>> >> directives should reliably enforce the requested alignment.
>> >> Specifically requests to align back to insn granularity may not be
>> >> ignored (where, as a subcase thereof, the ordering of ".option norvc"
>> >> and e.g. ".p2align 2" should not matter; so far the alignment directive
>> >> needs to come first to have any effect). Similarly ahead of emitting
>> >> NOPs alignment first needs to be forced back to insn granularity.
>> >>
>> >> The new testcases actually point out a corner case issue in the
>> >> disassembler as well, which is being corrected at the same time: We
>> >> don't want to print "0x" without any subsequent digits.
>> >
>> > Sorry for being slow here. Nelson and I talked about this a few times.
>> > IIRC a bunch of the complexity in the code was related to trying to
>> > avoid mixing RVC instructions into non-RVC regions in order to re-gain
>> > alignment. I remember there being some reason that we didn't want to
>> > emit instructions when changing rvc->norvc, but I can't remember what
>> > that reason was. So maybe I'm just crazy here...
>> >
>> > Jim might remember? IIRC he was at least sitting there talking about
>> > this stuff as it was getting fixed, I forget if he wrote the code.
>> >
>> > That said, I think you just found another bug: certainly ignoring the
>> > alignment directive is going to break things, so that's bad. I don't
>> > see anything wrong with the actual code here, though...
>> >
>> > So Nelson is going to run some regressions and see what's up. If they
>> > all pass and nobody can remember a reason this wouldn't work, then I
>> > think we just go with it.
>>
>> Mind me asking if there has been any progress here?
>>
>> Jan
For the new test relax-align.s, gas without this patch seems to
produce `ret` at a wrong location.
In contrast, `llvm-mc -filetype=obj -triple=riscv64 -mattr=+relax`
produces the correct `ret`.
I haven't checked the functionality, but the new test looks correct.
The behavior difference from the current behavior (wrong; not used by
GCC codegen) should not hold off landing this patch.
(Dropping maskray@google.com as I am leaving Google and the email
address will bounce soon :)
next prev parent reply other threads:[~2024-09-26 6:51 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-08-12 15:06 [PATCH 0/3] RISC-V: alignment in " Jan Beulich
2024-08-12 15:07 ` [PATCH 1/3] RISC-V: process rs_align_code also when relaxing Jan Beulich
2024-08-20 2:44 ` Nelson Chu
2024-08-20 7:34 ` Jan Beulich
2024-08-12 15:08 ` [PATCH 2/3] RISC-V: correct alignment directive handling for text sections Jan Beulich
2024-09-03 23:02 ` Palmer Dabbelt
2024-09-03 23:25 ` Nelson Chu
2024-09-24 8:31 ` Jan Beulich
2024-09-26 5:44 ` Nelson Chu
2024-09-26 6:44 ` Fangrui Song [this message]
2024-08-12 15:08 ` [PATCH 3/3] RISC-V: odd data padding vs mapping symbols Jan Beulich
2024-08-20 5:19 ` Nelson Chu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SN4PR01MB7440092DC814B0AF354C71DACB6A2@SN4PR01MB7440.prod.exchangelabs.com \
--to=i@maskray.me \
--cc=andrew@sifive.com \
--cc=binutils@sourceware.org \
--cc=jbeulich@suse.com \
--cc=jim.wilson.gcc@gmail.com \
--cc=kito.cheng@sifive.com \
--cc=nelson@rivosinc.com \
--cc=palmer@dabbelt.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).