In D12968#259931, @clayborg wrote:

This can't be the real fix for this. We must be able to trust the empty() function call. We must have someone playing with the collection without locking the mutex. Please find the real bug.

In D12968#261762, @labath wrote:

I have committed D13056 with the event hijacking portion from your patch. I think your use case should be working now. I am planning to return to this later, as I believe there are still some edge cases lurking here.

Regarding how to reproduce - this problem looks remote target specific only - in my case gdb-remote modules involved.

In D12968#256563, @labath wrote:

This is definitely not a proper fix for this problem, it merely sweeps the problem under the carpet. If something like this makes a difference then it means something has gone horribly wrong a long time ago. I haven't been able to reproduce this locally, but I'd recommend trying to set up some memory-allocation/thread-race checker and see what it produces.

I still think the original solution of hijacking the listener was correct, but we need to figure out what is the right point at which to hijack it. I'll try to look into this when I get a bit of time.

Test has been marked as XFAILed for Linux (x86_64) case.

I realized what m_events.empty check in Listener::FindNextEventInternal() method returned event collection is NOT empty - probably because list.empty() just compare begin != end pointers, but pointers may be absolutely invalid.

m_lldbResult usage has beed removed in ExecRun::Acknowledge() method, corresponding member removed from cmd class.

In D12977#255390, @ki.stfu wrote:

lgtm apart a lack of description about the changes in tools/lldb-mi/MICmdCmdSupportList.cpp

Requested changes applied, updated patch uploaded.

Is this patch is fine for you now ?

"CLI" intepreter not used in ExecRun handler in reworked patch.

Patch reworked for suggested "-exec-run --start" manner, no "CLI" interpreter use and check if --start option supported via -list-features.

After more deep investigation I think setting listener for hijacking also looks like workaround. Setting additional listener just change code flow path and buggy path not executed, thus no crash. At top level - crash appeared in Process::m_listener involved - as no hijacked listener was set in Destroy().

As little summary what should be added to existing lldb-mi in context of this review:

In D12968#251719, @labath wrote:

In D12968#251696, @KLapshin wrote:

Regarding matter of this patch - I just tried to make process stop synchronous, see Process::ResumeSynchronous() - same technique was applied months ago (@ki.stfu).

Agreed what crash may be related to freed memory reuse, but not investigated it yet in deep.

Fair enough, your patch definitely makes things better. I'm just trying to understand the root cause so we can fix this definitively.

If you look at ResumeSynchronous, you see that the hijacking happens there before we attempt to resume the process (PrivateResume()). This makes it race-free because we grab the events before it gets a chance to generate any. In your case, things are a bit more complicated, since the process is already running and can come to a stop at any moment, and we need to make sure we don't miss those events.

Do you have the ability to run lldb-mi under valgrind or msan? If you can, I'd be interested in taking a look at the output they produce in this case. Or if you have a simple repro case, I can try to do it myself.

pl

Done.

I updated patch against actual source in SVN - i.e. with taking into account your change (Halt*->Stop*).

Due to @labath reworked and replaced HaltForDestroyOrDetach to StopHaltForDestroyOrDetach method (see http://reviews.llvm.org/D13056) and his patch already approved by @clayborg and crash still reproducible with just race condition fix patch from @labath I suggest to apply this patch first, then patch for race condition - merge should be fine.

@clayborg, I reworked initial workaround solution, now this is exact fix.

See http://reviews.llvm.org/D12968 also - fix for missed hijacked listener set in Process::HaltForDestroyOrDetach()

In D12977#249437, @enlight wrote:

According to the GDB-MI spec the exec-run command already has a start option. Support for the start option can be detected by checking for exec-run-start-option in the list of features returned by the list-features command. So, what's the rationale for diverging from the spec in this case?

In GDB your example would be written as:
-exec-run --start

Minor initial diff cleanup.

Patch recalled due to concept was not accepted as good enough to apply.

Hello Greg,

In D7302#116115, @clayborg wrote:

I really can't take the memory hit that is incurred by adding anything to the Mangled class. lldb_private::Symbols are one of the largest sources of memory consumption by a large debug session in LLDB and adding and extra 64 bits (32 for the enum and 1 byte for the bool and then padding) to each one it too much. Can we make this change in a way that doesn't require the extra storage?

Can't we tell what is mangled by looking at the prefix in a Mangled instance? If not, you will need to make a MangleWithLanguage instance for places the language needs to be used if it can't be deduced by the mangling prefix. The "m_language" is not really used inside the the Mangled class, so it isn't required for anything inside of the Mangled class.

You should also not have to modify the DWARF parser at all, you can just get the lldb_private::CompileUnit from the DWARF compile unit and ask the lldb_private::CompileUnit for its language (lldb_private::CompileUnit::GetLanguage()).

In D7302#116129, @clayborg wrote:

To make my comments clearer:

1 - Can't we tell the language by looking at the Mangled prefix? "_Z" = C++, "$" = C++?
2 - Where do we need to know the language? I the JIT? If so we might need a new MangledWithLanguage class and use it only where we need it so it doesn't make lldb_private::Symbol and lldb_private::Function and lldb_private::Block's inlined info take up more space. We spent a lot of time doing memory analysis on LLDB and lldb_private::Symbol objects were consistently at the top of the heap.

Changes requested in this review were taken into account in reworked patch - see http://reviews.llvm.org/D7409.