propagate shadow values through floating-point registers #471
Comments
|
From bruen...@google.com on June 22, 2011 08:49:49 This was a design decision, so this is more a feature request and not a bug. Summary: propagate shadow values through floating-point registers |
|
From timurrrr@google.com on June 22, 2011 11:01:04 You're reporting an uninit when anything uninit goes to FPU, right? What do you think about marking the whole FPU register as uninit if any bit of the input is uninit |
|
From bruen...@google.com on June 22, 2011 11:12:56 could use reduced granularity for fp and multimedia regs, yes. |
|
From rnk@google.com on March 21, 2012 08:30:21 We hit a similar issue in http://crbug.com/115326 and again in a copy ctor for FormDataElement in a recent full run: http://build.chromium.org/p/chromium.fyi/builders/Windows%20Tests%20%28DrMemory%20full%29/builds/1136 The reports:
This is one case where it would be nice to know what the overloaded param types are, since it's not obvious that this is the copy ctor. Looking at all the ctors shows that it's in the copy ctor PC range: 0:000> x unit_tests!WebCore::FormDataElement::FormDataElement The assembly for it: 0:000> uf 0438bf60 The key being these instructions: Just propagating the bits through registers would fix this. |
|
From rnk@google.com on December 12, 2012 12:49:16 I hit another instance of this in layout tests. Here's the full report: UNINITIALIZED READ: reading 0x0018ea44-0x0018ea4c 8 byte(s) 0 webkit.dll!WebCore::CachedResourceLoader::InitiatorInfo::InitiatorInfo +0x19 (0x11468379 <webkit.dll+0x1468379>)1 webkit.dll!WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo>::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo>+0x1f [d:\src\src\third_party\webkit\source\wtf\wtf\hashtraits.h:200](0x11468320 <webkit.dll+0x1468320)2 webkit.dll!WTF::KeyValuePairHashTraits<WTF::HashTraits<WebCore::CachedResource *>,WTF::HashTraitsWebCore::CachedResourceLoader::InitiatorInfo>::emptyValue+0x5f [d:\src\src\third_party\webkit\source\wtf\wtf\hashtraits.h:222](0x11467f80 <webkit.dll+0x1467f80)3 webkit.dll!WTF::HashTableBucketInitializer<0>::initialize<WTF::HashMapValueTraits<WTF::HashTraits<WebCore::CachedResource *>,WTF::HashTraitsWebCore::CachedResourceLoader::InitiatorInfo >,WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::In+0x27 [d:\src\src\third_party\webkit\source\wtf\wtf\hashtable.h:787] (0x11466ea8 <webkit.dll+0x1466ea8>)4 webkit.dll!WTF::HashTable<WebCore::CachedResource *,WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo>,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo> >,WT+0xb [d:\src\src\third_party\webkit\source\wtf\wtf\hashtable.h:804](0x114659fc <webkit.dll+0x14659fc)5 webkit.dll!WTF::HashTable<WebCore::CachedResource *,WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo>,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo> >,WT+0x55 [d:\src\src\third_party\webkit\source\wtf\wtf\hashtable.h:1079](0x11468636 <webkit.dll+0x1468636)6 webkit.dll!WTF::HashTable<WebCore::CachedResource *,WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo>,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo> >,WT+0x3c [d:\src\src\third_party\webkit\source\wtf\wtf\hashtable.h:1129](0x11466ddd <webkit.dll+0x1466ddd)7 webkit.dll!WTF::HashTable<WebCore::CachedResource *,WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo>,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo> >,WT+0x1a [d:\src\src\third_party\webkit\source\wtf\wtf\hashtable.h:456](0x114669eb <webkit.dll+0x14669eb)8 webkit.dll!WTF::HashTable<WebCore::CachedResource *,WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo>,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo> >,WT+0x47 [d:\src\src\third_party\webkit\source\wtf\wtf\hashtable.h:1032](0x11465958 <webkit.dll+0x1465958)9 webkit.dll!WTF::HashTable<WebCore::CachedResource *,WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo>,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo> >,WT+0x1a [d:\src\src\third_party\webkit\source\wtf\wtf\hashtable.h:1006](0x11463ceb <webkit.dll+0x1463ceb)#10 webkit.dll!WTF::HashTable<WebCore::CachedResource *,WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo>,WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::CachedResource *,WebCore::CachedResourceLoader::InitiatorInfo> >,WT+0x87 [d:\src\src\third_party\webkit\source\wtf\wtf\hashtable.h:1052](0x11461a08 <webkit.dll+0x1461a08) What's happening in this case is someone is using a hashmap or InitiatorInfo structs, which has a double field. So it looks like some part of the hashtable implementation is asking for emptyValue of the key-value pair type, which is just a default initialized struct. The default empty ctor leaves it uninitialized. Then it calls the copy constructor on it, where we report the bug. Suppressing this in chrome. |
|
From bruen...@google.com on May 20, 2013 13:15:19 xref issue #1188 where it looks like this is causing tons of false pos on spec2k6 wrf xref issue #931 Labels: Bug-FalsePositive |
|
From bruen...@google.com on December 18, 2013 22:19:44 Another instance: http://build.chromium.org/p/chromium.fyi/builders/Windows%20Unit%20%28DrMemory%20full%29%20%282%29/builds/1084/steps/memory%20test%3A%20unit/logs/stdio [ RUN ] FieldMapWrapperTest.BothShippingAndBillingCanCoexist 0213d390 unit_tests!autofill::DetailInput::DetailInput (struct autofill::DetailInput *) We have a default copy constructor copying from a TEST(FieldMapWrapperTest, BothShippingAndBillingCanCoexist) { DetailInput billing_street; 172 0213c451 8d8d74ffffff lea ecx,[ebp-8Ch] The test just uses the "type" field and leaves the other fields, including a float, alone. These constructors are not initializing the floating-point field, but nothing |
|
From bruen...@google.com on December 19, 2013 08:41:02 Just to add a little more clarity to the last post: the code is initializing and copying a struct that has a float field. The default constructor leaves the field alone but the copy constructor copies it with this familiar pattern: 0213d3bf d9410c fld dword ptr [ecx+0Ch] Until we have full shadowing and propagation, perhaps we can add a heuristic to handle copy constructors that looks for "fld;fst"? |
|
From bruen...@google.com on January 15, 2014 08:14:29 The short-term plan is to add a heuristic that, on an uninit in an fld, if there's a subsequent fstp, does a slowpath propagation. We considered trying to work that into the fastpath, but that gets complex (as it also has to support the slowpath, and so can't rely on just IR notes), and we don't support 8-byte mem2mem propagation in any case -- so we're going with slowpath handling of this case. Long-term we'll implement the actual fp reg shadowing. Here are some thoughts: ** TODO could use reduced granularity for fp and multimedia regs For fp, 2 bits per each of 8 st* registers => only 2 bytes needed. For multimedia regs, however, it's a very straightforward 1-to-1 mapping, ** TODO how track physical fp registers vs stN stack? If an instruction references stN, we need to map it to a physical register. Can read TOP field (bits 11..13: see Figure 8-4) of x87 FPU status word to Can get status word into %ax (or into memory) via OP_fnstsw. fnstw ax Should be able to ignore stack pops (i.e., treat OP_fstp as OP_fst). Every bb w/ fp instrs should try to use eax as global spill (or could do fnstw to TLS). Labels: -Priority-Low Priority-Medium |
|
From bruen...@google.com on January 24, 2014 07:15:34 The heuristic (under internal option -fld_fstp_prop) is in r1679 and seems to work well: it addresses all the copy constructor false positives in Chromium as well as fixing issue #931 . |
|
From bruen...@google.com on January 28, 2014 07:57:46 Chromium Release unit_tests doesn't match the heuristic: [ RUN ] FieldMapWrapperTest.BothShippingAndBillingCanCoexist There's an instr in between: 01f9fa49 d94724 fld dword ptr [edi+24h] W/ new scheme of marking dst mem as bitlevel, seems like we can |
|
From bruen...@google.com on January 29, 2014 09:21:32 ** TODO chrome issue #471a suppression: not a copy constructor! This one has no additional information: UNINITIALIZED READ It's not being used on today's media_ bot (nor on any others: I checked Most likely it's similar to Timur's minimal reproducer (what he's calling TEST(FloatTests, Multiply) { Which does result in a report (which is why I did not keep it in float_tests.cpp):
Its code looks like this: TAG 0x00fcb6d0 Recent build, targeting the subtests that likely had this code: % ~/drmemory/git/build_x86_dbg/bin/drmemory.exe -dr d:/derek/dr/git/exports -batch -- ./media_unittests.exe --gtest_filter=AudioRendererAlgorithmTest.* --single-process-tests => no uninits at all. ** TODO chrome Release: ipc!IPC::ParamTraits::Write has 2 instrs in between fld+fstp Running Release full-mode browser_tests hits a [ RUN ] IFrameTest.InEmptyFrame Error 0 ipc.dll!IPC::ParamTraits<>::Write [ipc\ipc_message_utils.h:220]1 content.dll!IPC::ParamTraits<>::Write [content\public\common\common_param_traits_macros.h:206]Note: @0:00:38.752 in thread 14004 0:000> Uf 10002820 static void Write(Message* m, const param_type& p) { Goes through Pickle::WriteFloat which ends up at Pickle::WriteBytesCommon() ** TODO webkit!WebCore::RenderBlock::computeInlinePreferredLogicalWidths needs full fp prop Debug browser_tests: [ RUN ] IdentityInternalsSingleTokenWebUITest.getAllTokens Error 0 webkit.dll!WebCore::RenderBlock::computeInlinePreferredLogicalWidths [third_party\webkit\source\core\rendering\renderblock.cpp:4196]1 webkit.dll!WebCore::RenderBlock::computeIntrinsicLogicalWidths [third_party\webkit\source\core\rendering\renderblock.cpp:3757]Note: instruction: fld 0xffffff44(%ebp) -> %st0 It needs full issue #471: |
From timurrrr@google.com on June 22, 2011 11:39:27
AFAIK, Dr. Memory issues a report when any uninit data goes into an FPU register.
Today I've found that results in false positive reports on a few Chromium media_unittests.
What they do in those tests is:
create X bytes of random garbage -> process -> make sure the output size is still X bytes and nothing crashed.
Some call it Garbage-In-Garbage-Out :)
Here's a short repro.
It is Valgrind-clean but Dr. Memory report an uninit on the "fild" instruction (at line 3)
int main() {
int *value = new int;
*value = *value * 0.33;
delete value;
return 0;
}
Original issue: http://code.google.com/p/drmemory/issues/detail?id=471
The text was updated successfully, but these errors were encountered: