| Summary: | Missing return value in TCMalloc_ThreadCache::GetThreadHeap() | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Benbuck Nason <bnason> |
| Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> |
| Status: | UNCONFIRMED --- | ||
| Severity: | Trivial | ||
| Priority: | P5 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Other | ||
| OS: | Other | ||
FastMalloc is never built with HAVE_TLS defined. If it were built with that defined then it would fail to build with a link error, since the KernelSupportsTLS function is only provided if WTF_CHANGES is not defined. config.h always sets WTF_CHANGES. I don’t think there’s anything to do here. I am building it with HAVE_TLS defined because TLS is *much* faster than pthread_getspecific() on my platform. Also looking at the code, I don't think you are correct:
FastMalloc.cpp line 489:
// Even if we have support for thread-local storage in the compiler
// and linker, the OS may not support it. We need to check that at
// runtime. Right now, we have to keep a manual set of "bad" OSes.
#if defined(HAVE_TLS)
static bool kernel_supports_tls = false; // be conservative
static inline bool KernelSupportsTLS() {
return kernel_supports_tls;
}
# if !HAVE_DECL_UNAME // if too old for uname, probably too old for TLS
static void CheckIfKernelSupportsTLS() {
kernel_supports_tls = false;
}
# else
# include <sys/utsname.h> // DECL_UNAME checked for <sys/utsname.h> too
static void CheckIfKernelSupportsTLS() {
struct utsname buf;
if (uname(&buf) != 0) { // should be impossible
MESSAGE("uname failed assuming no TLS support (errno=%d)\n", errno);
kernel_supports_tls = false;
} else if (strcasecmp(buf.sysname, "linux") == 0) {
// The linux case: the first kernel to support TLS was 2.6.0
if (buf.release[0] < '2' && buf.release[1] == '.') // 0.x or 1.x
kernel_supports_tls = false;
else if (buf.release[0] == '2' && buf.release[1] == '.' &&
buf.release[2] >= '0' && buf.release[2] < '6' &&
buf.release[3] == '.') // 2.0 - 2.5
kernel_supports_tls = false;
else
kernel_supports_tls = true;
} else { // some other kernel, we'll be optimisitic
kernel_supports_tls = true;
}
// TODO(csilvers): VLOG(1) the tls status once we support RAW_VLOG
}
# endif // HAVE_DECL_UNAME
#endif // HAVE_TLS
(In reply to comment #2) > I am building it with HAVE_TLS defined because TLS is *much* faster than > pthread_getspecific() on my platform. Ok. You’ll probably want to be aware of the fact that this code-path is completely untested in our version of TCMalloc. > Also looking at the code, I don't think you are correct. You’re right, I didn’t match up the nested #if’s correctly. It’s still true that HAVE_TLS is never defined on any platforms that build from the WebKit tree. In that case, what is the reason for having the HAVE_TLS checks in there at all? That code is present in the upstream version of TCMalloc. |
It is possible to have a missing return value for the case where HAVE_TLS is defined, but KernelSupportsTLS() returns false: inline TCMalloc_ThreadCache* TCMalloc_ThreadCache::GetThreadHeap() { #ifdef HAVE_TLS // __thread is faster, but only when the kernel supports it if (KernelSupportsTLS()) return threadlocal_heap; #elif COMPILER(MSVC) return static_cast<TCMalloc_ThreadCache*>(TlsGetValue(tlsIndex)); #else return static_cast<TCMalloc_ThreadCache*>(pthread_getspecific(heap_key)); #endif }