Page MenuHomePhabricator

[dfsan] Add -dfsan-fast-8-labels flag
ClosedPublic

Authored by gbalats on Mar 16 2021, 12:15 PM.

Details

Summary

This is only adding support to the dfsan instrumentation pass but not
to the runtime.

Added more RUN lines for testing: for each instrumentation test that
had a -dfsan-fast-16-labels invocation, a new invocation was added
using fast8.

Diff Detail

Event Timeline

gbalats created this revision.Mar 16 2021, 12:15 PM
gbalats requested review of this revision.Mar 16 2021, 12:15 PM
llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
62

Please update this diagram for 8-bit layout.

1067

Like aarch64 it may not be easy to test this. Can we make this as a TODO?

1901

Since this expression is used multiple times, please create a function.

2049–2050

32bit is a special case when Size is small. Please add some comments here to explain when 32bit is used.
We also ignore cases for loadFast16ShadowFast when ShadowSize is like 12, 20. Please also explain this
design choice in the comments.

2074

const

2076

We call loadFast16ShadowFast only when Size > 2 and Size % 8 = 0 or Size == 4. So Size is at least 4.
If we assert (ShadowSize >= 4), this avoids reasoning if the code still works when ShadowSize < 4.

2121–2122

This also needs to update about when 32bit is used.

2130

const

2268

something like "load shadow directly without optimizing instrumentation".

2529

The assert prevents using ShadowBytes > 2.
But not every number less than 128 works.
Maybe calculating ShadowVecSize = 8bitmode? 64/ShadowWidthBits: 128/ShadowWidthBits; makes this easy to follow.

llvm/test/Instrumentation/DataFlowSanitizer/array.ll
9

renamed to FAST?

llvm/test/Instrumentation/DataFlowSanitizer/struct.ll
9

FAST16->FAST?

gbalats updated this revision to Diff 331342.Mar 17 2021, 12:17 PM

Rename FAST16 check prefix to FAST.

gbalats marked 2 inline comments as done.Mar 17 2021, 12:18 PM
gbalats added inline comments.
llvm/test/Instrumentation/DataFlowSanitizer/array.ll
9

Renamed.

llvm/test/Instrumentation/DataFlowSanitizer/struct.ll
9

Renamed them.

gbalats marked 2 inline comments as done.Mar 17 2021, 12:18 PM
llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
779

When releasing fast8labels, we set ClFast8Labels = true by default.
If users wanted to use 16bit mode by -dfsan-fast-16-labels=true, this assert happens. So they have to do -dfsan-fast-16-labels=true -dfsan-fast-8-labels=false.
This message can suggest this if we found both are true. Something like:
"cannot set both -dfsan-fast-8-labels and -dfsan-fast-16-labels. -dfsan-fast-8-labels is true by default.
16mode will be deprecated. To use 16 mode, set -dfsan-fast-16-labels=true -dfsan-fast-8-labels=false."

Or when -dfsan-fast-16-labels is true, or we found -dfsan-fast-8-labels=false, this code can print out the deprecation warning.

LGTM once you get Jianzhou's approval.

gbalats updated this revision to Diff 331648.Mar 18 2021, 12:04 PM
gbalats marked 9 inline comments as done.

Added comments and updated description.
Introduced hasFastLabelsEnabled() method.

gbalats added inline comments.Mar 18 2021, 12:14 PM
llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
779

I'm not sure I follow. Did you mean ClFast16Labels perhaps, since the ClFast8Labels has just been introduced by this path? Where is it set? The default value of both flags is false, so the statement "-dfsan-fast-*-labels is true by default" isn't accurate.

I think the deprecation warning should be introduced after fast8 is properly supported by the dfsan runtime.

1067

So what should the behavior be if one runs on MIPS? I think if we don't change the shadow ptr mask, it won't be correct either so I'm not sure just keeping the older value is preferable. Should we report a fatal error if ClFast8Labels is used on MIPS? I'm not sure how this would play out with continuous integration and if it will introduce failures.

2049–2050

Added comments and assertion.

2074

Done. I assume this referred to the ShadowSize variable. Moved to the top.

2076

Added assertion and changed <= to == 4.

2121–2122

Added comment.

2529

I'm not sure why the suggestion makes this easier to follow. It only makes it easier if you know what the code was before, imo, but I think the original intention was that the vector type should fit in 128 bits regardless of the mode being used (which is what this assertion is trying to enforce).

Removed the assertion to support ShadowBytes > 2.

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
69–70

If users read top to down, it seems helpful if we move this sentence above the first layout.
I suggest we first explain there are two kinds of shadow layouts. One uses 2 bytes bit for fast-16-labels mode with 16 labels and legacy mode with 2^16 labels, and the other uses 1 byte for fast-8-labels mode with 8 labels. Then we show the two layouts.

85

Addresses below 0x000000010000 is for kernel usage.
We can mark unused from 0x000000010000 to 0x100000008000.

779

thought about this again. Since fast-8-labels is not fully supported yet, and set to false by default, we do not have the issue for now.
When we set its default to true, this message could be updated accordingly.

2059

A comment with more context could be
"LLVM bitcode supports loading integers with byte size 1, 12, 20....
But loadFast16ShadowFast only optimizes normal cases loading i32 i64 i128 etc..
so it is fine to support only size=4 or size % 8 == 0. "

2121–2122

64 -> 64 or 32

gbalats updated this revision to Diff 331671.Mar 18 2021, 1:22 PM
gbalats marked 3 inline comments as done.

Amend comments and description.

gbalats marked 2 inline comments as done.Mar 18 2021, 1:24 PM
gbalats added inline comments.
llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
69–70

You're right the bottom part relates to the 16-shadow-bits layout. Changed the order and added more text to explain better.

2059

Expanded comment.

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

My first comment was confusing. It was not suggesting supporting ShadowBytes>2.
The first version with assertion(ShadowBytes * ShadowVecSize <= 128) is better, because with that the code can only use 64bit or 128bit.
Please add that assertion back. sorry for the confusing suggestion.

gbalats updated this revision to Diff 331700.Mar 18 2021, 3:15 PM
gbalats marked an inline comment as done.

Re-added shadow vector assertion.

gbalats marked an inline comment as done.Mar 18 2021, 3:16 PM
gbalats added inline comments.
llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
2529

Re-added the assertion.

llvm/lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
19–51

Please add the missing /// at line 20.

2524

nit: if assert is considered as a nop in an optimized build, ShadowVecBitSize may be considered as a not-used variable. In that case it can be inlined into the assert.

This revision is now accepted and ready to land.Mar 18 2021, 3:25 PM
gbalats updated this revision to Diff 331707.Mar 18 2021, 3:34 PM
gbalats marked an inline comment as done.

Fixed assert and empty line in description.

gbalats marked 2 inline comments as done.Mar 18 2021, 3:36 PM
This revision was landed with ongoing or failed builds.Mar 18 2021, 4:30 PM
This revision was automatically updated to reflect the committed changes.