Advice for submitting a PR to fix Debug::ArchiveDebug and Debug::RestoreDebug?

[Ben Newman]

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

[Jakob Kummerow]

[+Yang, Jakob]

--
--
v8-users mailing list
v8-u...@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+u...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ben Noordhuis]

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

[Jakob Gruber]

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.

[b...@meteor.com]

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