<div dir="ltr">The problems I have with current directory lookup are:<div><br></div><div>* It makes the behavior dependent on the environment, much like using an environment variable.  This is a potential source of flakiness in tests, or different behavior on different peoples' machines.</div><div>* It doesn't match WinDbg or MSVC</div><div>* It's temporary functionality, and temporary functionality more often than not ends up not being so temporary, thereby contributing to technical debt.</div><div>* We already know what the permanent solution is, and we're going to have to implement it anyway, so we could avoid this by just implementing the permanent solution first.</div><div><br></div><div>Anyway, I don't want to hold this up any longer, but in the future let's just implement the permanent solutions first so that we can avoid this type of unnecessary blockage.</div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 13, 2018 at 1:10 PM Leonard Mosescu <<a href="mailto:mosescu@google.com">mosescu@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I think we can fix that by changing the line to:<div><br></div><div>if (!object_file || object_file->GetFileSpec() == symbol_fspec) {<br></div><div>}</div></blockquote><div><br></div></div></div><div dir="ltr"><div dir="ltr"><div>The problem is that SymbolFileNativePDB returns the module object file as it's own object file. Are you suggesting we should track the module object file separate from SymbolFile::m_obj_file? In a way that would be a more accurate representation of what's going on (we don't have an ObjectFilePDB yet) - but the check is still a bit nonsensical (using the lack of an object file to indicate that the operation succeeded)</div><div><br></div><div>I agree that the case that Pavel pointed out is an issue, although the non-nonsensical response only happens in the unlikely case you already have symbols (so there's little reason to explicitly re-add symbols). It's also not happening with PDBs (which is why it seemed safe).</div><div><br></div><div>At this point I have to ask: what is the problem with the current directory lookup again? I seems to me that we complicated ourselves for no good reason (using a questionable functionality in a high level Windows IDE as reference).</div><div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 13, 2018 at 12:23 PM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I think we can fix that by changing the line to:<div><br></div><div>```</div><div>if (!object_file || object_file->GetFileSpec() == symbol_fspec) {</div><div>}</div><div>```</div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 13, 2018 at 12:04 PM Pavel Labath <<a href="mailto:pavel@labath.sk" target="_blank">pavel@labath.sk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 13/12/2018 19:32, Leonard Mosescu wrote:<br>
> What's the consensus?<br>
> <br>
> Personally I think that, even considering the potential issue that Paval <br>
> pointed out, the "target symbols add ..." is the most conservative <br>
> approach in terms of introducing new behavior. I'm fine with the current <br>
> directory lookup as well (the original change) since it's consistent <br>
> with DWARF lookup.<br>
<br>
<br>
Yes, but it also regresses existing functionality. Now if I do something <br>
completely nonsensical like:<br>
(lldb) target create "/bin/ls"<br>
Current executable set to '/bin/ls' (x86_64).<br>
(lldb) target symbols add  -s /bin/ls /tmp/a.txt<br>
error: symbol file '/tmp/a.txt' does not match any existing module<br>
<br>
lldb will print a nice error for me. If I remove the safeguards like you <br>
did in your patch, it turns into this:<br>
(lldb) target create "/bin/ls"<br>
Current executable set to '/bin/ls' (x86_64).<br>
(lldb) target symbols add  -s /bin/ls /tmp/a.txt<br>
symbol file '/tmp/a.txt' has been added to '/bin/ls'<br>
<br>
which is a blatant lie, because /bin/ls will continue to use symbols <br>
from the object file.<br>
</blockquote></div>
</blockquote></div>
</blockquote></div>