You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by we...@apache.org on 2018/07/21 15:08:17 UTC
[arrow] branch master updated: ARROW-2883: [C++] Fix Clang warnings
in code built with -DARROW_GPU=ON
This is an automated email from the ASF dual-hosted git repository.
wesm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new 35c541a ARROW-2883: [C++] Fix Clang warnings in code built with -DARROW_GPU=ON
35c541a is described below
commit 35c541ae51295694103d66beb75fac1215488897
Author: Wes McKinney <we...@apache.org>
AuthorDate: Sat Jul 21 11:08:12 2018 -0400
ARROW-2883: [C++] Fix Clang warnings in code built with -DARROW_GPU=ON
I also opened issues ARROW-2887, ARROW-2888 which surfaced on handling these warnings
Author: Wes McKinney <we...@apache.org>
Closes #2298 from wesm/ARROW-2883 and squashes the following commits:
4ab24744 <Wes McKinney> Fix clang-6 warnings in GPU-related code paths
---
cpp/src/arrow/gpu/CMakeLists.txt | 2 ++
cpp/src/arrow/gpu/cuda_memory.h | 4 ++--
cpp/src/plasma/client.cc | 4 ++--
cpp/src/plasma/protocol.cc | 13 +++++++------
cpp/src/plasma/store.cc | 33 +++++++++++++++++----------------
cpp/src/plasma/test/client_tests.cc | 4 ++--
6 files changed, 32 insertions(+), 28 deletions(-)
diff --git a/cpp/src/arrow/gpu/CMakeLists.txt b/cpp/src/arrow/gpu/CMakeLists.txt
index 7192c43..2235915 100644
--- a/cpp/src/arrow/gpu/CMakeLists.txt
+++ b/cpp/src/arrow/gpu/CMakeLists.txt
@@ -26,6 +26,8 @@ endif()
find_package(CUDA REQUIRED)
include_directories(SYSTEM ${CUDA_INCLUDE_DIRS})
+message(STATUS "CUDA Libraries: ${CUDA_LIBRARIES}")
+
set(ARROW_GPU_SRCS
cuda_arrow_ipc.cc
cuda_context.cc
diff --git a/cpp/src/arrow/gpu/cuda_memory.h b/cpp/src/arrow/gpu/cuda_memory.h
index 7eb8b88..669a183 100644
--- a/cpp/src/arrow/gpu/cuda_memory.h
+++ b/cpp/src/arrow/gpu/cuda_memory.h
@@ -133,7 +133,7 @@ class ARROW_EXPORT CudaIpcMemHandle {
class ARROW_EXPORT CudaBufferReader : public io::BufferReader {
public:
explicit CudaBufferReader(const std::shared_ptr<Buffer>& buffer);
- ~CudaBufferReader();
+ ~CudaBufferReader() override;
/// \brief Read bytes into pre-allocated host memory
/// \param[in] nbytes number of bytes to read
@@ -157,7 +157,7 @@ class ARROW_EXPORT CudaBufferReader : public io::BufferReader {
class ARROW_EXPORT CudaBufferWriter : public io::WriteableFile {
public:
explicit CudaBufferWriter(const std::shared_ptr<CudaBuffer>& buffer);
- ~CudaBufferWriter();
+ ~CudaBufferWriter() override;
/// \brief Close writer and flush buffered bytes to GPU
Status Close() override;
diff --git a/cpp/src/plasma/client.cc b/cpp/src/plasma/client.cc
index e7d9d44..2d977ec 100644
--- a/cpp/src/plasma/client.cc
+++ b/cpp/src/plasma/client.cc
@@ -280,7 +280,7 @@ PlasmaBuffer::~PlasmaBuffer() { ARROW_UNUSED(client_->Release(object_id_)); }
PlasmaClient::Impl::Impl() {
#ifdef PLASMA_GPU
- CudaDeviceManager::GetInstance(&manager_);
+ DCHECK_OK(CudaDeviceManager::GetInstance(&manager_));
#endif
}
@@ -408,7 +408,7 @@ Status PlasmaClient::Impl::Create(const ObjectID& object_id, int64_t data_size,
if (metadata != NULL) {
// Copy the metadata to the buffer.
CudaBufferWriter writer(std::dynamic_pointer_cast<CudaBuffer>(*data));
- writer.WriteAt(object.data_size, metadata, metadata_size);
+ RETURN_NOT_OK(writer.WriteAt(object.data_size, metadata, metadata_size));
}
#else
ARROW_LOG(FATAL) << "Arrow GPU library is not enabled.";
diff --git a/cpp/src/plasma/protocol.cc b/cpp/src/plasma/protocol.cc
index 5f761c4..da56249 100644
--- a/cpp/src/plasma/protocol.cc
+++ b/cpp/src/plasma/protocol.cc
@@ -131,7 +131,7 @@ Status SendCreateReply(int sock, ObjectID object_id, PlasmaObject* object,
flatbuffers::Offset<fb::CudaHandle> ipc_handle;
if (object->device_num != 0) {
std::shared_ptr<arrow::Buffer> handle;
- object->ipc_handle->Serialize(arrow::default_memory_pool(), &handle);
+ RETURN_NOT_OK(object->ipc_handle->Serialize(arrow::default_memory_pool(), &handle));
ipc_handle =
fb::CreateCudaHandle(fbb, fbb.CreateVector(handle->data(), handle->size()));
}
@@ -171,8 +171,8 @@ Status ReadCreateReply(uint8_t* data, size_t size, ObjectID* object_id,
object->device_num = message->plasma_object()->device_num();
#ifdef PLASMA_GPU
if (object->device_num != 0) {
- CudaIpcMemHandle::FromBuffer(message->ipc_handle()->handle()->data(),
- &object->ipc_handle);
+ RETURN_NOT_OK(CudaIpcMemHandle::FromBuffer(message->ipc_handle()->handle()->data(),
+ &object->ipc_handle));
}
#endif
return PlasmaErrorStatus(message->error());
@@ -503,7 +503,7 @@ Status SendGetReply(int sock, ObjectID object_ids[],
#ifdef PLASMA_GPU
if (object.device_num != 0) {
std::shared_ptr<arrow::Buffer> handle;
- object.ipc_handle->Serialize(arrow::default_memory_pool(), &handle);
+ RETURN_NOT_OK(object.ipc_handle->Serialize(arrow::default_memory_pool(), &handle));
handles.push_back(
fb::CreateCudaHandle(fbb, fbb.CreateVector(handle->data(), handle->size())));
}
@@ -538,8 +538,9 @@ Status ReadGetReply(uint8_t* data, size_t size, ObjectID object_ids[],
plasma_objects[i].device_num = object->device_num();
#ifdef PLASMA_GPU
if (object->device_num() != 0) {
- CudaIpcMemHandle::FromBuffer(message->handles()->Get(handle_pos)->handle()->data(),
- &plasma_objects[i].ipc_handle);
+ const void* ipc_handle = message->handles()->Get(handle_pos)->handle()->data();
+ RETURN_NOT_OK(
+ CudaIpcMemHandle::FromBuffer(ipc_handle, &plasma_objects[i].ipc_handle));
handle_pos++;
}
#endif
diff --git a/cpp/src/plasma/store.cc b/cpp/src/plasma/store.cc
index 999aa6a..58ea7a4 100644
--- a/cpp/src/plasma/store.cc
+++ b/cpp/src/plasma/store.cc
@@ -114,7 +114,7 @@ PlasmaStore::PlasmaStore(EventLoop* loop, int64_t system_memory, std::string dir
store_info_.directory = directory;
store_info_.hugepages_enabled = hugepages_enabled;
#ifdef PLASMA_GPU
- CudaDeviceManager::GetInstance(&manager_);
+ DCHECK_OK(CudaDeviceManager::GetInstance(&manager_));
#endif
}
@@ -156,12 +156,12 @@ PlasmaError PlasmaStore::CreateObject(const ObjectID& object_id, int64_t data_si
return PlasmaError::ObjectExists;
}
// Try to evict objects until there is enough space.
- uint8_t* pointer;
+ uint8_t* pointer = nullptr;
#ifdef PLASMA_GPU
std::shared_ptr<CudaBuffer> gpu_handle;
std::shared_ptr<CudaContext> context_;
if (device_num != 0) {
- manager_->GetContext(device_num - 1, &context_);
+ DCHECK_OK(manager_->GetContext(device_num - 1, &context_));
}
#endif
while (true) {
@@ -175,7 +175,7 @@ PlasmaError PlasmaStore::CreateObject(const ObjectID& object_id, int64_t data_si
if (device_num == 0) {
pointer =
reinterpret_cast<uint8_t*>(dlmemalign(kBlockSize, data_size + metadata_size));
- if (pointer == NULL) {
+ if (pointer == nullptr) {
// Tell the eviction policy how much space we need to create this object.
std::vector<ObjectID> objects_to_evict;
bool success =
@@ -191,7 +191,7 @@ PlasmaError PlasmaStore::CreateObject(const ObjectID& object_id, int64_t data_si
}
} else {
#ifdef PLASMA_GPU
- context_->Allocate(data_size + metadata_size, &gpu_handle);
+ DCHECK_OK(context_->Allocate(data_size + metadata_size, &gpu_handle));
break;
#endif
}
@@ -217,7 +217,7 @@ PlasmaError PlasmaStore::CreateObject(const ObjectID& object_id, int64_t data_si
entry->device_num = device_num;
#ifdef PLASMA_GPU
if (device_num != 0) {
- gpu_handle->ExportForIpc(&entry->ipc_handle);
+ DCHECK_OK(gpu_handle->ExportForIpc(&entry->ipc_handle));
result->ipc_handle = entry->ipc_handle;
}
#endif
@@ -238,8 +238,8 @@ PlasmaError PlasmaStore::CreateObject(const ObjectID& object_id, int64_t data_si
}
void PlasmaObject_init(PlasmaObject* object, ObjectTableEntry* entry) {
- DCHECK(object != NULL);
- DCHECK(entry != NULL);
+ DCHECK(object != nullptr);
+ DCHECK(entry != nullptr);
DCHECK(entry->state == ObjectState::PLASMA_SEALED);
#ifdef PLASMA_GPU
if (entry->device_num != 0) {
@@ -325,7 +325,7 @@ void PlasmaStore::UpdateObjectGetRequests(const ObjectID& object_id) {
for (size_t i = 0; i < num_requests; ++i) {
auto get_req = get_requests[index];
auto entry = GetObjectTableEntry(&store_info_, object_id);
- ARROW_CHECK(entry != NULL);
+ ARROW_CHECK(entry != nullptr);
PlasmaObject_init(&get_req->objects[object_id], entry);
get_req->num_satisfied += 1;
@@ -415,7 +415,7 @@ int PlasmaStore::RemoveFromClientObjectIds(ObjectTableEntry* entry, Client* clie
void PlasmaStore::ReleaseObject(const ObjectID& object_id, Client* client) {
auto entry = GetObjectTableEntry(&store_info_, object_id);
- ARROW_CHECK(entry != NULL);
+ ARROW_CHECK(entry != nullptr);
// Remove the client from the object's array of clients.
ARROW_CHECK(RemoveFromClientObjectIds(entry, client) == 1);
}
@@ -432,7 +432,7 @@ ObjectStatus PlasmaStore::ContainsObject(const ObjectID& object_id) {
void PlasmaStore::SealObject(const ObjectID& object_id, unsigned char digest[]) {
ARROW_LOG(DEBUG) << "sealing object " << object_id.hex();
auto entry = GetObjectTableEntry(&store_info_, object_id);
- ARROW_CHECK(entry != NULL);
+ ARROW_CHECK(entry != nullptr);
ARROW_CHECK(entry->state == ObjectState::PLASMA_CREATED);
// Set the state of object to SEALED.
entry->state = ObjectState::PLASMA_SEALED;
@@ -447,7 +447,7 @@ void PlasmaStore::SealObject(const ObjectID& object_id, unsigned char digest[])
int PlasmaStore::AbortObject(const ObjectID& object_id, Client* client) {
auto entry = GetObjectTableEntry(&store_info_, object_id);
- ARROW_CHECK(entry != NULL) << "To abort an object it must be in the object table.";
+ ARROW_CHECK(entry != nullptr) << "To abort an object it must be in the object table.";
ARROW_CHECK(entry->state != ObjectState::PLASMA_SEALED)
<< "To abort an object it must not have been sealed.";
auto it = client->object_ids.find(object_id);
@@ -467,7 +467,7 @@ PlasmaError PlasmaStore::DeleteObject(ObjectID& object_id) {
// TODO(rkn): This should probably not fail, but should instead throw an
// error. Maybe we should also support deleting objects that have been
// created but not sealed.
- if (entry == NULL) {
+ if (entry == nullptr) {
// To delete an object it must be in the object table.
return PlasmaError::ObjectNonexistent;
}
@@ -501,7 +501,8 @@ void PlasmaStore::DeleteObjects(const std::vector<ObjectID>& object_ids) {
// TODO(rkn): This should probably not fail, but should instead throw an
// error. Maybe we should also support deleting objects that have been
// created but not sealed.
- ARROW_CHECK(entry != NULL) << "To delete an object it must be in the object table.";
+ ARROW_CHECK(entry != nullptr)
+ << "To delete an object it must be in the object table.";
ARROW_CHECK(entry->state == ObjectState::PLASMA_SEALED)
<< "To delete an object it must have been sealed.";
ARROW_CHECK(entry->ref_count == 0)
@@ -811,7 +812,7 @@ class PlasmaStoreRunner {
// that maximum allowed size up front.
if (use_one_memory_mapped_file) {
void* pointer = plasma::dlmemalign(kBlockSize, system_memory);
- ARROW_CHECK(pointer != NULL);
+ ARROW_CHECK(pointer != nullptr);
plasma::dlfree(pointer);
}
@@ -863,7 +864,7 @@ void StartServer(char* socket_name, int64_t system_memory, std::string plasma_di
} // namespace plasma
int main(int argc, char* argv[]) {
- char* socket_name = NULL;
+ char* socket_name = nullptr;
// Directory where plasma memory mapped files are stored.
std::string plasma_directory;
bool hugepages_enabled = false;
diff --git a/cpp/src/plasma/test/client_tests.cc b/cpp/src/plasma/test/client_tests.cc
index e40f6d9..4d75443 100644
--- a/cpp/src/plasma/test/client_tests.cc
+++ b/cpp/src/plasma/test/client_tests.cc
@@ -480,9 +480,9 @@ void AssertCudaRead(const std::shared_ptr<Buffer>& buffer,
ASSERT_EQ(gpu_buffer->size(), data_size);
CudaBufferReader reader(gpu_buffer);
- uint8_t read_data[data_size];
+ std::vector<uint8_t> read_data(data_size);
int64_t read_data_size;
- ARROW_CHECK_OK(reader.Read(data_size, &read_data_size, read_data));
+ ARROW_CHECK_OK(reader.Read(data_size, &read_data_size, read_data.data()));
ASSERT_EQ(read_data_size, data_size);
for (size_t i = 0; i < data_size; i++) {