Page MenuHomePhabricator

[RISC-V][HWASAN] Support tagging global variables for RISC-V HWASAN
Needs ReviewPublic

Authored by smd on Aug 30 2022, 11:18 PM.

Details

Summary

[8/11] patch series to port HWASAN for riscv64

Depends On D132994

Diff Detail

Event Timeline

smd created this revision.Aug 30 2022, 11:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2022, 11:18 PM
smd requested review of this revision.Aug 30 2022, 11:18 PM
kito-cheng added inline comments.Sep 2 2022, 2:38 AM
llvm/test/CodeGen/RISCV/tagged-globals.ll
9

Maybe add one more testcase to show @global = internal global i32 0 will always access via GOT with +tagged-globals even --relocation-model=pic?

smd updated this revision to Diff 457808.Sep 3 2022, 10:55 AM

Addressing comments

smd added inline comments.Sep 3 2022, 10:56 AM
llvm/test/CodeGen/RISCV/tagged-globals.ll
9

Done, thanks

vitalybuka resigned from this revision.Sep 22 2022, 1:19 PM

LGTM, but I guess it's more for RISCV folks to review

smd added a comment.Oct 3 2022, 5:36 AM

Hi @jrtc27, @luismarques, @MaskRay, @craig.topper, @asb,

Could you please help with reviewing this patch.
Thanks

smd added a comment.Oct 19 2022, 6:16 AM

@jrtc27, @asb, @luismarques, @MaskRay, @craig.topper
Gengle ping, could you please help with reviewing this patch?
Thanks

Is the tagging actually incompatible with the code model or only with the address materialization instructions that we use as part of the implementation of the code model? Does the tagging actually change the location of globals? I was assuming the location was the same, and the tag was just changing how we encoded that location in a numerical value. My understanding of the code model specifications is that they only care about the actual address ranges. Although the RISC-V psABI gives examples of instruction sequences my interpretation of that is that it's non-normative. So if the location doesn't actually change in principle you could materialize those with a different instruction sequence and still claim you comply with the code model, is that not the case? That only impacts the comments in the patch, I'm not opposed to the approach of using the GOT.

The overall patch LGTM.

llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp
458–460

Nit: (spellchecking only)

// When HWASAN is used and tagging of global variables is enabled
// they should be accessed via the GOT, since the tagged address of a global
// is incompatible with existing code models. This also applies to non-pic mode.

See my broader patch review comment about the semantics.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4133–4138

Ditto.

llvm/test/CodeGen/RISCV/tagged-globals.ll
9–10

Nit: globalint makes it sound it's an integer, can you tweak that somehow?

9–10
smd added a comment.Oct 26 2022, 1:00 PM

Does the tagging actually change the location of globals?

No, it doesn't.

I was assuming the location was the same, and the tag was just changing how we encoded that location in a numerical value.

Yes, you're right.

My understanding of the code model specifications is that they only care about the actual address ranges.
So if the location doesn't actually change in principle you could materialize those with a different instruction sequence and still claim you comply with the code model, is that not the case?

I guess that you're right. However from what I can tell, this would require to make linker to distinguish different kind of addresses of variables. I guess that's why ARM has a separate symbol for such tags.

That only impacts the comments in the patch, I'm not opposed to the approach of using the GOT.

I'll fix the comments as you suggested, thanks.

smd updated this revision to Diff 470912.Oct 26 2022, 1:11 PM

Addressing comments

llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp
458–460

fixed, thanks

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4133–4138

fixed, thanks

llvm/test/CodeGen/RISCV/tagged-globals.ll
9–10

fixed, thanks

luismarques accepted this revision.Mon, Oct 31, 8:21 AM
This revision is now accepted and ready to land.Mon, Oct 31, 8:21 AM
jrtc27 requested changes to this revision.Mon, Oct 31, 8:39 AM
jrtc27 added inline comments.
llvm/test/CodeGen/RISCV/tagged-globals.ll
6–8

These shouldn't be here

This revision now requires changes to proceed.Mon, Oct 31, 8:39 AM
smd updated this revision to Diff 472549.Wed, Nov 2, 2:34 AM

Addressing comments

smd added inline comments.Wed, Nov 2, 2:41 AM
llvm/test/CodeGen/RISCV/tagged-globals.ll
6–8

fixed, thanks

smd edited the summary of this revision. (Show Details)Wed, Nov 2, 2:41 AM
smd added a comment.Sat, Nov 12, 11:55 PM

@jrtc27
Hi, could you please check that your request has been addressed?
Thanks