[SYCL] Trick to avoid strict pointer aliasing warnings

[AlexeySachkov]

The problem is that gcc < 7.2 emit the following warning:

warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
(*(T *)m_Mem).~T();
       ^

Interesting, that this only happens with -O2 optimization level

Replaced C-style casts with C++ reinterpret_cast and this is actually
quite a hack, because according to the docs 1:

the resulting pointer may only be dereferenced safely if allowed by
the type aliasing rules

Type aliasing rules allow to represent any object as char *, but not
the way around, i.e. array of characters cannot be reinterpreted as an
object.

std::memcpy also doesn't work here, because there is no requirement
that T is trivially copyable.

Another way to trick a compiler is to save pointer returned from
placement new: it already has type T *, so, we can store it within the
class and avoid casts. Hovewer, this is also a tricky thing, because since
m_Mem and this new pointer point to different types, compiler could
assume that they don't alias (when they actually point to the same memory
location) and perform some undesired transformations.

[AlexeySachkov]

@Ruyk, @Alexander-Johnston, could you please take a look? This seems to be a regression introduced in #1091 (in particular, here)

BTW, patch for the second possible trick mentioned in the description:

diff --git a/sycl/include/CL/sycl/property_list.hpp b/sycl/include/CL/sycl/property_list.hpp
index 439b2b1..b3d927a 100644
--- a/sycl/include/CL/sycl/property_list.hpp
+++ b/sycl/include/CL/sycl/property_list.hpp
@@ -77,48 +77,47 @@ public:

   PropertyHolder(const PropertyHolder &P) {
     if (P.isInitialized()) {
-      new (m_Mem) T(P.getProp());
-      m_Initialized = true;
+      m_MemPtr = new (m_Mem) T(P.getProp());
     }
   }

   ~PropertyHolder() {
-    if (m_Initialized) {
-      (*(T *)m_Mem).~T();
+    if (isInitialized()) {
+      m_MemPtr->~T();
+      m_MemPtr = nullptr;
     }
   }

   PropertyHolder &operator=(const PropertyHolder &Other) {
     if (this != &Other) {
-      if (m_Initialized) {
-        (*(T *)m_Mem).~T();
-        m_Initialized = false;
+      if (isInitialized()) {
+        m_MemPtr->~T();
+        m_MemPtr = nullptr;
       }

-      if (Other.m_Initialized) {
-        new (m_Mem) T(Other.getProp());
-        m_Initialized = true;
+      if (Other.isInitialized()) {
+        m_MemPtr = new (m_Mem) T(Other.getProp());
       }
     }
     return *this;
   }

   void setProp(const T &Rhs) {
-    new (m_Mem) T(Rhs);
-    m_Initialized = true;
+    m_MemPtr = new (m_Mem) T(Rhs);
   }

   const T &getProp() const {
-    assert(true == m_Initialized && "Property was not set!");
-    return *(const T *)m_Mem;
+    assert(isInitialized() && "Property was not set!");
+    return *m_MemPtr;
   }
-  bool isInitialized() const { return m_Initialized; }
+  bool isInitialized() const { return m_MemPtr != nullptr; }

 private:
   // Memory that is used for property allocation
   alignas(T) unsigned char m_Mem[sizeof(T)];
-  // Indicate whether property initialized or not.
-  bool m_Initialized = false;
+  // Used to avoid problems with strict aliasing rules (looks like a hack,
+  // though). Also used to indicate wheter property initialized or not
+  T *m_MemPtr = nullptr;
 };

 // This macro adds specialization of class Prop which provides possibility to

[Ruyk]

@steffenlarsen @StuartDAdams

The reinterpret_cast solution looks cleaner to me

[s-kanaev]

LGTM.
Just some minor suggestions.

[s-kanaev]

I believe, that this text the resulting pointer may only be dereferenced safely if allowed by the type aliasing rules only says that employing reinterpret_cast instead of static_cast and dynamic_cast increases possible unsafety of a C++ application and also increases responsibility of a programmer. I don't see a real hack here due to by nature reinterpret_cast is the same as C-style cast.

[steffenlarsen]

The reinterpret_cast solution looks cleaner to me

I agree, reinterpret_cast seems to be the best of the available options in my opinion.

[AlexeySachkov]

Just some minor suggestions.

@s-kanaev, reinterpret_cast I've employed still violates strict aliasing rules and the warning shows up again with your suggestions. This temporary variable helps to trick the compiler

I believe, that this text the resulting pointer may only be dereferenced safely if allowed by the type aliasing rules only says that employing reinterpret_cast instead of static_cast and dynamic_cast increases possible unsafety of a C++ application and also increases responsibility of a programmer.

I'm not quite sure. According to the Type aliasing section, the code I've written is a UB: when we try to reinterpret char[] (DynamicType) as an object of type T (AliasedType), none of three bullets is satisfied:

• AliasedType and DynamicType are not similar
• AliasedType is not (possibly cv-qualified_ signed or unsigned variant of DynamicType
• AlisasedType is not std::byte, char or unsigned char

I don't see a real hack here due to by nature reinterpret_cast is the same as C-style cast.

This is not true in general.
For example:

int A = 10; float F = 3.14;
A = (int)F; // works
A = reinterpret_cast<int>(F); // doesn't work
A = reinterpret_cast<int &>(F); // works, but this is UB (absolutely the same as I exploit in this PR)

[asavonic]

If you're sure that the compiler has no opportunity to break this code, and you just want to silence the warning, then #pragma GCC diagnostic ignored sounds like a more obvious way to do it.

[s-kanaev]

A = reinterpret_cast<int &>(F); // works, but this is UB (absolutely the same as I exploit in this PR)

What you've actually exploited in this PR is more like this (only need a char array somewhere about it):

A = *reinterpret_cast<int *>(&F);

[asavonic]

If anyone wonders what is "wrong" with the code: https://lkml.org/lkml/2009/1/12/369

[bader]

@AlexeySachkov, are you going to make any changes to this PR or I can merge?

[AlexeySachkov]

@AlexeySachkov, are you going to make any changes to this PR or I can merge?

@bader, go ahead with merge. Right now I don't have any better solution, so, no plans for changes