Fix undefined behavior in BitStruct.
Fixes broken bit_struct_test and subtype_check_test.
Test: m test-art-host-gtest
(tested on top of tree sync'd when they were broken).
Bug: 148125232
Change-Id: Iceee3ffc762ab28ea403d835b062766e5cbb088f
diff --git a/libartbase/base/bit_struct.h b/libartbase/base/bit_struct.h
index 292eca0..277e4b5 100644
--- a/libartbase/base/bit_struct.h
+++ b/libartbase/base/bit_struct.h
@@ -17,6 +17,9 @@
#ifndef ART_LIBARTBASE_BASE_BIT_STRUCT_H_
#define ART_LIBARTBASE_BASE_BIT_STRUCT_H_
+#include <type_traits>
+
+#include "base/casts.h"
#include "bit_struct_detail.h"
#include "bit_utils.h"
@@ -119,7 +122,7 @@
template <typename _ = void,
typename = std::enable_if_t<std::is_same<T, StorageType>::value, _>>
explicit operator StorageType() const {
- return GetStorage();
+ return BitFieldExtract(storage_, kBitOffset, kBitWidth);
}
BitStructField& operator=(T value) {
@@ -154,46 +157,27 @@
}
T Get() const {
- ValueStorage vs;
- vs.pod_.val_ = GetStorage();
- return vs.value_;
+ ExtractionType storage = static_cast<ExtractionType>(storage_);
+ ExtractionType extracted = BitFieldExtract(storage, kBitOffset, kBitWidth);
+ ConversionType to_convert = dchecked_integral_cast<ConversionType>(extracted);
+ return ValueConverter::FromUnderlyingStorage(to_convert);
}
void Set(T value) {
- ValueStorage value_as_storage;
- value_as_storage.value_ = value;
-
- storage_.pod_.val_ = BitFieldInsert(storage_.pod_.val_,
- value_as_storage.pod_.val_,
- kBitOffset,
- kBitWidth);
+ ConversionType converted = ValueConverter::ToUnderlyingStorage(value);
+ ExtractionType extracted = dchecked_integral_cast<ExtractionType>(converted);
+ storage_ = BitFieldInsert(storage_, extracted, kBitOffset, kBitWidth);
}
private:
- StorageType GetStorage() const {
- return BitFieldExtract(storage_.pod_.val_, kBitOffset, kBitWidth);
- }
+ using ValueConverter = detail::ValueConverter<T>;
+ using ConversionType = typename ValueConverter::StorageType;
+ using ExtractionType =
+ typename std::conditional<std::is_signed_v<ConversionType>,
+ std::make_signed_t<StorageType>,
+ StorageType>::type;
- // Underlying value must be wrapped in a separate standard-layout struct.
- // See below for more details.
- struct PodWrapper {
- StorageType val_;
- };
-
- union ValueStorage {
- // Safely alias pod_ and value_ together.
- //
- // See C++ 9.5.1 [class.union]:
- // If a standard-layout union contains several standard-layout structs that share a common
- // initial sequence ... it is permitted to inspect the common initial sequence of any of
- // standard-layout struct members.
- PodWrapper pod_;
- T value_;
- } storage_;
-
- // Future work: In theory almost non-standard layout can be supported here,
- // assuming they don't rely on the address of (this).
- // We just have to use memcpy since the union-aliasing would not work.
+ StorageType storage_;
};
// Base class for number-like BitStruct fields.
diff --git a/libartbase/base/bit_struct_detail.h b/libartbase/base/bit_struct_detail.h
index 60de1b6..380daa6 100644
--- a/libartbase/base/bit_struct_detail.h
+++ b/libartbase/base/bit_struct_detail.h
@@ -56,6 +56,50 @@
/* else */ type_unsigned>::type;
};
+// Helper for converting to and from T to an integral type.
+template <typename T>
+union ValueConverter {
+ using StorageType = typename MinimumTypeHelper<T, sizeof(T) * kBitsPerByte>::type;
+
+ static constexpr StorageType ToUnderlyingStorage(T value) {
+ ValueConverter converter;
+ converter.value_.val_ = value;
+ return converter.storage_.val_;
+ }
+
+ static constexpr T FromUnderlyingStorage(StorageType storage) {
+ ValueConverter converter;
+ converter.storage_.val_ = storage;
+ return converter.value_.val_;
+ }
+
+ // Underlying values must be wrapped in separate standard-layout structs.
+ // See below for more details.
+ struct StorageWrapper {
+ StorageType val_;
+ };
+ struct ValueWrapper {
+ T val_;
+ };
+
+ // Safely alias pod_ and value_ together.
+ //
+ // See C++ 9.5.1 [class.union]:
+ // If a standard-layout union contains several standard-layout structs that share a common
+ // initial sequence ... it is permitted to inspect the common initial sequence of any of
+ // standard-layout struct members.
+ StorageWrapper storage_;
+ ValueWrapper value_;
+#if __cplusplus >= 202000L
+#error "When upgrading to C++20, remove this error and check that this is OK for all use cases."
+ static_assert(std::is_layout_compatible_v<StorageWrapper, ValueWrapper>);
+#endif
+
+ // Future work: In theory almost non-standard layout can be supported here,
+ // assuming they don't rely on the address of (this).
+ // We just have to use memcpy since the union-aliasing would not work.
+};
+
// Denotes the beginning of a bit struct.
//
// This marker is required by the C++ standard in order to
