This is an archive of the discontinued LLVM Phabricator instance.

[ASAN] Support memory checks on vp.load/store.
ClosedPublic

Authored by fakepaper56 on Mar 16 2023, 2:59 AM.

Details

Summary

The patch adds new member MaybeEVL into InterestingMemoryOperand to represent
the effective vector length for vp intrinsics. It may be extended for some target intrinsics in the future.

Diff Detail

Event Timeline

fakepaper56 created this revision.Mar 16 2023, 2:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 2:59 AM
fakepaper56 requested review of this revision.Mar 16 2023, 2:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2023, 2:59 AM
reames requested changes to this revision.Mar 16 2023, 7:51 AM
reames added inline comments.
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1467

This looks wrong? What does the bitwidth have to do with the *value* of EVL?

1494

I may be missing something here, but isn't EVL just an upper bound on the loop? If so, maybe we shouldn't use for the ForEachLane variant and should instead have a variant which takes End explicitly?

This revision now requires changes to proceed.Mar 16 2023, 7:51 AM
fakepaper56 added inline comments.Mar 16 2023, 6:09 PM
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1494

Agree. I am too conservative to change the original code.

Create a new interface SplitBlockAndInsertForLanes, similar as
SplitBlockAndInsertForEachLane but have input argument to control end index.

Rebase and ping.

reames requested changes to this revision.Mar 24 2023, 8:57 AM
reames added inline comments.
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1491

I believe this case should be a trunc.

1497

I don't believe there's anything preventing AVL from being 0 here right? If so, this code is wrong. The routine you're calling has a (documented!) assumption that End > 0. To reuse, you need to add the loop guard and remove that assumption.

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
1641

It's this call where a dynamically zero End becomes problematic.

This revision now requires changes to proceed.Mar 24 2023, 8:57 AM
fakepaper56 planned changes to this revision.Mar 25 2023, 5:49 AM
fakepaper56 added inline comments.
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1497

Thank you for pointing my mistake. I am not positive to remove End > 0 assumption from SplitBlockAndInsertSimpleForLoop because it is used in MemorySanitizerVisitor::paintOrigin and there should not a loop guard to test End.

fakepaper56 updated this revision to Diff 508304.EditedMar 25 2023, 5:55 AM

Fix mistakes that Philip pointed.

fakepaper56 marked an inline comment as done.Mar 25 2023, 5:58 AM

Rebase and fix test fails.

Rebase and ping.

fakepaper56 updated this revision to Diff 514853.EditedApr 19 2023, 1:08 AM

Rebase and ping.

fakepaper56 marked 2 inline comments as done.Apr 19 2023, 1:12 AM
fakepaper56 planned changes to this revision.Apr 19 2023, 11:02 PM
fakepaper56 updated this revision to Diff 515260.EditedApr 20 2023, 2:22 AM

This update does,

  1. Only check whether EVL is zero when EVL is not nullptr. It helps not generate redundant zero check for non-EVL InterestingMemoryOperand.
  2. Clean up my patch.

I am sorry for that the quality of the patch was terrible. I guess it was due to
that I was too rushed to land the revision before my military educational
recall. It's reasonable that no one wanted to review the terrible patch.
I promise that I will improve the quality of my succeeding patches in LLVM.

fakepaper56 edited the summary of this revision. (Show Details)Apr 24 2023, 9:20 AM
fakepaper56 edited the summary of this revision. (Show Details)
craig.topper added inline comments.Apr 24 2023, 10:58 AM
llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
1477

Can we use IB.CreateZExtOrTrunc(EVL, IntptrTy) and remove the ifs?

1481

extacting -> extracting

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
1670

Why do we need to keep setting the insertion point every iteration? Will that cause the last index to be checked first?

Is there a test case for constant evl. I didn't see one.

fakepaper56 added inline comments.Apr 24 2023, 7:01 PM
llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
1670

Why do we need to keep setting the insertion point every iteration?

This is to follow the SplitBlockAndInsertForEachLane(ElementCount, Type *, Instruction *, std::function<void(IRBuilderBase&, Value*)>). And the old one is to follow the legacy code.

Will that cause the last index to be checked first?

I think it won't because the letter index will be inserted to the back of the former index.

Is there a test case for constant evl.

Fixed vector masked.load will have a constant evl here. store.v4f32.1110 in llvm/test/Instrumentation/AddressSanitizer/asan-masked-load-store.ll is an example.

Use CreateZExtOrTrunc and fix typos.

craig.topper added inline comments.Apr 24 2023, 7:37 PM
llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
1670

Why do we need to keep setting the insertion point every iteration?

This is to follow the SplitBlockAndInsertForEachLane(ElementCount, Type *, Instruction *, std::function<void(IRBuilderBase&, Value*)>). And the old one is to follow the legacy code.

Will that cause the last index to be checked first?

I think it won't because the letter index will be inserted to the back of the former index.

You're right. InsertBefore is still after whatever the previous iteration inserted so it didn't really change the insertion point.

Is there a test case for constant evl.

Fixed vector masked.load will have a constant evl here. store.v4f32.1110 in llvm/test/Instrumentation/AddressSanitizer/asan-masked-load-store.ll is an example.

kito-cheng added inline comments.Apr 28 2023, 1:30 AM
llvm/test/Instrumentation/AddressSanitizer/asan-vp-load-store.ll
30

Could you take a look to see why it use __asan_storeN, not __asan_store4? I saw D145198 can just generate __asan_store4?

fakepaper56 added inline comments.Apr 28 2023, 8:46 PM
llvm/test/Instrumentation/AddressSanitizer/asan-vp-load-store.ll
30

__asan_load4 needs the input address is 4-bytes aligned. I will refine the test to check that.

Update test to test the aligment of pointer.

@reames Do you have another concern about the revision? I will land the revision this weekend if the revision does not have comment needed to address. Thank you.

This revision was not accepted when it landed; it landed in state Needs Review.May 7 2023, 4:30 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.