This is an archive of the discontinued LLVM Phabricator instance.

[dfsan] Propagate origin tracking at load
ClosedPublic

Authored by stephan.yichao.zhao on Feb 26 2021, 12:08 PM.

Details

Summary

This is a part of https://reviews.llvm.org/D95835.

One issue is about origin load optimization: see the
comments of useCallbackLoadLabelAndOrigin

@gbalats This change may have some conflicts with your 8bit change. PTAL the change at visitLoad.

Diff Detail

Event Timeline

stephan.yichao.zhao requested review of this revision.Feb 26 2021, 12:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2021, 12:08 PM
gbalats added inline comments.Mar 1 2021, 11:58 AM
llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
578–580

I think it would be cleaner if we retained the older function (with its older return value) and introduced a new one:
loadShadowAndOrigin

1964–1968

Add assertion before returning to check that !ShouldTrackOrigins => Shadows.empty() && Origins.empty()

1965–1966

Shouldn't this be guarded by ShouldTrackOrigins flag? Even if it turns out to be a noop (when Shadows, Origins is empty), it's a little confusing for the reader.

2055–2057

Could instead use the flag, based on the assertion above.

2098

Is the initialization required here given the next line?

morehouse added inline comments.Mar 1 2021, 3:32 PM
llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
578–580

+1

1910
2047

What does "non-scaped" mean?

llvm/test/Instrumentation/DataFlowSanitizer/origin_ldst.ll
244

Nit: s/LABLE/LABEL

stephan.yichao.zhao marked 6 inline comments as done.

update

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
578–580

That would be great. I had also considered this.
At some points, loading origins depends on loading shadows.
One is loadFast16ShadowFast, using both shadows and origins to create some select instructions.
The other is by DFSanLoadLabelAndOriginFn.
So it is not straightforward to call loadShadow from loadShadowAndOrigin.

1964–1968

combineOrigins has a similar assert. Added a "if (ShouldTrackOrigins) {" here to improve readability.

2047

replaced by non-escaped, for alloca pointers are not used outside the function.

2098

Thank you.

This revision is now accepted and ready to land.Mar 2 2021, 7:46 AM
gbalats added inline comments.Mar 2 2021, 11:06 AM
llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
2177–2178

Initialization seems redundant given the std::tie below.

llvm/test/Instrumentation/DataFlowSanitizer/origin_ldst.ll
22

Can these i16 expressions be generalized as in https://reviews.llvm.org/D97723 so that it requires fewer changes when we also have fast8 support?

stephan.yichao.zhao marked an inline comment as done.

updated test cases to use meta data

updated test cases

gbalats accepted this revision.Mar 2 2021, 6:18 PM

LGTM

updated test cases

This revision was landed with ongoing or failed builds.Mar 2 2021, 8:33 PM
This revision was automatically updated to reflect the committed changes.