I agree with Jim, the extra lldb::target::addr_t is a bit too much since "lldb" is a debugger so any types it generates should be used for doing debugging a target. I don't mind the "lldb::host" namespace though, as this would clear things up a bit and those don't appear in the public API at all and rarely in internal headers.
> On Jun 27, 2014, at 10:02 AM, Zachary Turner <[email protected]> wrote: > > Thanks for the clarification. I had a similar idea related to your host > namespace. What about taking it one step further and making all the host / > target specific types live in either a lldb::host or lldb::target? So for > example this: > > namespace lldb > { > typedef uint64_t addr_t; > typedef uint64_t user_id_t; > typedef uint64_t pid_t; > typedef uint64_t tid_t; > typedef uint64_t offset_t; > typedef int32_t break_id_t; > typedef int32_t watch_id_t; > typedef void * clang_type_t; > typedef uint64_t queue_id_t; > } > > would become this: > > namespace lldb > { > namespace target > { > typedef uint64_t addr_t; > typedef uint64_t user_id_t; > typedef uint64_t pid_t; > typedef uint64_t tid_t; > typedef uint64_t offset_t; > } > > typedef int32_t break_id_t; > typedef int32_t watch_id_t; > typedef void * clang_type_t; > typedef uint64_t queue_id_t; > } > > I'm guessing there's already a number of inconsistencies related to using > these target types to reference things on the host, and vice versa. Making > the distinction more clear in both directions would probably be helpful. > > thoughts? > > > On Fri, Jun 27, 2014 at 9:47 AM, Greg Clayton <[email protected]> wrote: > > > On Jun 26, 2014, at 7:50 PM, Zachary Turner <[email protected]> wrote: > > > > Is there a clear explanation of the intention of each of lldb's typedefs? > > > > I've found a number of issues on Windows related to incorrect type usage, > > but the purpose of the types is ambiguous enough that I'm not yet sure how > > to fix these issues correctly. Some of the issues I've run into are: > > > > 1) We use off_t in some places. I think this is almost always an error, as > > off_t's purpose is to deal with files and only in a limited number of cases > > do we actually deal with files. > > ::off_t should only be used for cases where we are dealing with native files. > Anything else should use a value that is large enough to represent an offset > for _any_ target we debug. > > > 2) I'm not sure what lldb::offset_t is supposed to represent. Is it an > > offset in a process's address space? Then why not just use size_t in that > > case? > > I believe size_t can be 32 bit on 32 bit systems. If we are talking about an > offset, it needs to be large enough to represent an offset for a 64 bit > process being debugged in a 32 bit built lldb. > > > 3) Why is lldb::addr_t always 64-bit? I assume it's because a target > > process can be either 32 or 64-bit, is this just for convenience so that we > > only need a single type to represent a pointer in the target process? > > Yes. LLDB can debug any process even when we build lldb in 32 bit mode. The > addresses must be able to represent _any_ address from any target so > lldb::addr_t must be big enough to represent the largest address for any > target we support which is currently 64 bit. > > > 4) Windows has separate notions of pids and process handles, and similarly > > for threads. I'd like to abstract this into separate typedefs, if there's > > no objections (on non-Windows platforms, pid_t would just be the same type > > as process_handle_t), and update various methods to take handles instead of > > pids. > > There are some defines that aren't clear. lldb::pid_t is one of them. Right > now lldb::pid_t and lldb::tid_t are supposed to be able to represent any pid > or tid for any target we support. This is why they are 64 bit. > > As far as "pid_t" goes, do not change this. We used the longer > "lldb::thread_t" to represent a native host thread. These specific defines > are all defined in lldb-types.h: > > #ifdef _WIN32 > > #include <process.h> > > namespace lldb > { > typedef void* mutex_t; > typedef void* condition_t; > typedef void* rwlock_t; > typedef uintptr_t thread_t; // Host thread > type > typedef uint32_t thread_key_t; > typedef void * thread_arg_t; // Host thread > argument type > typedef unsigned thread_result_t; // Host thread > result type > typedef thread_result_t (*thread_func_t)(void *); // Host thread > function type > } > > #else > > #include <pthread.h> > > namespace lldb > { > //---------------------------------------------------------------------- > // MacOSX Types > //---------------------------------------------------------------------- > typedef ::pthread_mutex_t mutex_t; > typedef pthread_cond_t condition_t; > typedef pthread_rwlock_t rwlock_t; > typedef pthread_t thread_t; // Host thread > type > typedef pthread_key_t thread_key_t; > typedef void * thread_arg_t; // Host thread > argument type > typedef void * thread_result_t; // Host thread > result type > typedef void * (*thread_func_t)(void *); // Host thread > function type > } // namespace lldb > > #endif > > > These are the ones that change from system to system. > > The ones below are designed to be used for all platforms no matter what they > are and these defines must work for all targets: > > > namespace lldb > { > typedef uint64_t addr_t; > typedef uint64_t user_id_t; > typedef uint64_t pid_t; > typedef uint64_t tid_t; > typedef uint64_t offset_t; > typedef int32_t break_id_t; > typedef int32_t watch_id_t; > typedef void * clang_type_t; > typedef uint64_t queue_id_t; > } > > > So feel free to add a "typedef process_handle_t process_t;" for windows and > add a "typedef ::pid_t process_t;" for the #else clause. We could make this > clearer by adding an extra "host" namespace for all of these: > > > #ifdef _WIN32 > > #include <process.h> > > namespace lldb > { > namespace host > { > typedef void* mutex_t; > typedef void* condition_t; > typedef void* rwlock_t; > typedef uintptr_t thread_t; // Host > thread type > typedef uint32_t thread_key_t; > typedef void * thread_arg_t; // Host > thread argument type > typedef unsigned thread_result_t; // Host > thread result type > typedef thread_result_t (*thread_func_t)(void *); // Host > thread function type > } > } > > #else > > #include <pthread.h> > > namespace lldb > { > namespace host > { > > //---------------------------------------------------------------------- > // MacOSX Types > > //---------------------------------------------------------------------- > typedef ::pthread_mutex_t mutex_t; > typedef pthread_cond_t condition_t; > typedef pthread_rwlock_t rwlock_t; > typedef pthread_t thread_t; // Host > thread type > typedef pthread_key_t thread_key_t; > typedef void * thread_arg_t; // Host > thread argument type > typedef void * thread_result_t; // Host > thread result type > typedef void * (*thread_func_t)(void *); // Host > thread function type > } > } // namespace lldb > > #endif > > > Then we could overload the lldb::host::pid_t (::pid_t or ::process_handle_t > for windows) from lldb::pid_t (always uint64_t or largest unit required to > hold a process ID for any target). > > > So to sum up: > > for case #1 above, if there are places people are using ::off_t for something > that might be too small for any target, switch it over to use lldb::offset_t. > for case #2 leave lldb::offset_t alone, size_t can be 32 bits in 32 bit > builds, and lldb::offset_t represents an offset that must be valid for all > targets so 64 bit is required > for case #3 it should be obvious: lldb::addr_t must be able to handle any > address for any target and must be big enough for the largest address we > support for any target even in a 32 bit build > for case #4 feel free to add a "lldb::process_t" typedef and set it > accordingly. Feel free to add the "host" namespace if this makes things more > clear. If we add the "host" namespace, we can then add definitions for > lldb::host::pid_t and lldb::host::tid_t which can match the current systems > native notion of what those are. > > > Greg > _______________________________________________ lldb-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
