Thanks everyone! CL submitted here: https://chromium-review.googlesource.com/c/v8/v8/+/833260 V8 bug report here: https://bugs.chromium.org/p/v8/issues/detail?id=7230
On Tuesday, December 19, 2017 at 5:27:57 AM UTC-5, Jakob Gruber wrote: > > On Mon, Dec 18, 2017 at 9:39 PM, Ben Noordhuis <[email protected] > <javascript:>> wrote: > >> On Mon, Dec 18, 2017 at 9:06 PM, Ben Newman <[email protected] >> <javascript:>> wrote: >> > Hi folks! This is my first time posting here, so please let me know if I >> > should ask this question somewhere else instead. I'm asking here because >> > https://github.com/v8/v8 does not have GitHub issues enabled. >> > >> > In short, I have a project that embeds V8 and uses a single `Isolate` >> from >> > multiple threads. >> > >> > The program runs just fine, but sometimes the inspector doesn't stop on >> the >> > correct line after stepping over a statement that switches threads >> behind >> > the scenes, even though the original thread is restored by the time the >> next >> > statement is executed. >> > >> > From what I can tell, the key information that controls this behavior is >> > `thread_local_.target_frame_count_`, first set here, and then checked >> here. >> > >> > That check fails because `target_frame_count === -1`, which suggests it >> has >> > not been updated since it was last initialized here. >> > >> > Digging a bit deeper, I realized that the `Debug::ArchiveDebug` and >> > `Debug::RestoreDebug` methods, which should be responsible for >> > saving/restoring this `ThreadLocal` information when switching threads, >> > currently don't do anything. >> > >> > I can understand that throwing away debugger state when switching >> threads >> > might be OK if no one needs to debug a multi-threaded V8 program, but I >> > think I've found a legitimate use case for preserving that state, so I >> would >> > like to submit a PR to fix this. >> > >> > The essence of the PR would be: >> > >> > char* Debug::ArchiveDebug(char* storage) { >> > MemCopy(storage, reinterpret_cast<char*>(&thread_local_), >> > ArchiveSpacePerThread()); >> > return storage + ArchiveSpacePerThread(); >> > } >> > >> > >> > char* Debug::RestoreDebug(char* storage) { >> > MemCopy(reinterpret_cast<char*>(&thread_local_), storage, >> > ArchiveSpacePerThread()); >> > return storage + ArchiveSpacePerThread(); >> > } >> > >> > >> > int Debug::ArchiveSpacePerThread() { >> > return sizeof(ThreadLocal); >> > } >> > >> > Would this be a welcome PR? Is there anyone in particular I should ask >> to >> > review this? Any other advice for a first-time contributor? >> > >> > Thanks, >> > Ben >> >> If it's an obvious bug, then sure, send a CL. Preferably include a >> regression test in test/cctest. >> >> As to picking a reviewer: git-cl (from depot_tools) can do that for >> you but I personally find its suggestions less than helpful. I >> usually pick someone from the `git shortlog -ns` top 3 for the files I >> touch (which quite often turns out to be Yang Guo - either we have the >> same interests or he is a commit cannon.) >> >> Note that V8 doesn't use GitHub pull requests. The process is >> explained here: https://github.com/v8/v8/wiki/Contributing >> > > Right, please submit a CL. Mainly yangguo@ will be interested in this, but > feel free to add jgruber@ as reviewer as well. > > It's a known issue that debug infos aren't restored when switching > threads. I don't have too much background information on this, maybe Yang > will chime in with more. > -- -- v8-users mailing list [email protected] http://groups.google.com/group/v8-users --- You received this message because you are subscribed to the Google Groups "v8-users" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
