| Message ID | 20210412225948.13796-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, $SUBJECT might need improving ;-) 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? 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? Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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: >
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/ ? > 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@ideasonboard.com> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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: > >
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@ideasonboard.com> >> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@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: >>> >
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:
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. Signed-off-by: Laurent Pinchart <laurent.pinchart@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.