Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Glenn Morris-3
Paul Eggert wrote:

> branch: emacs-26
> commit 9e59de9449b53c3ecd85b624c11360ba9cafee75
[...]
>     Use GCALIGNED properly for GCC

This seems to cause build failure on 32-bit GNU/Linux.
build.i686-linux on hydra fails since this commit was merged to master:

https://hydra.nixos.org/eval/1408828

(Sadly the hydra build logs seem to be missing at the moment, so there's
nothing informative there.)

I reproduced the issue on 64-bit RHEL7 building the emacs-26 branch with

./configure --without-all --without-x CC="gcc -m32"

The command "./temacs --batch --load loadup bootstrap" simply exits
immediately with no output and status 0.

With 9e59de9 reverted, the same build completes ok.

Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

martin rudalics
... and trying my usual 32-bit build of the release branch on Windows
currently fails as

(gdb) run --batch  --load loadup bootstrap
Starting program: c:\emacs-git\release\dbg\src\temacs.exe --batch  --load loadup bootstrap
[New Thread 3424.0xcf4]

Breakpoint 1, terminate_due_to_signal (sig=22, backtrace_limit=2147483647) at ../../src/emacs.c:364
364  signal (sig, SIG_DFL);
(gdb) bt
#0  terminate_due_to_signal (sig=22, backtrace_limit=2147483647) at ../../src/emacs.c:364
#1  0x01184515 in die (msg=0x150ce54 <PSEUDOVECTOR_FLAG+48> "XTYPE (a) == type && XUNTAG (a, type) == ptr", file=0x150cdf0 <Qzlib+4> "../../src/lisp.h", line=1068) at ../../src/alloc.c:7431
#2  0x010fdc0e in make_lisp_ptr (ptr=0x12b3e94 <Sinternal_make_lisp_face>, type=Lisp_Vectorlike) at ../../src/lisp.h:1068
#3  0x011de6b2 in defsubr (sname=0x12b3e94 <Sinternal_make_lisp_face>) at ../../src/lread.c:4354
#4  0x010fd66a in syms_of_xfaces () at ../../src/xfaces.c:6460
#5  0x0110423a in main (argc=5, argv=0xa33fd0) at ../../src/emacs.c:1220
(gdb)

Reverting to b9d7c902603a49d2624bdd35efdfba1785a4bce5 makes the build
succeed again.

martin

Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Eli Zaretskii
> Date: Fri, 10 Nov 2017 08:10:54 +0100
> From: martin rudalics <[hidden email]>
> Cc: Paul Eggert <[hidden email]>
>
> ... and trying my usual 32-bit build of the release branch on Windows
> currently fails as

Paul, maybe we should simply go back a few notches and not mark
main_thread as GCALIGNED, except on that single platform which needed
it (i.e. via #ifdef)?  Then we could return all the other symbols to
their previous syntax.

Or maybe we should move GCALIGNED to yet another position, like at the
beginning or the end of the whole construct.  Like this:

   GCALIGNED struct foo FOO;
or
   struct foo FOO GCALIGNED;

Not sure what else can we do, but this seems to be an exceptionally
fragile feature, at least with GCC 7 that many people are using.

Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Paul Eggert
Eli Zaretskii wrote:

> Paul, maybe we should simply go back a few notches and not mark
> main_thread as GCALIGNED, except on that single platform which needed
> it (i.e. via #ifdef)?  Then we could return all the other symbols to
> their previous syntax.
>
> Or maybe we should move GCALIGNED to yet another position, like at the
> beginning or the end of the whole construct.  Like this:
>
>     GCALIGNED struct foo FOO;
> or
>     struct foo FOO GCALIGNED;
>
> Not sure what else can we do, but this seems to be an exceptionally
> fragile feature, at least with GCC 7 that many people are using.

I've investigated this further, and neither solution is likely to work. It is
merely the luck of the draw that we have seen the bug only on that platform so
far, and a similar bug is likely to occur on other platforms. Neither GCALIGNED
syntax works in general (see GCC bug 82914) and we've seen examples of failures
with either syntax.

I have thought of a solution that fixes the problem by dropping use of GCALIGNED
and instead using classic C unions along with 'char alignas (8)'. The idea is to
gcalign a 'struct foo' this way:

    union gcaligned_foo { struct foo s; char alignas (8) gcaligned; };

Something like this should work on all platforms that Emacs ports to. I plan to
work on a first cut tomorrow.

Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Eli Zaretskii
> Cc: [hidden email], [hidden email]
> From: Paul Eggert <[hidden email]>
> Date: Fri, 10 Nov 2017 00:26:19 -0800
>
> I have thought of a solution that fixes the problem by dropping use of GCALIGNED
> and instead using classic C unions along with 'char alignas (8)'. The idea is to
> gcalign a 'struct foo' this way:
>
>     union gcaligned_foo { struct foo s; char alignas (8) gcaligned; };

Wouldn't it be more reliable to use something like

  union gcaligned_foo { struct foo s; int64_t gcaligned; };

IOW, should we rely on alignas?  There could be dragons there too, no?

> Something like this should work on all platforms that Emacs ports to. I plan to
> work on a first cut tomorrow.

Thanks.

Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Stefan Monnier
>   union gcaligned_foo { struct foo s; int64_t gcaligned; };

Are int64_t necessarily aligned on multiples of 8 on 32bit platforms?

> IOW, should we rely on alignas?  There could be dragons there too, no?

FWIW, for the dummy alignment thingy I wouldn't use `char` (I wouldn't be
surprised to see errors in compilers when asking to align on multiples
of N for objects smaller than N), so maybe

    #define gc_aligned(typename) \
       union { typename s; int64_t alignas (GCALIGNMENT) dummy; };

I'm not super happy about the "64" in there (which hardcodes basically
the value of GCALIGNMENT).  Depending on how alignas can be used to
impose alignment of an array, we could try:

    typedef char gcsized_t[GCALIGNMENT];
    #define gc_aligned(typename) \
       union { typename s; gcsized_t alignas (GCALIGNMENT) dummy; }

but maybe getting rid of this 64 is not that important.


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Paul Eggert
Stefan Monnier wrote:
>>    union gcaligned_foo { struct foo s; int64_t gcaligned; };
>
> Are int64_t necessarily aligned on multiples of 8 on 32bit platforms?

No, unfortunately.

>> IOW, should we rely on alignas?  There could be dragons there too, no?
>
> FWIW, for the dummy alignment thingy I wouldn't use `char` (I wouldn't be
> surprised to see errors in compilers when asking to align on multiples
> of N for objects smaller than N), so maybe
>
>      #define gc_aligned(typename) \
>         union { typename s; int64_t alignas (GCALIGNMENT) dummy; };

That does not work either, alas, as C11 says 'alignas (8)' is an error when the
natural alignment of the object is less than 8. This is one of the problems that
we have encountered in earlier attempts to fix this bug. 'char alignas (8)'
avoids this problem.

If we run into a platform where alignas (8) does not work either natively or via
Gnulib emulation, the patch I'm working on has a 'verify' check that should
result in a build failure. Although I have some ideas for fixing the situation
if it arises, they would add some complexity (and would depend on the details of
any problematic hosts), and I'd rather avoid this if possible.

Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Stefan Monnier
>>> union gcaligned_foo { struct foo s; int64_t gcaligned; };
>> Are int64_t necessarily aligned on multiples of 8 on 32bit platforms?
> No, unfortunately.

That was my impression as well.

> That does not work either, alas, as C11 says 'alignas (8)' is an error when
> the natural alignment of the object is less than 8. This is one of the
> problems that we have encountered in earlier attempts to fix this bug. 'char
> alignas (8)' avoids this problem.

I don't follow: the natural alignment for `char` is definitely less than
8, so if "'alignas (8)' is an error when the natural alignment of the
object is less than 8", how can "char alignas (8)" avoid the problem?


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Philipp Stephani


Stefan Monnier <[hidden email]> schrieb am Fr., 10. Nov. 2017 um 19:03 Uhr:
>>> union gcaligned_foo { struct foo s; int64_t gcaligned; };
>> Are int64_t necessarily aligned on multiples of 8 on 32bit platforms?
> No, unfortunately.

That was my impression as well.

> That does not work either, alas, as C11 says 'alignas (8)' is an error when
> the natural alignment of the object is less than 8. This is one of the
> problems that we have encountered in earlier attempts to fix this bug. 'char
> alignas (8)' avoids this problem.

I don't follow: the natural alignment for `char` is definitely less than
8, so if "'alignas (8)' is an error when the natural alignment of the
object is less than 8", how can "char alignas (8)" avoid the problem?


It's the other way round: alignas(8) is an error when the natural alignment of the object is *greater* than 8.
Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Paul Eggert
Philipp Stephani wrote:
> It's the other way round: alignas(8) is an error when the natural alignment
> of the object is*greater*  than 8.

Yes, sorry, I misstated it backwards.


Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Stefan Monnier
>> It's the other way round: alignas(8) is an error when the natural alignment
>> of the object is*greater*  than 8.
> Yes, sorry, I misstated it backwards.

Then I don't understand what you meant by "that does not work either".


        Stefan

Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Paul Eggert
Stefan Monnier wrote:
> Then I don't understand what you meant by "that does not work either".

I was confused (:-). I suppose I was worried about the admittedly unlikely
possibility of a platform where alignof (int64_t) exceeds 8. This worry stemmed
from an earlier iteration of this fix, where we ran into such a type on
MS-Windows. Although perhaps I'm worrying too much, the point remains that
because int64_t's alignment is not always a multiple of 8, using int64_t for
alignment would not suffice for platforms where alignas does not work.

Also, there's at least one type (struct vectorlike_header) where alignment is
convenient, and where the type's size is smaller than int64_t on some platforms,
so using int64_t for alignment there would cause objects of this type (and of
its containing types) to grow unnecessarily.

Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Paul Eggert
In reply to this post by Paul Eggert
Paul Eggert wrote:

> I have thought of a solution that fixes the problem by dropping use of GCALIGNED
> and instead using classic C unions along with 'char alignas (8)'.

Patches attached. They work for me on various platforms including gcc -m32 on
Fedora 26 x86-64 (which is close to the platform Glenn mentioned). The 2nd patch
does the real work: most of it consists of changing private accessors to account
for the newly-introduced unions. It shrinks the source code a bit, which is a
good sign. I plan to test a bit more before installing; comments welcome.

0001-Change-vectorlike-from-struct-to-union.txt (16K) Download Attachment
0002-Use-alignas-to-fix-GCALIGN-related-bugs.txt (103K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Paul Eggert
Paul Eggert wrote:
> It shrinks the source code a bit, which is a
> good sign.

There's one more place we can simplify the code because Emacs no longer uses
__attribute__ ((aligned (8))). Further patch attached. As a bonus this
eliminates the slightly-ridiculous Fcons loop in emacs-module.c.

0001-Simplify-by-removing-HAVE_STRUCT_ATTRIBUTE_ALIGNED.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

martin rudalics
In reply to this post by Paul Eggert
 > Patches attached. They work for me on various platforms including gcc
 > -m32 on Fedora 26 x86-64 (which is close to the platform Glenn
 > mentioned). The 2nd patch does the real work: most of it consists of
 > changing private accessors to account for the newly-introduced
 > unions. It shrinks the source code a bit, which is a good sign. I plan
 > to test a bit more before installing; comments welcome.

Builds normally here on a 32-bit Windows XP with GCC 4.8.1.

martin

Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

martin rudalics
In reply to this post by Paul Eggert
 > There's one more place we can simplify the code because Emacs no
 > longer uses __attribute__ ((aligned (8))). Further patch attached. As
 > a bonus this eliminates the slightly-ridiculous Fcons loop in
 > emacs-module.c.

This patch doesn't apply here (on top of the two previous ones).

error: patch failed: admin/CPP-DEFINES:103
error: admin/CPP-DEFINES: patch does not apply
error: patch failed: configure.ac:5113
error: configure.ac: patch does not apply
error: patch failed: src/emacs-module.c:998
error: src/emacs-module.c: patch does not apply

martin

Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Paul Eggert
martin rudalics wrote:
> This patch doesn't apply here (on top of the two previous ones).

Odd, the copy I received via the list is identical to what I sent, and it
applies here. The copy archived here:

https://lists.gnu.org/archive/html/emacs-devel/2017-11/txtI3snHdSqSz.txt

has ">" prepended to the first "From " line and censors my email address, which
I think is normal for lists.gnu.org; it has no other changes. Anyway, I'm
attaching the patch again, this time with a ".txt" extension instead of a
".patch" extension; sometimes that helps.

0001-Simplify-by-removing-HAVE_STRUCT_ATTRIBUTE_ALIGNED.txt (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

martin rudalics
 > Anyway, I'm attaching the patch again, this time with a
 > ".txt" extension instead of a ".patch" extension; sometimes that
 > helps.

Applies and builds normally here now.

Thanks, martin

Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

Paul Eggert
In reply to this post by martin rudalics
martin rudalics wrote:
> Builds normally here on a 32-bit Windows XP with GCC 4.8.1.

Thanks for checking. I tested it a bit on more-obscure platforms, and found and
worked around a related problem on AIX xlc (a compiler bug), and installed the
result on the emacs-26 branch.

Reply | Threaded
Open this post in threaded view
|

Re: emacs-26 9e59de9: Use GCALIGNED properly for GCC

martin rudalics
 > Thanks for checking. I tested it a bit on more-obscure platforms, and
 > found and worked around a related problem on AIX xlc (a compiler bug),
 > and installed the result on the emacs-26 branch.

Thanks.  I suppose you can close a couple of bugs then ...

martin