Bug 37299 - Missing return value in TCMalloc_ThreadCache::GetThreadHeap()
Summary: Missing return value in TCMalloc_ThreadCache::GetThreadHeap()
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P5 Trivial
Assignee: Nobody
Depends on:
Reported: 2010-04-08 16:35 PDT by Benbuck Nason
Modified: 2010-04-08 19:07 PDT (History)
0 users

See Also:


Note You need to log in before you can comment on or make changes to this bug.
Description Benbuck Nason 2010-04-08 16:35:12 PDT
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;
    return static_cast<TCMalloc_ThreadCache*>(TlsGetValue(tlsIndex));
    return static_cast<TCMalloc_ThreadCache*>(pthread_getspecific(heap_key));
Comment 1 Mark Rowe (bdash) 2010-04-08 16:45:37 PDT
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.
Comment 2 Benbuck Nason 2010-04-08 16:53:05 PDT
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;
          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
Comment 3 Mark Rowe (bdash) 2010-04-08 18:01:18 PDT
(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.
Comment 4 Benbuck Nason 2010-04-08 18:03:13 PDT
In that case, what is the reason for having the HAVE_TLS checks in there at all?
Comment 5 Mark Rowe (bdash) 2010-04-08 19:07:58 PDT
That code is present in the upstream version of TCMalloc.