This is an archive of the discontinued LLVM Phabricator instance.

Update suspended threads info to be compatible with darwin
ClosedPublic

Authored by fjricci on Mar 29 2017, 11:18 AM.

Details

Summary

On Darwin, we need to track thread and tid as separate values.
This patch modifies suspended thread info to be more general, to
allow for this behavior.

Event Timeline

fjricci created this revision.Mar 29 2017, 11:18 AM
kubamracek added inline comments.Mar 29 2017, 1:31 PM
lib/sanitizer_common/sanitizer_stoptheworld_mac.cc
55–63

Does this return the same value as pthread_threadid_np? Can we use it instead? Or even better, use GetTid from sanitizer_mac.cc? Calling thread_info here sounds very expensive.

Why not store the thread_id's in the array directly? If we really need to know both thread_t and thread_id, then we should have a struct that contains both and store the struct in the array.

fjricci added inline comments.Mar 29 2017, 2:25 PM
lib/sanitizer_common/sanitizer_stoptheworld_mac.cc
55–63

I think it's reasonable to use a struct instead. There is a fair amount of existing code that requires the thread_id, but the code for accessing register info will require the thread_t, so there's definitely a need for both, especially if it's expensive to convert.

kubamracek added inline comments.Mar 29 2017, 2:27 PM
lib/sanitizer_common/sanitizer_stoptheworld_mac.cc
55–63

I don't think it's expensive to convert, but we should make sure to only do the conversion once per thread.

fjricci added inline comments.Mar 29 2017, 2:31 PM
lib/sanitizer_common/sanitizer_stoptheworld_mac.cc
55–63

Does that go for pthread_threadid_np as well? I don't think there's a good way to cache the values and ensure it's only called once, other than using a struct to store both.

fjricci added inline comments.Mar 29 2017, 2:42 PM
lib/sanitizer_common/sanitizer_stoptheworld_mac.cc
55–63

Looks like the pthread call takes a pthread_t and not a thread_t anyway. I'll try using the struct with a single call to thread_info.

kubamracek added inline comments.Mar 29 2017, 2:42 PM
lib/sanitizer_common/sanitizer_stoptheworld_mac.cc
55–63

Right. I'm suggesting to use the struct to store both.

fjricci updated this revision to Diff 93411.Mar 29 2017, 2:59 PM

Use a struct to store both thread and tid

I'm not sure how great this patch is, I don't love that we store
the tid twice on Linux.

If it's helpful for the purposes of design/review, I can also upload the 3 followup patches that actually use this functionality. I've been trying to avoid overwhelming with too many patches at once.

kubamracek added inline comments.Mar 29 2017, 5:01 PM
lib/sanitizer_common/sanitizer_stoptheworld.h
39–40

Can we use actual types here? The typedefs are just confusing.

How about this: The contents of this struct will be ifdef'd and use actual types (pid_t on Linux, thread_t+uptr64_t on Darwin). The SuspendedThreadsList API below will only deal with the whole struct, i.e. GetThread returns the whole struct, GetThreadID will be removed, Contains will search for matches of the whole struct (or select one field to be the "primary id", which we'll match for).

fjricci updated this revision to Diff 93489.Mar 30 2017, 9:13 AM

Refactor to address comments

kubamracek edited edge metadata.Mar 30 2017, 11:43 AM

Besides the issue with "uptr tid", LGTM.

lib/sanitizer_common/sanitizer_stoptheworld.h
28

uptr is not correct here; macOS has 64-bit thread IDs even on i386

kcc added inline comments.Mar 30 2017, 11:46 AM
lib/sanitizer_common/sanitizer_stoptheworld.h
20

please avoid any #if/#ifdef in this file (and in most others).
If you need something platform-specific, put it into a platform-specific file

Hmm. In order to do this without using any #ifdef's, I think we'll need to move the struct into two new header files - sanitizer_stoptheworld_linux.h and sanitizer_stoptheworld_mac.h. Other than that, I think we should be fine removing them, if that sounds like an okay solution. I'm not sure that any existing plaform-specific headers really work for this.

kcc added inline comments.Mar 30 2017, 1:26 PM
lib/sanitizer_common/sanitizer_stoptheworld.h
21

And more generally, please do not include *any* system headers in any sanitizer file, unless it already includes other system headers.

fjricci updated this revision to Diff 93538.Mar 30 2017, 2:06 PM

Don't pollute with system headers or use platform-specific ifdefs

Ping - this blocks several more Darwin LSAN patches I have waiting to send for review.

As an update to the concern raised by @kcc regarding platform-specific differences, the primary reason why we store two values (tid + thread_t) on Darwin and only one (tid) on linux is performance. We could remove platform-specific headers as well as the ifdef's if we only stored the tid on darwin as well, but that would require conversion between tid and thread_t in several cases (reading register state, etc). This is a relatively expensive conversion (requires a syscall), so we prefer to do the conversion only once and store the result, even though this does introduce some differences between the platforms.

I think that the current implementation, which uses platform-specific headers (but no system headers), and no ifdefs is a reasonable compromise. With this change: D31774, it could actually become even more straightforward, although platform-specific headers would still be required to store the thread_t. However, if we'd prefer to store only the tid on Darwin as well, and accept the cost of the conversion to thread_t each time it's required, I can upload a patch to do that.

kcc edited edge metadata.

Please add Aleksey to all lsan code reviews.

alekseyshl edited edge metadata.Apr 12 2017, 9:18 AM

Should we commit and merge with D31774 first?

lib/sanitizer_common/sanitizer_stoptheworld.h
33

const SuspendedThreadInfo& GetThread

38

I think we should stick to Contains(SuspendedThreadID thread_id), it's semantics is to check whether the thread info for the given thread id is already appended to the list, not that the list contains the same info. Maybe even rename it to ContainsTid.

Should we commit and merge with D31774 first?

You can probably feel free to add yourself as a reviewer to that diff, I assume it may not have been merged since we weren't sure whether it's a solution we want for this patch.

I'll rebase onto D31774 and add it as a dependency.

fjricci updated this revision to Diff 94989.Apr 12 2017, 9:53 AM

Rebase onto D31774

fjricci updated this revision to Diff 95019.Apr 12 2017, 12:57 PM

This breaks compilation of asan for windows unless we define a dummy struct there as well

fjricci updated this revision to Diff 95030.Apr 12 2017, 2:16 PM

the tid struct member is required, even on windows

Sorry for the push back, but the sanitizer_stoptheworld_win.h makes things really ugly. Now it feels like the entire SuspendedThreadsList implementation need to go into *_linux and *_mac specific files, leaving only the interface in sanitizer_stoptheworld.h:

class SuspendedThreadsList {
public:

virtual uptr GetThreadCount() const = 0;
virtual SuspendedThreadID GetThreadID(uptr index) const = 0;
virtual uptr RegisterCount() = 0;
virtual PtraceRegistersStatus GetRegistersAndSP(uptr index, uptr *buffer, uptr *sp) const = 0;

};

The actual implementation SuspendedThreadsListLinux/Mac is free to store whatever it needs. Does it look cleaner?

Yeah, I was growing pretty dissatisfied with the current state as well - I like the abstract class idea. However, it will mean we need a windows implementation. The stoptheworld header is included by lsan_common, which is included by asan, even when lsan is disabled. I could write a dummy implementation for windows, with CHECK(0 && "not implemented") in all the functions, if that seems acceptable.

fjricci updated this revision to Diff 95443.Apr 17 2017, 9:45 AM

Split out platform-specific implementations of suspended threads list

Turns out windows isn't needed for compilation.

(This does still require D31774)

fjricci updated this revision to Diff 95445.Apr 17 2017, 9:53 AM

Don't clobber the blame too much

Yes, CHECK(0 && "not implemented") makes much more sense to the reader than dummy tid struct, it means exactly what it looks like, the functionality is not implemented.

Yes, CHECK(0 && "not implemented") makes much more sense to the reader than dummy tid struct, it means exactly what it looks like, the functionality is not implemented.

It turns out that even the CHECK() isn't necessary for compilation on windows (I checked that asan+windows builds with this patch). Presumably if someone attempts to actually use stoptheworld on Windows, they'll hit a build error from the abstract class being unimplemented. I think this is exactly what we'd want (and even better than a CHECK).

fjricci updated this revision to Diff 95454.Apr 17 2017, 10:48 AM

Add in some functionality that we'll need later, and clean up append function

alekseyshl accepted this revision.Apr 17 2017, 10:52 AM

Looks much better, thank you!

lib/sanitizer_common/sanitizer_stoptheworld.h
39

Please rename it to ThreadCount.

lib/sanitizer_common/sanitizer_stoptheworld_linux_libcdep.cc
518

Remove this comment.

521

Add empty line to separate functions.

lib/sanitizer_common/sanitizer_stoptheworld_mac.cc
56

Remove this comment.

57

uptr SuspendedThreadsListMac::thread_count() const {

return threads_.size();

}

and add an empty line between functions.

This revision is now accepted and ready to land.Apr 17 2017, 10:52 AM
fjricci updated this revision to Diff 95456.Apr 17 2017, 10:57 AM

Address comments

fjricci updated this revision to Diff 95474.Apr 17 2017, 12:40 PM

Looks like we can't use __cxa_pure_virtual in the sanitizers - replace with UNIMPLEMENTED

fjricci requested review of this revision.Apr 17 2017, 12:41 PM
fjricci edited edge metadata.

Since it isn't quite a trivial change, want to verify it still looks okay.

This revision is now accepted and ready to land.Apr 17 2017, 1:32 PM
This revision was automatically updated to reflect the committed changes.