Page MenuHomePhabricator

[RISC-V][HWASAN] Support tagging global variables for RISC-V HWASAN
ClosedPublic

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

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
400–402

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
3859–3861

Ditto.

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

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

8–9
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
400–402

fixed, thanks

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3859–3861

fixed, thanks

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

fixed, thanks

luismarques accepted this revision.Oct 31 2022, 8:21 AM
This revision is now accepted and ready to land.Oct 31 2022, 8:21 AM
jrtc27 requested changes to this revision.Oct 31 2022, 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.Oct 31 2022, 8:39 AM
smd updated this revision to Diff 472549.Nov 2 2022, 2:34 AM

Addressing comments

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

fixed, thanks

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

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

smd added a comment.Nov 29 2022, 1:03 PM

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

@jrtc27 ping

luismarques accepted this revision.Nov 29 2022, 1:05 PM
smd added a comment.Dec 9 2022, 12:55 AM

Hi @jrtc27,
If you don't have any objections, I'd like to push this patch this weekend.
Thanks

This revision was not accepted when it landed; it landed in state Needs Review.Dec 13 2022, 3:58 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.