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

[Pip Cet]

On Tue, May 26, 2020 at 7:09 AM Paul Eggert <address@hidden> wrote:
> On 5/25/20 11:40 PM, Pip Cet wrote:
>
> > It makes the code much more
> > complicated, all for hypothetical systems that require alignment > 8
> > for a hypothetical 16-byte data type in a Lisp structure.
>
> It's true that we haven't run into these systems yet. However, I worry that it
> won't be all that hypothetical in the not-so-distant future, given that so 
> many
> SIMD instructions require multiple-of-16 alignment and Emacs pseudovectors use
> system types that may require such alignment.

(I believe some AVX instructions require multiple-of-32 alignment.)

> > I think we should be specific here and say it's the mingw.org 32-bit
> > version (or whatever Eli's using) only that has problems.
>
> It can also happen with GCC 7 + glibc 2.25. Some platforms are fitfully moving
> to alignment-of-16 malloc and there are mismatches between system pieces. I'm
> not sure we can catalog all the affected systems.

Then we shouldn't mention any system at all.

> >> -   generate better code.
> >> +   generate better code.  Also, such structs should be added to the
> >> +   emacs_align_type union.
> >
> > That's going to be a maintenance nightmare, since failures to do so
> > won't actually show up on real machines, and a lot of wasted memory if
> > someone does add an AVX512 type.
>
> In current master I've fixed this so that there is zero wasted memory; the 
> type
> is used only to calculate alignment, not to allocate memory.

roundup_size still uses LISP_ALIGNMENT here, so I don't see how that's
true. I'll look again.

> > I'd prefer a simple warning not to use long double or similarly
> > unusual types in pseudovectors, and an eassert (see below) to catch it
> > if people do that.
>
> That's not going to work if some platform uses an alignment-of-16 type in
> pthread_cond_t (or some other system type that Emacs uses).

Well, realistically, if pthread_cond_t is going to require greater
alignment, it's probably going to require alignment to a cache line:
64 bytes or more. We don't want that to silently work (wasting as much
memory as it would), we want it to generate an assertion error so we
know to move the pthread_cond_t to a malloc'd area.

> > 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?

> But instead, how about an alignment argument to allocate_pseudovector, or a
> variant of allocate_pseudovector that takes such an argument? Then Emacs could
> support any alignment-greater-than-16 types that turn up.

Precisely. But until then, let's leave it as an eassert. Here's the
patch I was going to propose before you wrote.

Maybe there should be a variant of eassert that generates a
compile-time error if its argument is a compile-time constant that
evaluates to 0? Something like "eassert_reachable", maybe?

0001-Fix-GCALIGNMENT.patch
Description: Text Data