[libcamera-devel] [Buildroot] [autobuild.buildroot.net] Your daily results for 2020-08-18

[Laurent Pinchart]

Hi Kieran,

On Fri, Aug 28, 2020 at 04:41:34PM +0100, Kieran Bingham wrote:
> On 28/08/2020 12:17, Arnout Vandecappelle wrote:
> > On 28/08/2020 11:08, Kieran Bingham wrote:
> >> 	enum ControlType   type_:8;                      /*     0: 0  4 */
> >> 	/* Bitfield combined with next fields */
> >> 	bool                       isArray_;             /*     1     1 */
> >> 	/* Bitfield combined with previous fields */
> >> 	size_t                     numElements_:32;      /*     0:16  8 */
> > 
> >  So this one is in fact unaligned? Did you never encounter a runtime error on
> > machines that don't allow unaligned access? Or perhaps gcc generates a different
> > layout in that case, with the hole before numElements instead of after it...
> 
> Well, this is the output of pahole on the binary generated by GCC, so I
> expect that is the actual layout.
> 
> Looking at the history of this struct, that field was originally 16
> bits, and was enlarged to prevent another packing failure. But all of
> this feels quite fragile indeed, so the fact that we're having to look
> at this at all indicates there is something drastically to improve here...
> 
> >> 	/* XXX 16 bits hole, try to pack */
> >> 	union {
> >> 		uint64_t           value_;               /*     8     8 */
> >> 		void *             storage_;             /*     8     8 */
> >> 	};                                               /*     8     8 */
> > 
> >  Having a layout like this in a struct that is ABI is asking for trouble IMO,
> > because some compile options (e.g. -fpack-struct) will change it. I think you
> > should add padding fields to make sure alignment is exactly as you expect, and
> > also give the bool member an explicit size (i.e. make it a bitfield too). The
> > unaligned field is bad news IMO, but changing that is an ABI change...
> 
> Well the good news is we're not ABI stable yet, so we have free reign to
> change things while we need to (which is a key reason why we haven't
> tagged an official 'release' of the library yet until we have analysed
> and determined that we can do so).
> 
> One of the key aspects of this class is to provide a generic container
> for arbitrary values, for passing properties around.
> 
> Last week we finally decided to just accept moving to C++17. This gives
> us std::any, so I wonder if that would fix all this up a bit. But even
> so I think we're still going to need some 'meta-data' like the isArray
> or such ... so maybe that doesn't actually improve things at this level ;-(

We'll have to rework this at some point. Let's just disable m68k for
now, and revisit later.

-- 
Regards,

Laurent Pinchart