[libcamera-devel] libcamera: controls: Fix strict aliasing violation
diff mbox series

Message ID 20200307212530.28053-3-laurent.pinchart@ideasonboard.com
State Accepted
Commit 4de31ccc9ef47e7b16330d226d071d5d006faa6d
Headers show
Series
  • [libcamera-devel] libcamera: controls: Fix strict aliasing violation
Related show

Commit Message

Laurent Pinchart March 7, 2020, 9:25 p.m. UTC 
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@ideasonboard.com>
---
 include/libcamera/controls.h |  5 ++++-
 src/libcamera/controls.cpp   | 20 ++++++++++----------
 2 files changed, 14 insertions(+), 11 deletions(-)

Comments

Kieran Bingham March 7, 2020, 11:37 p.m. UTC | #1 
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@ideasonboard.com>

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

Reviewed-by: Kieran Bingham <kieran.bingham@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);
>
Laurent Pinchart March 8, 2020, 12:01 a.m. UTC | #2 
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@ideasonboard.com>
> 
> For either kept separate, or squashed into the other patch that adds this:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@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);

Patch
diff mbox series 

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);