On Mon, Dec 18, 2017 at 9:39 PM, Ben Noordhuis <i...@bnoordhuis.nl> wrote:
> On Mon, Dec 18, 2017 at 9:06 PM, Ben Newman <b...@benjamn.com> 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 v8-users@googlegroups.com 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 v8-users+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
