[libcamera-devel] [PATCH 5/6] libcamera: controls: Rationalize idMap() function

[Kieran Bingham]

On 01/09/2021 15:37, Jacopo Mondi wrote:
> The ControlList class has a pair of accessor functions with a similar
> signature:
> 
> 	const ControlInfoMap *infoMap() const { return infoMap_; }
> 	const ControlIdMap *idMap() const { return idmap_; }
> 
> As ControlList::infoMap_ can be nullptr, the two functions returns the
> class members as pointers and not references.
> 
> The ControlInfoMap class has instead:
> 
> 	const ControlIdMap &idmap() const { return *idmap_; }
> 
> Which is disturbingly different in the signature and return type.
> 
> Rationalize the accessor function names by stabilizing on:
> 
> 	const ControlIdMap *idMap() const { return idmap_; }
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/controls.h         | 2 +-
>  src/libcamera/camera_sensor.cpp      | 2 +-
>  src/libcamera/control_serializer.cpp | 4 ++--
>  src/libcamera/controls.cpp           | 4 ++--
>  src/libcamera/delayed_controls.cpp   | 4 ++--
>  5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 8e5bc8b70b94..de6faf600e19 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -338,7 +338,7 @@ public:
>  	iterator find(unsigned int key);
>  	const_iterator find(unsigned int key) const;
>  
> -	const ControlIdMap &idmap() const { return *idmap_; }
> +	const ControlIdMap *idMap() const { return idmap_; }
>  
>  private:
>  	bool validate();
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 876685097f76..48af3a617ee1 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -176,7 +176,7 @@ int CameraSensor::validateSensorDriver()
>  		V4L2_CID_CAMERA_SENSOR_ROTATION,
>  	};
>  
> -	const ControlIdMap &controls = subdev_->controls().idmap();
> +	const ControlIdMap &controls = *subdev_->controls().idMap();

Aye? Is this legal?

Is that declaring a reference against a dereferenced pointer?

I can't tell if that's making a copy - or ... time for a compile-explorer:

	https://godbolt.org/z/j75669Mrf

Ok - so it's not making a copy, and it generates identical code to:

	const ControlIdMap *controls = subdev_->controls().idMap()

But without you having to change all the uses of controls to controls->
instead of controls. ?

But given there are only 3 ... ?


https://isocpp.org/wiki/faq/references#refs-vs-ptrs
 states
>  "Use references when you can, and pointers when you have to."

But https://isocpp.org/wiki/faq/references#refs-not-null
makes it clear:

> It means this is illegal:
> 
>     T* p = nullptr;
>     T& r = *p;  // illegal

So we have to be /really/ sure that idMap() would not return a nullptr,
or we get into undefined behaviour, and the cpu turns into a black hole.

I suspect a reference use here is dangerous ...





>  	for (uint32_t ctrl : optionalControls) {
>  		if (!controls.count(ctrl))
>  			LOG(CameraSensor, Debug)
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index d4377c024079..7eef041a16ee 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -190,7 +190,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
>  	for (const auto &ctrl : infoMap)
>  		valuesSize += binarySize(ctrl.second);
>  
> -	const ControlIdMap *idmap = &infoMap.idmap();
> +	const ControlIdMap *idmap = infoMap.idMap();
>  	enum ipa_controls_id_map_type idMapType;
>  	if (idmap == &controls::controls)
>  		idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
> @@ -530,7 +530,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
>  		}
>  
>  		const ControlInfoMap *infoMap = iter->first;
> -		idMap = &infoMap->idmap();
> +		idMap = infoMap->idMap();
>  	} else {
>  		switch (hdr->id_map_type) {
>  		case IPA_CONTROL_ID_MAP_CONTROLS:
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 96625edb1380..9b9bad212db3 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -778,7 +778,7 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
>  }
>  
>  /**
> - * \fn const ControlIdMap &ControlInfoMap::idmap() const
> + * \fn const ControlIdMap *ControlInfoMap::idMap() const
>   * \brief Retrieve the ControlId map
>   *
>   * Constructing ControlList instances for V4L2 controls requires a ControlIdMap
> @@ -832,7 +832,7 @@ ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)
>   * \param[in] validator The validator (may be null)
>   */
>  ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *validator)
> -	: validator_(validator), idmap_(&infoMap.idmap()), infoMap_(&infoMap)
> +	: validator_(validator), idmap_(infoMap.idMap()), infoMap_(&infoMap)
>  {
>  }
>  
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index 90ce7e0b5b52..763021ef09bb 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -130,7 +130,7 @@ void DelayedControls::reset()
>  	/* Seed the control queue with the controls reported by the device. */
>  	values_.clear();
>  	for (const auto &ctrl : controls) {
> -		const ControlId *id = device_->controls().idmap().at(ctrl.first);
> +		const ControlId *id = device_->controls().idMap()->at(ctrl.first);
>  		/*
>  		 * Do not mark this control value as updated, it does not need
>  		 * to be written to to device on startup.
> @@ -158,7 +158,7 @@ bool DelayedControls::push(const ControlList &controls)
>  	}
>  
>  	/* Update with new controls. */
> -	const ControlIdMap &idmap = device_->controls().idmap();
> +	const ControlIdMap &idmap = *device_->controls().idMap();

Another location of taking a reference from a pointer.
I think this is likely to flag up the UBSAN, even if it compiles cleanly.


>  	for (const auto &control : controls) {
>  		const auto &it = idmap.find(control.first);
>  		if (it == idmap.end()) {
>