Page MenuHomePhabricator

[X86][MC] Make -x86-pad-max-prefix-size compatible with --mc-relax-all
ClosedPublic

Authored by skan on Apr 9 2020, 7:45 PM.

Details

Summary

We allow non-relaxable instructions emitted into relaxable Fragment when we prefix padding branch. So we need to check if the instruction need relaxation before relaxing it. Without this patch, it currently triggers a report_fatal_error in llvm::MCAsmBackend::relaxInstruction when we prefix padding branch along with --mc-relax-all.

Diff Detail

Event Timeline

skan created this revision.Apr 9 2020, 7:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2020, 7:45 PM
skan edited the summary of this revision. (Show Details)Apr 9 2020, 7:49 PM
skan added a comment.Apr 9 2020, 8:20 PM

The failed tests are not related. I ran check-all locally and passed.

skan added a reviewer: reames.Apr 9 2020, 11:20 PM

LGTM. Better to wait for Philip accept.

MaskRay accepted this revision.EditedApr 10 2020, 12:23 AM
MaskRay retitled this revision from [X86][MC] Make -x86-pad-max-prefix-size can co-work with --mc-relax-all to [X86][MC] Make -x86-pad-max-prefix-size compatible with --mc-relax-all.
MaskRay added a subscriber: MaskRay.

Best to state that this currently triggers a report_fatal_error in llvm::MCAsmBackend::relaxInstruction.

llvm/test/MC/X86/align-branch-64-relax-all.s
2

You can actually strip -pc-linux-gnu. ELF is the default.

This revision is now accepted and ready to land.Apr 10 2020, 12:23 AM
skan edited the summary of this revision. (Show Details)Apr 10 2020, 12:39 AM
skan marked an inline comment as done.Apr 10 2020, 12:53 AM
skan added inline comments.
llvm/test/MC/X86/align-branch-64-relax-all.s
2

Thanks for the suggestion. I plan to strip all the -pc-linux-gnu and the blank before # RUN in the test files align-branch-* after landing this patch.

skan updated this revision to Diff 256541.Apr 10 2020, 3:38 AM

Nothing changed, just to trigger a new remote build

skan edited the summary of this revision. (Show Details)Apr 10 2020, 8:29 PM
This revision was automatically updated to reflect the committed changes.
niosHD added a subscriber: niosHD.Apr 16 2020, 5:21 AM

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated. A similar problem probably happens for the Hexagon and for the AMDGPU backend.

/cc @asb also FYI that this breaks the RISCV backend.

skan added a comment.Apr 16 2020, 7:03 PM

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated. A similar problem probably happens for the Hexagon and for the AMDGPU backend.

/cc @asb also FYI that this breaks the RISCV backend.

Could you provide a new fail case that can be reproduced?

skan added a comment.Apr 16 2020, 8:31 PM

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated. A similar problem probably happens for the Hexagon and for the AMDGPU backend.

/cc @asb also FYI that this breaks the RISCV backend.

Could you provide a new fail case that can be reproduced?

yeah, I got your point. We may need to add a lit test to gurantee this assumption will not be broken.

skan added a comment.Apr 16 2020, 9:48 PM

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated.

We can fix this in two ways

  1. Change the control flow in MCObjectStreamer.cpp
  2. or create a fresh uninitialized MCInst at the beginning of RISCVAsmBackend::relaxInstruction, and after relaxation, assign its value to Relaxed at the end of RISCVAsmBackend::relaxInstruction.

Which way do you think is better? Personally, I prefer the second one, since getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed) is called in a loop, the assumption that third argument of relaxInstruction is a fresh uninitialized MCInst is very strange.

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated.

We can fix this in two ways

  1. Change the control flow in MCObjectStreamer.cpp
  2. or create a fresh uninitialized MCInst at the beginning of RISCVAsmBackend::relaxInstruction, and after relaxation, assign its value to Relaxed at the end of RISCVAsmBackend::relaxInstruction.

Which way do you think is better? Personally, I prefer the second one, since getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed) is called in a loop, the assumption that third argument of relaxInstruction is a fresh uninitialized MCInst is very strange.

Special casing RISC-V looks good to me.

Which way do you think is better? Personally, I prefer the second one, since getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed) is called in a loop, the assumption that third argument of relaxInstruction is a fresh uninitialized MCInst is very strange.

Special casing RISC-V looks good to me.

A special case is probably also needed for the Hexagon and AMDGPU backend, which does similar stuff in their relaxInstruction implementation than the RISC-V backend.

Is their any reason why relaxInstruction takes three argurments? Since relaxed is given in two arguments which are aliased to the same variable, I would drop the third argument and let relaxInstruction to either modify the given instruction or assign it to a fresh one (RISC-V, Hexagon, AMDGPU case).

skan added a comment.Apr 17 2020, 2:39 AM

Which way do you think is better? Personally, I prefer the second one, since getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed) is called in a loop, the assumption that third argument of relaxInstruction is a fresh uninitialized MCInst is very strange.

Special casing RISC-V looks good to me.

A special case is probably also needed for the Hexagon and AMDGPU backend, which does similar stuff in their relaxInstruction implementation than the RISC-V backend.

Is their any reason why relaxInstruction takes three argurments? Since relaxed is given in two arguments which are aliased to the same variable, I would drop the third argument and let relaxInstruction to either modify the given instruction or assign it to a fresh one (RISC-V, Hexagon, AMDGPU case).

Droppng the third argument looks good to me.

skan added a comment.Apr 17 2020, 5:57 AM

Which way do you think is better? Personally, I prefer the second one, since getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed) is called in a loop, the assumption that third argument of relaxInstruction is a fresh uninitialized MCInst is very strange.

Special casing RISC-V looks good to me.

A special case is probably also needed for the Hexagon and AMDGPU backend, which does similar stuff in their relaxInstruction implementation than the RISC-V backend.

Is their any reason why relaxInstruction takes three argurments? Since relaxed is given in two arguments which are aliased to the same variable, I would drop the third argument and let relaxInstruction to either modify the given instruction or assign it to a fresh one (RISC-V, Hexagon, AMDGPU case).

I am working on this fix.

Could you provide a new fail case that can be reproduced?

https://bugs.llvm.org/show_bug.cgi?id=45580

this broke our Linux kernel builds of RISCV, too.

Has this been reverted? I see discussion of a fix being worked on, but has the change which triggered problems been reverted? I can't tell easily from the review history.

LuoYuanke added inline comments.Apr 17 2020, 6:08 PM
llvm/lib/MC/MCObjectStreamer.cpp
404

If target expected "Relaxed" be a fresh MCInst, then this line also has problem. I think for RISC-V, this line isn't been executed, so the problem is not exposed before. I prefer to fix the bug in MCObjectStreamer.cpp, because some other target may also append new operand to Relaxed MCInst. And the relaxInstruction() API seems imply the Relaxed MCInst is fresh.

while (getAssembler().getBackend().mayNeedRelaxation(Inst, STI)) {
  MCInst Relaxed;
  getAssembler().getBackend().relaxInstruction(Inst, STI, Relaxed);
  Inst = Relaxed;
}
skan added a comment.Apr 17 2020, 11:28 PM

Has this been reverted? I see discussion of a fix being worked on, but has the change which triggered problems been reverted? I can't tell easily from the review history.

No, we plan to fix based on this.

skan marked an inline comment as done.Apr 17 2020, 11:30 PM
skan added inline comments.
llvm/lib/MC/MCObjectStreamer.cpp
404

The code you paste doesn't make sense, Inst is a const MCInst &.