Re: master 9227864: Further fix for aborts due to GC losing pseudovectors

[Pip Cet]

On Tue, May 26, 2020 at 7:40 AM Paul Eggert <address@hidden> wrote:
> On 5/26/20 12:25 AM, Pip Cet wrote:
> > roundup_size still uses LISP_ALIGNMENT here, so I don't see how that's
> > true.
>
> Oh, you're right. No harm so far since LISP_ALIGNMENT is 8 on current 
> platforms.
> But this area could use some thinking if we want more efficiency on platforms
> where it's 16 (so far, I've been worried only about avoiding crashes on such
> machines).

Absolutely. I like the idea of an allocate_aligned_pseudovector API,
though it should be stubbed out (using eassert_reachable, or
_Static_assert) for now.

> >>> I think a simple eassert (GCALIGNMENT % alignof (type) == 0) in an
> >>> (inlined, obviously) version of allocate_pseudovector should suffice
> >>> to catch this hypothetical problem.
> >>
> >> I assume you meant 'verify' rather than 'eassert'? That'd catch the bug at
> >> compile time.
> >
> > I don't see how that would be possible using inline functions?
>
> We should use macros, as they'll catch this at compile-time.

Okay.

> (I don't know how to do an eassert_reachable.)

Something like

extern int __unreachable_function (int);

#define eassert(cond)                    \
  ({                            \
    if (__builtin_constant_p ((cond) && __unreachable_function((cond) != 0))) \
      {                            \
    *(int *)0 = 0;                    \
    __unreachable_function (0);            \
      }                            \
    eassert_1 (cond);                    \
  })

This provides a compile-time warning (easily upgraded to an error with
the right -Werror switch), a link-time error, and a run-time
assertion. It's not pretty, but it gets the job done.

FWIW, with this (invalid) version of eassert, the following correct,
but weird, code generates a link-time error:

    case Lisp_Int:
      eassert ("should not be dumping int: is self-representing" && 0);
      abort ();

But, at -O2/-O3, on x86_64-pc-linux-gnu, no other places in the build
appear to be using eassert (0).

> We're already using macros for ALLOCATE_PSEUDOVECTOR and the like, so this 
> should not be a big deal.

I'm finding it hard to do so, but I'm not used to working with
_Static_assert and will try again.