Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8-byte access alignment detection is wrong with autoconf #7938

Closed
vicuna opened this issue Mar 7, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link

commented Mar 7, 2019

Original bug ID: 7938
Reporter: @Reperator
Status: acknowledged (set by @xavierleroy on 2019-03-09T09:32:23Z)
Resolution: open
Priority: normal
Severity: minor
Version: 4.08.0+dev/beta1/beta2
Category: configure and build/install
Monitored by: @Reperator @nojb @shindere @stedolan @hcarty

Bug description

autoconf's AC_CHECK_ALIGNOF([long]) always returns "x8", regardless of architecture. This is not actually the legal alignment of memory accesses.

ARCH_ALIGN_INT64 used to be defined from the result of config/auto-aux/int64align.c, which actually checks for legal memory access requirements. This was changed in 4.08.

Reading int64 values from the heap is going to be slower.

https://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Generic-Compiler-Characteristics.html

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 8, 2019

Comment author: @nojb

The configure script does not seem to use AC_CHECK_ALIGNOF. Instead it does its own test to compute alignment, see

ocaml/configure

Lines 13325 to 13349 in d47ba6e

{ $as_echo "$as_me:${as_lineno-$LINENO}: checking alignment of long" >&5
$as_echo_n "checking alignment of long... " >&6; }
if ${ac_cv_alignof_long+:} false; then :
$as_echo_n "(cached) " >&6
else
if ac_fn_c_compute_int "$LINENO" "(long int) offsetof (ac__type_alignof_, y)" "ac_cv_alignof_long" "$ac_includes_default
#ifndef offsetof
# define offsetof(type, member) ((char *) &((type *) 0)->member - (char *) 0)
#endif
typedef struct { char x; long y; } ac__type_alignof_;"; then :
else
if test "$ac_cv_type_long" = yes; then
{ { $as_echo "$as_me:${as_lineno-$LINENO}: error: in \`$ac_pwd':" >&5
$as_echo "$as_me: error: in \`$ac_pwd':" >&2;}
as_fn_error 77 "cannot compute alignment of long
See \`config.log' for more details" "$LINENO" 5; }
else
ac_cv_alignof_long=0
fi
fi
fi
{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_alignof_long" >&5
$as_echo "$ac_cv_alignof_long" >&6; }

Is there a problem with this test?

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 8, 2019

Comment author: @stedolan

I think that link is to the generated configure, the source is

AC_CHECK_ALIGNOF([long])

I think the issue is that on 32-bit platforms, ocaml will now think that 64-bit reads and writes require 8-byte alignment, which will trigger the slow path whereby 64-bit accesses are emulated as a pair of 32-bit accesses.

What the old configure script checked was "will 64-bit accesses that are only 4-byte aligned trap?", while the new configure script asks "what is the optimal alignment of a 64-bit access?". There are several platforms (e.g. x86) where the optimal alignment is 8 bytes, yet 4-byte aligned accesses work fine.

The fix is to replace AC_CHECK_ALIGNOF with a macro that checks whether 4-byte alignment traps. I'm not enough of an autoconf expert to know if there's a standard macro for this, though.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 8, 2019

Comment author: @nojb

Indeed, sorry for the confusion.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 8, 2019

Comment author: @Reperator

Indeed. I'm working on a PR that additionally needs checks for 32-bit and 16-bit accesses in order to use atomic reads/writes for bigarrays on supported platforms. If anybody with autoconf knowledge were to implement these, that'd be great.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 9, 2019

Comment author: @xavierleroy

The old autodetection code was fragile and unreliable, so I advise against resurrecting it.

The plan we had with @shindere was to use the C compiler alignment as the default, and hard-code a small list of exceptions for which we know that the hardware supports reduced alignment, in particular x86.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

PR #8532 proposes a fix for this.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Fixed by PR#8532 in both trunk and the 4.08 branch.

@shindere shindere closed this Mar 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.