Re: Make max_align_t respect _Float128 [version 2]

[Florian Weimer <fweimer at redhat dot com>]

On 09/06/2016 01:59 PM, Bernd Schmidt wrote:
> On 09/06/2016 11:11 AM, Florian Weimer wrote:
>
> > I think this change is only safe because nothing relies on it.
> > max_align_t is a committee invention with no actual users.
>
> I tried to verify that and ran grep over all the packages in
> /usr/portage/distfiles. That did get a number of matches, fairly obvious
> and expected ones like gcc and glibc, and several more which match only
> because gnulib seems to try to provide a default, but I also found a few
> real uses:
>
> grep-2.25/lib/fts.c:
>         /* Align the allocation size so that it works for FTSENT,
>            so that trailing padding may be referenced by direct access
>            to the flexible array members, without triggering undefined
> behavior
>            by accessing bytes beyond the heap allocation.  This implicit
> access
>            was seen for example with ISDOT() and GCC 5.1.1 at -O2.
>            Do not use alignof (FTSENT) here, since C11 prohibits
>            taking the alignment of a structure containing a flexible
>            array member.  */
>         len += alignof (max_align_t) - 1;
>         len &= ~ (alignof (max_align_t) - 1);

The context looks like this:

  /*
   * The file name is a variable length array.  Allocate the FTSENT
   * structure and the file name in one chunk.
   */
  len = offsetof(FTSENT, fts_name) + namelen + 1;
  /* Align the allocation size so that it works for FTSENT,
     so that trailing padding may be referenced by direct access
     to the flexible array members, without triggering undefined behavior
     by accessing bytes beyond the heap allocation.  This implicit access
     was seen for example with ISDOT() and GCC 5.1.1 at -O2.
     Do not use alignof (FTSENT) here, since C11 prohibits
     taking the alignment of a structure containing a flexible
     array member.  */
  len += alignof (max_align_t) - 1;
  len &= ~ (alignof (max_align_t) - 1);
  if ((p = malloc(len)) == NULL)
          return (NULL);

This code was added to work around a GCC bug:

  <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66661>

It's rather dubious that this fix is necessary or even correct.  Where 
would this stop?  Would

  char buf[23];
  memset (buf, 'X', sizeof buf);
  strndup (buf, 23);

have to allocate 32 bytes so that the object size is a multiple of 16 on 
x86_64 (where alignof (max_align_t) is 16)?

> libvpx-v1.3.0/nestegg/halloc/src/halloc.c:
> /*
>  *      block control header
>  */
> typedef struct hblock
> {
> #ifndef NDEBUG
> #define HH_MAGIC    0x20040518L
>         long          magic;
> #endif
>         hlist_item_t  siblings; /* 2 pointers */
>         hlist_head_t  children; /* 1 pointer  */
>         max_align_t   data[1];  /* not allocated, see below */
>
> } hblock_t;

This is a false hit.  Upstream is here:

  https://github.com/apankrat/halloc

src/align.h has:

union max_align
{
        char   c;
        short  s;
        long   l;
        int    i;
        float  f;
        double d;
        void * v;
        void (*q)(void);
};

typedef union max_align max_align_t;

I'm not sure if it is an attempt to emulate max_align_t, or if it is a 
name collision.  It's from 2011, so perhaps the latter.  It's certainly 
incorrect for x86_64.  It works by accident in the halloc context 
because the object header is four words large, so it preserves the 
alignment of the underlying allocator.

Florian