This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add bounds check before use on returned iterator during frame lowering.
AbandonedPublic

Authored by anmolparalkar-nxp on Aug 20 2023, 7:14 PM.

Details

Reviewers
asb
wangpc
Summary

Check iterator validity before use; fixes a crash seen in RISCVFrameLowering
when executing the Prologue/Epilogue Insertion & Frame Finalization pass.
Note: -mattr=+zcmp is essential to the reproducibility of the issue.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2023, 7:14 PM
anmolparalkar-nxp requested review of this revision.Aug 20 2023, 7:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2023, 7:14 PM

Is this the same as D158256?

Oh, Vow, thank you! Yes, the change to llvm/lib/Target/RISCV/RISCVFrameLowering.cpp is identical indeed.
The test case, is derived using bugpoint, on an internal benchmark.

wangpc requested changes to this revision.Aug 21 2023, 9:02 PM

Is this the same as D158256?

Oh, Vow, thank you! Yes, the change to llvm/lib/Target/RISCV/RISCVFrameLowering.cpp is identical indeed.
The test case, is derived using bugpoint, on an internal benchmark.

Can we merge these two patches into one? You can add tests in this patch to D158256 and close this one.

This revision now requires changes to proceed.Aug 21 2023, 9:02 PM

Merged in test from D158256

PS: @wangpc Phabricator was not allowing me to update D158256 so I updated the patch here;
@garvitgupta08 please do upload this re-spin patch into D158256 so that the frame-lowering-crash.ll test gets merged in.

I am closing D158379.

anmolparalkar-nxp abandoned this revision.Aug 22 2023, 4:55 PM

@garvitgupta08 kindly upload current diff to D158256, thank you.

anmolparalkar-nxp updated this revision to Diff 553339.EditedAug 24 2023, 8:30 PM

@garvitgupta08

Addressed review comments to llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll from D158256:

craig.topper:

Do we need the hidden and local_unnamed_addr attributes?
DONE

icmp slt i32 poison, poison doesn't make sense. The compiler would be allowed to delete it. So a future optimization could break this test.
DONE

store i32 0, ptr null, align 4 is a store to null which is undefined behavior. So the compiler is allowed to delete it. Again a future optimization could delete it and break this test.
DONE

jrtc27:

Use update_llc_test_checks.py
DONE

Though why is there an IR test and a MIR test? The MIR test seems preferable, surely?
Please feel free to drop: llvm/test/CodeGen/RISCV/zcmp-prolog-epilog-crash.ll

Though it looks like the same code is generated for both so can share one prefix?
Actually, there is a difference ...

nounwind
DONE

Space between declaration and definition
DONE

Clean up the IR of these comments?
DONE

Dangling attribute reference
DONE

This seems overly-reduced
Asked for clarification ...

Fix // No newline at end of file
DONE?

anmolparalkar-nxp abandoned this revision.Aug 24 2023, 8:32 PM
anmolparalkar-nxp updated this revision to Diff 553340.EditedAug 24 2023, 8:55 PM

@garvitgupta08 respun post minor cleanup. Please use this latest version. Earlier checklist holds. Thanks!

anmolparalkar-nxp abandoned this revision.Aug 24 2023, 8:59 PM