[libcamera-devel] [PATCH] test: v4l2_device: Provide compile time assertions

[Laurent Pinchart]

Hi Kieran,

On Mon, Jan 21, 2019 at 12:39:53PM +0000, Kieran Bingham wrote:
> On 21/01/2019 12:15, Laurent Pinchart wrote:
> > On Mon, Jan 21, 2019 at 10:28:26AM +0000, Kieran Bingham wrote:
> >> The V4L2Capability structure is inherited from struct v4l2_capability only
> >> to provide an interface. It must not extend the type or data.
> >>
> >> Enforce this with a static assertion with sizeof() comparisons.
> >>
> >> There is no need here for a specific test binary which will always return
> >> TEST_PASS when compiled, as this test failure will be caught at compile time.
> >> In light of this - the static compile time assertion is added to the
> >> V4L2DeviceTest base class.
> >>
> >> Should there be a large number of static assertions required, they could be
> >> moved to their own unit for clarity.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  test/v4l2_device/v4l2_device_test.cpp | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
> >> index 362553712caa..97876f8d65db 100644
> >> --- a/test/v4l2_device/v4l2_device_test.cpp
> >> +++ b/test/v4l2_device/v4l2_device_test.cpp
> >> @@ -5,6 +5,7 @@
> >>   * libcamera V4L2 API tests
> >>   */
> >>  
> >> +#include <assert.h>
> >>  #include <iostream>
> >>  #include <sys/stat.h>
> >>  
> >> @@ -41,3 +42,8 @@ void V4L2DeviceTest::cleanup()
> >>  {
> >>  	delete dev_;
> >>  };
> >> +
> >> +/* Static compile time assertion tests */
> >> +
> >> +static_assert(sizeof(struct v4l2_capability) == sizeof(struct V4L2Capability),
> >> +	      "V4L2Capability must match v4l2_capability size");
> > 
> > I'm still not sure if we really need this, as this is more a check of
> > our C++ understanding than of our implementation :-) If you insist, I
> > prefer this check to be in a test case than in libcamera where it
> > originally was, so I won't push back too hard. The error message may
> > need some rewording though, as it sounds like it's something we can fix.
> 
> It's not a check of our understanding - it's a contract written in the
> code - that the type V4L2Capability is used directly as a
> v4l2_capability and as such must remain compatible.

But that's guaranteed by C++ (isn't it ?), and we don't have tests for
all contracts from the C++ standard :-)

> If this assertion fires, then it should be something to be fixed (or
> that developer takes responsibility for removing this check and all
> guarantees I make are removed)
> 
> I'm not expecting that the compiler might break the sizes arbitrarily -
> I'm ensuring that if a developer comes along and changes the size of the
> derived structure (s)he will get a warning, and be told off politely for
> changing the size of the V4L2Capability.
> 
> > By the way, do we really require the two to be equal ? V4L2Capability
> > could include more members (I don't see a real use case for that now,
> > but that's imaginable), in which case it would be laid ou with
> > v4l2_capability first followed by other members, and still work fine.
> 
> Yes, I agree - and originally I had planned for that to be possible.
> for instance - to cache the appropriate capabilities variable (i.e. if
> the device capabilities are to be referenced instead)
> 
> I plan to add a function which I believe will be inlined to handle that
> instead, but it states the point.
> 
> However, if this happens - it could get messy - if the base class (the
> kernel structure) members are overridden by the derived class people
> might not get the behaviour they expect. (ioctl writes to the base class
> struct, user reads from derived override)

I don't see how that could happen, as C++ should guarantee that the
object will be layed out in memory with the base class first (again,
doesn't it ?).

> Anyway - the point of this assert it to state the intentions of the
> definition, and provide a warning if anyone breaks that assumption - not
> to assert that two objects which we know are currently the same size -
> "are the same size".

My question was if we should test if the compiler breaks assumptions
that come from the C++ standard :-)

-- 
Regards,

Laurent Pinchart