This is an archive of the discontinued LLVM Phabricator instance.

[RISC-V][HWASAN] Don't explicitly load GOT entry to call hwasan mismatch routine
ClosedPublic

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

Details

Summary

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

Depends On D131343

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
smd updated this revision to Diff 457297.Sep 1 2022, 9:10 AM

Update test

Sorry, could you please explain why this required a manually written check? It's not obvious to me.

smd added a comment.Oct 26 2022, 12:51 PM

Sorry, could you please explain why this required a manually written check? It's not obvious to me.

I tried to figure out how to use update_llc_test_checks.py properly, but unfortunately I was not able to make it expand @llvm.hwasan.check.memaccess.shortgranules, so I had to add those checks manually.
If you have any suggestions how to fix this test in a proper way, they'll be more than welcome!

Btw this patch and previous one obviously should be merged and pushed together.'

Thanks!

I looked at this more carefully. I think the CHECKs for the .section were already not being updated automatically (not sure if that was the case when they were originally added), so you didn't change that (when I originally skimmed the patch I thought that was new). I'm not sure the two UTC_ARGS are actually needed.

The changes to the test itself seem fine, please do merge with the previous patch, as you indicated.

smd updated this revision to Diff 472546.Nov 2 2022, 2:30 AM

Addressing comments

smd retitled this revision from [RISC-V][HWASAN] Update memaccess test with proper checks to [RISC-V][HWASAN] Don't explicitly load GOT entry to call hwasan mismatch routine.Nov 2 2022, 2:30 AM
smd edited the summary of this revision. (Show Details)
smd added a reviewer: luismarques.
smd edited the summary of this revision. (Show Details)Nov 2 2022, 2:32 AM
smd added a comment.Nov 2 2022, 2:58 AM

@luismarques @jrtc27 Do you think it's ok if I commit this patch?
Thanks

As far as I can tell from looking at the relevant psABI discussions and GNU patches, this overall approach is OK.

Thinking about the specifics of the patch, what happens if you separately compile and assemble this? Does it lose the STO_RISCV_VARIANT_CC?

smd added a comment.Nov 12 2022, 11:38 PM

As far as I can tell from looking at the relevant psABI discussions and GNU patches, this overall approach is OK.

Thinking about the specifics of the patch, what happens if you separately compile and assemble this? Does it lose the STO_RISCV_VARIANT_CC?

Hi Luis,

Sorry for the delayed reply.
Regarding your question from what I can tell STO_RISCV_VARIANT_CC is present in the object file if it's compiled separately, the flag is present on the symbol:

clang -fsanitize=hwaddress -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions global.c -c -o /tmp/globals.o
...
llvm-readobj -a /tmp/globals.o
...
  Symbol {
    Name: __hwasan_tag_mismatch_v2 (650)
    Value: 0x0
    Size: 0
    Binding: Global (0x1)
    Type: None (0x0)
    Other [ (0x80)
      STO_RISCV_VARIANT_CC (0x80)
    ]
    Section: Undefined (0x0)
  }

And I expect it's also supported by the linkers by quickly looking into existing patches.

It seems we're missing a emitDirectiveVariantCC equivalent to AArch64's emitDirectiveVariantPCS, which is what should be used here I believe otherwise Luis is right, the assembly doesn't mark the symbol as a variant CC one, so round-tripping through assembly loses the flag.

What you showed is still compiling directly from C to an object file with the integrated assembler; you'd need to either use -no-integrated-as or use -S and then assemble the .s file.

smd added a comment.Nov 13 2022, 12:09 AM

It seems we're missing a emitDirectiveVariantCC equivalent to AArch64's emitDirectiveVariantPCS, which is what should be used here I believe otherwise Luis is right, the assembly doesn't mark the symbol as a variant CC one, so round-tripping through assembly loses the flag.

What you showed is still compiling directly from C to an object file with the integrated assembler; you'd need to either use -no-integrated-as or use -S and then assemble the .s file.

Indeed asm file lacks .variant_cc annotation.
I doubt supporting emitDirectiveVariantCC should be a part of this particular patch, so what's the best way to handle this situation?

smd added a comment.EditedNov 15 2022, 4:55 AM

I think one of possible solutions for the problem mentioned above might be the following:

  • mark __hwasan_tag_mismatch_v2 as .variant_cc directly in asm file
  • add support for reading/dumping .variant_cc as emitDirectiveVariantPCS does
  • remove setting ELF::STO_RISCV_VARIANT_CC for __hwasan_tag_mismatch_v2 in RISCVAsmPrinter.cpp

Supposedly (2) should be implemented/committed as a separate patch before this one. It looks like it's being done as part of https://reviews.llvm.org/D103435
I might have my own patch for .variant_cc support. Does it make sense to publish it for the review, since the D103435 has been sitting there for over a year?

@jrtc27 @luismarques what do you think of that?

I think one of possible solutions for the problem mentioned above might be the following:

  • mark __hwasan_tag_mismatch_v2 as .variant_cc directly in asm file
  • add support for reading/dumping .variant_cc as emitDirectiveVariantPCS does
  • remove setting ELF::STO_RISCV_VARIANT_CC for __hwasan_tag_mismatch_v2 in RISCVAsmPrinter.cpp

Supposedly (2) should be implemented/committed as a separate patch before this one. It looks like it's being done as part of https://reviews.llvm.org/D103435
I might have my own patch for .variant_cc support. Does it make sense to publish it for the review, since the D103435 has been sitting there for over a year?

@jrtc27 @luismarques what do you think of that?

It looks like it D103435 has started picking up activity again. You can always publish your patch with a description that references D103435, and possibly point out the ways in which the patches are different. For instance, sometimes if it's just a subset of another patch that can help that subset be merged earlier, without being blocked on other issues. In any case, perhaps now D103435 will move faster.

I suggest rebasing this patch on an implementation of (2), either D103435 or your alternative, as that seems like the proper solution. If those patches were to continue being blocked for a while I don't have a problem with (1), but I imagine other people might disagree.

smd added a comment.EditedNov 22 2022, 5:33 AM

I think one of possible solutions for the problem mentioned above might be the following:

  • mark __hwasan_tag_mismatch_v2 as .variant_cc directly in asm file
  • add support for reading/dumping .variant_cc as emitDirectiveVariantPCS does
  • remove setting ELF::STO_RISCV_VARIANT_CC for __hwasan_tag_mismatch_v2 in RISCVAsmPrinter.cpp

Supposedly (2) should be implemented/committed as a separate patch before this one. It looks like it's being done as part of https://reviews.llvm.org/D103435
I might have my own patch for .variant_cc support. Does it make sense to publish it for the review, since the D103435 has been sitting there for over a year?

@jrtc27 @luismarques what do you think of that?

It looks like it D103435 has started picking up activity again. You can always publish your patch with a description that references D103435, and possibly point out the ways in which the patches are different. For instance, sometimes if it's just a subset of another patch that can help that subset be merged earlier, without being blocked on other issues. In any case, perhaps now D103435 will move faster.

I suggest rebasing this patch on an implementation of (2), either D103435 or your alternative, as that seems like the proper solution. If those patches were to continue being blocked for a while I don't have a problem with (1), but I imagine other people might disagree.

Hi Luis,

I think all 3 of those steps might be necessary: it's impossible to do (1) without having changes from (2).
And (3) could be done after having (1) done. I doubt anyone else would use __hwasan_tag_mismatch_v2 except for this particular case, so marking it with .variant_cc might not hurt.
It looks like D138352 might do the trick and bring .variant_cc support pretty soon.

Other possible solution I see is to push this patch as is and later fix the issues you've raised after .variant_cc patch is commited.

smd updated this revision to Diff 483439.Dec 15 2022, 11:13 PM

Rebase and properly emit .variant_cc

smd added a comment.Dec 15 2022, 11:17 PM

With D139414 being pushed to upstream, .variant_cc symbol is now properly emitted by hwasan code:

clang -fsanitize=hwaddress -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions src/llvm-project/compiler-rt/test/hwasan/TestCases/global.c -S -c -o /tmp/globals.s | grep variant_cc /tmp/globals.s
        .variant_cc     __hwasan_tag_mismatch_v2
luismarques accepted this revision.Jan 4 2023, 10:46 AM

LGTM.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
275–276

Nit: missing period.

277–278

Nit. You can use auto for the variable since the type is obvious from the cast.

This revision is now accepted and ready to land.Jan 4 2023, 10:46 AM
This revision was landed with ongoing or failed builds.Jan 9 2023, 5:46 AM
This revision was automatically updated to reflect the committed changes.