This is an archive of the discontinued LLVM Phabricator instance.

RISCV: permit unaligned nop-slide padding emission
ClosedPublic

Authored by compnerd on Aug 23 2022, 9:06 AM.

Details

Summary

We may be requested to emit an unaligned nop sequence (e.g. 7-bytes or
3-bytes). These should be 0-filled even though that is not a valid
instruction. This matches the behaviour on other architectures like
ARM, X86, and MIPS. When a custom section is emitted, it may be
classified as text even though it may be a data section or we may be
emitting data into a text segment (e.g. a literal pool). In such cases,
we should be resilient to the emission request.

This was originally identified by the Linux kernel build and reported on
D131270 by Nathan Chancellor.

Diff Detail

Event Timeline

compnerd created this revision.Aug 23 2022, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2022, 9:06 AM
compnerd requested review of this revision.Aug 23 2022, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2022, 9:06 AM

Should the zeroes not come first instead of last? Otherwise you’re all but guaranteed to have unaligned instructions. Not that you should execute them either way, but it may confuse disassemblers. What does binutils do?

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
366

IALIGN; C changes this from 4 to 2

jrtc27 added inline comments.Aug 23 2022, 9:14 AM
llvm/test/MC/RISCV/nop-slide.ll
1

Just riscv64, not Linux-specific

4

These should be redundant

7

How does this test it? There’s no code here to align?

compnerd marked 3 inline comments as done.Aug 23 2022, 10:08 AM

binutils's behaviour is the opposite of what we have:

  1. 1 byte nop to align to 2.
  2. At most 1 2-byte nop sequence.
  3. Pad via 4-byte nop sequence.

I don't have a strong opinion on this, this is going to be identical in practice due to the modular nature.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
366

Sorry, I don't understand what you mean by IALIGN. Yeah, I should fix that to say "4-byte (or 2-byte under C)".

llvm/test/MC/RISCV/nop-slide.ll
1

:shrug: sure.

4

Agreed. Removed.

7

Correct, it is purely data, but the section is getting classified as text (as is the behaviour with gas). The other architectures already properly handled this, but RISC-V did not. The result was that the alignment of c required that we emit 7-bytes of nops to align, and that would fail. Added a comment.

compnerd updated this revision to Diff 454882.Aug 23 2022, 10:08 AM
compnerd marked 3 inline comments as done.

Address feedback from @jrtc27

Maybe @asb or @luismarques know why this isn't reproducible from assembler sources?
https://reviews.llvm.org/D131270#3740339 test case in C.
Maybe some -mattr flag is necessary? Perhaps pertaining to RISCV::FeatureStdExtC?

echo '.balign 4; .byte 0; .balign 4; auipc a0, 0' | llvm-mc -triple=riscv64 -filetype=obj with or without +c works as a test case for me...

Okay, I don't know what I did wrong with the original attempt to use assembly, but @jrtc27's suggestion is good. I'm just going to replace the test with that.

compnerd updated this revision to Diff 454894.Aug 23 2022, 10:50 AM

@jrtc27 any other concerns or do you think that this is okay to commit?

binutils's behaviour is the opposite of what we have:

  1. 1 byte nop to align to 2.
  2. At most 1 2-byte nop sequence.
  3. Pad via 4-byte nop sequence.

I don't have a strong opinion on this, this is going to be identical in practice due to the modular nature.

Modular nature? I'd prefer to do what binutils does wrt where to put the 0 (I don't care at which end the c.nop goes, that really doesn't matter), it makes more sense and is less likely to confuse tools, even if in practice nothing should be executing any of these bytes.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
363

Making this a loop is a bit odd when we know it's always executing 0 or 1 times

llvm/test/MC/RISCV/nop-slide.s
1 ↗(On Diff #454894)

Should probably be testing both +relax and -relax?

3–7 ↗(On Diff #454894)

Why indent? Our tests don't do so unless there are labels involved as it just looks weird otherwise.

10–11 ↗(On Diff #454894)

This test is more interesting if you enable C and/or bump up the alignment as currently it's just padding with 3 zeroes. Maybe do both so you can clearly see the 0, c.nop and nop in the output?

jrtc27 added inline comments.Aug 23 2022, 10:59 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
366

From the RISC-V unprivileged spec:

We use the term IALIGN (measured in bits) to refer to the instruction-address alignment constraint the implementation enforces. IALIGN is 32 bits in the base ISA, but some ISA extensions, including the compressed ISA extension, relax IALIGN to 16 bits. IALIGN may not take on any value other than 16 or 32.

compnerd marked 4 inline comments as done.Aug 23 2022, 12:45 PM
compnerd added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
363

It seems nicer and feels more in line with the non-C version.

compnerd updated this revision to Diff 454931.Aug 23 2022, 12:46 PM
compnerd marked an inline comment as done.
jrtc27 added inline comments.Aug 23 2022, 1:00 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
365

I would suggest not reordering the nop and c.nop in this commit, it's not required. However, if you are going to do it, this is wrong, as if Count is a multiple of 4 this will emit a c.nop, decrease Count by 2 and then emit 1 fewer nop than required.

jrtc27 added inline comments.Aug 23 2022, 1:03 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
365

Also for non-RVC you either need to emit an (illegal) c.nop to pad to a multiple of 4 (which is what binutils does) or emit more zeroes, otherwise you can have exactly the same problem as this patch tries to fix but on non-RVC when Count % 4 == 3.

llvm/test/MC/RISCV/nop-slide.s
1 ↗(On Diff #454894)

And test +c and -c to ensure both work, that way you'd notice this bug...

compnerd updated this revision to Diff 454951.Aug 23 2022, 1:50 PM

Slightly different in that this actually 0-fills rather than uses the compressed nop even without the compressed instructions which binutils does.

Can you add a comment documenting the binutils behaviour (where the zeroes, c.nop, full nop go, etc.) and indicating that we implement the same?

Don't forget to include the full context in your patches. It seems like this is going in the right direction with the great feedback from @jrtc27.

The test case may seem to be under-reduced due to the use of LLVM IR
rather than assembly for the input, however, I was unable to reproduce
the failure when using the assembly input.

Do the other targets have any MC assembly test for this? I guess you can easily check that by modifying their hook and checking the list of breaking MC tests.

compnerd edited the summary of this revision. (Show Details)Aug 23 2022, 2:26 PM

Can you add a comment documenting the binutils behaviour (where the zeroes, c.nop, full nop go, etc.) and indicating that we implement the same?

Sure, I can do that.

Don't forget to include the full context in your patches. It seems like this is going in the right direction with the great feedback from @jrtc27.

Yeah, it's just annoying to do since I'm working on a remote machine.

The test case may seem to be under-reduced due to the use of LLVM IR
rather than assembly for the input, however, I was unable to reproduce
the failure when using the assembly input.

Do the other targets have any MC assembly test for this? I guess you can easily check that by modifying their hook and checking the list of breaking MC tests.

I'm not sure, but the test case was reduced to what it should be thanks to @jrtc27's help.

compnerd updated this revision to Diff 454965.Aug 23 2022, 2:36 PM

add context, address comments from @luismarques

The code changes seem fine now. Any additional thoughts about that, @jrtc27?

The llvm/test/MC/RISCV/align.s test update needs to be fixed, see my inline comment. After that I think it LGTM.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
369

Nit: given the earlier adjustment, isn't this the same as if (Count % 4)? Maybe your way is clearer anyway...

llvm/test/MC/RISCV/align.s
44 ↗(On Diff #454965)

This is the wrong test update. This was supposed to test the condition mentioned in the earlier comment about only having one c.nop. I think what you want to do is re-order lines 51 and 52.

compnerd marked 6 inline comments as done.Aug 24 2022, 8:34 AM
compnerd added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
369

Yes, given the previous % 2, it should only ever be 0, 2, which means that it is equivalent to Count % 4. The reason for the 2 here is to be explicit (because I am not always quick enough to immediately spot those constraints) that we are guaranteed that the RVC_NOP will fit. If people think that this is too verbose, I am fine with removing the `== 2.

compnerd updated this revision to Diff 455232.Aug 24 2022, 8:34 AM
compnerd marked an inline comment as done.
luismarques accepted this revision.Aug 24 2022, 8:39 AM

Looks correct to me now. Let's wait a little bit to see if there's more feedback from others.

llvm/test/MC/RISCV/nop-slide.s
1 ↗(On Diff #455232)

@jrtc27 @asb I remember it's been discussed in the past but I now forget the conclusion, have we moved away from line wrapping the RUN lines?

This revision is now accepted and ready to land.Aug 24 2022, 8:39 AM
compnerd updated this revision to Diff 455304.Aug 24 2022, 11:29 AM

Fix lld test

luismarques accepted this revision.Aug 24 2022, 11:30 AM

thanks for working on this!

I don't see any issues with my RISC-V Linux kernel builds with latest tip of tree plus this change. Thanks for the fix!

This revision was automatically updated to reflect the committed changes.