[libcamera-devel] [PATCH/RFC] libcamera: bound_method: Please gcc undefined behaviour sanitizer

[Kieran Bingham]

On 13/04/2021 21:36, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Tue, Apr 13, 2021 at 02:02:46PM +0100, Kieran Bingham wrote:
>> Hi Laurent,
>>
>> $SUBJECT might need improving ;-)
> 
> s/Please/Please the/ ?

Aha, yes - now I understand what you meant!

It looks like some sort of odd malformed request otherwise.
--
Kieran



>> On 12/04/2021 23:59, Laurent Pinchart wrote:
>>> Enabling the gcc undefined behaviour sanitizer (with the meson configure
>>> -Db_sanitize=undefined option) causes many tests to fail, with errors
>>> such as the following (for test/object-invoke):
>>>
>>> ../../include/libcamera/bound_method.h:198:27: runtime error: member access within address 0x55fcd7bfbd38 which does not point to an object of type 'BoundMethodBase'
>>> 0x55fcd7bfbd38: note: object has invalid vptr
>>>  fc 55 00 00  2a 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  31 00 00 00 00 00 00 00  4b c6 72 88
>>>               ^~~~~~~~~~~~~~~~~~~~~~~
>>>               invalid vptr
>>> ../../include/libcamera/bound_method.h:198:41: runtime error: member call on null pointer of type 'struct InvokedObject'
>>> ../../include/libcamera/bound_method.h:198:41: runtime error: member access within null pointer of type 'struct InvokedObject'
>>> Segmentation fault
>>>
>>> The root cause isn't clear, but this change fixes the issue. It may be a
>>> bug in gcc.
>>
>> Given that this is just a small semantic change to separate the casting
>> of the object, and the calling, I don't think there's any harm in
>> applying this as is, given that even if we fix GCC we'd have to wait for
>> it to filter down?
> 
> I agree.
> 
>> However I have no real understanding of the underlying issues either I'm
>> afraid, so I don't think I can help ...
>>
>> Is it easy to reduce it down to a reproducible issue?
> 
> It's lots of template code, so it may not be that easy. I haven't tried
> yet. Ideally we should file a bug with gcc.
> 
>> Acked-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>>  include/libcamera/bound_method.h | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> If anyone is interested in trying the gcc undefined behaviour sanitizer,
>>> this patch is needed. Bonus points if you can spot why it helps.
>>>
>>> diff --git a/include/libcamera/bound_method.h b/include/libcamera/bound_method.h
>>> index f216e3b56826..de17fdee3612 100644
>>> --- a/include/libcamera/bound_method.h
>>> +++ b/include/libcamera/bound_method.h
>>> @@ -163,7 +163,8 @@ public:
>>>  
>>>  	R invoke(Args... args) override
>>>  	{
>>> -		return (static_cast<T *>(this->obj_)->*func_)(args...);
>>> +		T *obj = static_cast<T *>(this->obj_);
>>> +		return (obj->*func_)(args...);
>>>  	}
>>>  
>>>  private:
>>> @@ -195,7 +196,8 @@ public:
>>>  
>>>  	void invoke(Args... args) override
>>>  	{
>>> -		(static_cast<T *>(this->obj_)->*func_)(args...);
>>> +		T *obj = static_cast<T *>(this->obj_);
>>> +		(obj->*func_)(args...);
>>>  	}
>>>  
>>>  private:
>>>
> 

-- 
Regards
--
Kieran