RISCV: permit unaligned nop-slide padding emission

[compnerd]

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.

[jrtc27]

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]

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

1 ↗

(On Diff #454867)

Just riscv64, not Linux-specific

4 ↗

(On Diff #454867)

These should be redundant

7 ↗

(On Diff #454867)

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

[compnerd]

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 ↗

(On Diff #454867)

:shrug: sure.

4 ↗

(On Diff #454867)

Agreed. Removed.

7 ↗

(On Diff #454867)

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]

Address feedback from @jrtc27

[nickdesaulniers]

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?

[jrtc27]

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...

[compnerd]

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]

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

[jrtc27]

In D132482#3743100, @compnerd wrote:

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

2

Should probably be testing both +relax and -relax?

4–8

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

11–12

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]

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]

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

363

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

[jrtc27]

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]

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

2

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

[compnerd]

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

[luismarques]

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]

In D132482#3743723, @luismarques wrote:

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]

add context, address comments from @luismarques

[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

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]

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.

[luismarques]

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

2

@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?

[compnerd]

Fix lld test

[nickdesaulniers]

thanks for working on this!

[nathanchance]

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!