This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Fix crash for initial section alignment with .option norvc
ClosedPublic

Authored by luismarques on Mar 22 2022, 9:07 AM.

Details

Summary

The existing code wasn't getting the subtarget info from the fragment, so the current status of RVC would be ignored. This would cause a crash for the new test case when the target then reported it couldn't write the requested number of code alignment bytes.

Diff Detail

Event Timeline

luismarques created this revision.Mar 22 2022, 9:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2022, 9:07 AM
luismarques requested review of this revision.Mar 22 2022, 9:07 AM
luismarques added inline comments.Mar 22 2022, 9:11 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
610–611

This one wasn't needed to fix the crash but, presumably, this check should also be fixed?

This patch makes sense to me and I think it is a correct fix.
In LLVM MC, the STI's feature bits is not mutable, it was determined by the command line before parsing the file. And to deal with directives like .option rvc, the Parser maintains an STI and replaces it with a new STI when the subtarget extensions were enabled or disabled. So we need to use MCAlignFragment's STI which is generated using the replaced STI by the parser. Is my understanding correct?

StephenFan added inline comments.Mar 31 2022, 8:33 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
610–611

I agree this check should also be fixed. And if it would be better to have a test that can test this fix?

Add test for shouldInsertFixupForCodeAlign fix.

This patch makes sense to me and I think it is a correct fix.
In LLVM MC, the STI's feature bits is not mutable, it was determined by the command line before parsing the file. And to deal with directives like .option rvc, the Parser maintains an STI and replaces it with a new STI when the subtarget extensions were enabled or disabled. So we need to use MCAlignFragment's STI which is generated using the replaced STI by the parser. Is my understanding correct?

I wouldn't focus too much on it being mutable or not. I think the important point is that because we aren't doing everything in a single pass we need to track the state of the flags at various points in the assembly file. Which we do, but apparently we weren't then using the saved state for those locations, just the global state. Being mutable (and not copying it) would only help if we were doing everything in a single pass. At least that was my interpretation of the issue. Sorry to nitpick your explanation, I just wanted to clarify that point :) Broadly speaking I would say your explanation is correct.

luismarques marked an inline comment as done.Apr 6 2022, 4:11 AM

Can we get this landed on time for backporting into 14.0.1? (it took me some time to address the review feedback because I was sick).

asb accepted this revision.Apr 7 2022, 2:49 AM

This LGTM - thanks!

This revision is now accepted and ready to land.Apr 7 2022, 2:49 AM
This revision was landed with ongoing or failed builds.Apr 7 2022, 4:02 AM
This revision was automatically updated to reflect the committed changes.