<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Mar 20, 2015 at 11:22 AM, Timur Iskhodzhanov <span dir="ltr"><<a href="mailto:timurrrr@google.com" target="_blank">timurrrr@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Hi,</div><div>I'm working on enabling sanitizer coverage support for ASan on Windows.</div><div><br></div><div>I got a PoC working, but didn't like how the code fits one aspect of the architecture and wanted to discuss how to fix that.</div><div><br></div><br><div>Let's start with CoverageData::DumpOffsets().</div><div>lib/sanitizer_common/sanitizer_coverage_libcdep.cc:<br></div><div><div><div><div> 733 void CoverageData::DumpOffsets() {</div><div> ...</div><div> 739 for (uptr m = 0; m < module_name_vec.size(); m++) {</div><div> ...</div><div> 744 auto r = module_name_vec[m];</div><div> ...</div><div> 748 const char *module_name = "<unknown>";</div><div> 749 for (uptr i = r.beg; i < r.end; i++) {</div><div> ...</div><div> 753 uptr offset = 0;</div><div> 754 sym->GetModuleNameAndOffsetForPC(pc, &module_name, &offset);</div><div> 755 offsets.push_back(BundlePcAndCounter(offset, counter));</div><div> 756 } </div><div> ...</div><div> 770 module_name = StripModuleName(<a href="http://r.name" target="_blank">r.name</a>);</div></div></div></div><div><br></div><div>Looking at line 770, I think we don't need to query GetModuleNameAndOffsetForPC for the module name.</div><div>Looking at lines 754/755, why don't we just store the module boundaries in module_name_vec along with its name?</div><div>That'd save us one extra GetModuleNameAndOffsetForPC call [a linear lookup on the *fast* path].</div><div>We already know what module this PC belongs to, right?</div></div></blockquote><div><br></div><div>Yes, that may work. (you never know before you test it). </div><div>The code is like this because it got refactored from another state where the PCs from different modules were intermixed. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div><div><br></div><div>Now let's look at CoverageData::<u></u>UpdateModuleNameVec(...).</div><div>lib/sanitizer_common/sanitizer_coverage_libcdep.cc:<br></div><div><div> 333 void CoverageData::UpdateModuleNameVec(uptr caller_pc, uptr range_beg,</div><div> 334 uptr range_end) {</div><div> ...</div><div> 338 const char *module_name = sym->GetModuleNameForPc(caller_pc);</div><div> 339 if (!module_name) return;</div><div> 340 if (module_name_vec.empty() || module_name_vec.back().name != module_name)</div><div> 341 module_name_vec.push_back({module_name, range_beg, range_end});</div><div> ...</div><div> 344 }</div></div><div><br></div><div>GetModuleNameForPc(pc) returns const char* and we just store it in a vector? Intriguing.</div><div>Module name strings compared as pointers? Even more intriguing. [see "side note" below"]</div><div><br></div><div><div>lib/sanitizer_common/sanitizer_symbolizer.h:</div><div><div> 89 const char *GetModuleNameForPc(uptr pc) {</div><div> 90 const char *module_name = 0;</div><div> 91 uptr unused;</div><div> 92 if (GetModuleNameAndOffsetForPC(pc, &module_name, &unused))</div><div> 93 return module_name;</div></div><div><br></div><div>GetModuleNameAndOffsetForPC forwards to PlatformFindModuleNameAndOffsetForAddress, which on POSIX is:<br></div><div>lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc<br></div><div><div> 397 bool PlatformFindModuleNameAndOffsetForAddress(uptr address,</div><div> 398 const char **module_name,</div><div> 399 uptr *module_offset) override {</div><div> 400 LoadedModule *module = FindModuleForAddress(address);</div><div> 401 if (module == 0)</div><div> 402 return false;</div><div> 403 *module_name = module->full_name();</div><div> 404 *module_offset = address - module->base_address();</div><div> 405 return true;</div><div> 406 }</div></div><div>...</div><div><div> 368 LoadedModule *FindModuleForAddress(uptr address) {</div><div> 369 bool modules_were_reloaded = false;</div><div> 370 if (modules_ == 0 || !modules_fresh_) {</div><div> 371 modules_ = (LoadedModule*)(symbolizer_allocator_.Allocate(</div><div> 372 kMaxNumberOfModuleContexts * sizeof(LoadedModule)));</div><div>...</div><div> 374 n_modules_ = GetListOfModules(modules_, kMaxNumberOfModuleContexts,</div><div> 375 /* filter */ 0);</div><div>...</div><div> 378 modules_fresh_ = true;</div><div> 379 modules_were_reloaded = true;</div><div> 380 }</div><div> 381 for (uptr i = 0; i < n_modules_; i++) {</div><div> 382 if (modules_[i].containsAddress(address)) {</div><div> 383 return &modules_[i];</div><div> 384 }</div><div> 385 }</div><div> 386 // Reload the modules and look up again, if we haven't tried it yet.</div><div> 387 if (!modules_were_reloaded) {</div><div> 388 // FIXME: set modules_fresh_ from dlopen()/dlclose() interceptors.</div><div> 389 // It's too aggressive to reload the list of modules each time we fail</div><div> 390 // to find a module for a given address.</div><div> 391 modules_fresh_ = false;</div><div> 392 return FindModuleForAddress(address);</div><div> 393 }</div><div> 394 return 0;</div><div> 395 }</div></div><div><br></div><div>We seem to leak 80...160KB every time a new module is loaded [calls UpdateModuleNameVec].<br></div></div></div></blockquote><div><br></div><div>Good catch, fix it! </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div></div><div>Who owns the module name strings stored in module_name_vec?</div><div>OK, the LoadedModule strdup's a module name, so we also leak all the module names when a new module is loaded.</div><div>I'm not sure linear search is the right thing to do, though I'm OK with it if this function does not show in the profile.</div><div><br></div><div>Side note: It looks possible that dlopen+dlclosing a module would cause the same module to have two equal module names at different addresses, hence it will appear twice in module_name_vec -- is it a bug or a feature?</div></div></div></blockquote><div><br></div><div>Hm. Maybe yes, maybe not. We don't support dlclose particularly well. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><br></div><div><br></div><div>Let's now consider that I have to re-implement PlatformFindModuleNameAndOffsetForAddress for Windows.</div><div>I don't think I want to implement it the same way it's done for POSIX, as I find that implementation suboptimal.</div><div><br></div><div>It seems the usage pattern of PlatformFindModuleNameAndOffsetForAddress is to query batches of PCs from one module before going to the next one.</div><div>Assuming I can observe FreeLibrary [or dlclose], I can just cache the module info [name+boundaries] for the last lookup without the need to re-query the list of modules.</div><div>I think the same can be done on POSIX?</div><div>Maybe we should use GetModuleByPC(PC), store the result in module_for_the_last_pc_ and check if a new PC is there? </div><div><br></div><div>I think we should also change the way we own module names.</div><div>Can we store them in a string-set (do we have one?) </div></div></div></blockquote><div><br></div><div>we don't, but we can use an array :) </div><div>(sorted or not, depends on how often we access vs modify it)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>to make sure they are unique'd and pointer comparison is OK?</div><div>Currently there's no owner for module names on Windows, as we haven't needed to store LoadedModules before.</div><div>Querying most of the things was as simple as</div><div> char xyz[...];</div><div> GetWhatINeed(xyz, sizeof(xyz));</div><div>so the "owner" of the string was stack.</div><div>Doing a strdup on every call is impractical :)</div><div><br></div><div><br></div><div>Ideas?</div></div></div></blockquote><div><br></div><div>Fix all of these, one by one! </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>--</div><div>Timur</div><div><br></div></div></div>
</blockquote></div><br></div></div>