Page MenuHomePhabricator

[LowerTypeTests][clang] Implement and allow -fsanitize=cfi-icall for RISCV
ClosedPublic

Authored by twd2 on Jul 27 2021, 9:29 AM.

Details

Summary

This patch implements jump tables for RISC-V so that CFI-icall can be enabled for it.

Diff Detail

Event Timeline

twd2 created this revision.Jul 27 2021, 9:29 AM
twd2 created this object with edit policy "Administrators".
twd2 requested review of this revision.Jul 27 2021, 9:29 AM
twd2 changed the edit policy from "Administrators" to "All Users".Aug 5 2021, 2:19 AM
twd2 added a comment.Aug 5 2021, 2:24 AM

Ping?

The build failure is due to the name of the newly introduced kRISCVJumpTableEntrySize. Though I think we can put these magic numbers (4 and 8) just into LowerTypeTestsModule::getJumpTableEntrySize..

asb added a comment.Aug 19 2021, 5:26 AM

Is it possible to write a test case for this?

Is it possible to write a test case for this?

Good question. I had checked that the AArch64 implementation had included a test, but I think that was only for new target-specific stuff, which doesn't apply here. So I was assuming the answer was no, but it would be good to get confirmation. I was also confused by the comment about "the build failure", since this applied cleanly and built OK for me when I looked at it.

twd2 added a comment.Aug 19 2021, 8:15 AM

Is it possible to write a test case for this?

Good question. I had checked that the AArch64 implementation had included a test, but I think that was only for new target-specific stuff, which doesn't apply here. So I was assuming the answer was no, but it would be good to get confirmation. I was also confused by the comment about "the build failure", since this applied cleanly and built OK for me when I looked at it.

Hi, you can check here: https://buildkite.com/llvm-project/premerge-checks/builds/49755#814fd222-2e5a-4400-824d-d1a1f1293c01
The clang-tidy failed due to the invalid case style for variable 'kRISCVJumpTableEntrySize'.

Hi, you can check here: https://buildkite.com/llvm-project/premerge-checks/builds/49755#814fd222-2e5a-4400-824d-d1a1f1293c01
The clang-tidy failed due to the invalid case style for variable 'kRISCVJumpTableEntrySize'.

We can ignore the lint check failure. The new constant follows the same style as the existing ones.

ormris removed a subscriber: ormris.Jan 24 2022, 11:48 AM

Needs a test in llvm/test/Transforms/LowerTypeTests

MaskRay requested changes to this revision.Feb 11 2022, 1:30 AM
This revision now requires changes to proceed.Feb 11 2022, 1:30 AM
twd2 updated this revision to Diff 427901.May 7 2022, 3:20 PM

Add tests as suggested.
Use tail xxx@plt instruction instead of manually auipc + jr, which eliminates issues when linking dynamically-linked executables.

Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2022, 3:20 PM
MaskRay accepted this revision.May 13 2022, 4:25 PM

LGTM.

This revision is now accepted and ready to land.May 13 2022, 4:25 PM

I'll wait a bit before pushing to check whether that further opinions.

twd2 added a comment.May 14 2022, 1:52 PM

I'll wait a bit before pushing to check whether that further opinions.

Thanks! I don't have commit access, so could you please commit on my behalf when appropriate? Wende Tan <twd2.me@gmail.com>

MaskRay retitled this revision from [RISC-V] Implement jump tables for CFI-icall to [LowerTypeTests] Implement cfi-icall for RISCV.May 14 2022, 5:55 PM
MaskRay retitled this revision from [LowerTypeTests] Implement cfi-icall for RISCV to [LowerTypeTests][clang] Implement cfi-icall and allow -fsanitize=cfi-icall for RISCV.May 14 2022, 5:58 PM
This revision was landed with ongoing or failed builds.May 14 2022, 6:05 PM
This revision was automatically updated to reflect the committed changes.
MaskRay retitled this revision from [LowerTypeTests][clang] Implement cfi-icall and allow -fsanitize=cfi-icall for RISCV to [LowerTypeTests][clang] Implement and allow -fsanitize=cfi-icall for RISCV.May 14 2022, 6:05 PM
MaskRay removed rG LLVM Github Monorepo as the repository for this revision.