From 9645ad6d245f719845305ffdad05a4c8c18ffe5d Mon Sep 17 00:00:00 2001 From: Jesse Jones Date: Sat, 8 Dec 2012 20:10:39 -0800 Subject: [PATCH 1/7] Improvements to array_list, hash_map, and indexed_list: * Disabled copying. * Added const where appropiate. --- src/rt/util/array_list.h | 31 ++++++++++++++++++++++++------- src/rt/util/hash_map.h | 18 +++++++++++------- src/rt/util/indexed_list.h | 12 ++++++++++-- 3 files changed, 45 insertions(+), 16 deletions(-) diff --git a/src/rt/util/array_list.h b/src/rt/util/array_list.h index 6321611c81c98..c9531d1a0b237 100644 --- a/src/rt/util/array_list.h +++ b/src/rt/util/array_list.h @@ -6,25 +6,32 @@ #include /** - * A simple, resizable array list. + * A simple, resizable array list. Note that this only works with POD types + * (because data is grown via realloc). */ template class array_list { static const size_t INITIAL_CAPACITY = 8; size_t _size; T * _data; size_t _capacity; +private: + // private and left undefined to disable copying + array_list(const array_list& rhs); + array_list& operator=(const array_list& rhs); public: array_list(); ~array_list(); - size_t size(); + size_t size() const; int32_t append(T value); int32_t push(T value); bool pop(T *value); bool replace(T old_value, T new_value); - int32_t index_of(T value); - bool is_empty(); + int32_t index_of(T value) const; + bool is_empty() const; T* data(); + const T* data() const; T & operator[](size_t index); + const T & operator[](size_t index) const; }; template @@ -40,7 +47,7 @@ array_list::~array_list() { } template size_t -array_list::size() { +array_list::size() const { return _size; } @@ -87,7 +94,7 @@ array_list::replace(T old_value, T new_value) { } template int32_t -array_list::index_of(T value) { +array_list::index_of(T value) const { for (size_t i = 0; i < _size; i++) { if (_data[i] == value) { return i; @@ -101,8 +108,13 @@ array_list::operator[](size_t index) { return _data[index]; } +template const T & +array_list::operator[](size_t index) const { + return _data[index]; +} + template bool -array_list::is_empty() { +array_list::is_empty() const { return _size == 0; } @@ -111,4 +123,9 @@ array_list::data() { return _data; } +template const T* +array_list::data() const { + return _data; +} + #endif /* ARRAY_LIST_H */ diff --git a/src/rt/util/hash_map.h b/src/rt/util/hash_map.h index 6e8afbece2d52..253a7a06fb7b7 100644 --- a/src/rt/util/hash_map.h +++ b/src/rt/util/hash_map.h @@ -26,6 +26,10 @@ template class hash_map { UT_hash_handle hh; }; map_entry * _head; +private: + // private and left undefined to disable copying + hash_map(const hash_map& rhs); + hash_map& operator=(const hash_map& rhs); public: hash_map(); ~hash_map(); @@ -54,7 +58,7 @@ template class hash_map { * true if the value was found and updates the specified *value parameter * with the associated value, or false otherwise. */ - bool get(K key, V *value); + bool get(K key, V *value) const; /** * Removes a key-value pair from this hash map. @@ -71,7 +75,7 @@ template class hash_map { * returns: * true if the specified key exists in this hash map, or false otherwise. */ - bool contains(K key); + bool contains(K key) const; /** * Removes the value associated with the specified key from this hash map. @@ -86,9 +90,9 @@ template class hash_map { /** * Returns the number of key-value pairs in this hash map. */ - size_t count(); + size_t count() const; - bool is_empty() { + bool is_empty() const { return count() == 0; } @@ -124,7 +128,7 @@ hash_map::put(K key, V value) { } template bool -hash_map::get(K key, V *value) { +hash_map::get(K key, V *value) const { map_entry *entry = NULL; HASH_FIND(hh, _head, &key, sizeof(K), entry); if (entry == NULL) { @@ -146,7 +150,7 @@ hash_map::set(K key, V value) { } template bool -hash_map::contains(K key) { +hash_map::contains(K key) const { V value; return get(key, &value); } @@ -184,7 +188,7 @@ hash_map::remove(K key) { } template size_t -hash_map::count() { +hash_map::count() const { return HASH_CNT(hh, _head); } diff --git a/src/rt/util/indexed_list.h b/src/rt/util/indexed_list.h index aae6ecb8a7811..51e94b8ba2656 100644 --- a/src/rt/util/indexed_list.h +++ b/src/rt/util/indexed_list.h @@ -45,14 +45,15 @@ template class indexed_list { * Same as pop(), except that it returns NULL if the list is empty. */ virtual T* pop_value(); - virtual size_t length() { + virtual size_t length() const { return list.size(); } - virtual bool is_empty() { + virtual bool is_empty() const { return list.is_empty(); } virtual int32_t remove(T* value); virtual T * operator[](int32_t index); + virtual const T * operator[](int32_t index) const; virtual ~indexed_list() {} }; @@ -104,4 +105,11 @@ indexed_list::operator[](int32_t index) { return value; } +template const T * +indexed_list::operator[](int32_t index) const { + T *value = list[index]; + assert(value->list_index == index); + return value; +} + #endif /* INDEXED_LIST_H */ From 0266474637d9b98170bc1223f7f413579ece3112 Mon Sep 17 00:00:00 2001 From: Jesse Jones Date: Sat, 8 Dec 2012 20:12:12 -0800 Subject: [PATCH 2/7] polymorphic indexed_list fixes: 1) indexed_list no longer has virtual methods. It's not actually subclassed and there is very rarely good reason to subclass collection classes. 2) Added a virtual dtor to indexed_list_object which is intended to be subclassed. This allows derived dtors to be called if the object is deleted with a indexed_list_object*. --- src/rt/util/indexed_list.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/rt/util/indexed_list.h b/src/rt/util/indexed_list.h index 51e94b8ba2656..88f4f2776186b 100644 --- a/src/rt/util/indexed_list.h +++ b/src/rt/util/indexed_list.h @@ -17,6 +17,7 @@ class indexed_list_object { public: + virtual ~indexed_list_object() {} int32_t list_index; }; @@ -39,22 +40,22 @@ class indexed_list_element : public indexed_list_object { template class indexed_list { array_list list; public: - virtual int32_t append(T *value); - virtual bool pop(T **value); + int32_t append(T *value); + bool pop(T **value); /** * Same as pop(), except that it returns NULL if the list is empty. */ - virtual T* pop_value(); - virtual size_t length() const { + T* pop_value(); + size_t length() const { return list.size(); } - virtual bool is_empty() const { + bool is_empty() const { return list.is_empty(); } - virtual int32_t remove(T* value); - virtual T * operator[](int32_t index); - virtual const T * operator[](int32_t index) const; - virtual ~indexed_list() {} + int32_t remove(T* value); + T * operator[](int32_t index); + const T * operator[](int32_t index) const; + ~indexed_list() {} }; template int32_t From 306f510e49ab8bd0960963908b213ae080fb7afe Mon Sep 17 00:00:00 2001 From: Jesse Jones Date: Sat, 8 Dec 2012 21:34:26 -0800 Subject: [PATCH 3/7] Check for realloc failure and bad subscripts --- src/rt/util/array_list.h | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/rt/util/array_list.h b/src/rt/util/array_list.h index c9531d1a0b237..30996a91aea56 100644 --- a/src/rt/util/array_list.h +++ b/src/rt/util/array_list.h @@ -4,6 +4,7 @@ #include #include +#include /** * A simple, resizable array list. Note that this only works with POD types @@ -59,8 +60,12 @@ array_list::append(T value) { template int32_t array_list::push(T value) { if (_size == _capacity) { - _capacity = _capacity * 2; - _data = (T *) realloc(_data, _capacity * sizeof(T)); + size_t new_capacity = _capacity * 2; + void* buffer = realloc(_data, new_capacity * sizeof(T)); + if (buffer == NULL) + throw std::bad_alloc(); + _data = (T *) buffer; + _capacity = new_capacity; } _data[_size ++] = value; return _size - 1; @@ -105,11 +110,13 @@ array_list::index_of(T value) const { template T & array_list::operator[](size_t index) { + assert(index < size()); return _data[index]; } template const T & array_list::operator[](size_t index) const { + assert(index < size()); return _data[index]; } From 2b6603aa68844ce3f2391ccaf9e2050ecb295690 Mon Sep 17 00:00:00 2001 From: Jesse Jones Date: Sat, 8 Dec 2012 21:45:43 -0800 Subject: [PATCH 4/7] Instead of returning a bool (which everyone ignored) pop asserts --- src/rt/util/array_list.h | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/rt/util/array_list.h b/src/rt/util/array_list.h index 30996a91aea56..f8014e48cb247 100644 --- a/src/rt/util/array_list.h +++ b/src/rt/util/array_list.h @@ -25,7 +25,7 @@ template class array_list { size_t size() const; int32_t append(T value); int32_t push(T value); - bool pop(T *value); + void pop(T *value); bool replace(T old_value, T new_value); int32_t index_of(T value) const; bool is_empty() const; @@ -71,17 +71,14 @@ array_list::push(T value) { return _size - 1; } -template bool +template void array_list::pop(T *value) { - if (_size == 0) { - return false; - } + assert(_size > 0); if (value != NULL) { *value = _data[-- _size]; } else { -- _size; } - return true; } /** From 9122c1498e7ebf97e7485836ac1ac178ba5aa084 Mon Sep 17 00:00:00 2001 From: Jesse Jones Date: Sat, 8 Dec 2012 22:06:46 -0800 Subject: [PATCH 5/7] Made a bunch more classes non-copyable --- src/rt/boxed_region.h | 5 +++++ src/rt/circular_buffer.h | 5 +++++ src/rt/memory_region.h | 5 +++++ src/rt/rust_sched_launcher.h | 5 +++++ src/rt/rust_sched_loop.h | 5 +++++ src/rt/rust_scheduler.h | 5 +++++ src/rt/rust_signal.h | 6 ++++++ src/rt/rust_task.h | 5 +++++ 8 files changed, 41 insertions(+) diff --git a/src/rt/boxed_region.h b/src/rt/boxed_region.h index 8e7203b723aff..5578a7198b055 100644 --- a/src/rt/boxed_region.h +++ b/src/rt/boxed_region.h @@ -35,6 +35,11 @@ class boxed_region { return v; } +private: + // private and undefined to disable copying + boxed_region(const boxed_region& rhs); + boxed_region& operator=(const boxed_region& rhs); + public: boxed_region(rust_env *e, memory_region *br) : env(e) diff --git a/src/rt/circular_buffer.h b/src/rt/circular_buffer.h index c54ee8a33e41e..695b42ede7f11 100644 --- a/src/rt/circular_buffer.h +++ b/src/rt/circular_buffer.h @@ -25,6 +25,11 @@ circular_buffer : public kernel_owned { bool is_empty(); size_t size(); +private: + // private and undefined to disable copying + circular_buffer(const circular_buffer& rhs); + circular_buffer& operator=(const circular_buffer& rhs); + private: size_t initial_size(); void grow(); diff --git a/src/rt/memory_region.h b/src/rt/memory_region.h index be8689672a897..ac91f5ca74066 100644 --- a/src/rt/memory_region.h +++ b/src/rt/memory_region.h @@ -59,6 +59,11 @@ class memory_region { void release_alloc(void *mem); void claim_alloc(void *mem); +private: + // private and undefined to disable copying + memory_region(const memory_region& rhs); + memory_region& operator=(const memory_region& rhs); + public: memory_region(rust_env *env, bool synchronized); memory_region(memory_region *parent); diff --git a/src/rt/rust_sched_launcher.h b/src/rt/rust_sched_launcher.h index 72c9fd2dd36fa..d308fe31c1ad8 100644 --- a/src/rt/rust_sched_launcher.h +++ b/src/rt/rust_sched_launcher.h @@ -13,6 +13,11 @@ class rust_sched_launcher : public kernel_owned { private: rust_sched_loop sched_loop; +private: + // private and undefined to disable copying + rust_sched_launcher(const rust_sched_launcher& rhs); + rust_sched_launcher& operator=(const rust_sched_launcher& rhs); + protected: rust_sched_driver driver; diff --git a/src/rt/rust_sched_loop.h b/src/rt/rust_sched_loop.h index b664f2f3f7bc8..2d47ed1bef2b2 100644 --- a/src/rt/rust_sched_loop.h +++ b/src/rt/rust_sched_loop.h @@ -72,6 +72,11 @@ struct rust_sched_loop void pump_loop(); +private: + // private and undefined to disable copying + rust_sched_loop(const rust_sched_loop& rhs); + rust_sched_loop& operator=(const rust_sched_loop& rhs); + public: rust_kernel *kernel; rust_scheduler *sched; diff --git a/src/rt/rust_scheduler.h b/src/rt/rust_scheduler.h index 019f69f7a3160..b94a71ad33502 100644 --- a/src/rt/rust_scheduler.h +++ b/src/rt/rust_scheduler.h @@ -48,6 +48,11 @@ class rust_scheduler : public kernel_owned { // Called when refcount reaches zero void delete_this(); +private: + // private and undefined to disable copying + rust_scheduler(const rust_scheduler& rhs); + rust_scheduler& operator=(const rust_scheduler& rhs); + public: rust_scheduler(rust_kernel *kernel, size_t max_num_threads, rust_sched_id id, bool allow_exit, bool killed, diff --git a/src/rt/rust_signal.h b/src/rt/rust_signal.h index 0f6ecb943035f..bfea68a1aad50 100644 --- a/src/rt/rust_signal.h +++ b/src/rt/rust_signal.h @@ -16,6 +16,12 @@ class rust_signal { public: virtual void signal() = 0; virtual ~rust_signal() {} + rust_signal() {} + +private: + // private and undefined to disable copying + rust_signal(const rust_signal& rhs); + rust_signal& operator=(const rust_signal& rhs); }; #endif /* RUST_SIGNAL_H */ diff --git a/src/rt/rust_task.h b/src/rt/rust_task.h index e71e1b3707a85..4feda9937cd3b 100644 --- a/src/rt/rust_task.h +++ b/src/rt/rust_task.h @@ -295,6 +295,11 @@ rust_task : public kernel_owned void wakeup_inner(rust_cond *from); bool blocked_on(rust_cond *cond); +private: + // private and undefined to disable copying + rust_task(const rust_task& rhs); + rust_task& operator=(const rust_task& rhs); + public: // Only a pointer to 'name' is kept, so it must live as long as this task. From c7760fd4028066e8e6bd8f375b6ab92790d2b795 Mon Sep 17 00:00:00 2001 From: Jesse Jones Date: Tue, 11 Dec 2012 19:16:00 -0800 Subject: [PATCH 6/7] Abort instead of throwing on oom --- src/rt/util/array_list.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/rt/util/array_list.h b/src/rt/util/array_list.h index f8014e48cb247..591e790f98f76 100644 --- a/src/rt/util/array_list.h +++ b/src/rt/util/array_list.h @@ -62,8 +62,10 @@ array_list::push(T value) { if (_size == _capacity) { size_t new_capacity = _capacity * 2; void* buffer = realloc(_data, new_capacity * sizeof(T)); - if (buffer == NULL) - throw std::bad_alloc(); + if (buffer == NULL) { + fprintf(stderr, "array_list::push> Out of memory allocating %ld bytes", new_capacity * sizeof(T)); + abort(); + } _data = (T *) buffer; _capacity = new_capacity; } From 01f1aa1ab7273110b431a8be25f52fcd0438ecb8 Mon Sep 17 00:00:00 2001 From: Jesse Jones Date: Tue, 11 Dec 2012 19:20:39 -0800 Subject: [PATCH 7/7] Check for oom in a few more places --- src/rt/memory_region.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/rt/memory_region.cpp b/src/rt/memory_region.cpp index 7096657999102..e31774497aa80 100644 --- a/src/rt/memory_region.cpp +++ b/src/rt/memory_region.cpp @@ -77,6 +77,10 @@ memory_region::realloc(void *mem, size_t orig_size) { size_t size = orig_size + HEADER_SIZE; alloc_header *newMem = (alloc_header *)::realloc(alloc, size); + if (newMem == NULL) { + fprintf(stderr, "memory_region::realloc> Out of memory allocating %ld bytes", size); + abort(); + } # if RUSTRT_TRACK_ALLOCATIONS >= 1 assert(newMem->magic == MAGIC); @@ -108,6 +112,10 @@ memory_region::malloc(size_t size, const char *tag, bool zero) { size_t old_size = size; size += HEADER_SIZE; alloc_header *mem = (alloc_header *)::malloc(size); + if (mem == NULL) { + fprintf(stderr, "memory_region::malloc> Out of memory allocating %ld bytes", size); + abort(); + } # if RUSTRT_TRACK_ALLOCATIONS >= 1 mem->magic = MAGIC;