<div dir="ltr">Yea. This is pretty silly. <div>Essentially, we are leaking modules_ in sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc</div><div>every time we call FindModuleForAddress after a dlopen/dlclose. </div><div>r233037 changes the code in coverage so that it does not depend on this leak, <br></div><div>but the leak itself should be fixed too. </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 23, 2015 at 8:36 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"><br><br><div class="gmail_quote"><div><div class="h5">On Fri, Mar 20, 2015 at 9:59 PM Kostya Serebryany <<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Mar 20, 2015 at 11:36 AM, Kostya Serebryany <span dir="ltr"><<a href="mailto:kcc@google.com" target="_blank">kcc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div>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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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></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><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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></div><div>Good catch, fix it! </div></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Probably not that bad. </div><div>At least the implementation in sanitizer_symbolizer_posix_libcdep.cc returns the same pointer for module name every time, </div><div>unless new modules were loaded. So, on linux everything should work fine.</div></div></div></div></blockquote><div><br></div></div></div><div>Depends on the app, I suppose.</div><div>If an app loads e.g. plugins, we leak ~(M*(N+M/2)) memory, where N is the number of modules loaded at startup and M is the number of modules loaded [one by one] dynamically at run-time.  This can be a lot [think Photoshop].</div><div>Maybe not that bad for Chromium.</div><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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></span><div>Hm. Maybe yes, maybe not. We don't support dlclose particularly well. </div></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>>> <span style="font-size:12.8000001907349px">Re: bug or feature question -- what's the expected result dump if we load/unload/load the same library?  I'd expect the counters/bits to be merged</span></div><div><span style="font-size:12.8000001907349px"><br></span></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><span style="font-size:12.8000001907349px">It would be nice to merge them, but at least if the library is loaded at different address we won't do that. </span></div><div><span style="font-size:12.8000001907349px">Again, we don't support dlclose very well, and thi sis not the only trouble with it. </span></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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></span><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><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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></span><div>Fix all of these, one by one! </div><span><font color="#888888"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div>--</div><div>Timur</div><div><br></div></div></div>
</blockquote></font></span></div><br></div></div>
</blockquote></div></div></div></blockquote></div></div></div></div>
</blockquote></div><br></div>