<div dir="ltr">I just started looking at how to do this (having a separate mutex for the description) and I think I found another bug. Or maybe I'm missing some assumption.<div><br></div><div>On one hand, Module::SetArchitecture won't assign the new value if m_arch is already valid, just return m_arch.IsCompatibleWith(new_arch). On the other hand, in Module::MergeArchitecture we have both code that assumes that SetArchitecture replaces the previous value:<div><br></div><div><div style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(0,0,0);font-family:monospace;font-size:medium;white-space:pre"> <span class="gmail-cm-keyword" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(119,0,136)">if</span> (!<a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/include/lldb/Core/Module.h;rcl=355613052;l=939" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">m_arch</a>.<a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/source/Utility/ArchSpec.cpp;rcl=355613052;l=934" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none"><span class="gmail-search-match-layer gmail-search-match gmail-match-105" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);background-color:rgb(206,234,214);font-weight:700;line-height:0">IsCompatibleMatch</span></a>(<a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/source/Core/Module.cpp;l=1620?q=IsCompatibleMatch%20lldb&ct=os&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">arch_spec</a>)) {<br></div><div style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(0,0,0);font-family:monospace;font-size:medium;white-space:pre"> <span class="gmail-cm-comment" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(136,0,0)">// The new architecture is different, we just need to replace it.
</span></div><div style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(0,0,0);font-family:monospace;font-size:medium;white-space:pre"><span class="gmail-cm-comment" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(136,0,0)"></span> <span class="gmail-cm-keyword" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(119,0,136)">return</span> <a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/source/Core/Module.cpp;l=1544?q=IsCompatibleMatch%20lldb&ct=os&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">SetArchitecture</a>(<a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/source/Core/Module.cpp;l=1620?q=IsCompatibleMatch%20lldb&ct=os&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">arch_spec</a>);
</div><div style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(0,0,0);font-family:monospace;font-size:medium;white-space:pre"> }</div></div><div><br></div><div>and right after it we have code that works around the fact that SetArchitecture doesn't replace the previous value sometimes</div><div><br></div><div><div style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(0,0,0);font-family:monospace;font-size:medium;white-space:pre"> <span class="gmail-cm-comment" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(136,0,0)">// Merge bits from arch_spec into "merged_arch" and set our architecture.
</span></div><div style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(0,0,0);font-family:monospace;font-size:medium;white-space:pre"><span class="gmail-cm-comment" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(136,0,0)"></span> <a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/include/lldb/Utility/ArchSpec.h;rcl=355613052;l=33" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">ArchSpec</a> <a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/source/Core/Module.cpp;bpv=1;bpt=1;l=1633?q=IsCompatibleMatch%20lldb&ct=os&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental&gsn=merged_arch&gs=kythe%3A%2F%2Fgoogle3%3Flang%3Dc%252B%252B%3Fpath%3Dthird_party%2Fllvm%2Fllvm-project%2Flldb%2Fsource%2FCore%2FModule.cpp%23f1bDLAwnC-gD__Xt6Bs-4h5T-ePUhWBq8L6dQxa8QEA" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">merged_arch</a>(<a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/include/lldb/Core/Module.h;rcl=355613052;l=939" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">m_arch</a>);
</div><div style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(0,0,0);font-family:monospace;font-size:medium;white-space:pre"> <a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/source/Core/Module.cpp;l=1633?q=IsCompatibleMatch%20lldb&ct=os&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">merged_arch</a>.<a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/source/Utility/ArchSpec.cpp;rcl=355613052;l=801" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">MergeFrom</a>(<a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/source/Core/Module.cpp;l=1620?q=IsCompatibleMatch%20lldb&ct=os&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">arch_spec</a>);
</div><div style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(0,0,0);font-family:monospace;font-size:medium;white-space:pre"> <span class="gmail-cm-comment" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(136,0,0)">// SetArchitecture() is a no-op if m_arch is already valid.
</span></div><div style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(0,0,0);font-family:monospace;font-size:medium;white-space:pre"><span class="gmail-cm-comment" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(136,0,0)"></span> <a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/include/lldb/Core/Module.h;rcl=355613052;l=939" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">m_arch</a> <a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/include/lldb/Utility/ArchSpec.h;rcl=355613052;l=33" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">=</a> <a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/source/Utility/ArchSpec.cpp;rcl=355613052;l=510" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">ArchSpec</a>();
</div><div style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(0,0,0);font-family:monospace;font-size:medium;white-space:pre"> <span class="gmail-cm-keyword" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:rgb(119,0,136)">return</span> <a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/source/Core/Module.cpp;l=1544?q=IsCompatibleMatch%20lldb&ct=os&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">SetArchitecture</a>(<a class="gmail-semantic-decoration" href="https://source.corp.google.com/piper///depot/google3/third_party/llvm/llvm-project/lldb/source/Core/Module.cpp;l=1633?q=IsCompatibleMatch%20lldb&ct=os&sq=package:piper%20file:%2F%2Fdepot%2Fgoogle3%20-file:google3%2Fexperimental" style="margin:0px;padding:0px;box-sizing:border-box;border-color:rgba(0,0,0,0.12);color:inherit;text-decoration-line:none">merged_arch</a>);</div></div><div><br></div><div>My guess is that the first of the snippets above has a bug (it intends to replace the architecture but doesn't) and that replacing a module's architecture with an incompatible one is something that doesn't happen often and the bug went unnoticed. Does this make sense?</div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Feb 4, 2021 at 5:38 PM Raphael Isemann <<a href="mailto:teemperor@gmail.com">teemperor@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">We could also just give the Module a std::string with the description<br>
and update it in the few places where we actually update it. The<br>
m_arch already has a setter in place that just needs to be used in a<br>
few more places, so the infrastructure is kind of already there (at<br>
least for m_arch). The description would just have its own mutex.<br>
<br>
<br>
Am Fr., 5. Feb. 2021 um 00:39 Uhr schrieb Jorge Gorbe Moya via<br>
lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a>>:<br>
><br>
> Wouldn't it be preferable to try_lock in GetDescription (which is the one currently acquiring the mutex) instead? ReportError doesn't touch any mutex itself and will happily report the rest of the error if GetDescription bails out. For the test case I sent it would look like this:<br>
><br>
> error: {0x0000000b}: invalid abbreviation code 123, please file a bug and attach the file at the start of this error message<br>
> error: {0x0000000b}: invalid abbreviation code 123, please file a bug and attach the file at the start of this error message<br>
> error: {0x0000000b}: invalid abbreviation code 123, please file a bug and attach the file at the start of this error message<br>
><br>
> which is way better than a deadlock IMO.<br>
><br>
><br>
><br>
><br>
> On Thu, Feb 4, 2021 at 12:16 PM Pavel Labath <<a href="mailto:pavel@labath.sk" target="_blank">pavel@labath.sk</a>> wrote:<br>
>><br>
>> Please have a look at<br>
>> <<a href="https://lists.llvm.org/pipermail/lldb-dev/2020-October/016479.html" rel="noreferrer" target="_blank">https://lists.llvm.org/pipermail/lldb-dev/2020-October/016479.html</a>>,<br>
>> which is the last time this came up.<br>
>><br>
>><br>
>> One quick'n'dirty solution would be to have `Module::ReportError` _try_<br>
>> to get the module lock, and if it fails, just bail out. That obviously<br>
>> means you won't get to see the error message which triggerred the<br>
>> deadlock (though we could also play around with that and try printing<br>
>> the error message without the module description or something), but it<br>
>> will at least get you past that point...<br>
>><br>
>> pl<br>
>><br>
>> On 04/02/2021 21:04, Jorge Gorbe Moya via lldb-dev wrote:<br>
>> > Hi,<br>
>> ><br>
>> > I've found a deadlock in lldb (see attached test case, you can build it<br>
>> > with just `clang -o test test.s`), but I'm a total newbie and I have no<br>
>> > idea what's the right way to fix it.<br>
>> ><br>
>> > The problem happens when an error is found during DIE extraction when<br>
>> > preloading symbols. As far as I can tell, it goes like this:<br>
>> ><br>
>> > 1. Module::PreloadSymbols locks Module::m_mutex<br>
>> > 2. A few layers below it, we end up in ManualDWARFIndex::Index, which<br>
>> > dispatches DIE extractions to a thread pool:<br>
>> ><br>
>> > |for (size_t i = 0; i < units_to_index.size(); ++i)<br>
>> > pool.async(extract_fn, i); pool.wait(); |<br>
>> ><br>
>> > 3. extract_fn in the snippet above ends up executing<br>
>> > DWARFDebugInfoEntry::Extract and when there's an error during<br>
>> > extraction, Module::GetDescription is called while generating the error<br>
>> > message.<br>
>> > 4. Module::GetDescription tries to acquire Module::m_mutex from a<br>
>> > different thread, while the main thread has the mutex already locked and<br>
>> > it's waiting for DIE extraction to end, causing a deadlock.<br>
>> ><br>
>> > If we make Module::GetDescription not lock the problem disappears, so<br>
>> > the diagnosis looks correct, but I don't know what would be the right<br>
>> > way to fix it. Module::GetDescription looks more or less safe to call<br>
>> > without locking: it just prints m_arch, m_file, and m_object_name to a<br>
>> > string, and those look like fields that wouldn't change after the Module<br>
>> > is initialized, so maybe it's okay? But I feel like there must be a<br>
>> > better solution anyway. Any advice?<br>
>> ><br>
>> > Best,<br>
>> > Jorge<br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> ><br>
>> > _______________________________________________<br>
>> > lldb-dev mailing list<br>
>> > <a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a><br>
>> > <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a><br>
>> ><br>
>><br>
> _______________________________________________<br>
> lldb-dev mailing list<br>
> <a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a><br>
</blockquote></div>