From 42e88bd6b18597fe0a46ee9663d4e2cf2f7a4e57 Mon Sep 17 00:00:00 2001 From: Mehdi Amini Date: Sat, 14 Nov 2020 00:46:30 +0000 Subject: [PATCH 1/4] Replace sequences of v.push_back(v[i]); v.erase(&v[i]); with std::rotate (NFC) The code has a few sequence that looked like: Ops.push_back(Ops[0]); Ops.erase(Ops.begin()); And are equivalent to: std::rotate(Ops.begin(), Ops.begin() + 1, Ops.end()); The latter has the advantage of never reallocating the vector, which would be a bug in the original code as push_back would read from the memory it deallocated. --- clang/lib/CodeGen/CGBuiltin.cpp | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp index 7f17a14de63d1b..9cc58cf9a736cc 100644 --- a/clang/lib/CodeGen/CGBuiltin.cpp +++ b/clang/lib/CodeGen/CGBuiltin.cpp @@ -10748,8 +10748,7 @@ Value *CodeGenFunction::EmitAArch64BuiltinExpr(unsigned BuiltinID, case NEON::BI__builtin_neon_vld2q_lane_v: { llvm::Type *Tys[2] = { VTy, Ops[1]->getType() }; Function *F = CGM.getIntrinsic(Intrinsic::aarch64_neon_ld2lane, Tys); - Ops.push_back(Ops[1]); - Ops.erase(Ops.begin()+1); + std::rotate(Ops.begin() + 1, Ops.begin() + 2, Ops.end()); Ops[1] = Builder.CreateBitCast(Ops[1], Ty); Ops[2] = Builder.CreateBitCast(Ops[2], Ty); Ops[3] = Builder.CreateZExt(Ops[3], Int64Ty); @@ -10762,8 +10761,7 @@ Value *CodeGenFunction::EmitAArch64BuiltinExpr(unsigned BuiltinID, case NEON::BI__builtin_neon_vld3q_lane_v: { llvm::Type *Tys[2] = { VTy, Ops[1]->getType() }; Function *F = CGM.getIntrinsic(Intrinsic::aarch64_neon_ld3lane, Tys); - Ops.push_back(Ops[1]); - Ops.erase(Ops.begin()+1); + std::rotate(Ops.begin() + 1, Ops.begin() + 2, Ops.end()); Ops[1] = Builder.CreateBitCast(Ops[1], Ty); Ops[2] = Builder.CreateBitCast(Ops[2], Ty); Ops[3] = Builder.CreateBitCast(Ops[3], Ty); @@ -10777,8 +10775,7 @@ Value *CodeGenFunction::EmitAArch64BuiltinExpr(unsigned BuiltinID, case NEON::BI__builtin_neon_vld4q_lane_v: { llvm::Type *Tys[2] = { VTy, Ops[1]->getType() }; Function *F = CGM.getIntrinsic(Intrinsic::aarch64_neon_ld4lane, Tys); - Ops.push_back(Ops[1]); - Ops.erase(Ops.begin()+1); + std::rotate(Ops.begin() + 1, Ops.begin() + 2, Ops.end()); Ops[1] = Builder.CreateBitCast(Ops[1], Ty); Ops[2] = Builder.CreateBitCast(Ops[2], Ty); Ops[3] = Builder.CreateBitCast(Ops[3], Ty); @@ -10791,16 +10788,14 @@ Value *CodeGenFunction::EmitAArch64BuiltinExpr(unsigned BuiltinID, } case NEON::BI__builtin_neon_vst2_v: case NEON::BI__builtin_neon_vst2q_v: { - Ops.push_back(Ops[0]); - Ops.erase(Ops.begin()); + std::rotate(Ops.begin(), Ops.begin() + 1, Ops.end()); llvm::Type *Tys[2] = { VTy, Ops[2]->getType() }; return EmitNeonCall(CGM.getIntrinsic(Intrinsic::aarch64_neon_st2, Tys), Ops, ""); } case NEON::BI__builtin_neon_vst2_lane_v: case NEON::BI__builtin_neon_vst2q_lane_v: { - Ops.push_back(Ops[0]); - Ops.erase(Ops.begin()); + std::rotate(Ops.begin(), Ops.begin() + 1, Ops.end()); Ops[2] = Builder.CreateZExt(Ops[2], Int64Ty); llvm::Type *Tys[2] = { VTy, Ops[3]->getType() }; return EmitNeonCall(CGM.getIntrinsic(Intrinsic::aarch64_neon_st2lane, Tys), @@ -10808,16 +10803,14 @@ Value *CodeGenFunction::EmitAArch64BuiltinExpr(unsigned BuiltinID, } case NEON::BI__builtin_neon_vst3_v: case NEON::BI__builtin_neon_vst3q_v: { - Ops.push_back(Ops[0]); - Ops.erase(Ops.begin()); + std::rotate(Ops.begin(), Ops.begin() + 1, Ops.end()); llvm::Type *Tys[2] = { VTy, Ops[3]->getType() }; return EmitNeonCall(CGM.getIntrinsic(Intrinsic::aarch64_neon_st3, Tys), Ops, ""); } case NEON::BI__builtin_neon_vst3_lane_v: case NEON::BI__builtin_neon_vst3q_lane_v: { - Ops.push_back(Ops[0]); - Ops.erase(Ops.begin()); + std::rotate(Ops.begin(), Ops.begin() + 1, Ops.end()); Ops[3] = Builder.CreateZExt(Ops[3], Int64Ty); llvm::Type *Tys[2] = { VTy, Ops[4]->getType() }; return EmitNeonCall(CGM.getIntrinsic(Intrinsic::aarch64_neon_st3lane, Tys), @@ -10825,16 +10818,14 @@ Value *CodeGenFunction::EmitAArch64BuiltinExpr(unsigned BuiltinID, } case NEON::BI__builtin_neon_vst4_v: case NEON::BI__builtin_neon_vst4q_v: { - Ops.push_back(Ops[0]); - Ops.erase(Ops.begin()); + std::rotate(Ops.begin(), Ops.begin() + 1, Ops.end()); llvm::Type *Tys[2] = { VTy, Ops[4]->getType() }; return EmitNeonCall(CGM.getIntrinsic(Intrinsic::aarch64_neon_st4, Tys), Ops, ""); } case NEON::BI__builtin_neon_vst4_lane_v: case NEON::BI__builtin_neon_vst4q_lane_v: { - Ops.push_back(Ops[0]); - Ops.erase(Ops.begin()); + std::rotate(Ops.begin(), Ops.begin() + 1, Ops.end()); Ops[4] = Builder.CreateZExt(Ops[4], Int64Ty); llvm::Type *Tys[2] = { VTy, Ops[5]->getType() }; return EmitNeonCall(CGM.getIntrinsic(Intrinsic::aarch64_neon_st4lane, Tys), From 2c196bbc6bd897b3dcc1d87a3baac28e1e88df41 Mon Sep 17 00:00:00 2001 From: Mehdi Amini Date: Fri, 13 Nov 2020 22:37:25 +0000 Subject: [PATCH 2/4] Add an assertion in SmallVector::push_back() This assertion ensures the input value isn't part of the vector when growing is required. In such cases the vector will grow and the input value is invalidated before being read from. This found 14 failed Tests. Reviewed By: bkramer Differential Revision: https://reviews.llvm.org/D84293 --- llvm/include/llvm/ADT/SmallVector.h | 10 ++++++++++ llvm/include/llvm/MC/MCInst.h | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/llvm/include/llvm/ADT/SmallVector.h b/llvm/include/llvm/ADT/SmallVector.h index e042497473db24..fbc8ede255d2b0 100644 --- a/llvm/include/llvm/ADT/SmallVector.h +++ b/llvm/include/llvm/ADT/SmallVector.h @@ -136,6 +136,13 @@ class SmallVectorTemplateCommon this->Size = this->Capacity = 0; // FIXME: Setting Capacity to 0 is suspect. } + void assertSafeToPush(const void *Elt) { + assert( + (Elt < begin() || Elt >= end() || this->size() < this->capacity()) && + "Attempting to push_back to the vector an element of the vector without" + " enough space reserved"); + } + public: using size_type = size_t; using difference_type = ptrdiff_t; @@ -251,6 +258,7 @@ class SmallVectorTemplateBase : public SmallVectorTemplateCommon { public: void push_back(const T &Elt) { + this->assertSafeToPush(&Elt); if (LLVM_UNLIKELY(this->size() >= this->capacity())) this->grow(); ::new ((void*) this->end()) T(Elt); @@ -258,6 +266,7 @@ class SmallVectorTemplateBase : public SmallVectorTemplateCommon { } void push_back(T &&Elt) { + this->assertSafeToPush(&Elt); if (LLVM_UNLIKELY(this->size() >= this->capacity())) this->grow(); ::new ((void*) this->end()) T(::std::move(Elt)); @@ -353,6 +362,7 @@ class SmallVectorTemplateBase : public SmallVectorTemplateCommon { public: void push_back(const T &Elt) { + this->assertSafeToPush(&Elt); if (LLVM_UNLIKELY(this->size() >= this->capacity())) this->grow(); memcpy(reinterpret_cast(this->end()), &Elt, sizeof(T)); diff --git a/llvm/include/llvm/MC/MCInst.h b/llvm/include/llvm/MC/MCInst.h index 360dbda58fcbdf..2ce2ee063daa7a 100644 --- a/llvm/include/llvm/MC/MCInst.h +++ b/llvm/include/llvm/MC/MCInst.h @@ -181,7 +181,7 @@ class MCInst { MCOperand &getOperand(unsigned i) { return Operands[i]; } unsigned getNumOperands() const { return Operands.size(); } - void addOperand(const MCOperand &Op) { Operands.push_back(Op); } + void addOperand(const MCOperand Op) { Operands.push_back(Op); } using iterator = SmallVectorImpl::iterator; using const_iterator = SmallVectorImpl::const_iterator; From 6c0cd5676e0a0feaf836e0399023a6e21224467b Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Fri, 13 Nov 2020 17:30:34 -0800 Subject: [PATCH 3/4] [lldb] Make `process connect` behave the same in sync and async mode. I think the check for whether the process is connected is totally bogus in the first place, but on the off-chance that's it's not, we should behave the same in synchronous and asynchronous mode. --- .../Platform/gdb-server/PlatformRemoteGDBServer.cpp | 12 ------------ .../Platform/gdb-server/PlatformRemoteGDBServer.h | 6 ------ .../test/Shell/Commands/command-process-connect.test | 9 +++++++++ 3 files changed, 9 insertions(+), 18 deletions(-) create mode 100644 lldb/test/Shell/Commands/command-process-connect.test diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp index 0e0b61f1534f74..e8bfbd889299ff 100644 --- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp +++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp @@ -837,18 +837,6 @@ std::string PlatformRemoteGDBServer::MakeUrl(const char *scheme, return std::string(result.GetString()); } -lldb::ProcessSP PlatformRemoteGDBServer::ConnectProcess( - llvm::StringRef connect_url, llvm::StringRef plugin_name, - lldb_private::Debugger &debugger, lldb_private::Target *target, - lldb_private::Status &error) { - if (!IsRemote() || !IsConnected()) { - error.SetErrorString("Not connected to remote gdb server"); - return nullptr; - } - return Platform::ConnectProcess(connect_url, plugin_name, debugger, target, - error); -} - size_t PlatformRemoteGDBServer::ConnectToWaitingProcesses(Debugger &debugger, Status &error) { std::vector connection_urls; diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h index 297b482eb87ad6..e43cd0e55c6d3e 100644 --- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h +++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h @@ -154,12 +154,6 @@ class PlatformRemoteGDBServer : public Platform, private UserIDResolver { const lldb::UnixSignalsSP &GetRemoteUnixSignals() override; - lldb::ProcessSP ConnectProcess(llvm::StringRef connect_url, - llvm::StringRef plugin_name, - lldb_private::Debugger &debugger, - lldb_private::Target *target, - lldb_private::Status &error) override; - size_t ConnectToWaitingProcesses(lldb_private::Debugger &debugger, lldb_private::Status &error) override; diff --git a/lldb/test/Shell/Commands/command-process-connect.test b/lldb/test/Shell/Commands/command-process-connect.test new file mode 100644 index 00000000000000..c4761360d54117 --- /dev/null +++ b/lldb/test/Shell/Commands/command-process-connect.test @@ -0,0 +1,9 @@ +# Synchronous +# RUN: %lldb -o 'platform select remote-gdb-server' -o 'process connect connect://localhost:4321' 2>&1 | FileCheck %s + +# Asynchronous +# RUN: echo -e 'platform select remote-gdb-server\nprocess connect connect://localhost:4321' | %lldb 2>&1 | FileCheck %s + +# CHECK: Platform: remote-gdb-server +# CHECK: Connected: no +# CHECK: error: Failed to connect port From e7ed27653292b2ec545e87204031282b4b237754 Mon Sep 17 00:00:00 2001 From: Mehdi Amini Date: Sat, 14 Nov 2020 02:18:18 +0000 Subject: [PATCH 4/4] Switch libLLVMFrontendOpenACC to be a regular CMake library and not a "component" This library is only used in Flang at the moment and not tested withing LLVM. Having it as a component is breaking llvm-config: $ bin/llvm-config --shared-mode llvm-config: error: component libraries and shared library llvm-config: error: missing: [...]/lib/libLLVMFrontendOpenACC.a This will reverted when unit-tests are provided for it. Reviewed By: clementval Differential Revision: https://reviews.llvm.org/D91470 --- llvm/lib/Frontend/OpenACC/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/lib/Frontend/OpenACC/CMakeLists.txt b/llvm/lib/Frontend/OpenACC/CMakeLists.txt index ba340ab9c5619a..1c924611147419 100644 --- a/llvm/lib/Frontend/OpenACC/CMakeLists.txt +++ b/llvm/lib/Frontend/OpenACC/CMakeLists.txt @@ -2,7 +2,7 @@ set(LLVM_TARGET_DEFINITIONS ${LLVM_MAIN_INCLUDE_DIR}/llvm/Frontend/OpenACC/ACC.t tablegen(LLVM ACC.cpp --gen-directive-impl) add_public_tablegen_target(acc_cpp) -add_llvm_component_library(LLVMFrontendOpenACC +add_llvm_library(LLVMFrontendOpenACC ACC.cpp # Generated by tablegen above ADDITIONAL_HEADER_DIRS