until have bit granularity, use heuristics for typical bitfield code to avoid false pos at potential false neg risk

[derekbruening]

From [email protected] on March 27, 2012 11:11:05

I implemented DrMem under the assumption I'd immediately add bit-level granularity but that never became high priority b/c all the target apps up to this point had very, very few bitfields or other features that need bit-level granularity, but we're starting to hit such issues ( issue #792 , issue #412 in system libs, issue #513 in V8).

until we have issue #113 one proposal has been to set the full byte as defined if any bit is set. there are very simple ways to do this but they may give up too much accuracy and add too many false negatives on bit tests. currently investigating.

Original issue: http://code.google.com/p/drmemory/issues/detail?id=849

[derekbruening]

From [email protected] on March 27, 2012 13:18:19

testing a bit in a dword ends up compiled down to OP_and or OP_test with a constant that contains at least one zero bit. the result thus has at least one bit set. but treating that whole result byte as defined is unacceptable as the tool will then not catch a bug where a completely undefined value is tested for having a bit set.

for non-/GL V8 and most bitfields, relaxing the combination rules for OP_or to "def + undef => def" could be a hack that might work w/o too many false negatives (under an option of course). however, for /GL V8 (our release build), the bitfields are initialized using the xor sequence seen in issue #489 :

bitvar = ((bitvar ^ value) & bitcoverage) ^ bitvar

(keeps the non-bitfield bits of bitvar unchanged and sets the bitfield bits of bitvar to the bitfield bits of value)

bit-level granularity actually would NOT solve this, and as pointed out in issue #489 memcheck doesn't handle this either. this is the type of optimized code that thwarts these tools. we could try to pattern-match it as mentioned in issue #489 , or remove /GL.

Summary: until have bit granularity, use heuristics for typical bitfield code to avoid false pos at potential false neg risk

[derekbruening]

From [email protected] on March 28, 2012 13:14:38

*** DONE have DR work on non-xor sequences: OP_or and OP_and
CLOSED: [2012-03-28 Wed 15:59]
- State "DONE" [2012-03-28 Wed 15:59]
**** DONE change OP_or to be "def + undef => def"
CLOSED: [2012-03-28 Wed 13:43]
- State "DONE" [2012-03-28 Wed 13:43]

easy to impl since in slowpath. note that relaxing to allow OP_or with a
non-0 value is not enough b/c often the fully-defined val is 0.

should we require the undef side to be at least partially defined? no, b/c
that doesn't happen even in my simple examples.

so basically I'm re-writing the rules for combining operands via OP_or:
def + undef => def

**** DONE handle OP_and init of bitfield
CLOSED: [2012-03-28 Wed 13:43]
- State "DONE" [2012-03-28 Wed 13:43]

from V8 code above:
58 0241737c 81e10000e0ff and ecx,0xffe00000
58 0241738d 80e1fe and cl,0xfe

how tell difference between this OP_and and an OP_and that's a test? rely
on a test either using OP_test or a non-const or using a const w/ just one
bit set?

**** TODO what are the expected false negatives from this?

what code uses OP_or? how about OP_and with multi-1-bit mask?

all our tests pass

**** DONE evaluate on V8 => eliminates all
CLOSED: [2012-03-28 Wed 13:43]
- State "DONE" [2012-03-28 Wed 13:43]

% ~/drmemory/git/build_drmemory/bin/drmemory.exe -pause_at_assert -batch -no_results_to_stderr -dr e:/src/dr/git/exports -- ./unit_tests.exe --gtest_filter="FrameworkUnitTest.*"

HEAD:
Dr.M 3137 unique, 221660 total uninitialized access(es)

just OP_or relaxation, on FrameworkUnitTest Debug:
Dr.M 117 unique, 258 total uninitialized access(es)

115 of those 117 are some variant of
unibrow::Predicateunibrow::WhiteSpace,128::get @ unicode-inl.h:37

no errors in my bitv8.cpp (compiled just /O2 so has sequence similar to
Debug CacheEntry constructor)

investigating further: it's the OP_and init of the default constructor

adding in OP_and:
Dr.M 0 unique, 0 total uninitialized access(es)

**** TODO evaluate on riched (issue #791) => eliminates some

% ~/drmemory/git/build_drmemory/bin/drmemory.exe -strict_bitops -pause_at_assert -batch -no_results_to_stderr -dr e:/src/dr/git/exports -- ./unit_tests.exe --gtest_filter="GenericInfoViewTest.GenericInfoView"

-strict_bitops:
Dr.M 31 unique, 145 total uninitialized access(es)
defaults:
Dr.M 20 unique, 80 total uninitialized access(es)

some of the remaining are uninits in riched.dll.
so it removed only some of them.
should investigate: maybe the rest are the xor sequence.

**** DONE evaluate on issue #493 => eliminates all
CLOSED: [2012-03-28 Wed 15:06]
- State "DONE" [2012-03-28 Wed 15:06]

usp10 and other uninits

% bin/drmemory.exe -strict_bitops -batch -dr e:/src/dr/git/exports -- e:/src/dr/test/gui-inject.exe
% grep -A 25 ^SUPP ls -1td logs/D*|head -1/g*
SUPPRESSIONS USED:
3x: default issue #462 (write beyond TOS)
1x: default issue #494 (custom data not all initialized)
3x: default issue #493 (bit manip)
9x: default issue #493 (bit manip)
6x: default issue #493 (bit manip)
8x: default issue #493 (bit manip)
2x: default issue #493 (bit manip)
1x: default issue #493 (bit manip)
9x: default issue #337 (real bug in RtlpLowFragHeapAllocFromContext)
1x (leaked 264 bytes): default issue #18 LockEntry::ThreadInit pool leak (nosyms win7)
1x (leaked 24 bytes): default issue #18 CreatePoolEntry mid-chunk linked list leak (nosyms win7)
1x (leaked 768 bytes): default issue #733 (nosyms)
2x (leaked 64 bytes): default issue #306 (critical section 8-byte-in pointer)
ERRORS FOUND:
0 unique, 0 total unaddressable access(es)
1 unique, 4 total uninitialized access(es)

% bin/drmemory.exe -batch -dr e:/src/dr/git/exports -- e:/src/dr/test/gui-inject.exe
% grep -A 25 ^SUPP ls -1td logs/D*|head -1/g*
SUPPRESSIONS USED:
3x: default issue #462 (write beyond TOS)
1x: default issue #494 (custom data not all initialized)
9x: default issue #337 (real bug in RtlpLowFragHeapAllocFromContext)
1x (leaked 264 bytes): default issue #18 LockEntry::ThreadInit pool leak (nosyms win7)
1x (leaked 24 bytes): default issue #18 CreatePoolEntry mid-chunk linked list leak (nosyms win7)
1x (leaked 768 bytes): default issue #733 (nosyms)
2x (leaked 64 bytes): default issue #306 (critical section 8-byte-in pointer)
ERRORS FOUND:
0 unique, 0 total unaddressable access(es)
0 unique, 0 total uninitialized access(es)

but I'm inclined to leave the default supp for cases where people turn on
-strict_bitops

**** DONE evaluate on issue #497 => eliminated
CLOSED: [2012-03-28 Wed 15:06]
- State "DONE" [2012-03-28 Wed 15:06]

% bin/drmemory.exe -strict_bitops -batch -dr e:/src/dr/git/exports -- calc
% grep -A 25 ^SUPP ls -1td logs/D*|head -1/g*
SUPPRESSIONS USED:
3x: default issue #462 (write beyond TOS)
7x: default issue #497 (gdiplus bit manip)
1x: default issue #494 (custom data not all initialized)
3x: default issue #493 (bit manip)
8x: default issue #493 (bit manip)
8x: default issue #493 (bit manip)
12x: default issue #493 (bit manip)
1x: default issue #493 (bit manip)
30x: default issue #337 (real bug in RtlpLowFragHeapAllocFromContext)
1x (leaked 264 bytes): default issue #18 LockEntry::ThreadInit pool leak (nosyms win7)
2x (leaked 1584 bytes): default issue #286 (activation context leak)
1x (leaked 24 bytes): default issue #18 CreatePoolEntry mid-chunk linked list leak (nosyms win7)
1x (leaked 160 bytes): default issue #733 (nosyms)
1x (leaked 32 bytes): default issue #306 (critical section 8-byte-in pointer)
1x (leaked 32 bytes): default issue #306 (critical section 8-byte-in pointer)
ERRORS FOUND:
0 unique, 0 total unaddressable access(es)
4 unique, 4 total uninitialized access(es)

% bin/drmemory.exe -batch -dr e:/src/dr/git/exports -- calc
% grep -A 25 ^SUPP ls -1td logs/D*|head -1/g*
SUPPRESSIONS USED:
3x: default issue #462 (write beyond TOS)
1x: default issue #494 (custom data not all initialized)
30x: default issue #337 (real bug in RtlpLowFragHeapAllocFromContext)
1x (leaked 264 bytes): default issue #18 LockEntry::ThreadInit pool leak (nosyms win7)
2x (leaked 1584 bytes): default issue #286 (activation context leak)
1x (leaked 24 bytes): default issue #18 CreatePoolEntry mid-chunk linked list leak (nosyms win7)
1x (leaked 160 bytes): default issue #733 (nosyms)
1x (leaked 32 bytes): default issue #306 (critical section 8-byte-in pointer)
1x (leaked 32 bytes): default issue #306 (critical section 8-byte-in pointer)
ERRORS FOUND:
0 unique, 0 total unaddressable access(es)
4 unique, 4 total uninitialized access(es)

**** TODO put check for one side of OP_or being defined on fastpath?
do whole dword: go to slowpath for some bytes being defined
**** DONE add test
CLOSED: [2012-03-28 Wed 13:42]
- State "DONE" [2012-03-28 Wed 13:42]
bitfield.cpp, run w/ default ops and -strict_bitops
*** INFO commit log

fixes issue #849 + add option -strict_bitops, off by default, and when on
it matches today's behavior which has no false negatives

• when -strict_bitops is off, two relaxations are added
  to handle typical bitfield operations:
  • OP_or with a defined value (not enough to just allow non-0 value)
    used to set a bitfield var to a non-const-zero value
    XXX: could put this on fastpath for perf.
    XXX issue uninit from double xor bitfield set in MaximumLength field in LARGE_STRING write in NtUserCreateWindowEx and elsewhere #489: another sequence used to set is a double xor
    sequence which will require pattern matching.
  • OP_and with a constant that has only one sequence of 0's and at
    least a few 1's, used to set a bitfield var to zero
• these try to minimize false negatives. it's not clear how much
  normal code will end up with false negatives with these relaxations.
• added a test based on the V8 bitfields that is run with and
  without -strict_bitops
• -no_strict_bitops eliminates the need for suppressions for
  issue Task: run V8 tests under Dr. Memory #513, issue UNINITs in USP10.dll that likely all need per-bit granularity #493, issue UNINIT on calc in gdiplus.dll!DpRegion::FreeData: needs per-bit granularity #497, and some of issue Bit-level false positives in riched20.dll (rich edit) #791, and probably others that I
  did not have a chance to evaluate. my inclination is to leave in
  place the default suppressions for usp10.dll and others for
  cases where we run with -strict_bitops.

[derekbruening]

From [email protected] on March 28, 2012 15:04:53

This issue was closed by revision r812 .

Status: Fixed