Page MenuHomePhabricator

KLapshin (Kirill Lapshin)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 4 2015, 5:34 AM (250 w, 13 h)

Recent Activity

Oct 16 2015

KLapshin updated subscribers of D9782: Only check for _ZN function prefix in Linux and FreeBSD targets in SymbolFileDWARF.
Oct 16 2015, 7:28 AM
KLapshin updated subscribers of D9754: Enable workaround for finding functions in global namespace on linux binaries on all hosts..
Oct 16 2015, 7:27 AM

Oct 7 2015

KLapshin added a comment to D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.

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.

Oct 7 2015, 11:52 AM
KLapshin added a comment to D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.

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.

Oct 7 2015, 11:51 AM

Oct 1 2015

KLapshin added a comment to D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.

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

Oct 1 2015, 11:41 AM

Sep 30 2015

KLapshin added a comment to D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.

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.

Sep 30 2015, 10:55 AM
KLapshin added inline comments to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.
Sep 30 2015, 10:50 AM
KLapshin added a comment to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.
Sep 30 2015, 10:45 AM
KLapshin updated the diff for D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.

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

Sep 30 2015, 10:44 AM
KLapshin added inline comments to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.
Sep 30 2015, 10:38 AM

Sep 29 2015

KLapshin added inline comments to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.
Sep 29 2015, 11:29 AM
KLapshin updated the diff for D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.

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.

Sep 29 2015, 11:05 AM
KLapshin added a comment to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.

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

Sep 29 2015, 7:36 AM
KLapshin added a comment to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.

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

Sep 29 2015, 7:34 AM
KLapshin updated the diff for D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.
Sep 29 2015, 7:26 AM

Sep 28 2015

KLapshin added a comment to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.

Requested changes applied, updated patch uploaded.

Sep 28 2015, 7:13 AM
KLapshin updated the diff for D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.
Sep 28 2015, 7:06 AM

Sep 26 2015

KLapshin added a comment to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.

Is this patch is fine for you now ?

Sep 26 2015, 3:06 AM

Sep 25 2015

KLapshin updated subscribers of D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.
Sep 25 2015, 11:15 AM
KLapshin added a comment to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.

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

Sep 25 2015, 11:07 AM
KLapshin updated the diff for D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.

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

Sep 25 2015, 11:05 AM
KLapshin updated subscribers of D13158: Allow to construct CMIUtilString using std::string directly + cleanup CMIUtilString (MI).
Sep 25 2015, 7:32 AM

Sep 24 2015

KLapshin added a comment to D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.

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().

Sep 24 2015, 11:34 AM

Sep 23 2015

KLapshin added a comment to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.

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

Sep 23 2015, 10:52 AM
KLapshin added a parent revision for D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped: D13056: Fix race condition during process detach.
Sep 23 2015, 9:56 AM
KLapshin added a child revision for D13056: Fix race condition during process detach: D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.
Sep 23 2015, 9:56 AM
KLapshin added a comment to D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.

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

Sep 23 2015, 7:36 AM
KLapshin added a comment to D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.

Done.

Sep 23 2015, 7:11 AM
KLapshin set the repository for D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped to rL LLVM.
Sep 23 2015, 7:07 AM
KLapshin added a comment to D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.

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

Sep 23 2015, 7:06 AM
KLapshin updated the diff for D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.
Sep 23 2015, 6:55 AM

Sep 22 2015

KLapshin updated subscribers of D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.

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.

Sep 22 2015, 10:58 AM
KLapshin updated subscribers of D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.
Sep 22 2015, 10:43 AM
KLapshin added a comment to D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.

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

Sep 22 2015, 10:40 AM
KLapshin updated the diff for D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.
Sep 22 2015, 10:39 AM
KLapshin added a comment to D13056: Fix race condition during process detach.

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

Sep 22 2015, 10:36 AM
KLapshin updated D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.
Sep 22 2015, 10:31 AM
KLapshin updated the diff for D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.
Sep 22 2015, 10:30 AM
KLapshin updated subscribers of D13056: Fix race condition during process detach.
Sep 22 2015, 8:17 AM

Sep 21 2015

KLapshin added inline comments to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.
Sep 21 2015, 11:04 AM
KLapshin added a comment to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.

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

Sep 21 2015, 11:00 AM
KLapshin added inline comments to D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.
Sep 21 2015, 10:55 AM

Sep 18 2015

KLapshin set the repository for D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run to rL LLVM.
Sep 18 2015, 11:00 AM
KLapshin updated the diff for D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.

Minor initial diff cleanup.

Sep 18 2015, 10:59 AM
KLapshin updated D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.
Sep 18 2015, 10:50 AM
KLapshin retitled D12977: LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run from to LLDB MI addition for getting process stopped at first instruction right after launch via -exec-run.
Sep 18 2015, 10:48 AM
KLapshin updated D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.
Sep 18 2015, 9:15 AM
KLapshin updated D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.
Sep 18 2015, 9:13 AM
KLapshin updated D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.
Sep 18 2015, 9:13 AM
KLapshin retitled D12968: Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped from to Fix for lldb-mi crash in Listener code if -exec-abort MI command was invoked without getting process stopped.
Sep 18 2015, 8:54 AM

Sep 1 2015

KLapshin updated subscribers of D12523: Bug in LLDB RSP client causes '$O' packets to be decoded incorrectly .
Sep 1 2015, 5:06 AM

Feb 5 2015

KLapshin abandoned D7409: Mangled class additions for handling language dependent demangling cases.

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

Feb 5 2015, 7:55 AM
KLapshin added a comment to D7409: Mangled class additions for handling language dependent demangling cases.

Hello Greg,

Feb 5 2015, 6:09 AM

Feb 4 2015

KLapshin added a comment to D7302: Patch for LLDB demangler for demangling upon actual language.

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()).

Feb 4 2015, 7:55 AM
KLapshin added a comment to D7302: Patch for LLDB demangler for demangling upon actual language.

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.

Feb 4 2015, 7:37 AM
KLapshin added a comment to D7302: Patch for LLDB demangler for demangling upon actual language.

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

Feb 4 2015, 7:31 AM
KLapshin retitled D7409: Mangled class additions for handling language dependent demangling cases from to Mangled class additions for handling language dependent demangling cases.
Feb 4 2015, 7:27 AM
KLapshin updated subscribers of D7302: Patch for LLDB demangler for demangling upon actual language.
Feb 4 2015, 5:35 AM