[libcamera-devel] [PATCH] libcamera: controls: Fix strict aliasing violation

[Kieran Bingham]

Hi Laurent,

On 07/03/2020 21:25, Laurent Pinchart wrote:
> gcc 8.3.0 for ARM complains about strict aliasing violations:
> 
> ../../src/libcamera/controls.cpp: In member function ‘void libcamera::ControlValue::release()’:
> ../../src/libcamera/controls.cpp:111:13: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
>    delete[] *reinterpret_cast<char **>(&storage_);
> 
> Fix it and simplify the code at the same time.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

For either kept separate, or squashed into the other patch that adds this:

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Though I wonder, could the above error also have been resolved with use
of a uintptr_t?

But the union does seem to make the duality of use more clear to me all
the same.



> ---
>  include/libcamera/controls.h |  5 ++++-
>  src/libcamera/controls.cpp   | 20 ++++++++++----------
>  2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 4767e2d3fc8c..0e111ab72bce 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -160,7 +160,10 @@ private:
>  	ControlType type_ : 8;
>  	bool isArray_ : 1;
>  	std::size_t numElements_ : 16;
> -	uint64_t storage_;
> +	union {
> +		uint64_t value_;
> +		void *storage_;
> +	};
>  
>  	void release();
>  	void set(ControlType type, bool isArray, const void *data,
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 94bdbdd9c388..4326174adf86 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -107,9 +107,9 @@ void ControlValue::release()
>  {
>  	std::size_t size = numElements_ * ControlValueSize[type_];
>  
> -	if (size > sizeof(storage_)) {
> -		delete[] *reinterpret_cast<char **>(&storage_);
> -		storage_ = 0;
> +	if (size > sizeof(value_)) {
> +		delete[] reinterpret_cast<uint8_t *>(storage_);
> +		storage_ = nullptr;
>  	}
>  }
>  
> @@ -176,9 +176,9 @@ ControlValue &ControlValue::operator=(const ControlValue &other)
>  Span<const uint8_t> ControlValue::data() const
>  {
>  	std::size_t size = numElements_ * ControlValueSize[type_];
> -	const uint8_t *data = size > sizeof(storage_)
> -			    ? *reinterpret_cast<const uint8_t * const *>(&storage_)
> -			    : reinterpret_cast<const uint8_t *>(&storage_);
> +	const uint8_t *data = size > sizeof(value_)
> +			    ? reinterpret_cast<const uint8_t *>(storage_)
> +			    : reinterpret_cast<const uint8_t *>(&value_);
>  	return { data, size };
>  }
>  
> @@ -308,11 +308,11 @@ void ControlValue::set(ControlType type, bool isArray, const void *data,
>  	std::size_t size = elementSize * numElements;
>  	void *storage;
>  
> -	if (size > sizeof(storage_)) {
> -		storage = reinterpret_cast<void *>(new char[size]);
> -		*reinterpret_cast<void **>(&storage_) = storage;
> +	if (size > sizeof(value_)) {
> +		storage_ = reinterpret_cast<void *>(new uint8_t[size]);
> +		storage = storage_;
>  	} else {
> -		storage = reinterpret_cast<void *>(&storage_);
> +		storage = reinterpret_cast<void *>(&value_);
>  	}
>  
>  	memcpy(storage, data, size);
> 

-- 
Regards
--
Kieran