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

[Laurent Pinchart]

Hi Kieran,

On Sat, Mar 07, 2020 at 11:37:21PM +0000, Kieran Bingham wrote:
> 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?

I don't think so. Strict aliasing means, in a nutshell, that the
compiler can assume that pointers to different types do not point to the
same memory. There's an exception for char * that can alias anything.
The aliasing rules allow for interesting optimisations, but can also
create "interesting" behaviour.

#include <iostream>

void foo(int *i, char *c)
{
	*i = 0x42;
	c[0] = 0;
	c[1] = 1;
	c[2] = 2;
	c[3] = 3;

	std::cout << std::hex << *i << std::endl;
}

void foo(int *i, short *s)
{
	*i = 0x42;
	s[0] = 0;
	s[1] = 1;

	std::cout << std::hex << *i << std::endl;
}

int main(int argc __attribute__((__unused__)), char *argv[] __attribute__((__unused__)))
{
	int i = 0;
	short *s = reinterpret_cast<short *>(&i);
	char *c = reinterpret_cast<char *>(&i);

	foo(&i, c);
	foo(&i, s);

	return 0;
}

$ g++ -W -Wall -O1 -o aliasing aliasing.cpp 
$ ./aliasing 
3020100
10000
$ g++ -W -Wall -O2 -o aliasing aliasing.cpp 
$ ./aliasing 
3020100
42

clang++ exhibits the same behaviour. No idea why neither prints any
warning in the above example though :-S

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

I like the union better too.

> > ---
> >  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,

Laurent Pinchart