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

Kieran Bingham kieran.bingham at ideasonboard.com
Fri Aug 28 15:41:34 UTC 2020


Hi Arnout,

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 ;-(


>  Regards,
>  Arnout
> 

-- 
Regards
--
Kieran


More information about the buildroot mailing list