Iox 1394 fix axivion violations in cxx function #1698

MatthiasKillat · 2022-09-29T14:35:15Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
Unit tests have been written for new behavior
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Closes Address Autosar Violations using Axivion #1394 (partial)

MatthiasKillat · 2022-09-29T14:39:34Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.hpp

@@ -93,19 +94,13 @@ class storable_function<Capacity, signature<ReturnType, Args...>>
    /// @param args arguments to invoke the stored function with
    /// @return return value of the stored function
    ///
-    /// @note 1) Invoking the function if there is no stored function (i.e. operator bool returns false)


This was incorrect.

It will now not terminate anymore since it cannot be null by design (except after move, but this is illegal).

It now has noexcept (already had it) but actually should not since the callable we store can happily throw. This is an issue in all our template code IMO.

MatthiasKillat · 2022-09-29T14:41:00Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

 template <uint64_t Capacity, typename ReturnType, typename... Args>
 template <typename Functor, typename>
 inline storable_function<Capacity, signature<ReturnType, Args...>>::storable_function(const Functor& functor) noexcept
 {
    storeFunctor(functor);
 }

+// AXIVION Next Construct AutosarC++19_03-A12.1.5: constructor delegation is not feasible here due
+// to lack of sufficient common initialization
+// AXIVION Next Construct AutosarC++19_03-A12.6.1: members are initialized in body before read access


This rule is questionable. A disabling candidate for me.

It cannot deal properly with default initialization and with the fact that something will be properly initialized before use. Here we have both. As long as it is initialized before reading is perfectly fine to initialize in the ctor body (initializer list preferred for large objects but this is not the case here) or even later as long as it is done (by design!) before read access.

Axivion does not cover this properly.

MatthiasKillat · 2022-09-29T14:46:32Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

 template <uint64_t Capacity, typename ReturnType, typename... Args>
 inline storable_function<Capacity, signature<ReturnType, Args...>>::storable_function(
    ReturnType (*function)(Args...)) noexcept
-    : /// @NOLINTJUSTIFICATION we use type erasure in combination with compile time template arguments to restore
+    : // AXIVION Next Construct AutosarC++19_03-M5.2.6: the converted pointer is only used


Many, many Axivion findings are due to the fact that generic type erasure requires low level pointer casts and Axivion complains. However, they are fine and portable if the object is only accessed through a pointer of its actual type later.

This happens here, it is just not possible to prove for Axivion but to some degree, type erasure cannot avoid this so we need those comments.

MatthiasKillat · 2022-09-29T14:48:39Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

      ///                      the correct type whenever the callable is used
      /// @NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
    m_callable(reinterpret_cast<void*>(function))
    , m_invoker(invokeFreeFunction)
 {
    cxx::Expects(function);

-    m_operations.copyFunction = copyFreeFunction;
-    m_operations.moveFunction = moveFreeFunction;
+    m_operations.copyFunction = &copyFreeFunction;


A questionable rule. A function name is no function pointer but it implicitly converts to one (mandated by standard). There is no other interpretation when assigning a function to a function pointer (signature is checked). The & is superfluous as is the rule IMO.

MatthiasKillat · 2022-09-29T14:51:17Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

-    auto p = &object;
+    const auto p = &object;
+    // AXIVION Next Construct AutosarC++19_03-A7.1.1: type erased functor lambda cannot be const
+    // as it may change by calling its operator() later (hence stored as non-const)


Will be stored in a non-const object, so this would not work. I am fairly sure it is not easy to fix for all cases at once, again due to type erasure. We cannot store the object in a const void* later, and even if we could, this would be lying as invoking anything on it later may change the stored object.

This is difficult to prove for Axivion I guess.

MatthiasKillat · 2022-09-29T14:54:07Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

@@ -78,6 +92,8 @@ inline storable_function<Capacity, signature<ReturnType, Args...>>::storable_fun
    m_operations.copy(other, *this);
 }

+// AXIVION Next Construct AutosarC++19_03-A12.8.4: we copy only the operation pointer table


The move is tricky due to type erasure. We need to copy a few function pointers (moving them is wrong and I think leads to complaints as well). The actual move is then performed according to one of the function pointers.

Essentially a vtable dependent on the stored object and not the class.

MatthiasKillat · 2022-09-29T14:54:36Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

@@ -125,16 +141,19 @@ inline storable_function<Capacity, signature<ReturnType, Args...>>::~storable_fu
    m_operations.destroy(*this);
 }

+// AXIVION Next Construct AutosarC++19_03-A2.10.1: false positive, args does not hide anything


Disabling candidate maybe. Not sure what problem it has with variadic args.

MatthiasKillat · 2022-09-29T14:56:59Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

@@ -153,18 +172,21 @@ inline void storable_function<Capacity, signature<ReturnType, Args...>>::storeFu
    auto ptr = m_storage.template allocate<StoredType>();
    cxx::Expects(ptr != nullptr);

+    // AXIVION Next Construct AutosarC++19_03-A18.5.10: false positive, m_storage.allocate<T>


Pointer trickery with invariants not known to Axivion. Requires semantic understanding that allocate provides an aligned pointer for StoredType which is non-trivial to infer automatically.

Note that we need to be prepared to store any callable type (with some size limitations), which makes a generic aligned alloc necessary

codecov · 2022-09-29T14:57:52Z

Codecov Report

Merging #1698 (d1bf26c) into master (6afa11e) will increase coverage by 0.01%.
The diff coverage is 92.00%.

@@            Coverage Diff             @@
##           master    #1698      +/-   ##
==========================================
+ Coverage   77.35%   77.36%   +0.01%     
==========================================
  Files         366      366              
  Lines       14182    14187       +5     
  Branches     1984     1983       -1     
==========================================
+ Hits        10970    10976       +6     
  Misses       2584     2584              
+ Partials      628      627       -1

Flag	Coverage Δ
unittests	`77.02% <92.00%> (+0.01%)`	⬆️
unittests_timing	`15.64% <76.00%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...lude/iceoryx_hoofs/internal/cxx/static_storage.hpp	`100.00% <ø> (ø)`
...e/iceoryx_hoofs/internal/cxx/storable_function.hpp	`100.00% <ø> (ø)`
...e/iceoryx_hoofs/internal/cxx/storable_function.inl	`95.00% <90.90%> (+0.12%)`	⬆️
...lude/iceoryx_hoofs/internal/cxx/static_storage.inl	`93.33% <100.00%> (ø)`
...oofs/source/log/building_blocks/console_logger.cpp	`77.61% <0.00%> (-1.50%)`	⬇️
iceoryx_hoofs/include/iceoryx_hoofs/cxx/vector.hpp	`100.00% <0.00%> (ø)`
...oofs/include/iceoryx_hoofs/internal/cxx/vector.inl	`98.67% <0.00%> (+0.01%)`	⬆️
...e/iceoryx_hoofs/internal/concurrent/smart_lock.inl	`94.11% <0.00%> (+1.96%)`	⬆️
iceoryx_hoofs/source/concurrent/loffli.cpp	`82.85% <0.00%> (+2.85%)`	⬆️

MatthiasKillat · 2022-09-29T15:00:18Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

 }

+// AXIVION Next Construct AutosarC++19_03-A8.4.8: output parameter required by design and for efficiency


Not a good rule. Disabling candidate.

Output parameters have their place, and here it is more that the intention is to modify one or both input objects which previously exist and hence should not be constructed and returned by the function. This is due to design and efficiency reasons.

MatthiasKillat · 2022-09-29T15:03:41Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

+    // AXIVION Next Construct AutosarC++19_03-M5.2.8: type erasure - conversion to compatible type
+    const auto obj = static_cast<CallableType*>(src.m_callable);
+
+    // AXIVION Next Construct AutosarC++19_03-A5.3.2: obj is guaranteed not to be


Torn on this rule. Of course we do not want to dereference a nullptr.
However, this is often guaranteed by invariants, e.g. we know m_callable is not null by design (except for moved from objects, which are illegal to access). Axivion does often fails to infer this, which is understandable as it is a non-trivial problem. The solution is not to introduce checks, because it
a) degrades performances
b) requires a branch to proceed if the pointer is nullptr, even if it cannot happen ...

The usefulness of this rules depends on how powerful the checking is. I have not enough data about this.

@MatthiasKillat I disagree here. I think A5.3.2 is there for a reason. A user could run into the issue of using a moved-from object and then copying it, hence we should check for nullptr here with Expects/assert.

Yes but then much of the advantage of non-nullability goes right out of the window (being able to assume it is not null).

Note that I do not agree to have this construct not be null in the first place, as it is useful in some designs where we want to store a null function (this better should have been an extension/separate implementation). This is another issue, however.

MatthiasKillat · 2022-09-29T15:07:56Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

@@ -204,9 +239,11 @@ template <uint64_t Capacity, typename ReturnType, typename... Args>
 template <typename CallableType>
 inline void storable_function<Capacity, signature<ReturnType, Args...>>::destroy(storable_function& f) noexcept
 {
-    if (f.m_callable)
+    if (f.m_callable != nullptr)


Being explicit here may be useful, but IMO should be optional. The meaning is well-defined to my knowledge.

MatthiasKillat · 2022-09-29T15:09:01Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

 {
+    // AXIVION Next Construct AutosarC++19_03-A18.9.2: false positive, perfect forwarding


I need to understand this rule better, but it appears to take issue with std::forward which is the idiomatic way with perfect forwarding.

If so, it is a disabling candidate.

std::forward is even recommended by the rule...

But it still complains, how great is that? I do not get it, but to my knowledge this is idiomatic perfect forwarding and hence correct. std::forward exists precisely for this. Without going into details about reference collapsing and so on, its purpose is to chose the correct "cast" automatically, which could have the same effect as std::move in specific circumstances but we do not need to care about it.

A18.9.2 should be closely monitored, if it does not work properly we should disable it.

MatthiasKillat · 2022-09-29T15:13:28Z

Dear future reviewers. I took the liberty to comment on some rules and add a little more reasoning why I think the suppression comments are necessary.

Long story short, it is mainly due to Axivion having difficulties with low level pointer casts required for type erasure which should be perfectly safe here (potential bugs excluded). This is mainly since we take care to properly align pointers and only access (e.g. call) the wrapped function with a pointer of its true type (or at least compatible by standard).

Also, I do not see an easy way around deferring some initialization to the ctor bodies here, which should be fine. This is mainly bad practice with complex object initialization, which is not the case here except for the wrapped payload function. It is initialized once in a suitable buffer in the ctor body, which is efficient.

I commented on some rules I consider problematic or superfluous for various reasons.

FerdinandSpitzschnueffler

Review not finished

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.hpp

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

FerdinandSpitzschnueffler · 2022-09-30T09:55:30Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

 {
+    // AXIVION Next Construct AutosarC++19_03-A18.9.2: false positive, perfect forwarding


std::forward is even recommended by the rule...

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

MatthiasKillat · 2022-09-30T13:27:58Z

@FerdinandSpitzschnueffler Fixed what was missing, may have missed something (merged justifications, removed redundant justification). Local scan is inconclusive on its own.

mossmaurice

Thanks for the PR, LGTM in general! I think it's important to adhere to the rule AutosarC++19_03-A5.3.2, checking pointers for nullptr before using them is a best practise and in the current state can be violated by a user when using a moved-from object. Please remove all suppressions for this rule and replace them with an Expects check.

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

mossmaurice · 2022-10-04T12:05:53Z

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

+    // AXIVION Next Construct AutosarC++19_03-M5.2.8: type erasure - conversion to compatible type
+    const auto obj = static_cast<CallableType*>(src.m_callable);
+
+    // AXIVION Next Construct AutosarC++19_03-A5.3.2: obj is guaranteed not to be


@MatthiasKillat I disagree here. I think A5.3.2 is there for a reason. A user could run into the issue of using a moved-from object and then copying it, hence we should check for nullptr here with Expects/assert.

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

Signed-off-by: Matthias Killat <[email protected]>

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

Signed-off-by: Matthias Killat <[email protected]>

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl

MatthiasKillat requested review from mossmaurice and FerdinandSpitzschnueffler September 29, 2022 14:36

MatthiasKillat added the AUTOSAR Warning of an Adpative Autosar C++14 rule label Sep 29, 2022

MatthiasKillat self-assigned this Sep 29, 2022

MatthiasKillat commented Sep 29, 2022

View reviewed changes

FerdinandSpitzschnueffler reviewed Sep 29, 2022

View reviewed changes

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.hpp Outdated Show resolved Hide resolved

MatthiasKillat added the globex label Sep 30, 2022

FerdinandSpitzschnueffler reviewed Sep 30, 2022

View reviewed changes

MatthiasKillat force-pushed the iox-1394-fix-axivion-violations-in-cxx-function branch 2 times, most recently from 8e608b3 to 070d840 Compare September 30, 2022 13:26

MatthiasKillat requested a review from FerdinandSpitzschnueffler September 30, 2022 13:26

FerdinandSpitzschnueffler previously approved these changes Oct 4, 2022

View reviewed changes

mossmaurice requested changes Oct 4, 2022

View reviewed changes

MatthiasKillat dismissed FerdinandSpitzschnueffler’s stale review via ccd9247 October 18, 2022 08:57

iox-eclipse-iceoryx#1394 Fix axivion warnings in storable_function

15ded86

Signed-off-by: Matthias Killat <[email protected]>

FerdinandSpitzschnueffler reviewed Oct 18, 2022

View reviewed changes

MatthiasKillat added 2 commits October 18, 2022 11:57

iox-eclipse-iceoryx#1394 Fix axivion warnings in static_storage

de5be91

Signed-off-by: Matthias Killat <[email protected]>

iox-eclipse-iceoryx#1394 Fix axivion warnings in storable_function

6c7ae7f

Signed-off-by: Matthias Killat <[email protected]>

MatthiasKillat force-pushed the iox-1394-fix-axivion-violations-in-cxx-function branch from ccd9247 to 8f3d4de Compare October 18, 2022 09:57

iox-eclipse-iceoryx#1394 Add null checks in storable_function

d1bf26c

Signed-off-by: Matthias Killat <[email protected]>

MatthiasKillat force-pushed the iox-1394-fix-axivion-violations-in-cxx-function branch from 8f3d4de to d1bf26c Compare October 18, 2022 13:17

MatthiasKillat requested review from mossmaurice and FerdinandSpitzschnueffler October 18, 2022 13:19

FerdinandSpitzschnueffler approved these changes Oct 18, 2022

View reviewed changes

mossmaurice approved these changes Oct 18, 2022

View reviewed changes

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl Show resolved Hide resolved

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl Show resolved Hide resolved

iceoryx_hoofs/include/iceoryx_hoofs/internal/cxx/storable_function.inl Show resolved Hide resolved

MatthiasKillat merged commit 9a96e81 into eclipse-iceoryx:master Oct 18, 2022

MatthiasKillat deleted the iox-1394-fix-axivion-violations-in-cxx-function branch October 18, 2022 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Iox 1394 fix axivion violations in cxx function #1698

Iox 1394 fix axivion violations in cxx function #1698

MatthiasKillat commented Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 29, 2022

MatthiasKillat Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 29, 2022

MatthiasKillat Sep 29, 2022

MatthiasKillat Sep 29, 2022 •

edited

Loading

codecov bot commented Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 29, 2022

MatthiasKillat Sep 29, 2022 •

edited

Loading

mossmaurice Oct 4, 2022

MatthiasKillat Oct 17, 2022

MatthiasKillat Sep 29, 2022

MatthiasKillat Sep 29, 2022

FerdinandSpitzschnueffler Sep 30, 2022

MatthiasKillat Sep 30, 2022 •

edited

Loading

MatthiasKillat commented Sep 29, 2022 •

edited

Loading

FerdinandSpitzschnueffler left a comment

FerdinandSpitzschnueffler Sep 30, 2022

MatthiasKillat commented Sep 30, 2022 •

edited

Loading

mossmaurice left a comment

mossmaurice Oct 4, 2022

		}

		// AXIVION Next Construct AutosarC++19_03-A8.4.8: output parameter required by design and for efficiency

Iox 1394 fix axivion violations in cxx function #1698

Iox 1394 fix axivion violations in cxx function #1698

Conversation

MatthiasKillat commented Sep 29, 2022 • edited Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

MatthiasKillat Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

MatthiasKillat Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

MatthiasKillat Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasKillat Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasKillat Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 29, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

MatthiasKillat Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasKillat Sep 30, 2022 • edited Loading

Choose a reason for hiding this comment

MatthiasKillat commented Sep 29, 2022 • edited Loading

FerdinandSpitzschnueffler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasKillat commented Sep 30, 2022 • edited Loading

mossmaurice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasKillat commented Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 29, 2022 •

edited

Loading

codecov bot commented Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 29, 2022 •

edited

Loading

MatthiasKillat Sep 30, 2022 •

edited

Loading

MatthiasKillat commented Sep 29, 2022 •

edited

Loading

MatthiasKillat commented Sep 30, 2022 •

edited

Loading