ValveSoftware / vogl

OpenGL capture / playback debugger.
MIT License
1.42k stars 126 forks source link

[build] Issue with 32-bit build (i686) #185

Open kingtaurus opened 9 years ago

kingtaurus commented 9 years ago

I'm trying to build vogl on an i686 Ubuntu 14.04 machine

gking@ubuntu:~/Programming/vogl/clang$ uname -a
Linux ubuntu 3.13.0-41-generic #70-Ubuntu SMP Tue Nov 25 14:41:08 UTC 2014 i686 i686 i686 GNU/Linux
gking@ubuntu:/etc$ cat os-release 
NAME="Ubuntu"
VERSION="14.04.1 LTS, Trusty Tahr"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 14.04.1 LTS"
VERSION_ID="14.04"
HOME_URL="http://www.ubuntu.com/"
SUPPORT_URL="http://help.ubuntu.com/"
BUG_REPORT_URL="http://bugs.launchpad.net/ubuntu/"

So, pulling from vogl/master (making sure I'm up to date):

gking@ubuntu:~/Programming/vogl/clang$ date && git pull upstream master
Wed Jan  7 21:05:25 PST 2015
From https://github.com/ValveSoftware/vogl
 * branch            master     -> FETCH_HEAD
Already up-to-date.

Build Error:

/home/gking/Programming/vogl/src/voglcore/vogl_port_posix.cpp:111:12: error: reinterpret_cast from 'pthread_t' (aka 'unsigned long') to
      'uintptr_t' (aka 'unsigned int') is not allowed [Semantic Issue]
    return reinterpret_cast<uintptr_t>(pthread_self());
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This issue arises because (/usr/include/stdint.h):

/* Types for `void *' pointers.  */
#if __WORDSIZE == 64
# ifndef __intptr_t_defined
typedef long int                intptr_t;
#  define __intptr_t_defined
# endif
typedef unsigned long int       uintptr_t;
#else
# ifndef __intptr_t_defined
typedef int                     intptr_t;
#  define __intptr_t_defined
# endif
typedef unsigned int            uintptr_t;
#endif

Now pthread_t (/usr/include/i386-linux-gnu/bits/pthreadtypes.h):

/* Thread identifiers.  The structure of the attribute type is not
   exposed on purpose.  */
typedef unsigned long int pthread_t;

I believe that unsigned long int in this case is the same type as unsigned int (i386) - thus the reinterpret_cast<> is invalid. I can write up a simple fix (just surprised that no one else has stumbled on this) - however I feel that I should report this first.

Edit: improved formatting and fixed a few small mistakes

computerquip commented 9 years ago

Issue https://github.com/ValveSoftware/vogl/issues/197 also includes solutions. Apologies for duplicating.

kingtaurus commented 9 years ago

So, pthread_self() returns pthread_t (according to pthread_self specification). So I'm confused as to why the reinterpret_cast<> is required.

computerquip commented 9 years ago

pthread_t is an abstract type. It can be, quite literally, anything. EDIT: Well, for what it's worth, I'd say it's healthier for the long term to avoid the reinterpret cast in this case. Some minor design changes can fix the issue but I'm not really sure what would be a desirable way to go about it without some insight.

kingtaurus commented 9 years ago

Well the options are to avoid the cast altogether: (1) both 32-bit and 64-bit linux define a pthread_t through the following typedef:

typedef unsigned long int pthread_t;

See /usr/include/x86_64-linux-gnu/bits/pthreadtypes.h and /usr/include/i386-linux-gnu/bits/pthreadtypes.h; which should convert directly to uint64_t without issue.

This would require OS X specific code to handle the pthread_t (and its associated pthread_t definition). _pthread_types.h , pthread.h

typedef __darwin_pthread_t      pthread_t;

and

typedef struct _opaque_pthread_t
            *__darwin_pthread_t;    /* [???] Used for pthreads */

finally,

struct _opaque_pthread_t {
    long __sig;
    struct __darwin_pthread_handler_rec  *__cleanup_stack;
    char __opaque[__PTHREAD_SIZE__];
};

where

#if defined(__LP64__)
#define __PTHREAD_SIZE__        8176
//...
#else // !__LP64__
#define __PTHREAD_SIZE__        4088
//...
#endif

and

struct __darwin_pthread_handler_rec {
    void (*__routine)(void *);  // Routine to call
    void *__arg;            // Argument to pass
    struct __darwin_pthread_handler_rec *__next;
};

Although this information is really recent, it appears that the storage class is a signed long (so, unpacking this from the pthread_t would convert directly to uint64_t easily). Further the reinterpret_cast<> just converts the address of _opaque_pthread_t that is allocated on the heap and not the actual pthread_t id (which most likely is a bug).

(2) Move to C++11 (or boost::thread)and use std::thread, std::mutex, and what not. This would probably require a re-write within the core libraries (and a lot more testing).