This is an archive of the discontinued LLVM Phabricator instance.

[dfsan] Propagate origin tracking at store
ClosedPublic

Authored by stephan.yichao.zhao on Mar 2 2021, 11:01 AM.

Diff Detail

Event Timeline

stephan.yichao.zhao requested review of this revision.Mar 2 2021, 11:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2021, 11:01 AM

update tests cases

How many patches are left for origin tracking?

When can we start adding end-to-end tests in compiler-rt?

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
211–212
610

Do we need this function at all? Maybe just inline the increment at the one place we do it.

658
660
2269

Do we still need to do an intptr cast here?

2276

I think these names would be clearer.

2289
2313

We no longer honor ClPreserveAlignment here. TBH I don't know what the original purpose of the flag is, but why are we OK to remove this?

2319
2495–2497

Nit

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

This is a dangling pointer to p. In real code this would be a bug.

I'm not sure if its undefined behavior or not; if it is, the compiler could optimize the "escape" away.

stephan.yichao.zhao marked 11 inline comments as done.Mar 3 2021, 12:16 PM

How many patches are left for origin tracking?

When can we start adding end-to-end tests in compiler-rt?

We are able to add end-to-end tests after this one because ld/st are common.

After this one, the left patches are

  1. adding origin tracking report and the set of end-to-end tests the current instrumentation can support
  2. support phi and memory transfer instructions
  3. those custom function extensions to support origin. This could be split into 2-3.
llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
610

removed the function.

2269

Just checked DataLayout::getIntPtrType from llvm/lib/IR/DataLayout.cpp. It returns an int type.
So the CreateIntCast is more like adjusting bitwidth by ext or trunc. It is fine if the code returns Origin directly.

2313

We moved this into getShadowAlign. So both load and store, and other related code can share them.

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

Thank you. Replaced ret %p by foo(%p).

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

update

This revision is now accepted and ready to land.Mar 3 2021, 2:08 PM
gbalats added inline comments.Mar 3 2021, 3:20 PM
llvm/test/Instrumentation/DataFlowSanitizer/origin_ldst.ll
306

Do these have to be exact? If we abstract them to [[#ADDR]] it will be easier to support fast8 later or other layout changes.

update test cases

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

For most other cases I think using #ADDR should work.
We may want to keep one such a check to 'cover' this addressing pattern.
I moved this pattern check into basic.ll with -D.

gbalats added inline comments.Mar 4 2021, 12:07 PM
llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
2498

For this function, Origin is really an optional argument (as it's just a nullptr when origin tracking is not enabled).
I think that the previous name (storePrimitiveShadow) is clearer in that respect as it better captures its main purpose.

llvm/test/Instrumentation/DataFlowSanitizer/origin_ldst.ll
385–396

Are these expectations exhaustive? Or are there missing lines? In the first case, adding the -NEXT part should be straightforward.
Also, my personal preference is to use numeric substitution blocks for registers (e.g., [[#R:]]) if the names do not increase readability (that's just a nit though).

update test cases

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
2498

This name is consistent with other function names that operate on both shadow and origins.
If we only use Shadow but not Origin, following other function names, one may assume it does not do anything with Origin: storeZeroPrimitiveShadow is a case.

llvm/test/Instrumentation/DataFlowSanitizer/origin_ldst.ll
385–396

Added NEXT for most cases.

These names help read the IR to understand which are origins and shadows, and operations on them at IR level.

gbalats accepted this revision.Mar 4 2021, 2:57 PM
This revision was landed with ongoing or failed builds.Mar 4 2021, 3:35 PM
This revision was automatically updated to reflect the committed changes.