This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Insert R_RISCV_ALIGN relocation type and Nops for code alignment when linker relaxation enabled
ClosedPublic

Authored by shiva0217 on Jun 4 2018, 7:15 PM.

Details

Summary

Linker relaxation may change code size. We need to fix up the alignment of alignment directive in text section by inserting Nops and R_RISCV_ALIGN relocation type. So then linker could satisfy the alignment by removing Nops.

To do this:

  1. Add shouldInsertExtraNopBytesForCodeAlign target hook to calculate the Nops we need to insert.
  2. Add shouldInsertFixupForCodeAlign target hook to insert R_RISCV_ALIGN fixup type.

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Jun 4 2018, 7:15 PM
asb added a comment.Jun 14 2018, 7:05 AM

Hi Shiva. I think your tests need to demonstrate the behaviour when there is no C extension support as well.

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
25–30

Given that the implementations of these always cast to MCAlignFragment, why not take an MCAlignFragment instead of MCFragment?

shiva0217 updated this revision to Diff 151452.Jun 14 2018, 7:17 PM

Update patch to address Alex's comments.

asb added a comment.Aug 16 2018, 6:40 AM

My main concern with this patch currently is the ability to handle the relax option changing due to .option relax/norelax. So it's probably blocked on D46423.

Note that D45961 which adds MCSubtargetInfo to MCAlignFragment may also be relevant.

asb added a comment.Aug 17 2018, 5:28 AM

It would also be worth testing the treatment of alignment directives emitted when generating a constant pool. Currently ConstantPool::emitEntries will use EmitCodeAlignment, but D45961 suggests changing it to EmitCodeAlignment. This affects whether EmitNops is true for the MCAlignFragment. However this patch doesn't query that property anyway. It might be worth documenting that limitation and perhaps double-checking GCC behaviour. I assume that if you use .align with a value to fill with, GCC ignores that value if linker relaxation is enabled?

Rebase and add constant pool test case.

Hi Alex, Thanks for the review. Could the new constant pool test case cover the case you concern with?

Jim added a subscriber: Jim.Nov 27 2018, 7:14 PM
shiva0217 edited the summary of this revision. (Show Details)

Rebase and rename insertNopBytesForAlignDirectiveInTextSection to NopBytesToInsertForAlignDirectiveInTextSection

Hi Alex, I have added a constant pool test case at the end of align.s to verify the padding behavior for constant pool. Once D45961 landing and changing the behavior, we should be able to aware. It seems that GCC will insert Nops for padding constant pool. Could you illustrate more about "use .align with a value to fill with"? I could test on GCC.

jrtc27 added inline comments.Jan 7 2019, 3:56 AM
lib/MC/MCAssembler.cpp
840

This should be moved into the if (and the dyn_cast should never have existed on its own in the first place; if the result is discarded it should be an isa).

844

Hard-coding the section name seems wrong. What if somebody uses a section attribute? GNU as will do the right thing for any executable section regardless of its name, and we should too; from an initial glance it seems that MCSection::UseCodeAlign() will tell you exactly the right thing, and as a bonus is no longer ELF-specific.

shiva0217 marked 2 inline comments as done.Jan 7 2019, 11:03 PM
shiva0217 added inline comments.
lib/MC/MCAssembler.cpp
840

Hi James,
Thanks for the correction.

844

It seems to be a much graceful and universal approach, thanks a lot.

shiva0217 updated this revision to Diff 180618.Jan 7 2019, 11:06 PM
shiva0217 retitled this revision from [RISCV] Insert R_RISCV_ALIGN relocation type and Nops for alignment when linker relaxation enabled to [RISCV] Insert R_RISCV_ALIGN relocation type and Nops for code alignment when linker relaxation enabled.

Update patch to reflect James comments.

asb added a comment.Jan 10 2019, 8:48 AM

Hi Alex, I have added a constant pool test case at the end of align.s to verify the padding behavior for constant pool. Once D45961 landing and changing the behavior, we should be able to aware. It seems that GCC will insert Nops for padding constant pool. Could you illustrate more about "use .align with a value to fill with"? I could test on GCC.

I meant that .align/.p2align in GNU as accept a padding value https://sourceware.org/binutils/docs/as/Align.html#Align. It seems that for RISC-V, it accepts and uses this padding value even when relaxation is enabled, but won't emit the R_RISCV_RELAX in that case (which I suppose makes sense, as the linker wouldn't respect the padding value). I'm not saying we should definitely replicate GNU as here - obviously performing alignment without generating R_RISCV_RELAX is quite liable to break things unexpectedly. But it might not be a bad starting point if it's easy to support. Either way, we should have tests that demonsrate how we handle those cases.

shiva0217 updated this revision to Diff 181416.Jan 11 2019, 7:03 PM
shiva0217 edited the summary of this revision. (Show Details)
  1. Add test case for alignment directive with a specific padding value
  2. Rename the new target hooks to NopBytesToInsertForAlignDirectiveInCodeSection and insertFixupForAlignDirectiveInCodeSection
In D47755#1352910, @asb wrote:

Hi Alex, I have added a constant pool test case at the end of align.s to verify the padding behavior for constant pool. Once D45961 landing and changing the behavior, we should be able to aware. It seems that GCC will insert Nops for padding constant pool. Could you illustrate more about "use .align with a value to fill with"? I could test on GCC.

I meant that .align/.p2align in GNU as accept a padding value https://sourceware.org/binutils/docs/as/Align.html#Align. It seems that for RISC-V, it accepts and uses this padding value even when relaxation is enabled, but won't emit the R_RISCV_RELAX in that case (which I suppose makes sense, as the linker wouldn't respect the padding value). I'm not saying we should definitely replicate GNU as here - obviously performing alignment without generating R_RISCV_RELAX is quite liable to break things unexpectedly. But it might not be a bad starting point if it's easy to support. Either way, we should have tests that demonsrate how we handle those cases.

Hi Alex,
Thanks for the explanation. I tested on GNU with .p2align 4, 1, as you said, GNU won't emit relocation types, so the alignment requirement may break when relaxation is enabled. For the same case with this patch, LLVM will emit relocation types, so the alignment requirement will be satisfied. I have added the test case. Thanks for catching this.

asb added a comment.Jan 17 2019, 7:03 AM

@shiva0217 the alignment required will be satisfied, but the linker may overwrite some of the bytes with nop/c.nop which seems undesirable.

I added a few comments. Have you looked much at what gas does for .align in other sections. Maybe it's error on my part but I'm not sure that it's doing the right thing...

include/llvm/MC/MCAsmBackend.h
91

How about: "Hook to check if extra nop bytes must be inserted for alignment directive. For some targets this may be necessary in order to support linker relaxation. The number of bytes to insert are returned in Size."

93

Should start with a lower-case letter.

Might be better named "shouldInsterExtraNopBytesForCodeAlign"? The use of CodeAlign aligns with the way the term is used in MCSection.h for UseCodeAlign.

98

How about "Hook which indicates if the target requires a fixup to be generated when handling an align directive in an executable section (as determined by MCSection::UseCodeAlign())."

99

Maybe "shouldInsertFixupForCodeAlign"?

shiva0217 edited the summary of this revision. (Show Details)

Hi @asb, I got your point, we could guard the target hook with AF.hasEmitNops(), so the behavior of .align 4, 1 will be the same as gnu assembler. What do you think?

I've tested .align in data section when relaxation enabled for gnu toolchain, gnu toolchain will not insert R_RISCV_ALIGN, and the alignment position will be correct. It seems that we only need to insert R_RISCV_ALIGN for the relaxable section. As far as I know, it would be code section, but I may miss something.

The target hook's names and comments have been updated, thanks for the revision.

asb added a comment.Jan 24 2019, 7:13 AM

Hi @asb, I got your point, we could guard the target hook with AF.hasEmitNops(), so the behavior of .align 4, 1 will be the same as gnu assembler. What do you think?

That sounds good

I've tested .align in data section when relaxation enabled for gnu toolchain, gnu toolchain will not insert R_RISCV_ALIGN, and the alignment position will be correct. It seems that we only need to insert R_RISCV_ALIGN for the relaxable section. As far as I know, it would be code section, but I may miss something.

gas seems to have a bug here, I've filed https://sourceware.org/bugzilla/show_bug.cgi?id=24129

asb added a comment.Jan 24 2019, 7:48 AM
In D47755#1369341, @asb wrote:

gas seems to have a bug here, I've filed https://sourceware.org/bugzilla/show_bug.cgi?id=24129

gas issue was my mistake in analysing the output (should have disassembled without aliases). So gas does do the sensible thing.

asb accepted this revision.Jan 30 2019, 1:42 AM

Apologies Shiva, I'd missed that you'd included the AF.hasEmitNops change in the last patch update. This looks good to me - thanks!

This revision is now accepted and ready to land.Jan 30 2019, 1:42 AM
This revision was automatically updated to reflect the committed changes.