Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[RISC-V][HWASAN] Add support for HWASAN code instrumentation for RISC-V
ClosedPublic

Authored by smd on Aug 10 2022, 8:04 AM.

Diff Detail

Event Timeline

smd created this revision.Aug 10 2022, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2022, 8:04 AM
smd requested review of this revision.Aug 10 2022, 8:04 AM

some test needs to be updated/added

smd added a comment.Aug 15 2022, 5:23 AM

some test needs to be updated/added

The requested tests have been added as part of D131344.
Thanks.

smd updated this revision to Diff 453090.Aug 16 2022, 12:03 PM

Addressing comments

vitalybuka added inline comments.Aug 16 2022, 10:40 PM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
838

Code should respect ClUseShortGranules, even if it's not useful mode.
Otherwise UseShortGranules should be true for non android

997

Why not here?

smd added inline comments.Aug 17 2022, 9:03 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
838

@vitalybuka
Could you please clarify:
If I get you right, you're suggesting allowing users to override UseShortGranules and make it false, even if the platform doesn't support this mode, right?
For cases rather than this, short granules would be always used. If the user decides to use non-short granules for riscv, in this case the compilation would fail with:

fatal error: error in backend: Cannot select: intrinsic %llvm.hwasan.check.memaccess

which is sort of ok, I guess. But maybe an explicit check like (isRISCV64() && !UseShortGranules) with an error message would be a better idea?

vitalybuka added inline comments.Aug 17 2022, 9:22 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
838

@vitalybuka
Could you please clarify:
If I get you right, you're suggesting allowing users to override UseShortGranules and make it false, even if the platform doesn't support this mode, right?
For cases rather than this, short granules would be always used. If the user decides to use non-short granules for riscv, in this case the compilation would fail with:

fatal error: error in backend: Cannot select: intrinsic %llvm.hwasan.check.memaccess

which is sort of ok, I guess. But maybe an explicit check like (isRISCV64() && !UseShortGranules) with an error message would be a better idea?

I assume hwasan.check.memaccess is just not implemented yet?
copt<> are internal flags, if some one using them, it's their responsibility to understand consequences.
So I would prefer we respect user provided ClUseShortGranules and special error is not needed.

in this case these lines not need to change

smd added inline comments.Aug 17 2022, 9:26 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
838

I assume hwasan.check.memaccess is just not implemented yet?

Indeed it's not implemented, but to be honest I didn't think it should be implemented. I got an impression that short_granules(v2) is a preferred way of checking tags, while v1 function is kept for older targets.
Or am I wrong?

smd updated this revision to Diff 453355.Aug 17 2022, 11:14 AM

Addressing comments

vitalybuka accepted this revision.Aug 17 2022, 1:59 PM
vitalybuka added inline comments.
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
583–585

clang-format please

605
This revision is now accepted and ready to land.Aug 17 2022, 1:59 PM
smd updated this revision to Diff 453634.Aug 18 2022, 5:41 AM

Addressing comments

smd added inline comments.Aug 18 2022, 5:49 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
583–585

It's already formatted

605

Fixed, thanks

smd updated this revision to Diff 453922.Aug 19 2022, 1:30 AM

Don't push tagged globals option for riscv64 hwasan tests

Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2022, 1:30 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
smd updated this revision to Diff 453934.Aug 19 2022, 2:20 AM

Fixing bug

smd added a comment.Aug 22 2022, 9:54 AM

Hi @vitalybuka
Could you please take another look at this patch series and if you wouldn't have any objections, I'd like to push it.
Thanks

vitalybuka accepted this revision.Aug 22 2022, 10:11 AM

Done.
I guess you don't need another round if it's accepted and you just addressing trivial issues.

compiler-rt/test/hwasan/lit.cfg.py
26 ↗(On Diff #453934)

It belongs to a different patch?

Please try to start landing accepted patches to have a few hours delay in between.
If you land all at once, it would be annoying to revert entire stack to fix a bot.

smd updated this revision to Diff 456866.Aug 30 2022, 11:19 PM

Addressing comments

smd edited the summary of this revision. (Show Details)Nov 2 2022, 2:42 AM
arsenm added a subscriber: arsenm.Dec 13 2022, 7:21 AM
arsenm added inline comments.
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
923

/home/matt/src/llvm-project/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:923:72: warning: implicit conversion turns string literal into bool: 'const char[6]' to 'bool' [-Wstring-conversion]

"ebreak\naddiw x0, x0, " + itostr(0x40 + AccessInfo), "{x10}", "{x11}",
smd added inline comments.Dec 13 2022, 8:14 AM
llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
923

For some reason this code works with g++, but indeed fails with clang++. I'll investigate this, thanks:

g++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/build/llvm-project/lib/Transforms/Instrumentation -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/lib/Transforms/Instrumentation -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/build/llvm-project/include -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-uninitialized  -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -g  -fno-exceptions -fno-rtti -std=c++17 -o CMakeFiles/LLVMInstrumentation.dir/HWAddressSanitizer.cpp.o -c /tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp -Wall -Werror

smd@neptune:/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/build/llvm-project/lib/Transforms/Instrumentation$ clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/build/llvm-project/lib/Transforms/Instrumentation -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/lib/Transforms/Instrumentation -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/build/llvm-project/include -I/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-uninitialized  -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -g  -fno-exceptions -fno-rtti -std=c++17 -o CMakeFiles/LLVMInstrumentation.dir/HWAddressSanitizer.cpp.o -c /tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp -Wall -Werror -Wstring-conversion
/tank/work/dev/share/nfs/ablue/hwasan_upstream/riscv_hwasan/src/llvm-project/llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:923:72: error: implicit conversion turns string literal into bool: 'const char [6]' to 'bool' [-Werror,-Wstring-conversion]
        "ebreak\naddiw x0, x0, " + itostr(0x40 + AccessInfo), "{x10}", "{x11}",
                                                                       ^~~~~~~
1 error generated.