This is an archive of the discontinued LLVM Phabricator instance.

MSAN: Allow emitting checks for struct types
ClosedPublic

Authored by guiand on Jun 26 2020, 12:48 PM.

Details

Summary

This is a precursor to the general eager-checks patch.

Diff Detail

Event Timeline

guiand created this revision.Jun 26 2020, 12:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2020, 12:48 PM
eugenis added inline comments.Jun 26 2020, 3:43 PM
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
1152

this can be improved now by moving the origin update code behind the shadow check

1397

extra "ShadowItem ="

1398

I don't think this is possible

1401

I'd prefer if this generated less pointless comparisons.

It would be nice if CreateICmpNE(i1 x, i1 false) automatically folded to "x". I think this is a reasonable change to make in IRBuilder, even though this is probably a very unlikely situation.

As an alternative, make convertShadowToScalar return i1 and remove icmp in the callers (call it something like convertShadowToBool). Check Combiner class how not to emit a "false" constant. As for the Maybe* callers, it's ok to not handle aggregates there.

guiand marked an inline comment as done.Jun 29 2020, 10:04 AM
guiand added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
1152

This check covers arrays as well, so I don't think I can get rid of it completely but I can change it to check for isArrayType(). I wrote some code to let convertShadowToScalar handle Array types as well, but I thought I would leave that to a separate patch.

guiand updated this revision to Diff 274172.Jun 29 2020, 10:19 AM

Addressed comments

eugenis marked an inline comment as done.Jun 29 2020, 1:49 PM
eugenis added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
1152

Ack.

llvm/test/Instrumentation/MemorySanitizer/check-struct.ll
10

Please check that the test passes when LLVM is build without assertions. AFAIR temp names are lost in that case.

19

There is still this unnecessary icmp.

guiand updated this revision to Diff 274876.Jul 1 2020, 11:10 AM

Don't emit unnecessary icmp

guiand marked 2 inline comments as done.Jul 1 2020, 11:31 AM
guiand added inline comments.
llvm/test/Instrumentation/MemorySanitizer/check-struct.ll
10

As far as I can tell it's passing without assertions enabled, yeah.

guiand updated this revision to Diff 276110.Jul 7 2020, 9:52 AM

Fixed msan_x86_bts_asm test after optimizing away icmp i1 %x, false

Please upload with context.

guiand updated this revision to Diff 279928.Jul 22 2020, 1:14 PM

Reuploaded with context

guiand marked 3 inline comments as done.Jul 22 2020, 1:14 PM
eugenis accepted this revision.Jul 22 2020, 1:35 PM

LGTM with 2 comments

llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
1414

Mention that the result may not be the same bit width as the original type in the comment (this is the usual property of shadow types).

llvm/test/Instrumentation/MemorySanitizer/check-struct.ll
19

could this line say

; CHECK: br i1 [[F1_OR]]

?

This revision is now accepted and ready to land.Jul 22 2020, 1:35 PM
vitalybuka accepted this revision.Jul 22 2020, 8:38 PM
vitalybuka added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
1415

this code do not uses {} for one line blocks

1428

do you see a difference without this check?

1430

this comment is not very useful

This revision was automatically updated to reflect the committed changes.
guiand marked 5 inline comments as done.Jul 23 2020, 9:52 AM
guiand added inline comments.
llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
1428

If you look at msan_x86_bts_asm.ll, this check eliminates an unnecessary icmp in some cases.