[snapshot] Add support for thread names [crashpad/crashpad : main]
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen.
1 comment:
File snapshot/fuchsia/thread_snapshot_fuchsia.cc:
Patch Set #17, Line 65:
thread_name_ = thread.name;Thank you to the Fuchsia folks who made the thread name already available for me!
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Patchset:
OK, I updated this CL with tests for all platforms (Linux, Windows, Fuchsia, macOS, and iOS) and made sure the tests all pass.
PTAL!
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Patchset:
Tested this manually by running `generate_dump` on macOS, then running `minidump_dump` from Breakpad with https://crrev.com/c/3690389 applied.
Confirmed the dump contained thread names as expected.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen, Mark Mentovai.
1 comment:
Patchset:
Adding mark@ since I had to touch each of the snapshot process_reader implementations.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Mark Mentovai.
3 comments:
File minidump/minidump_thread_name_list_writer.cc:
Patch Set #41, Line 37:
DCHECKDCHECK_NE
File snapshot/linux/process_reader_linux.h:
Patch Set #41, Line 64:
std::string name;nit: These (and any other structs) are meant to be in tightly packable order and I generally assume heap-allocating types are some multiple of 64-bits, which would place this member before |tid|.
File test/fuchsia/thread_name.cc:
LT to prevent the nul terminator overwriting the last character?
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen.
14 comments:
File snapshot/linux/process_reader_linux.cc:
Patch Set #43, Line 20:
#include <stdio.h>For what?
Patch Set #43, Line 83:
name.resize(name.size() - 1);`name.pop_back();`
File snapshot/mac/process_reader_mac.h:
Patch Set #43, Line 79:
std::string name;Same as Joshua’s comment in the Linux file, this belongs before `id`.
File snapshot/mac/process_reader_mac.cc:
Patch Set #43, Line 79:
name(),…thus here too.
Patch Set #43, Line 379:
MAXTHREADNAMESIZEYou want to write “size of the thing I’m holding”, not “some constant that hopefully is the size of the thing I’m holding”:
sizeof(extended_info.pth_name)
Same in the iOS implementation.
File snapshot/mac/thread_snapshot_mac.h:
Patch Set #43, Line 85:
std::string thread_name_;Move above `thread_id`.
File snapshot/mac/thread_snapshot_mac.cc:
Patch Set #43, Line 30:
thread_name_(),and here.
File snapshot/win/process_reader_win.h:
Patch Set #43, Line 89:
std::string name;Before `id`.
File snapshot/win/process_reader_win.cc:
Patch Set #43, Line 288:
name() {}also here.
static const auto get_thread_description = []() {
HINSTANCE kernel32 = GetModuleHandleW(L"Kernel32.dll");
return reinterpret_cast<decltype(GetThreadDescription)*>(
GetProcAddress(kernel32, "GetThreadDescription"));
}();Don’t roll your own. Use our `GET_FUNCTION` macro from `util/win/get_function.h`. And spell `kernel32` with a lowercase `k` like we do everywhere else in Crashpad.
Patch Set #43, Line 472:
PWSTR thread_description;Crashpad style is to use `WSTR*` instead of the more obtuse `PWSTR`.
Patch Set #43, Line 477:
LocalFree(thread_description);Don’t do the C thing, you’re in C++. `#include "util/win/scoped_local_alloc.h"` and use `ScopedLocalAlloc`.
otherwise `LOG(WARNING)` as you’ve done on other platforms?
File test/BUILD.gn:
Patch Set #43, Line 65:
"mac/thread_name.cc",Do you use any of these outside of the snapshot test?
If no, they belong in the snapshot test, not in the generic test support library that’s visible to all (including non-snapshot) tests.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen, Mark Mentovai.
17 comments:
File minidump/minidump_thread_name_list_writer.cc:
Patch Set #41, Line 37:
DCHECKDCHECK_NE
Hmm. Looks like there's no `ostream` `operator<<` for a map iterator:
```
In file included from ../../third_party/mini_chromium/mini_chromium/base/logging.h:16:
../../third_party/mini_chromium/mini_chromium/base/check_op.h:15:23: error: invalid operands to binary expression ('basic_ostream<char>' and 'const std::__map_const_iterator<std::__tree_const_iterator<std::__value_type<unsigned long long, unsigned int>, std::__tree_node<std::__value_type<unsigned long long, unsigned int>, void *> *, long>>')
ss << names << " (" << v1 << " vs. " << v2 << ")";
~~~~~~~~~~~~~~~~~~~ ^ ~~(snip)
../../minidump/minidump_thread_name_list_writer.cc:38:3: note: in instantiation of function template specialization 'logging::CheckNEImpl<std::__map_const_iterator<std::__tree_const_iterator<std::__value_type<unsigned long long, unsigned int>, std::__tree_node<std::__value_type<unsigned long long, unsigned int>, void *> *, long>>, std::__map_const_iterator<std::__tree_const_iterator<std::__value_type<unsigned long long, unsigned int>, std::__tree_node<std::__value_type<unsigned long long, unsigned int>, void *> *, long>>>' requested here
DCHECK_NE(it, thread_id_map.end());
^
```I couldn't find any other examples of using `DCHECK_NE()` on a map iterator in Chromium or Crashpad, so I left this one as-is.
File snapshot/linux/process_reader_linux.h:
Patch Set #41, Line 64:
std::string name;
nit: These (and any other structs) are meant to be in tightly packable order and I generally assume […]
Great point, I didn't know that. Fixed here and elsewhere.
File snapshot/linux/process_reader_linux.cc:
Patch Set #43, Line 20:
#include <stdio.h>For what?
Oops, I used `snprintf()` in an earlier revision but replaced it with `base::StringPrintf()`. Fixed.
Patch Set #43, Line 83:
name.resize(name.size() - 1);
`name. […]
Done
File snapshot/mac/process_reader_mac.h:
Patch Set #43, Line 79:
std::string name;Same as Joshua’s comment in the Linux file, this belongs before `id`.
Done
File snapshot/mac/process_reader_mac.cc:
Patch Set #43, Line 79:
name(),…thus here too.
Done
Patch Set #43, Line 379:
MAXTHREADNAMESIZE
You want to write “size of the thing I’m holding”, not “some constant that hopefully is the size of […]
Done
File snapshot/mac/thread_snapshot_mac.h:
Patch Set #43, Line 85:
std::string thread_name_;Move above `thread_id`.
Done
File snapshot/mac/thread_snapshot_mac.cc:
Patch Set #43, Line 30:
thread_name_(),and here.
Done
File snapshot/win/process_reader_win.h:
Patch Set #43, Line 89:
std::string name;Before `id`.
Done
File snapshot/win/process_reader_win.cc:
Patch Set #43, Line 288:
name() {}also here.
Done
static const auto get_thread_description = []() {
HINSTANCE kernel32 = GetModuleHandleW(L"Kernel32.dll");
return reinterpret_cast<decltype(GetThreadDescription)*>(
GetProcAddress(kernel32, "GetThreadDescription"));
}();
Don’t roll your own. Use our `GET_FUNCTION` macro from `util/win/get_function.h`. […]
Fixed here and above in call to get `InitializeContext2()`.
Patch Set #43, Line 472:
PWSTR thread_description;Crashpad style is to use `WSTR*` instead of the more obtuse `PWSTR`.
Done
Patch Set #43, Line 477:
LocalFree(thread_description);
Don’t do the C thing, you’re in C++. `#include "util/win/scoped_local_alloc. […]
Done
otherwise `LOG(WARNING)` as you’ve done on other platforms?
Done
File test/BUILD.gn:
Patch Set #43, Line 65:
"mac/thread_name.cc",
Do you use any of these outside of the snapshot test? […]
Yes, I use it in `client/ios_handler/in_process_intermediate_dump_handler_test.cc` as well:
File test/fuchsia/thread_name.cc:
LT to prevent the nul terminator overwriting the last character?
Done
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen, Mark Mentovai.
Patch set 44:Code-Review +1
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen, Mark Mentovai.
1 comment:
Patchset:
Patch Set 44: Code-Review+1
The vote Code-Review+1 is ignored as code-owner approval since the label doesn't allow self approval of the patch set uploader.
Dang it, I hit it again! That button is not so useful and it's right next to CQ dry run..
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen, Mark Mentovai.
Patch set 46:Code-Review +1Commit-Queue +1
1 comment:
File snapshot/win/process_reader_win.cc:
Patch Set #43, Line 472:
PWSTR thread_description;Done
Hmm, looks like there is no such type `WSTR` in Win32, only `PWSTR` and `LPWSTR`:
I changed this to `wchar_t*`.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen, Mark Mentovai.
Patch set 49:Code-Review +1
1 comment:
Patchset:
iOS test failure is due to the RVA64 misaligned pointer fix in https://crrev.com/c/3693846. Added comments here:
https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3693846/comments/1933669e_fad36832
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen, Mark Mentovai.
Patch set 50:Code-Review +1
1 comment:
Patchset:
I updated this CL to:
1) Add tests for `MINIDUMP_THREAD_NAME`s containing strings with leading padding bytes
2) Re-implement `MinidumpThreadNameListWriter` and `MinidumpThreadNameWriter` to go back to using `MinidumpWritable::RegisterRVA()` to register the RVA64, but use an (aligned) `RVA64` member variable of `MinidumpThreadNameWriter` instead of a (packed, unaligned) member variable of `MINIDUMP_THREAD_NAME`PTAL!
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton, Joshua Peraza, Justin Cohen.
30 comments:
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #50, Line 813:
thread_extended_info extended_info;Blank line before this, as you’re starting something new.
File minidump/minidump_thread_name_list_writer.cc:
Patch Set #50, Line 174:
minidump_thread_names.resize(thread_names_.size());I think this would be cleaner if you used `.reserve` instead of `.resize` here, and then built up the vector by calling `.emplace_back` (or, if you have to, `.push_back`) in the loop body below. That way, the vector won’t ever have ambiguously bogus elements while you’re building it.
As it is now, the 7 lines of loop body that you have are just enough of a reading assignment to have to prove that nothing is being mishandled because the vector will have bogus elements while being built.
Patch Set #50, Line 175:
for (size_t i = 0; i < thread_names_.size(); ++i) {That would also let you iterate directly over thread_names_ (`for const auto& thread_name : thread_names_)`) since you would no longer need a loop variable (`i`). That’s nicer too.
Patch Set #50, Line 182:
iovecs.emplace_back(iov);Since you already have a `WritableIoVec` object in your hands, why are you emplacing without move?
(move wouldn’t be valid here either, unless `iov` was declared inside the loop body.)
File snapshot/linux/process_reader_linux.h:
Patch Set #50, Line 64:
std::string name;Doesn’t seem to be sorted for nice packing.
File snapshot/linux/process_reader_linux.cc:
Patch Set #50, Line 77:
return false;Is `comm` guaranteed to exist as far back as we need?
File snapshot/linux/thread_snapshot_linux.h:
Patch Set #50, Line 85:
std::string thread_name_;Still not great for packing.
File snapshot/win/process_reader_win.cc:
static const auto get_thread_description = []() {
HINSTANCE kernel32 = GetModuleHandleW(L"Kernel32.dll");
return reinterpret_cast<decltype(GetThreadDescription)*>(
GetProcAddress(kernel32, "GetThreadDescription"));
}();
Fixed here and above in call to get `InitializeContext2()`.
Thanks for catching that also.
Patch Set #43, Line 472:
PWSTR thread_description;
Hmm, looks like there is no such type `WSTR` in Win32, only `PWSTR` and `LPWSTR`:
This is fine.
File snapshot/win/process_reader_win.cc:
Patch Set #50, Line 475:
LOG(WARNING) << "GetThreadDescription failed: "Local style is just `”GetThreadDescription: “`. The message that follows will make clear that there was a failure.
File test/fuchsia/thread_name.cc:
Patch Set #50, Line 31:
zx::thread::self()->get_property(Check the return value (see later comment too).
Patch Set #50, Line 48:
CHECK_EQ(status, ZX_OK) << "set_property(ZX_PROP_NAME)";This should be ZX_CHECK. Same on line 54.
File test/linux/thread_name.cc:
Patch Set #50, Line 28:
// From TASK_COMM_LEN in <linux/sched.h>. Includes the trailing NUL byte.Why can’t you #include <linux/sched.h> and use `TASK_COMM_LEN`, then?
// namespace
Patch Set #50, Line 36:
PCHECK(pthread_getname_np(pthread_self(), result.data(), result.length()) ==(Relatively) recent POSIX additions, including the pthreads family, don’t _set_ `errno` but instead _return_ an error number. `PCHECK` operates on `errno`. So what this really needs to do is:
PCHECK((errno = pthread_getname_np(…)) == 0) << “pthread_getname_np”;
Same deal on lines 52 and 57.
Patch Set #50, Line 40:
PCHECK(result_nul_idx != std::string::npos)`PCHECK` is for when you want to carry information from `errno` to the message. This condition has nothing to do with `errno`, and `errno` will be indeterminate, resulting in a confusing message. You want a regular `CHECK`.
Patch Set #50, Line 49:
CHECK_LT(new_thread_name.length(), kPthreadNameMaxLen)Doesn’t this need “base/check_op.h” now?
File test/mac/thread_name.cc:
Patch Set #50, Line 21:
#include <mach/thread_info.h> // for MAXTHREADNAMESIZEWe don’t need to know why you included it.
This kind of comment becomes outdated even before it’s checked in, as you often wind up relying on that header for more things without even realizing.
Patch Set #50, Line 31:
PCHECK(pthread_getname_np(pthread_self(), result.data(), result.length()) ==Same comments as in the Linux file apply here.
In fact, this file seems to be entirely identical to the Linux file except for the maximum. Seems like you should just have a single scoped_set_thread_name_posix.cc and, if necessary, use an #ifdef to select the proper constant on a per-OS basis.
Patch Set #50, Line 45:
static_cast<const unsigned long>(MAXTHREADNAMESIZE))size_t
Patch Set #50, Line 46:
<< "pthread_setname_np() only supports strings up to "This is really wordy. I don’t really think you need all of this. The CHECK_LT will already show the parameters that failed the comparison. You can have a little text if you want and you think it helps (although I wouldn’t have complained if there was no extra text at all), but you definitely don’t need to stream any parameters.
File test/thread_name.h:
Seems like all of these files should be named scoped_set_thread_name.{h,cc} (impact to #include guards too).
Patch Set #50, Line 24:
std::string CurrentThreadName();Why do you need this exposed as a public interface? Why isn’t it a private implementation detail of the class?
(You may have used it somewhere, but I’m not on a real computer and “find” actually isn’t working, so I can’t check easily. Seems that it isn’t necessary as a public interface, though.)
Patch Set #50, Line 27:
class ScopedThreadNameOverride final {Probably just `ScopedSetThreadName`, seems clearer. You’re not really “overriding” anything, you’re just setting it during the lifetime of the object.
Patch Set #50, Line 29:
ScopedThreadNameOverride(const std::string& new_thread_name);`explicit`
File test/win/thread_name.cc:
Patch Set #50, Line 26:
PWSTR thread_description;`wchar_t*`
Patch Set #50, Line 28:
CHECK(SUCCEEDED(hr)) << "GetThreadDescription failed: " << hr;Formatting per my other comment, also on lines 40 and 47.
Patch Set #50, Line 30:
LocalFree(thread_description);Use the scoper.
Patch Set #50, Line 40:
CHECK(SUCCEEDED(hr)) << "SetThreadDescription(" << new_thread_nameAlso, we don’t really need to see new_thread_name. Same with original_name_ on line 47.
Patch Set #50, Line 45:
const std::wstring woriginal_name = base::UTF8ToWide(original_name_);Why didn’t you just save the original name as a wstring, instead of converting it to and from UTF-8?
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen, Mark Mentovai.
Patch set 50:Code-Review +1
28 comments:
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #50, Line 813:
thread_extended_info extended_info;Blank line before this, as you’re starting something new.
Done
File minidump/minidump_thread_name_list_writer.cc:
Patch Set #50, Line 174:
minidump_thread_names.resize(thread_names_.size());
I think this would be cleaner if you used `.reserve` instead of `. […]
Good call, done.
Patch Set #50, Line 175:
for (size_t i = 0; i < thread_names_.size(); ++i) {
That would also let you iterate directly over thread_names_ (`for const auto& thread_name : thread_n […]
Done
Patch Set #50, Line 182:
iovecs.emplace_back(iov);
Since you already have a `WritableIoVec` object in your hands, why are you emplacing without move? […]
Fixed to directly `emplace_back()` in a proper fashion.
File snapshot/linux/process_reader_linux.h:
Patch Set #50, Line 64:
std::string name;Doesn’t seem to be sorted for nice packing.
Done
File snapshot/linux/process_reader_linux.cc:
Patch Set #50, Line 77:
return false;
Looks like Linux added `/proc/$pid/comm` in Linux 2.6.33, released on 2010-02-24:
https://man7.org/linux/man-pages/man5/proc.5.html
/proc/[pid]/comm (since Linux 2.6.33)
In addition, glibc added its `pthread_getname_np()` / `pthread_setname_np()` wrappers in glibc 2.12, released on 2011-02-01.
I couldn't find Crashpad's system requirements anywhere, but I'm guessing 2010 is old enough to not have to worry about.
Anyway, I changed this to warn and continue on rather than fail if it can't read the thread name.
File snapshot/linux/thread_snapshot_linux.h:
Patch Set #50, Line 85:
std::string thread_name_;Still not great for packing.
Done
File snapshot/win/process_reader_win.cc:
Patch Set #50, Line 475:
LOG(WARNING) << "GetThreadDescription failed: "
Local style is just `”GetThreadDescription: “`. […]
Done
File test/fuchsia/thread_name.cc:
Patch Set #50, Line 31:
zx::thread::self()->get_property(Check the return value (see later comment too).
Done
Patch Set #50, Line 48:
CHECK_EQ(status, ZX_OK) << "set_property(ZX_PROP_NAME)";This should be ZX_CHECK. Same on line 54.
Done
File test/linux/thread_name.cc:
Patch Set #50, Line 28:
// From TASK_COMM_LEN in <linux/sched.h>. Includes the trailing NUL byte.
Why can’t you #include <linux/sched. […]
For some reason I thought nothing else `#include`d kernel headers here, but it looks like there are a few. Fixed.
// namespace
Done
Patch Set #50, Line 36:
PCHECK(pthread_getname_np(pthread_self(), result.data(), result.length()) ==
(Relatively) recent POSIX additions, including the pthreads family, don’t _set_ `errno` but instead […]
Got it, fixed all of them.
Patch Set #50, Line 40:
PCHECK(result_nul_idx != std::string::npos)
`PCHECK` is for when you want to carry information from `errno` to the message. […]
Whoops! Fixed, thanks.
Patch Set #50, Line 49:
CHECK_LT(new_thread_name.length(), kPthreadNameMaxLen)
Doesn’t this need “base/check_op. […]
Sure enough, fixed.
File test/mac/thread_name.cc:
Patch Set #50, Line 21:
#include <mach/thread_info.h> // for MAXTHREADNAMESIZE
We don’t need to know why you included it. […]
Merged this file into `thread_name_posix.cc`.
Patch Set #50, Line 31:
PCHECK(pthread_getname_np(pthread_self(), result.data(), result.length()) ==
Same comments as in the Linux file apply here. […]
Yep! There's one other difference (`pthread_setname_np()` takes 2 arguments on Linux and 1 arguments on Apple), but I was able to merge them into a `thread_name_posix.cc` file.
Patch Set #50, Line 45:
static_cast<const unsigned long>(MAXTHREADNAMESIZE))size_t
Merged into `scoped_set_thread_name_posix.cc`.
Patch Set #50, Line 46:
<< "pthread_setname_np() only supports strings up to "
This is really wordy. I don’t really think you need all of this. […]
Merged into `scoped_set_thread_name_posix.cc`.
File test/thread_name.h:
Seems like all of these files should be named scoped_set_thread_name. […]
Done
Patch Set #50, Line 24:
std::string CurrentThreadName();
Why do you need this exposed as a public interface? Why isn’t it a private implementation detail of […]
Moved to a private implementation detail.
Patch Set #50, Line 27:
class ScopedThreadNameOverride final {
Probably just `ScopedSetThreadName`, seems clearer. […]
Done
Patch Set #50, Line 29:
ScopedThreadNameOverride(const std::string& new_thread_name);`explicit`
Done
File test/win/thread_name.cc:
Patch Set #50, Line 26:
PWSTR thread_description;`wchar_t*`
Done
Patch Set #50, Line 28:
CHECK(SUCCEEDED(hr)) << "GetThreadDescription failed: " << hr;Formatting per my other comment, also on lines 40 and 47.
Done
Patch Set #50, Line 30:
LocalFree(thread_description);Use the scoper.
Done
Patch Set #50, Line 40:
CHECK(SUCCEEDED(hr)) << "SetThreadDescription(" << new_thread_nameAlso, we don’t really need to see new_thread_name. Same with original_name_ on line 47.
Done
Patch Set #50, Line 45:
const std::wstring woriginal_name = base::UTF8ToWide(original_name_);Why didn’t you just save the original name as a wstring, instead of converting it to and from UTF-8?
Sure, added an `#if BUILDFLAG(IS_WIN)` in the header to toggle the name to a wstring on Windows.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton, Joshua Peraza, Justin Cohen.
18 comments:
File minidump/minidump_thread_name_list_writer.cc:
iovecs.emplace_back(
WritableIoVec{&thread_name_list_, sizeof(thread_name_list_)});This still isn’t what you want.
`WritableIoVec{…}` instantiates an automatic `WritableIoVec` object. You then feed that to `emplace_back`, which forwards it to the constructor of another `WritableIoVec`. This loses all of the benefit of `emplace_back` over `push_back`.
What you want to do is invoke `emplace_back` with the constructor arguments directly. That’s where `emplace_back` is the winner.
It’s unlikely to make any difference for this `WritableIoVec` class because it’s so trivial and everything is visible to the compiler, but you should get into the habit of emplacing correctly.
Please review the rest of this change (and any other recent changes, if appropriate) for your use of emplace and correct where needed.
File snapshot/linux/process_reader_linux.cc:
Patch Set #51, Line 83:
PLOG(WARNING) << "ReadFileContents";ReadFileContents doesn’t promise to leave errno set to anything in particular, so you can’t use `PLOG`. But it does promise to log something for you if it fails, so you don’t actually need to do anything here.
I just wanted to make sure that you weren’t going to return early if the file might not exist, if its existence was in question. No other error handling is really necessary.
File snapshot/linux/process_reader_linux_test.cc:
Patch Set #52, Line 75:
// From TASK_COMM_LEN in <linux/sched.h>. Includes the trailing NUL byte.Use that constant here too, then.
File snapshot/linux/thread_snapshot_linux.h:
Patch Set #52, Line 84:
std::string thread_name_;Not ordered properly here either.
When reviewers provide comments like this, please ensure that the fix is applied uniformly across the entire change. Joshua mentioned the ordering once and I’ve already mentioned it twice before this comment.
File snapshot/mac/process_reader_mac_test.cc:
Patch Set #52, Line 491:
char expected_thread_name[256];This 256 should be a constant shared by the two things that use it. But see below.
char expected_thread_name[256];
snprintf(expected_thread_name,
sizeof(expected_thread_name),
"%s",
current_thread_name.c_str());`snprintf` won’t zero out the rest of the buffer, so you should start `expected_thread_name[]` out with a `= {}` initializer to avoid passing garbage around.
But (and considering that the same concern appears again below): it would be preferable to not have to pass all of that padding around at all. Why don’t you pass this as a (size_t, bytes) pair? Then you could do the formatting here and below with base::StringPrintf.
It’s two writes and reads instead of one, but it’s also less quirky than this recycled fixed-size buffer that’s more likely to trip up some future maintainer.
snprintf(expected_thread_name,
sizeof(expected_thread_name),
"%s-%zu",
thread_name_prefix_.c_str(),
thread_index);You should have something to clean up the trailing garbage in the buffer here too.
File test/BUILD.gn:
Patch Set #52, Line 124:
"fuchsia/scoped_set_thread_name.cc",Move this into the parent directory as scoped_set_thread_name_fuchsia.cc. That follows the pattern of the multiprocess_exec_fuchsia.cc file above.
Do the same for the Windows implementation on line 111.
The theory here is: for interfaces common to multiple platforms, where the implementations are in different files, put the files all in the same directory, and add a _platform tail to the filename. For interfaces that only make sense on a single platform, put everything into a platform-specific subdirectory.
This follows general Chromium practice.
File test/fuchsia/scoped_set_thread_name.cc:
Patch Set #52, Line 33:
ZX_CHECK(zx::thread::self()->get_property(This use of `ZX_CHECK` doesn’t look right to me. `ZX_CHECK` takes two parameters. Same on line 50.
File test/scoped_set_thread_name_posix.cc:
Patch Set #52, Line 17:
#include <string>C++ system headers go after C system headers: swap this with `<pthread.h>`.
Patch Set #52, Line 26:
#define CRASHPAD_THREAD_NAME_MAX_SIZE TASK_COMM_LENWe should avoid yet another macro (`CRASHPAD_THREAD_NAME_MAX_SIZE`) here, at the expense of having to repeat `#if BUILDFLAG` where you have `kPthreadNameMaxLen` below.
Two reasons:
1. macros.
2. This section up here is for #includes. Readers will gloss over it because they don’t expect to find any interesting declarations up here.
#else
#define CRASHPAD_THREAD_NAME_MAX_SIZE 16What’s this `#ifdef` about? I thought we were trusting `<linux/sched.h>` to provide `TASK_COMM_LEN`.
#include "base/check.h"
#include "base/check_op.h"These belong in the same block as the build_config.h #include above.
PCHECK(pthread_setname_np(pthread_self(), thread_name.c_str()) == 0)
<< "pthread_setname_np";
#elif BUILDFLAG(IS_APPLE)
PCHECK(pthread_setname_np(thread_name.c_str()) == 0) << "pthread_setname_np";Referring back to https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3671776/50/test/linux/thread_name.cc#b36, that comment was incompletely addressed. I wrote at the bottom:
Same deal on lines 52 and 57.
You wrote:
Got it, fixed all of them.
But both of these are still wrong. Maybe something got lost in when you merged the Apple and Linux files into a single POSIX file.
Patch Set #52, Line 58:
std::string CurrentThreadName() {name `GetCurrentThreadName` for optimal contrast with the existing `SetCurrentThreadName` above. Since the implementations are so similar, carry this change to the Windows and Fuchsia files too.
std::string result;
result.resize(kPthreadNameMaxLen);Do in one step:
std::string result(kPthreadNameMaxLen, '\0');
Patch Set #52, Line 61:
PCHECK((errno = pthread_getname_np(`#include <errno.h>`
CHECK_LT(new_thread_name.length(), kPthreadNameMaxLen)
<< "pthread_setname_np() only supports names up to "
<< kPthreadNameMaxLen - 1 << " bytes in length";Referring you back to https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3671776/50/test/mac/thread_name.cc#b46 - that comment wasn’t addressed.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen, Mark Mentovai.
Patch set 55:Code-Review +1
18 comments:
File minidump/minidump_thread_name_list_writer.cc:
iovecs.emplace_back(
WritableIoVec{&thread_name_list_, sizeof(thread_name_list_)});
This still isn’t what you want. […]
Sorry for the churn, I appreciate your patient explanation.
Since there's no applicable constructor for `WritableIoVec`, I had previously used automatic instantiation, assuming the optimizer would do the right thing to construct the value in place.
I see now that I can't avoid the copy, so I just rewrote it to use push_back(), since this isn't worth optimizing.
File snapshot/linux/process_reader_linux.cc:
Patch Set #51, Line 83:
PLOG(WARNING) << "ReadFileContents";
ReadFileContents doesn’t promise to leave errno set to anything in particular, so you can’t use `PLO […]
Ah, that makes sense. Removed the log here so we can rely on the one in `ReadFileContents` instead.
File snapshot/linux/process_reader_linux_test.cc:
Patch Set #52, Line 75:
// From TASK_COMM_LEN in <linux/sched.h>. Includes the trailing NUL byte.Use that constant here too, then.
I removed this in favor of the strategy you mentioned in the Mac test of writing the thread name length and then the bytes of the thread name.
File snapshot/linux/thread_snapshot_linux.h:
Patch Set #52, Line 84:
std::string thread_name_;
Not ordered properly here either. […]
Thanks for your patience. I synced up with jperaza@ who explained the requirement is to put strings after 64-bit values but before 32-bit values (like `pid_t` and `int`).
This was definitely wrong in a number of places in this change (which I think I've fixed now, but do double-check my work.)
For the case of this specific data structure, jperaza@ mentioned that the order seems correct here (`CPUContext`, `MemorySnapshotGeneric`, and `LinuxVMAddress` should all be 64-bit aligned, so it ought to be OK to put a `std::string` after them), but please let me know if I'm off base there.
File snapshot/mac/process_reader_mac_test.cc:
Patch Set #52, Line 491:
char expected_thread_name[256];This 256 should be a constant shared by the two things that use it. But see below.
Fixed to write the size and then the bytes.
char expected_thread_name[256];
snprintf(expected_thread_name,
sizeof(expected_thread_name),
"%s",
current_thread_name.c_str());
`snprintf` won’t zero out the rest of the buffer, so you should start `expected_thread_name[]` out w […]
Thanks, that's a great idea. I fixed this to write the size and then the bytes afterwards.
snprintf(expected_thread_name,
sizeof(expected_thread_name),
"%s-%zu",
thread_name_prefix_.c_str(),
thread_index);You should have something to clean up the trailing garbage in the buffer here too.
Removed in favor of `base::StringPrintf()`.
File test/BUILD.gn:
Patch Set #52, Line 124:
"fuchsia/scoped_set_thread_name.cc",
Move this into the parent directory as scoped_set_thread_name_fuchsia.cc. […]
Makes sense, done.
File test/fuchsia/scoped_set_thread_name.cc:
Patch Set #52, Line 33:
ZX_CHECK(zx::thread::self()->get_property(This use of `ZX_CHECK` doesn’t look right to me. `ZX_CHECK` takes two parameters. Same on line 50.
Yep, CI caught me here as well. Fixed each place to pass the boolean separately from the status.
File test/scoped_set_thread_name_posix.cc:
Patch Set #52, Line 17:
#include <string>C++ system headers go after C system headers: swap this with `<pthread.h>`.
Done
Patch Set #52, Line 26:
#define CRASHPAD_THREAD_NAME_MAX_SIZE TASK_COMM_LEN
We should avoid yet another macro (`CRASHPAD_THREAD_NAME_MAX_SIZE`) here, at the expense of having t […]
Done
#else
#define CRASHPAD_THREAD_NAME_MAX_SIZE 16What’s this `#ifdef` about? I thought we were trusting `<linux/sched.h>` to provide `TASK_COMM_LEN`.
Hmm. It looks like `TASK_COMM_LEN` does exist in the kernel headers:
but not in the copy used in userspace. Reverted this back to just 16.
#include "base/check.h"
#include "base/check_op.h"These belong in the same block as the build_config.h #include above.
Done
PCHECK(pthread_setname_np(pthread_self(), thread_name.c_str()) == 0)
<< "pthread_setname_np";
#elif BUILDFLAG(IS_APPLE)
PCHECK(pthread_setname_np(thread_name.c_str()) == 0) << "pthread_setname_np";
Referring back to https://chromium-review.googlesource. […]
Very embarrassing on my part. I fixed the rest of the calls, thanks.
Patch Set #52, Line 58:
std::string CurrentThreadName() {
name `GetCurrentThreadName` for optimal contrast with the existing `SetCurrentThreadName` above. […]
Done
std::string result;
result.resize(kPthreadNameMaxLen);
Do in one step: […]
Done here and in `process_reader_mac_test.cc` as well.
Patch Set #52, Line 61:
PCHECK((errno = pthread_getname_np(
`#include <errno. […]
Done
CHECK_LT(new_thread_name.length(), kPthreadNameMaxLen)
<< "pthread_setname_np() only supports names up to "
<< kPthreadNameMaxLen - 1 << " bytes in length";
Referring you back to https://chromium-review.googlesource. […]
Removed wordiness.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton, Joshua Peraza, Justin Cohen.
3 comments:
Patchset:
I didn't reread the code yet, am just responding to comments.
File minidump/minidump_thread_name_list_writer.cc:
iovecs.emplace_back(
WritableIoVec{&thread_name_list_, sizeof(thread_name_list_)});
Sorry for the churn, I appreciate your patient explanation. […]
You will be able to achieve this with emplace_back in C++20, but I agree that it's not worth futzing with. As I said, since everything is visible to the compiler, it's likely to optimize away the unnecessary operations even when you've written push_back with a temporary.
File snapshot/linux/thread_snapshot_linux.h:
Patch Set #52, Line 84:
std::string thread_name_;
Thanks for your patience. I synced up with jperaza@ who explained the requirement is to put strings after 64-bit values but before 32-bit values (like `pid_t` and `int`).
This was definitely wrong in a number of places in this change (which I think I've fixed now, but do double-check my work.)
For the case of this specific data structure, jperaza@ mentioned that the order seems correct here (`CPUContext`, `MemorySnapshotGeneric`, and `LinuxVMAddress` should all be 64-bit aligned, so it ought to be OK to put a `std::string` after them), but please let me know if I'm off base there.
I guess I'm in a 64-bit mindset now, and put pointer-aligned structs ahead of 64-bit integers.
Joshua, have I advanced too far in my thinking? Are we putting things like std::string that are pointer-aligned in between the fixed 64- and 32-bit-aligned members because sometimes pointers are 64 and sometimes they're 32?
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Mark Mentovai.
1 comment:
File snapshot/linux/thread_snapshot_linux.h:
Patch Set #52, Line 84:
std::string thread_name_;
> Thanks for your patience. […]
The possibility of things like std::string not being fully 64-bits wide has sometimes factored into my placing them between 64 and 32-bit things. We don't do this all the time though, sometimes placing them before 64-bit values and usually clumping them together when there are multiple.
For Android, we're almost always using 32-bit pointers and I think other platforms were also using 32-bit pointers when a lot of this code was written.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Joshua Peraza (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Mark Mentovai.
1 comment:
File snapshot/linux/thread_snapshot_linux.h:
Patch Set #52, Line 84:
std::string thread_name_;
The possibility of things like std::string not being fully 64-bits wide has sometimes factored into […]
Why place 64-bit pointer aligned structs before 64-bit values?
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton, Joshua Peraza, Justin Cohen.
1 comment:
File snapshot/linux/thread_snapshot_linux.h:
Patch Set #52, Line 84:
std::string thread_name_;
Why place 64-bit pointer aligned structs before 64-bit values?
Doesn't make any difference for packing, but on the assumption that most structs (under a 64-bit pointer ABI) will be 64-bit-aligned, it helps with organization to put all the structs together, and then all the ints together.
(I'd like it if, in cases where order is insignificant, there was enough leeway for the compiler to reorder things. Sometimes there's tension between declaration order for optimum packing and required construction/destruction order. This could help. Mostly unrelated musings, though.)
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen, Mark Mentovai.
Patch set 59:Code-Review +1
1 comment:
File snapshot/linux/thread_snapshot_linux.h:
Patch Set #52, Line 84:
std::string thread_name_;
> Why place 64-bit pointer aligned structs before 64-bit values? […]
OK! I'll consider this one resolved for now — maybe we can automate some of these checks with ClangTidy or a similar static analyzer in the future.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton, Joshua Peraza, Justin Cohen.
5 comments:
File minidump/minidump_thread_name_list_writer.cc:
Patch Set #59, Line 112:
for (const ThreadSnapshot* thread_snapshot : thread_snapshots) {We’ll probably see threads with empty names on some platforms. Do we want to exclude them from the thread name list? What do dbghelp-produced minidumps do here?
File test/scoped_set_thread_name_fuchsia.cc:
Patch Set #59, Line 47:
CHECK_LT(new_thread_name.length(), ZX_MAX_NAME_LEN);Will `set_property` have its own failure if the name is too long?
See the comment in the POSIX file for more on this thought.
File test/scoped_set_thread_name_posix.cc:
PCHECK((errno = pthread_setname_np(thread_name.c_str())) == 0)
<< "pthread_setname_np";Turns out that on Apples, this behaves “traditionally”: on failure, it returns nonzero and has already set `errno` for you. So this Apple branch should leave `errno` alone and just do:
PCHECK(pthread_setname_np(…) != 0) << "pthread_setname_np";
Patch Set #59, Line 59:
PCHECK((errno = pthread_getname_np(but I verified that this one does behave in line with expectations for the pthread_* family even on Apples, so this is correct as now written. Go figure.
Patch Set #59, Line 73:
CHECK_LT(new_thread_name.length(), kPthreadNameMaxLen);Leave this to the `pthread_setname_np` to fail.
If you want to keep your own check, make it a `DCHECK`, since it’s otherwise redundant.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen, Mark Mentovai.
Patch set 59:Code-Review +1
5 comments:
Patchset:
Adding brucedawson@ who I suspect will be very happy to see thread name support here and in Breakpad (https://crrev.com/c/3690389).
File minidump/minidump_thread_name_list_writer.cc:
Patch Set #59, Line 112:
for (const ThreadSnapshot* thread_snapshot : thread_snapshots) {
We’ll probably see threads with empty names on some platforms. […]
Great question. I tried using `windbg`'s `.dump /mt` command on Chrome, but the resulting minidump was missing the `THREAD_NAME_LIST_STREAM`.
Let's revisit this after this review to see if we should exclude empty thread names from the thread name list.
File test/scoped_set_thread_name_fuchsia.cc:
Patch Set #59, Line 47:
CHECK_LT(new_thread_name.length(), ZX_MAX_NAME_LEN);
Will `set_property` have its own failure if the name is too long? […]
Looks like Fuchsia behaves the same as Apple — both silently truncate names that are too long (unlike Linux, which fails with `ERANGE`).
That suggests this check should still provide value.
Fuchsia:
Apple:
Added a comment.
File test/scoped_set_thread_name_posix.cc:
PCHECK((errno = pthread_setname_np(thread_name.c_str())) == 0)
<< "pthread_setname_np";
Turns out that on Apples, this behaves “traditionally”: on failure, it returns nonzero and has alrea […]
Done
Patch Set #59, Line 73:
CHECK_LT(new_thread_name.length(), kPthreadNameMaxLen);
Leave this to the `pthread_setname_np` to fail. […]
TIL that glibc returns `ERANGE` instead of truncating:
Since Apple doesn't check:
I made this specific to Apple and added a comment.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton, Joshua Peraza, Justin Cohen.
2 comments:
File test/scoped_set_thread_name_fuchsia.cc:
Patch Set #59, Line 47:
CHECK_LT(new_thread_name.length(), ZX_MAX_NAME_LEN);
Looks like Fuchsia behaves the same as Apple — both silently truncate names that are too long (unlike Linux, which fails with `ERANGE`).
That suggests this check should still provide value.
Fuchsia:
Apple:
Added a comment.
File test/scoped_set_thread_name_posix.cc:
Patch Set #59, Line 73:
CHECK_LT(new_thread_name.length(), kPthreadNameMaxLen);
TIL that glibc returns `ERANGE` instead of truncating:
Since Apple doesn't check:
I made this specific to Apple and added a comment.
I disagree. The `__proc_info` call winds up in 12.4 https://github.com/apple-oss-distributions/xnu/blob/xnu-8020.121.3/bsd/kern/proc_info.c#L3159, where
3166 if (buffersize > (MAXTHREADNAMESIZE - 1)) {
3167 return ENAMETOOLONG;
3168 }Back out in userspace, 12.4 https://github.com/apple-oss-distributions/libpthread/blob/libpthread-486.100.11/src/pthread.c#L1140 (corrected your source link to be the proper canonical and authoritative one), `pthread_setname_np` will not reach the point of setting the user-space view of the thread name as stored in the `pthread` structure unless `__proc_info` succeeded and xnu was satisfied with the name—if the name was too long, xnu will not have been satisfied, and `ENAMETOOLONG` will appear in `errno`.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen, Mark Mentovai.
Patch set 60:Code-Review +1
1 comment:
File test/scoped_set_thread_name_posix.cc:
Patch Set #59, Line 73:
CHECK_LT(new_thread_name.length(), kPthreadNameMaxLen);
> TIL that glibc returns `ERANGE` instead of truncating: […]
Sure enough! I was lazy and didn't trace `__proc_info(5, getpid(), 2, (uint64_t)0, (void*)name, (int)len);` fully. (In my defense, that is just the tiniest bit impenetrable).
Removed check and updated comments here and above.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton, Joshua Peraza, Justin Cohen.
Patch set 61:Code-Review +1
3 comments:
Patchset:
I hereby award the coveted LGTM prize to one Ben Hamilton, right in the nick of time. Congratulations, Ben!
File test/scoped_set_thread_name_fuchsia.cc:
Patch Set #61, Line 23:
#include "base/check.h"You actually use check_op.h macros in this file.
File test/scoped_set_thread_name_posix.cc:
Patch Set #61, Line 23:
#include "base/check_op.h"Unused now.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen.
Patch set 61:Code-Review +1Commit-Queue +1
3 comments:
Patchset:
I hereby award the coveted LGTM prize to one Ben Hamilton, right in the nick of time. […]
Huzzah! I learned quite a lot about C++ in this CL stack. I'm not sure if I should feel proud or foolish about that, considering I've been coding C++ since some time in the previous millennium..
File test/scoped_set_thread_name_fuchsia.cc:
Patch Set #61, Line 23:
#include "base/check.h"You actually use check_op.h macros in this file.
Fixed.
File test/scoped_set_thread_name_posix.cc:
Patch Set #61, Line 23:
#include "base/check_op.h"Unused now.
Removed.
To view, visit change 3671776. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Joshua Peraza, Justin Cohen.
Patch set 62:Commit-Queue +2
Crashpad LUCI CQ (Gerrit)
Crashpad LUCI CQ submitted this change.
61 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: test/scoped_set_thread_name_posix.cc
Insertions: 0, Deletions: 1.
@@ -20,7 +20,6 @@
#include <string>
#include "base/check.h"
-#include "base/check_op.h"
#include "build/build_config.h"
#if BUILDFLAG(IS_APPLE)
```
```
The name of the file: test/scoped_set_thread_name_fuchsia.cc
Insertions: 1, Deletions: 1.
@@ -20,7 +20,7 @@
#include <zircon/syscalls/object.h>
#include <zircon/types.h>
-#include "base/check.h"
+#include "base/check_op.h"
#include "base/fuchsia/fuchsia_logging.h"
namespace crashpad {
```
[snapshot] Add support for thread names
This CL adds a new method ThreadSnapshot::ThreadName(), implements
it in each snapshot implementation, and adds tests for iOS, macOS,
Linux, Windows, and Fuchsia.
Bug: crashpad:327
Change-Id: I35031975223854c19d977e057dd026a40d33fd41
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/3671776
Reviewed-by: Mark Mentovai <ma...@chromium.org>
Commit-Queue: Ben Hamilton <benha...@google.com>
Reviewed-by: Ben Hamilton <benha...@google.com>
---
M client/ios_handler/in_process_intermediate_dump_handler.cc
M client/ios_handler/in_process_intermediate_dump_handler_test.cc
M minidump/minidump_file_writer.cc
M minidump/minidump_thread_name_list_writer.cc
M minidump/minidump_thread_name_list_writer.h
M minidump/minidump_thread_name_list_writer_test.cc
M snapshot/fuchsia/process_reader_fuchsia_test.cc
M snapshot/fuchsia/thread_snapshot_fuchsia.cc
M snapshot/fuchsia/thread_snapshot_fuchsia.h
M snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc
M snapshot/ios/thread_snapshot_ios_intermediate_dump.cc
M snapshot/ios/thread_snapshot_ios_intermediate_dump.h
M snapshot/linux/process_reader_linux.cc
M snapshot/linux/process_reader_linux.h
M snapshot/linux/process_reader_linux_test.cc
M snapshot/linux/thread_snapshot_linux.cc
M snapshot/linux/thread_snapshot_linux.h
M snapshot/mac/process_reader_mac.cc
M snapshot/mac/process_reader_mac.h
M snapshot/mac/process_reader_mac_test.cc
M snapshot/mac/thread_snapshot_mac.cc
M snapshot/mac/thread_snapshot_mac.h
M snapshot/minidump/process_snapshot_minidump.cc
M snapshot/minidump/process_snapshot_minidump.h
M snapshot/minidump/process_snapshot_minidump_test.cc
M snapshot/minidump/thread_snapshot_minidump.cc
M snapshot/minidump/thread_snapshot_minidump.h
M snapshot/sanitized/thread_snapshot_sanitized.cc
M snapshot/sanitized/thread_snapshot_sanitized.h
M snapshot/test/test_thread_snapshot.cc
M snapshot/test/test_thread_snapshot.h
M snapshot/thread_snapshot.h
M snapshot/win/process_reader_win.cc
M snapshot/win/process_reader_win.h
M snapshot/win/process_reader_win_test.cc
M snapshot/win/process_snapshot_win.cc
M snapshot/win/thread_snapshot_win.cc
M snapshot/win/thread_snapshot_win.h
M test/BUILD.gn
A test/scoped_set_thread_name.h
A test/scoped_set_thread_name_fuchsia.cc
A test/scoped_set_thread_name_posix.cc
A test/scoped_set_thread_name_win.cc
M util/ios/ios_intermediate_dump_format.h
44 files changed, 992 insertions(+), 79 deletions(-)