Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

iox-#1196 activate clang-tidy check on posix wrapper #1538

Merged
merged 3 commits into from
Jul 25, 2022
Merged

iox-#1196 activate clang-tidy check on posix wrapper #1538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e98d3d
iox-#1196 Add posix to clang-tidy diff scan list and move test files …
elfenpiff Jul 22, 2022
c7db02c
iox-#1196 Fix overlooked warnings
elfenpiff Jul 22, 2022
28200ba
iox-#1196 Add issue numbers to suppression todos
elfenpiff Jul 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions .clang-tidy-diff-scans.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@
./iceoryx_hoofs/test/moduletests/test_unit*
./iceoryx_hoofs/source/units/*

./iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/*
./iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/*
./iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object/*
./iceoryx_hoofs/source/posix_wrapper/*
./iceoryx_hoofs/source/posix_wrapper/shared_memory_object/*
./iceoryx_hoofs/test/moduletests/test_allocator.cpp
./iceoryx_hoofs/test/moduletests/test_shared_memory.cpp
./iceoryx_hoofs/test/moduletests/test_shared_memory_object.cpp
./iceoryx_hoofs/test/moduletests/test_posix*

# IMPORTANT:
# after the first # everything is considered a comment, add new files and
Expand Down
2 changes: 1 addition & 1 deletion iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class SharedMemoryObject
public:
using Builder = SharedMemoryObjectBuilder;

static constexpr void* NO_ADDRESS_HINT = nullptr;
static constexpr const void* const NO_ADDRESS_HINT = nullptr;
mossmaurice marked this conversation as resolved.
Show resolved Hide resolved
SharedMemoryObject(const SharedMemoryObject&) = delete;
SharedMemoryObject& operator=(const SharedMemoryObject&) = delete;
SharedMemoryObject(SharedMemoryObject&&) noexcept = default;
Expand Down
2 changes: 2 additions & 0 deletions iceoryx_hoofs/source/posix_wrapper/message_queue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ MessageQueue& MessageQueue::operator=(MessageQueue&& other) noexcept
}
CreationPattern_t::operator=(std::move(other));

/// NOLINTJUSTIFICATION iox-#1036 will be fixed with the builder pattern
/// NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved)
m_name = std::move(other.m_name);
m_attributes = other.m_attributes;
m_mqDescriptor = other.m_mqDescriptor;
Expand Down
17 changes: 14 additions & 3 deletions iceoryx_hoofs/source/posix_wrapper/named_pipe.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,8 @@ NamedPipe& NamedPipe::operator=(NamedPipe&& rhs) noexcept
IOX_DISCARD_RESULT(destroy());
CreationPattern_t::operator=(std::move(rhs));

/// NOLINTJUSTIFICATION iox-#1036 will be fixed with the builder pattern
/// NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved)
m_sharedMemory = std::move(rhs.m_sharedMemory);
m_data = rhs.m_data;
rhs.m_data = nullptr;
Expand All @@ -153,9 +155,18 @@ NamedPipe::~NamedPipe() noexcept
template <typename Prefix>
IpcChannelName_t NamedPipe::convertName(const Prefix& p, const IpcChannelName_t& name) noexcept
{
return IpcChannelName_t(cxx::TruncateToCapacity,
// NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic)
cxx::concatenate(p, (name.c_str()[0] == '/') ? *name.substr(1) : name).c_str());
IpcChannelName_t channelName = p;

if (name[0] == '/')
{
name.substr(1).and_then([&](auto& s) { channelName.append(cxx::TruncateToCapacity, s); });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we at least drop a warning or information to the user that we remove the leading slash here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No not at all. In the end the ipc channel contract is to provide a valid file name (which does not allow slashes) in the IPC channel.
This still needs to be refactored somehow, since some IPC channels require slashes (message queue), some allow full paths (named pipe) and some only require a valid file name (unix domain socket).

This all has to be handled by an abstraction above those constructs which will be introduced in: #1451

Until then I would leave it as it is and then refactor it step by step especially since the NamedPipe is only used in Windows.

}
else
{
channelName.append(cxx::TruncateToCapacity, name);
}

return channelName;
}

cxx::expected<IpcChannelError> NamedPipe::destroy() noexcept
Expand Down
25 changes: 19 additions & 6 deletions iceoryx_hoofs/source/posix_wrapper/shared_memory_object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,22 @@ namespace iox
{
namespace posix
{
constexpr void* SharedMemoryObject::NO_ADDRESS_HINT;
constexpr const void* const SharedMemoryObject::NO_ADDRESS_HINT;
constexpr uint64_t SIGBUS_ERROR_MESSAGE_LENGTH = 1024U + platform::IOX_MAX_SHM_NAME_LENGTH;

/// NOLINTJUSTIFICATION global variables are only accessible from within this compilation unit
/// NOLINTBEGIN(cppcoreguidelines-avoid-non-const-global-variables)
///
/// NOLINTJUSTIFICATION c array required to print a signal safe error message in memsetSigbusHandler
/// NOLINTNEXTLINE(hicpp-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays)
static char sigbusErrorMessage[SIGBUS_ERROR_MESSAGE_LENGTH];
static std::mutex sigbusHandlerMutex;
/// NOLINTEND(cppcoreguidelines-avoid-non-const-global-variables)

static void memsetSigbusHandler(int) noexcept
{
auto result = write(STDERR_FILENO, sigbusErrorMessage, strnlen(sigbusErrorMessage, SIGBUS_ERROR_MESSAGE_LENGTH));
auto result =
write(STDERR_FILENO, &sigbusErrorMessage[0], strnlen(&sigbusErrorMessage[0], SIGBUS_ERROR_MESSAGE_LENGTH));
IOX_DISCARD_RESULT(result);
_exit(EXIT_FAILURE);
}
Expand All @@ -52,7 +59,10 @@ cxx::expected<SharedMemoryObject, SharedMemoryObjectError> SharedMemoryObjectBui
auto printErrorDetails = [this] {
LogError() << "Unable to create a shared memory object with the following properties [ name = " << m_name
<< ", sizeInBytes = " << m_memorySizeInBytes << ", access mode = " << asStringLiteral(m_accessMode)
<< ", open mode = " << asStringLiteral(m_openMode) << ", baseAddressHint = "
<< ", open mode = " << asStringLiteral(m_openMode)
<< ", baseAddressHint = "
// NOLINTJUSTIFICATION baseAddressHint printed on failure to make debugging easier as uint64 hex
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
<< ((m_baseAddressHint) ? log::HexFormat(reinterpret_cast<uint64_t>(*m_baseAddressHint))
: log::HexFormat(static_cast<uint64_t>(0U)))
<< ((m_baseAddressHint) ? "" : " (no hint set)")
Expand Down Expand Up @@ -108,8 +118,11 @@ cxx::expected<SharedMemoryObject, SharedMemoryObjectError> SharedMemoryObjectBui
return cxx::error<SharedMemoryObjectError>(SharedMemoryObjectError::INTERNAL_LOGIC_FAILURE);
}

snprintf(
sigbusErrorMessage,
// NOLINTJUSTIFICATION snprintf required to populate char array so that it can be used signal safe in
// a possible signal call
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg,hicpp-vararg)
IOX_DISCARD_RESULT(snprintf(
&sigbusErrorMessage[0],
SIGBUS_ERROR_MESSAGE_LENGTH,
"While setting the acquired shared memory to zero a fatal SIGBUS signal appeared caused by memset. The "
"shared memory object with the following properties [ name = %s, sizeInBytes = %llu, access mode = %s, "
Expand All @@ -120,7 +133,7 @@ cxx::expected<SharedMemoryObject, SharedMemoryObjectError> SharedMemoryObjectBui
asStringLiteral(m_accessMode),
asStringLiteral(m_openMode),
(m_baseAddressHint) ? *m_baseAddressHint : nullptr,
std::bitset<sizeof(mode_t)>(static_cast<mode_t>(m_permissions)).to_ulong());
std::bitset<sizeof(mode_t)>(static_cast<mode_t>(m_permissions)).to_ulong()));

memset(memoryMap->getBaseAddress(), 0, m_memorySizeInBytes);
}
Expand Down
0 .../test/moduletests/test_access_control.cpp → ...moduletests/test_posix_access_control.cpp
View file
Open in desktop
File renamed without changes.
0 ...hoofs/test/moduletests/test_allocator.cpp → ...test/moduletests/test_posix_allocator.cpp
View file
Open in desktop
File renamed without changes.
2 changes: 2 additions & 0 deletions iceoryx_hoofs/test/moduletests/test_posix_semaphore_interface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,8 @@ TYPED_TEST(SemaphoreInterfaceTest, FailingTimedWaitDoesNotChangeSemaphoreValue)
{
::testing::Test::RecordProperty("TEST_ID", "cb2f5ded-7c04-4e2d-ab41-bbe231a520c7");
constexpr uint64_t NUMBER_OF_DECREMENTS = 4U;
// NOLINTJUSTIFICATION user defined literal no risk of number-letter mixup
// NOLINTNEXTLINE(hicpp-uppercase-literal-suffix,readability-uppercase-literal-suffix)
constexpr iox::units::Duration timeToWait = 2_us;

for (uint64_t i = 0; i < NUMBER_OF_DECREMENTS; ++i)
Expand Down
0 ...s/test/moduletests/test_shared_memory.cpp → .../moduletests/test_posix_shared_memory.cpp
View file
Open in desktop
File renamed without changes.
0 ...moduletests/test_shared_memory_object.cpp → ...tests/test_posix_shared_memory_object.cpp
View file
Open in desktop
File renamed without changes.