Make --enable-debug to pick better options

practicalswift commented at 8:47 am on April 17, 2018: contributor

Cherry-picked (and rebased) 94189645e67f364c4445d62e2b00c282d885cbbf from the “up for grabs” PR: “[build] Make –enable-debug pick better options” (#12695).

See previous review in #12695.

Various changes:

 * Don't check $GCC and $GXX
 * Prefer -Og instead of -O0
 * If -g3 isn't available, use -g

This also incidentally fixes compiler warnings with GCC and glibc when using
--enable-debug, as the old default values mixed poorly with the hardening flags.

9e49db2426

fanquake added the label Build system on Apr 17, 2018

practicalswift commented at 9:09 am on April 17, 2018: contributor

One small thing that needs to be fixed before merge is that CPPFLAGS, CXXFLAGS and LDFLAGS gets stuffed with redundant spaces when printing which looks a bit messy:

0$ ./configure
1…
2  CC            = gcc
3  CFLAGS        = -g -O2
4  CPPFLAGS      =   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS
5  CXX           = g++ -std=c++11
6  CXXFLAGS      =   -Wstack-protector -fstack-protector-all   -g -O2 -Wall -Wextra -Wformat -Wvla -Wformat-security -Wno-unused-parameter
7  LDFLAGS       = -pthread  -Wl,-z,relro -Wl,-z,now -pie
8  ARFLAGS       = cr

I’ll find a way to fix that.

practicalswift force-pushed on Apr 17, 2018

practicalswift commented at 9:44 am on April 17, 2018: contributor

Added a commit fixing the annoying extra whitespace:

0$ ./configure
1…
2  CC            = gcc
3  CFLAGS        = -g -O2
4  CPPFLAGS      = -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS
5  CXX           = g++ -std=c++11
6  CXXFLAGS      = -Wstack-protector -fstack-protector-all -g -O2 -Wall -Wextra -Wformat -Wvla -Wformat-security -Wno-unused-parameter
7  LDFLAGS       = -pthread -Wl,-z,relro -Wl,-z,now -pie
8  ARFLAGS       = cr

Please review :-)

practicalswift force-pushed on Apr 17, 2018

laanwj commented at 9:48 am on April 18, 2018: member

re-utACK Ping @theuni - he proposed some changes in #12695, so would like to know if he’s ok with the current state.

ryanofsky commented at 8:07 pm on April 18, 2018: member

utACK 8a0ad1a2333b329d6eb8d34a25277181ee675f23. The first commit is just rebased and has no changes since my previous review in the other PR. The second commit is new and looks ok, though I think the echo_joined function is overkill. I think you should just drop quotes around the variables and let the shell collapse spaces:

0echo "  CXXFLAGS      =" $DEBUG_CXXFLAGS $HARDENED_CXXFLAGS $ERROR_CXXFLAGS $CXXFLAGS

theuni commented at 11:17 pm on April 18, 2018: member

Agree with @ryanofsky about echo_joined. I’d prefer to just drop the second commit. utACK otherwise.

eklitzke commented at 0:17 am on April 19, 2018: contributor

Apologies, I was travelling without access to my laptop while this was put up for grabs. This seems fine but I think we came to the conclusion that we should not try to join the output, which was where my first commit was at. We can fix the whitespace formatting in another pass using AX_APPEND_COMPILE_FLAGS.

practicalswift commented at 5:53 am on April 19, 2018: contributor

@eklitzke Good idea about fixing the whitespace with AX_APPEND_COMPILE_FLAGS. Could you help with a diff for that?

theuni commented at 8:10 pm on May 8, 2018: member

@practicalswift That’s worth handling in a separate PR. It will be a pretty huge diff.

practicalswift commented at 6:57 am on May 9, 2018: contributor

@theuni Oh, I see. @eklitzke @theuni Assuming AX_APPEND_COMPILE_FLAGS is fixed in a follow-up PR do you have time re-review this PR as-is and give your ACK/utACK/NACK? :-)

fanquake commented at 7:37 am on May 9, 2018: member

@practicalswift From reading the above discussion, and on the original PR, you still need to drop the second commit. Then this can be reviewed, and a follow-up PR can introduce the AX_APPEND_FLAG / AX_APPEND_COMPILE_FLAGS changes.

practicalswift force-pushed on May 9, 2018

practicalswift commented at 8:42 am on May 9, 2018: contributor

@fanquake Updated! Please re-review! :-)

laanwj commented at 1:30 pm on May 14, 2018: member

re-utACK 9e49db2426cd4508d9dd95c7461e107bab7fd7de

laanwj merged this on May 14, 2018

laanwj closed this on May 14, 2018

laanwj referenced this in commit cb088b1461 on May 14, 2018

in configure.ac:251 in 9e49db2426

250+  # Prefer -Og, fall back to -O0 if that is unavailable.
251+  AX_CHECK_COMPILE_FLAG(
252+    [-Og],
253+    [[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -Og"]],
254+    [AX_CHECK_COMPILE_FLAG([-O0],[[DEBUG_CXXFLAGS="$DEBUG_CXXFLAGS -O0"]],,[[$CXXFLAG_WERROR]])],
255+    [[$CXXFLAG_WERROR]])

luke-jr commented at 7:58 pm on June 12, 2018:

What if -Og and -O0 are unavailable? (Both of these may compromise spectre mitigations, so I have my x86 compiler not support them at all)

practicalswift commented at 8:01 pm on June 12, 2018:

Good question! @eklitzke, what do you say? :-)

theuni commented at 8:08 pm on June 12, 2018:

I’d prefer making this -O1. Completely un-optimized code barely resembles anything that ever gets run, and macro/inline expansions make it hard to deal with anyway.

zkbot referenced this in commit 8e8a9350c3 on Jan 30, 2020

practicalswift deleted the branch on Apr 10, 2021

UdjinM6 referenced this in commit 744ac1a5f9 on May 21, 2021

UdjinM6 referenced this in commit c646686fc0 on May 25, 2021

gades referenced this in commit 74da6a3f74 on Jun 19, 2022

DrahtBot locked this on Aug 16, 2022

Make –enable-debug to pick better options #13005