Merge pull request #363 from DrChat/obj_handle_ref_counting

Track object handle reference counts in the object table.

src/xenia/kernel/object_table.cc

@@ -22,13 +22,12 @@ ObjectTable::ObjectTable()
     : table_capacity_(0), table_(nullptr), last_free_entry_(0) {}
 
 ObjectTable::~ObjectTable() {
-  std::lock_guard<xe::mutex> lock(table_mutex_);
+  std::lock_guard<xe::recursive_mutex> lock(table_mutex_);
 
   // Release all objects.
   for (uint32_t n = 0; n < table_capacity_; n++) {
     ObjectTableEntry& entry = table_[n];
     if (entry.object) {
-      entry.object->ReleaseHandle();
       entry.object->Release();
     }
   }
@@ -90,7 +89,7 @@ X_STATUS ObjectTable::AddHandle(XObject* object, X_HANDLE* out_handle) {
 
   uint32_t slot = 0;
   {
-    std::lock_guard<xe::mutex> lock(table_mutex_);
+    std::lock_guard<xe::recursive_mutex> lock(table_mutex_);
 
     // Find a free slot.
     result = FindFreeSlot(&slot);
@@ -99,9 +98,9 @@ X_STATUS ObjectTable::AddHandle(XObject* object, X_HANDLE* out_handle) {
     if (XSUCCEEDED(result)) {
       ObjectTableEntry& entry = table_[slot];
       entry.object = object;
+      entry.handle_ref_count = 1;
 
       // Retain so long as the object is in the table.
-      object->RetainHandle();
       object->Retain();
     }
   }
@@ -128,6 +127,36 @@ X_STATUS ObjectTable::DuplicateHandle(X_HANDLE handle, X_HANDLE* out_handle) {
   return result;
 }
 
+X_STATUS ObjectTable::RetainHandle(X_HANDLE handle) {
+  std::lock_guard<xe::recursive_mutex> lock(table_mutex_);
+
+  ObjectTableEntry* entry = LookupTable(handle);
+  if (!entry) {
+    return X_STATUS_INVALID_HANDLE;
+  }
+
+  entry->handle_ref_count++;
+  return X_STATUS_SUCCESS;
+}
+
+X_STATUS ObjectTable::ReleaseHandle(X_HANDLE handle) {
+  std::lock_guard<xe::recursive_mutex> lock(table_mutex_);
+
+  ObjectTableEntry* entry = LookupTable(handle);
+  if (!entry) {
+    return X_STATUS_INVALID_HANDLE;
+  }
+
+  if (--entry->handle_ref_count == 0) {
+    // No more references. Remove it from the table.
+    return RemoveHandle(handle);
+  }
+
+  // FIXME: Return a status code telling the caller it wasn't released
+  // (but not a failure code)
+  return X_STATUS_SUCCESS;
+}
+
 X_STATUS ObjectTable::RemoveHandle(X_HANDLE handle) {
   X_STATUS result = X_STATUS_SUCCESS;
 
@@ -138,7 +167,7 @@ X_STATUS ObjectTable::RemoveHandle(X_HANDLE handle) {
 
   XObject* object = NULL;
   {
-    std::lock_guard<xe::mutex> lock(table_mutex_);
+    std::lock_guard<xe::recursive_mutex> lock(table_mutex_);
 
     // Lower 2 bits are ignored.
     uint32_t slot = handle >> 2;
@@ -167,6 +196,23 @@ X_STATUS ObjectTable::RemoveHandle(X_HANDLE handle) {
   return result;
 }
 
+ObjectTable::ObjectTableEntry* ObjectTable::LookupTable(X_HANDLE handle) {
+  handle = TranslateHandle(handle);
+  if (!handle) {
+    return nullptr;
+  }
+
+  std::lock_guard<xe::recursive_mutex> lock(table_mutex_);
+
+  // Lower 2 bits are ignored.
+  uint32_t slot = handle >> 2;
+  if (slot <= table_capacity_) {
+    return &table_[slot];
+  }
+
+  return nullptr;
+}
+
 XObject* ObjectTable::LookupObject(X_HANDLE handle, bool already_locked) {
   handle = TranslateHandle(handle);
   if (!handle) {
@@ -203,7 +249,7 @@ XObject* ObjectTable::LookupObject(X_HANDLE handle, bool already_locked) {
 
 void ObjectTable::GetObjectsByType(XObject::Type type,
                                    std::vector<object_ref<XObject>>& results) {
-  std::lock_guard<xe::mutex> lock(table_mutex_);
+  std::lock_guard<xe::recursive_mutex> lock(table_mutex_);
   for (uint32_t slot = 0; slot < table_capacity_; ++slot) {
     auto& entry = table_[slot];
     if (entry.object) {
@@ -229,7 +275,7 @@ X_HANDLE ObjectTable::TranslateHandle(X_HANDLE handle) {
 }
 
 X_STATUS ObjectTable::AddNameMapping(const std::string& name, X_HANDLE handle) {
-  std::lock_guard<xe::mutex> lock(table_mutex_);
+  std::lock_guard<xe::recursive_mutex> lock(table_mutex_);
   if (name_table_.count(name)) {
     return X_STATUS_OBJECT_NAME_COLLISION;
   }
@@ -238,7 +284,7 @@ X_STATUS ObjectTable::AddNameMapping(const std::string& name, X_HANDLE handle) {
 }
 
 void ObjectTable::RemoveNameMapping(const std::string& name) {
-  std::lock_guard<xe::mutex> lock(table_mutex_);
+  std::lock_guard<xe::recursive_mutex> lock(table_mutex_);
   auto it = name_table_.find(name);
   if (it != name_table_.end()) {
     name_table_.erase(it);
@@ -247,7 +293,7 @@ void ObjectTable::RemoveNameMapping(const std::string& name) {
 
 X_STATUS ObjectTable::GetObjectByName(const std::string& name,
                                       X_HANDLE* out_handle) {
-  std::lock_guard<xe::mutex> lock(table_mutex_);
+  std::lock_guard<xe::recursive_mutex> lock(table_mutex_);
   auto it = name_table_.find(name);
   if (it == name_table_.end()) {
     *out_handle = X_INVALID_HANDLE_VALUE;

src/xenia/kernel/object_table.h

@@ -28,7 +28,10 @@ class ObjectTable {
 
   X_STATUS AddHandle(XObject* object, X_HANDLE* out_handle);
   X_STATUS DuplicateHandle(X_HANDLE orig, X_HANDLE* out_handle);
+  X_STATUS RetainHandle(X_HANDLE handle);
+  X_STATUS ReleaseHandle(X_HANDLE handle);
   X_STATUS RemoveHandle(X_HANDLE handle);
+
   template <typename T>
   object_ref<T> LookupObject(X_HANDLE handle) {
     auto object = LookupObject(handle, false);
@@ -48,6 +51,12 @@ class ObjectTable {
   }
 
  private:
+  typedef struct {
+    int handle_ref_count = 0;
+    XObject* object = nullptr;
+  } ObjectTableEntry;
+
+  ObjectTableEntry* LookupTable(X_HANDLE handle);
   XObject* LookupObject(X_HANDLE handle, bool already_locked);
   void GetObjectsByType(XObject::Type type,
                         std::vector<object_ref<XObject>>& results);
@@ -55,9 +64,7 @@ class ObjectTable {
   X_HANDLE TranslateHandle(X_HANDLE handle);
   X_STATUS FindFreeSlot(uint32_t* out_slot);
 
-  typedef struct { XObject* object; } ObjectTableEntry;
+  xe::recursive_mutex table_mutex_;
-
-  xe::mutex table_mutex_;
   uint32_t table_capacity_;
   ObjectTableEntry* table_;
   uint32_t last_free_entry_;

src/xenia/kernel/objects/xthread.cc

@@ -292,6 +292,7 @@ X_STATUS XThread::Create() {
 
   // Always retain when starting - the thread owns itself until exited.
   Retain();
+  RetainHandle();
 
   xe::threading::Thread::CreationParameters params;
   params.stack_size = 16 * 1024 * 1024;  // Ignore game, always big!
@@ -350,6 +351,7 @@ X_STATUS XThread::Exit(int exit_code) {
 
   running_ = false;
   Release();
+  ReleaseHandle();
 
   // NOTE: this does not return!
   xe::threading::Thread::Exit(exit_code);
@@ -361,6 +363,7 @@ X_STATUS XThread::Terminate(int exit_code) {
 
   running_ = false;
   Release();
+  ReleaseHandle();
 
   thread_->Terminate(exit_code);
   return X_STATUS_SUCCESS;

src/xenia/kernel/xboxkrnl_ob.cc

@@ -190,8 +190,7 @@ dword_result_t NtDuplicateObject(dword_t handle, lpdword_t new_handle_ptr,
 DECLARE_XBOXKRNL_EXPORT(NtDuplicateObject, ExportTag::kImplemented);
 
 dword_result_t NtClose(dword_t handle) {
-  // FIXME: This needs to be removed once handle count reaches 0!
+  return kernel_state()->object_table()->ReleaseHandle(handle);
-  return kernel_state()->object_table()->RemoveHandle(handle);
 }
 DECLARE_XBOXKRNL_EXPORT(NtClose, ExportTag::kImplemented);
 

src/xenia/kernel/xobject.cc

@@ -21,7 +21,6 @@ namespace kernel {
 
 XObject::XObject(KernelState* kernel_state, Type type)
     : kernel_state_(kernel_state),
-      handle_ref_count_(0),
       pointer_ref_count_(1),
       type_(type),
       handle_(X_INVALID_HANDLE_VALUE),
@@ -34,7 +33,6 @@ XObject::XObject(KernelState* kernel_state, Type type)
 }
 
 XObject::~XObject() {
-  assert_zero(handle_ref_count_);
   assert_zero(pointer_ref_count_);
 
   if (allocated_guest_object_) {
@@ -58,20 +56,20 @@ XObject::Type XObject::type() { return type_; }
 
 X_HANDLE XObject::handle() const { return handle_; }
 
-void XObject::RetainHandle() { ++handle_ref_count_; }
+void XObject::RetainHandle() {
+  kernel_state_->object_table()->RetainHandle(handle_);
+}
 
 bool XObject::ReleaseHandle() {
-  if (--handle_ref_count_ == 0) {
+  // FIXME: Return true when handle is actually released.
-    return true;
+  return kernel_state_->object_table()->ReleaseHandle(handle_) ==
-  }
+         X_STATUS_SUCCESS;
-  return false;
 }
 
 void XObject::Retain() { ++pointer_ref_count_; }
 
 void XObject::Release() {
   if (--pointer_ref_count_ == 0) {
-    assert_true(pointer_ref_count_ >= handle_ref_count_);
     delete this;
   }
 }

src/xenia/kernel/xobject.h

@@ -180,7 +180,6 @@ class XObject {
   KernelState* kernel_state_;
 
  private:
-  std::atomic<int32_t> handle_ref_count_;
   std::atomic<int32_t> pointer_ref_count_;
 
   Type type_;