Skip to content

Commit

Permalink
Fix aliasing issues with storage, put/get, keys
Browse files Browse the repository at this point in the history 
Use char* rather than uint8_t*, since they are not the same
with respect to aliasing.

Use void* in signatures to the hide implementation details
like the above.

Assert that keys are a sequence of bytes (char) such that
we can construct them from a byte pointer.

Signed-off-by: Younes Manton <ymanton@ca.ibm.com>
  • Loading branch information
@ymanton
ymanton committed Nov 20, 2020
1 parent a8f59c1 commit ee49651
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 37 deletions.
25 changes: 15 additions & 10 deletions compiler/codegen/CCData.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
* SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 OR LicenseRef-GPL-2.0 WITH Assembly-exception
*******************************************************************************/

#include <type_traits>

#include "omrcomp.h"
#include "OMR/Bytes.hpp"

Expand All @@ -41,8 +43,11 @@ size_t CCData::dataAlignmentFromBytesAlignment(size_t alignmentBytes)
return (alignmentBytes + OMR_ALIGNOF(data_t) - 1) / OMR_ALIGNOF(data_t);
}

CCData::key_t CCData::key(const uint8_t * const data, const size_t sizeBytes)
CCData::key_t CCData::key(const void * const data, const size_t sizeBytes)
{
#if __cpp_static_assert
static_assert(std::is_same<key_t::value_type, char>::value, "Keys need to be constructible from a sequence of chars.");
#endif
return key_t(reinterpret_cast<const key_t::value_type *>(data), sizeBytes);
}

Expand All @@ -51,7 +56,7 @@ CCData::key_t CCData::key(const char * const str)
return key_t(reinterpret_cast<const key_t::value_type *>(str));
}

CCData::CCData(uint8_t * const storage, const size_t sizeBytes)
CCData::CCData(void * const storage, const size_t sizeBytes)
: _putIndex(0), _lock(TR::Monitor::create("CCDataMutex"))
{
if (sizeBytes > 0)
Expand All @@ -70,7 +75,7 @@ CCData::CCData(uint8_t * const storage, const size_t sizeBytes)
}
}

bool CCData::put(const uint8_t * const value, const size_t sizeBytes, const size_t alignmentBytes, const key_t * const key, index_t &index)
bool CCData::put(const void * const value, const size_t sizeBytes, const size_t alignmentBytes, const key_t * const key, index_t &index)
{
const OMR::CriticalSection critsec(_lock);

Expand Down Expand Up @@ -102,24 +107,24 @@ bool CCData::put(const uint8_t * const value, const size_t sizeBytes, const size

if (value != NULL)
{
std::copy(value,
value + sizeBytes,
reinterpret_cast<uint8_t *>(_data + _putIndex));
std::copy(reinterpret_cast<const char *>(value),
reinterpret_cast<const char *>(value) + sizeBytes,
reinterpret_cast<char *>(_data + _putIndex));
}

_putIndex += sizeDataUnits;

return true;
}

bool CCData::get(const index_t index, uint8_t * const value, const size_t sizeBytes) const
bool CCData::get(const index_t index, void * const value, const size_t sizeBytes) const
{
if (index >= _capacity)
return false;

std::copy(reinterpret_cast<const uint8_t *>(_data + index),
reinterpret_cast<const uint8_t *>(_data + index) + sizeBytes,
value);
std::copy(reinterpret_cast<const char *>(_data + index),
reinterpret_cast<const char *>(_data + index) + sizeBytes,
reinterpret_cast<char *>(value));

return true;
}
Expand Down
14 changes: 5 additions & 9 deletions compiler/codegen/CCData.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class CCData
{
private:
/** \typedef data_t Implementation detail. This type represents the units of the table. Typically bytes, but can be some other data type, as long as it can be default-constructed. */
typedef uint8_t data_t;
typedef char data_t;

public:
/** \typedef index_t This type represents the indices defined in the public interface of this class. They must behave like integral types. */
Expand All @@ -63,10 +63,6 @@ class CCData
/** \typedef key_t This type represents the keys defined in the public interface of this class. The constructor is unspecified, use CCData_t::key() to create keys. */
typedef std::string key_t;

#if __cpp_static_assert
static_assert(sizeof(key_t::value_type) == 1, "Unimplemented key unit size, remaining bytes need to be zeroed to support key units >1.");
#endif

private:
/** \typedef map_t Implementation detail. This type represents the associative container that maps keys to indices. It must behave like std::unordered_map. */
typedef std::map<key_t, index_t> map_t;
Expand Down Expand Up @@ -107,7 +103,7 @@ class CCData
* @param[In] sizeBytes The size (in bytes) of the data.
* @return The key.
*/
static key_t key(const uint8_t * const data, const size_t sizeBytes);
static key_t key(const void * const data, const size_t sizeBytes);

/**
* @brief Creates a key_t from the given null-terminated C string.
Expand All @@ -125,7 +121,7 @@ class CCData
* @param[In] storage A pointer to a memory buffer that CCData will use.
* @param[In] sizeBytes The amount of data (in bytes) that the buffer contains.
*/
CCData(uint8_t * const storage, const size_t sizeBytes);
CCData(void * const storage, const size_t sizeBytes);

/**
* @brief Puts the given value in the table, optionally mapped to the given key (if any), aligned to the value's natural type, and returns the index to the value. Synchronized.
Expand All @@ -149,7 +145,7 @@ class CCData
* @param[Out] index The index that refers to the value.
* @return True if the value exists in the table, or the key was already mapped to an index, false otherwise.
*/
bool put(const uint8_t * const value, const size_t sizeBytes, const size_t alignmentBytes, const key_t * const key, index_t &index);
bool put(const void * const value, const size_t sizeBytes, const size_t alignmentBytes, const key_t * const key, index_t &index);

/**
* @brief Gets the value referred to by the index from the table.
Expand All @@ -170,7 +166,7 @@ class CCData
* @param[In] sizeBytes The size (in bytes) of the value pointed to. This parameter is ignored unless the operation succeeds and this function returns true.
* @return True if the index refers to an existing value, false otherwise.
*/
bool get(const index_t index, uint8_t * const value, const size_t sizeBytes) const;
bool get(const index_t index, void * const value, const size_t sizeBytes) const;

/**
* @brief Gets a pointer to the value referred to by the index from the table.
Expand Down
7 changes: 4 additions & 3 deletions compiler/codegen/CCData_inlines.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ CCData::key_t CCData::key(const T value)
// std::has_unique_object_representations is only available in C++17.
static_assert(std::has_unique_object_representations<T>::value == true, "T must have unique object representations.");
#endif
return key(reinterpret_cast<const uint8_t *>(&value), sizeof(value));
return key(&value, sizeof(value));
}

template <typename T>
Expand All @@ -46,7 +46,8 @@ bool CCData::put(const T value, const key_t * const key, index_t &index)
//#if __cpp_static_assert
// static_assert(std::is_trivially_copyable<T>::value == true, "T must be trivially copyable.");
//#endif
return put(reinterpret_cast<const uint8_t *>(&value), sizeof(value), alignof(value), key, index);
return put(&value, sizeof(value), alignof(value), key, index);
}
}

template <typename T>
Expand All @@ -56,7 +57,7 @@ bool CCData::get(const index_t index, T &value) const
//#if __cpp_static_assert
// static_assert(std::is_trivially_copyable<T>::value == true, "T must be trivially copyable.");
//#endif
return get(index, reinterpret_cast<uint8_t *>(&value), sizeof(value));
return get(index, &value, sizeof(value));
}

template <typename T>
Expand Down
30 changes: 15 additions & 15 deletions fvtest/compilertest/tests/CCDataTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ TYPED_TEST_CASE(CCDataTest, CCDataTestTypes);

TYPED_TEST(CCDataTest, test_basics_templated)
{
std::vector<uint8_t> storage(256);
std::vector<char> storage(256);
CCData table(&storage[0], storage.size());
const TypeParam data = 99;
// Generate a key from the data being stored.
Expand Down Expand Up @@ -250,20 +250,20 @@ TYPED_TEST(CCDataTest, test_basics_templated)

TYPED_TEST(CCDataTest, test_arbitrary_data_templated)
{
std::vector<uint8_t> storage(256);
std::vector<char> storage(256);
CCData table(&storage[0], storage.size());
const TypeParam d = 99;
const TypeParam data[3] = {d, d, d};
// Generate a key from the data being stored.
const CCData::key_t key = CCData::key(reinterpret_cast<const uint8_t *>(&data[0]), sizeof(data));
const CCData::key_t key = CCData::key(&data[0], sizeof(data));

// Nothing should be found by this key yet.
EXPECT_FALSE(table.find(key));

CCData::index_t index = INVALID_INDEX;

// Put the data in the table, associate it with the key, retrieve the index.
EXPECT_TRUE(table.put(reinterpret_cast<const uint8_t *>(&data[0]), sizeof(data), OMR_ALIGNOF(data), &key, index));
EXPECT_TRUE(table.put(&data[0], sizeof(data), OMR_ALIGNOF(data), &key, index));
// Make sure the index was written.
EXPECT_NE(index, INVALID_INDEX);

Expand All @@ -276,22 +276,22 @@ TYPED_TEST(CCDataTest, test_arbitrary_data_templated)
// Make sure it was written correctly.
EXPECT_TRUE(std::equal(data, data + sizeof(data)/sizeof(data[0]), dataPtr));
// We should be able to find something with this key now.
EXPECT_TRUE(table.find(CCData::key(reinterpret_cast<const uint8_t *>(&data[0]), sizeof(data))));
EXPECT_TRUE(table.find(CCData::key(&data[0], sizeof(data))));

const TypeParam dminus = -d; // Negating `d` here avoids narrowing conversion warnings/errors on the next line.
TypeParam out_data[3] = {dminus, dminus, dminus};

// Retrieve the data via the index.
EXPECT_TRUE(table.get(index, reinterpret_cast<uint8_t *>(&out_data[0]), sizeof(out_data)));
EXPECT_TRUE(table.get(index, &out_data[0], sizeof(out_data)));
// Make sure it matches what was stored.
EXPECT_TRUE(std::equal(data, data + sizeof(data)/sizeof(data[0]), out_data));
// Make sure both copies generate equal keys.
EXPECT_EQ(CCData::key(reinterpret_cast<const uint8_t *>(&data[0]), sizeof(data)), CCData::key(reinterpret_cast<const uint8_t *>(&out_data[0]), sizeof(out_data)));
EXPECT_EQ(CCData::key(&data[0], sizeof(data)), CCData::key(&out_data[0], sizeof(out_data)));

CCData::index_t find_index = INVALID_INDEX;

// Find the index via the key.
EXPECT_TRUE(table.find(CCData::key(reinterpret_cast<const uint8_t *>(&data[0]), sizeof(data)), &find_index));
EXPECT_TRUE(table.find(CCData::key(&data[0], sizeof(data)), &find_index));
// Make sure it's the same index.
EXPECT_EQ(index, find_index);

Expand All @@ -301,7 +301,7 @@ TYPED_TEST(CCDataTest, test_arbitrary_data_templated)

TYPED_TEST(CCDataTest, test_no_data_reservation_templated)
{
std::vector<uint8_t> storage(256);
std::vector<char> storage(256);
CCData table(&storage[0], storage.size());
size_t reservationSize = 32, reservationAlignment = 16;
// Generate an arbitrary key.
Expand Down Expand Up @@ -347,7 +347,7 @@ TYPED_TEST(CCDataTest, test_no_data_reservation_templated)

TYPED_TEST(CCDataTest, test_arbitrary_keys_templated)
{
std::vector<uint8_t> storage(256);
std::vector<char> storage(256);
CCData table(&storage[0], storage.size());
const TypeParam data = 99;
const CCData::key_t keyFromData = CCData::key(data);
Expand Down Expand Up @@ -376,7 +376,7 @@ TYPED_TEST(CCDataTest, test_arbitrary_keys_templated)
TYPED_TEST(CCDataTest, test_error_conditions_templated)
{
{
std::vector<uint8_t> storage(sizeof(TypeParam), 0);
std::vector<char> storage(sizeof(TypeParam), 0);
CCData zeroTable(&storage[0], 0);
const TypeParam data = 99;
CCData::index_t index = INVALID_INDEX;
Expand All @@ -386,15 +386,15 @@ TYPED_TEST(CCDataTest, test_error_conditions_templated)
// Make sure the index didn't change.
EXPECT_EQ(index, INVALID_INDEX);
// Make sure storage wasn't written to.
for (std::vector<uint8_t>::iterator i = storage.begin(); i != storage.end(); i++)
for (std::vector<char>::iterator i = storage.begin(); i != storage.end(); i++)
{
EXPECT_EQ(*i, 0);
}
}

{
const size_t smallSize = 16;
std::vector<uint8_t> storage(sizeof(TypeParam) * smallSize);
std::vector<char> storage(sizeof(TypeParam) * smallSize);
CCData smallTable(&storage[0], storage.size());
const TypeParam data = 99;
CCData::index_t lastIndex = INVALID_INDEX;
Expand All @@ -420,7 +420,7 @@ TYPED_TEST(CCDataTest, test_error_conditions_templated)

TYPED_TEST(CCDataTest, test_updating_templated)
{
std::vector<uint8_t> storage(256);
std::vector<char> storage(256);
CCData table(&storage[0], storage.size());
const TypeParam data1 = 99;
const TypeParam data2 = -data1;
Expand Down Expand Up @@ -455,7 +455,7 @@ TYPED_TEST(CCDataTest, test_updating_templated)

TEST(AllTypesCCDataTest, test_basics)
{
std::vector<uint8_t> storage(256);
std::vector<char> storage(256);
CCData table(&storage[0], storage.size());
const void * const classAptr = reinterpret_cast<void *>(0x1), * const classBptr = reinterpret_cast<void *>(0x2);
const void * const funcAptr = reinterpret_cast<void *>(0x100), * const funcBptr = reinterpret_cast<void *>(0x200);
Expand Down

0 comments on commit ee49651

Please sign in to comment.