This is an archive of the discontinued LLVM Phabricator instance.

[MSan] Enable MSAN for loongarch64
ClosedPublic

Authored by Ami-zhang on Dec 21 2022, 11:20 PM.

Details

Summary

This patch adds basic memory sanitizer support for loongarch64
with 47-bit VMA, which memory layout is based on x86_64.

The LLVM part of the LoongArch memory sanitizer implementation will
be done separately, which will fix failing tests in check-msan.
These failing tests fail with the following same error: "error in
backend: unsupported architecture".

Diff Detail

Event Timeline

tangyouling created this revision.Dec 21 2022, 11:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2022, 11:20 PM
tangyouling requested review of this revision.Dec 21 2022, 11:20 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 21 2022, 11:20 PM
ERROR   git-clang-format returned an non-zero exit code 1

Please format the code. Maybe, you can use the following command:

git diff -U0 --no-color HEAD^ | clang/tools/clang-format/clang-format-diff.py -i -p1

Add code format check.

SixWeining added inline comments.Dec 26 2022, 6:16 PM
compiler-rt/lib/msan/msan.h
192

We fall to this? But not line 208?

compiler-rt/test/msan/mmap.cpp
22–24

These addresses are the same as x86_64 above. Is this expected?

tangyouling added inline comments.Dec 26 2022, 6:50 PM
compiler-rt/lib/msan/msan.h
192

I will move the contents of line 208 to line 91.

compiler-rt/test/msan/mmap.cpp
22–24

Because the app-{1,2,3} definition of kMemoryLayout is consistent with x86_64, it is the same here.

Modify the kMemoryLayout definition position of loongarch64.

Seems that missing atest in llvm/test/Instrumentation/MemorySanitizer/.

vitalybuka added inline comments.Jan 12 2023, 2:30 PM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
5879 ↗(On Diff #485334)

it looks like exact copy of VarArgMIPS64Helper
why not "new VarArgMIPS64Helper"?

Seems that missing atest in llvm/test/Instrumentation/MemorySanitizer/.

Yes, need to add corresponding test for vararg.

vitalybuka requested changes to this revision.Jan 18 2023, 12:14 AM

I assume work in progress

This revision now requires changes to proceed.Jan 18 2023, 12:14 AM

The patch will continue to be updated by @Ami-zhang

Ami-zhang commandeered this revision.Jun 1 2023, 6:52 PM
Ami-zhang added a reviewer: tangyouling.
Ami-zhang added inline comments.Jun 1 2023, 8:16 PM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
5879 ↗(On Diff #485334)

Yes, VarArgLoongArch64Helper is here based on Mips.
I will verify whether there is any difference in the implementations regarding vararg between the two ABI later, and then add VarArgHelper for LoongArch in a separate patch.

Ami-zhang updated this revision to Diff 527711.Jun 1 2023, 8:18 PM

Remove vararg implementation, and add it in a separate patch later.

Test results are as follows:

$ ninja check-msan
Testing Time: 72.08s
  Unsupported      :   9
  Passed           : 154
  Expectedly Failed:   3
xen0n added inline comments.Jun 1 2023, 8:28 PM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
5879 ↗(On Diff #485334)

In reality LoongArch's ABI is much more similar to that of RISCV than Mips. You may want to double-check that.

vitalybuka added inline comments.Jun 9 2023, 3:46 PM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
437 ↗(On Diff #527711)

can you please move this file into a separate patch and add some tests

e.g. autogenerated like this:
llvm/utils/update_test_checks.py --opt-binary ../out/bin/opt llvm-project/llvm/test/Instrumentation/MemorySanitizer/AArch64/vararg.ll --check-globals

Ami-zhang added inline comments.Jun 12 2023, 2:55 AM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
437 ↗(On Diff #527711)

Ok, I have moved it into another patch.

Without LLVM part of the LoongArch memory sanitizer implementation, there are many failing tests with "error in backend: unsupported architecture" in check-msan. And with that implementation, the tests all pass.

Currently, VarArgHelper implementation on LoongArch isn't supported yet, which will be done separately later. But we will add a test for VarArgNoOpHelper in llvm/test/Instrumentation/MemorySanitizer/LoongArch, which is based on the X86 one.

Ami-zhang updated this revision to Diff 530442.EditedJun 12 2023, 2:57 AM
Ami-zhang retitled this revision from [msan] Add msan support for loongarch64 to [MSan] Enable MSAN for loongarch64.
Ami-zhang edited the summary of this revision. (Show Details)

move MemorySanitizer.cpp file into another patch D152692

Ping
@vitalybuka I've updated this. Could you please take the time to review it again? Thank you!

vitalybuka accepted this revision.Jun 26 2023, 2:26 PM
This revision is now accepted and ready to land.Jun 26 2023, 2:26 PM
MaskRay accepted this revision.Jun 26 2023, 2:30 PM
MaskRay added inline comments.
compiler-rt/lib/msan/msan_allocator.cpp
86

namespace-scope const is automatically internal linkage, so you can omit static

Ami-zhang added inline comments.Jun 26 2023, 7:31 PM
compiler-rt/lib/msan/msan_allocator.cpp
86

Ok, I have omitted static in the namespace-scope const.

Ami-zhang updated this revision to Diff 534820.Jun 26 2023, 7:32 PM

Omit static in the namespace-scope const.

This revision was automatically updated to reflect the committed changes.