Skip to content

Commit

Permalink
Try using -Ofast / -O3 by default
Browse files Browse the repository at this point in the history
  • Loading branch information
@jedisct1
jedisct1 committed Nov 13, 2022
1 parent b677924 commit ad4584d
Showing 1 changed file with 14 additions and 1 deletion.
15 changes: 14 additions & 1 deletion configure.ac
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,23 @@ AS_IF([pwd | fgrep ' ' > /dev/null 2>&1],
[AC_MSG_ERROR([The build directory contains whitespaces - This can cause tests/installation to fail due to limitations of some libtool versions])]
)

sodium_CFLAGS=${CFLAGS+set}
: ${CFLAGS=""}

AC_PROG_CC
AM_PROG_AS
AC_USE_SYSTEM_EXTENSIONS

dnl Default optimization flags

if test "$sodium_CFLAGS" != "set" ; then
AX_CHECK_COMPILE_FLAG([-Ofast], [CFLAGS="$CFLAGS -Ofast"],
[AX_CHECK_COMPILE_FLAG([-O3], [CFLAGS="$CFLAGS -O3"],
[AX_CHECK_COMPILE_FLAG([-O2], [CFLAGS="$CFLAGS -O2"],
[AX_CHECK_COMPILE_FLAG([-O1], [CFLAGS="$CFLAGS -O1"],
[AX_CHECK_COMPILE_FLAG([-O], [CFLAGS="$CFLAGS -O"])])])])])
fi

dnl Switches

AC_ARG_ENABLE(ssp,
Expand Down Expand Up @@ -197,11 +210,11 @@ AC_ARG_ENABLE(opt,
[AS_HELP_STRING(--enable-opt,Optimize for the native CPU - The resulting library will be faster but not portable)],
[
AS_IF([test "x$enableval" = "xyes"], [
AX_CHECK_COMPILE_FLAG([-Ofast], [CFLAGS="$CFLAGS -Ofast"])
AX_CHECK_COMPILE_FLAG([-ftree-vectorize], [CFLAGS="$CFLAGS -ftree-vectorize"])
AX_CHECK_COMPILE_FLAG([-ftree-slp-vectorize], [CFLAGS="$CFLAGS -ftree-slp-vectorize"])
AX_CHECK_COMPILE_FLAG([-fomit-frame-pointer], [CFLAGS="$CFLAGS -fomit-frame-pointer"])
AX_CHECK_COMPILE_FLAG([-march=native], [CFLAGS="$CFLAGS -march=native"])
AX_CHECK_COMPILE_FLAG([-mcpu=native], [CFLAGS="$CFLAGS -mcpu=native"])
])
])

Expand Down

5 comments on commit ad4584d

@vcunat
Copy link

@vcunat vcunat commented on ad4584d Oct 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jedisct1: can you please elaborate on why even consider -Ofast in a crypto-library? The option is clearly designed to break normal correctness guarantees.

@vcunat
Copy link

@vcunat vcunat commented on ad4584d Oct 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should mention that apparently with gcc the option also triggers -ffast-math and its semantics can apparently leak outside of libsodium into executables that dynamically link against it – even if those executables did not get compiled with any dangerous options (like -ffast-math is).

@vcunat
Copy link

@vcunat vcunat commented on ad4584d Oct 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuing to copy important bits from NixOS chat.

The linking propagation is probably fixed with >= gcc 13.1.

@vcunat
Copy link

@vcunat vcunat commented on ad4584d Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, I assume this is the reaction: 7b5c887

BTW, I'm a bit annoyed by that commit message. Let's look into clang docs:

-Ofast Enables all the optimizations from -O3 along with other aggressive optimizations that may violate strict compliance with language standards.

@hmenke
Copy link

@hmenke hmenke commented on ad4584d Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if linking propagation may be fixed, -ffast-math still alters floating point flags which are set globally in the library constructor and therefore contaminate the entire process environment:
https://github.com/gcc-mirror/gcc/blob/releases/gcc-13.2.0/libgcc/config/i386/crtfastmath.c#L82-L98

Please sign in to comment.