Created attachment 116191 [details] [review] patch to configure.ac to configure assert() Poppler has a number of calls to assert() that are enabled by default. Some of them could be expensive, for example, Splash::gouraudTriangleShadedFill() in splash.cc has two assert() tests inside an inner loop, and one of the tests does a real number calculation for each pixel: assert(fabs(colorinterp - (scanColorMap[0] * X + scanColorMap[1])) < 1e-10); assert(bitmapOff == Y * rowSize + colorComps * X && scanLineOff == Y * rowSize); In addition, it would be bad for an application that used poppler to abort (possibly losing work) just because poppler had an internal inconsistency that might have corrupted an image but would not have caused a bad memory access. The attached patch to configure.ac adds a --disable-assert option to configure that adds -DNDEBUG to CPPFLAGS. Some headers include <assert.h> before (or without) including config.h, so I think that it is safer to pass -DNDEBUG on the command line than to try to trace every include <assert.h> in each header to verify that every path to include the header has already included config.h. William
This has two problems: a) Misses the cmake side of things b) It's misleading in the name, you say "disables asserts" but what it does is add a define to the define list, which can do lots of other things.
Thanks for reviewing it. Does that mean that you would accept it if I add patches for cmake and change the text of the description? The man page for assert(3) on Fedora 20 mentions NDEBUG and lists the behavior as conforming to POSIX.1-2001, C89, and C99, so I think that setting NDEBUG is the correct way to disable assertions and that applications that use NDEBUG for their own purposes are risking portability problems.
I'm not saying it is or not the correct way to disable assert, i'm just saying it may also disable other stuff. Also NDEBUG is already defined in release mode, isn't it? Do you really need a debug but with no debug mode? Or maybe NDEBUG in release only happens in cmake?
As far as I know, autoconf does not have an equivalent to cmake Release and Debug modes. I think that autoconf projects are often set up so that the default "./configure ; make install" makes a release, and authors of the project might provide --enable-... or -with-... options to ./configure that enable debugging builds for developers. I have been building poppler with autoconf/configure, and I just realized that I have been doing the equivalent of cmake Debug builds. cmake/modules/PopplerMacros.cmake sets -DNDEBUG for Release builds. I would like to make an easy way to do the same with builds using ./configure.
Created attachment 116815 [details] [review] revised patch This patch creates a ./configure option called --enable-release-mode that adds-O2 -DNDEBUG to CFLAGS and CXXFLAGS just like the options in cmake/modules/PopplerMacros.cmake that are enabled by cmake -DCMAKE_BUILD_TYPE=Release. $ grep -i release cmake/modules/PopplerMacros.cmake set(CMAKE_CXX_FLAGS_RELEASE "-O2 -DNDEBUG") set(CMAKE_C_FLAGS_RELEASE "-O2 -DNDEBUG") set(CMAKE_CXX_FLAGS_RELEASE "-O2 -DNDEBUG") set(CMAKE_C_FLAGS_RELEASE "-O2 -DNDEBUG") Is that better? It creates a configure option with a name similar to the cmake option that adds the same flags as the cmake option.
Is it worth making a --enable-build-type=XXX option that corresponds to the cmake -DCMAKE_BUILD_TYPE=XXX options defined in PopplerMacros.cmake and sets the same arguments? Is there a safe way to test the compiler other than checking for gcc if $GXX = yes? http://stackoverflow.com/questions/3116645/default-compiler-flags-with-autotools/16672757#16672757 http://stackoverflow.com/questions/24819461/how-can-i-check-a-particular-gcc-feature-in-configure-ac $ grep 'set(CMAKE_.*_FLAGS_' PopplerMacros.cmake set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "-O2 -g") set(CMAKE_CXX_FLAGS_RELEASE "-O2 -DNDEBUG") set(CMAKE_CXX_FLAGS_DEBUG "-g -O2 -fno-reorder-blocks -fno-schedule-insns -fno-inline") set(CMAKE_CXX_FLAGS_DEBUGFULL "-g3 -fno-inline") set(CMAKE_CXX_FLAGS_PROFILE "-g3 -fno-inline -ftest-coverage -fprofile-arcs") set(CMAKE_C_FLAGS_RELWITHDEBINFO "-O2 -g") set(CMAKE_C_FLAGS_RELEASE "-O2 -DNDEBUG") set(CMAKE_C_FLAGS_DEBUG "-g -O2 -fno-reorder-blocks -fno-schedule-insns -fno-inline") set(CMAKE_C_FLAGS_DEBUGFULL "-g3 -fno-inline") set(CMAKE_C_FLAGS_PROFILE "-g3 -fno-inline -ftest-coverage -fprofile-arcs") set(CMAKE_CXX_FLAGS_RELWITHDEBINFO "-O2 -g") set(CMAKE_CXX_FLAGS_RELEASE "-O2 -DNDEBUG") set(CMAKE_CXX_FLAGS_DEBUG "-O2 -g -0b0 -noalign") set(CMAKE_CXX_FLAGS_DEBUGFULL "-g -Ob0 -noalign") set(CMAKE_C_FLAGS_RELWITHDEBINFO "-O2 -g") set(CMAKE_C_FLAGS_RELEASE "-O2 -DNDEBUG") set(CMAKE_C_FLAGS_DEBUG "-O2 -g -Ob0 -noalign") set(CMAKE_C_FLAGS_DEBUGFULL "-g -Ob0 -noalign")
Having consistency between both build systems (since we can't agree to kill either one of them) is a good thing, i'm afraid i don't know much of autotools to answer your detecting gcc question though :/
Created attachment 116868 [details] [review] revised patch for --enable-build-type This patch adds --enable-build-type with values relwithdebinfo, release, debug, debugfull, and profile similar to the cmake -DCMAKE_BUILD_TYPE options in cmake/modules/PopplerMacros.cmake I tested the options on Fedora 20 x86_64.
should you set also the cflags for different than release?
If ./configure sets CFLAGS, it becomes tricky to override options on the command line. I set CFLAGS only for "release" because it was necessary for adding the -DNDEBUG. The default CFLAGS is -O2 -g, and the other cases follow it with CXXFLAGS which sets -g3, so touching the -g in CFLAGS has no effect. Dealing with the -O2 would be messy. An option that set CFLAGS="$CFLAGS -O0" would prevent you from being able to override CFLAGS with an optimization option. Setting CFLAGS="-O0 $CFLAGS" would still use the -O2 that ./configure puts into CFLAGS unless you overrode CFLAGS on the command line. I think that there is not a good solution, and it is safer not to touch CFLAGS unless it is necessary, but if you want me to set it, for example, CFLAGS="$CFLAGS -O0 -g3", let me know what you want, I can make a new patch. Do you have any opinion about using -Ofast instead of -O2 for releases? -Ofast generates code that is usually faster than -O2 but about 15% larger. http://www.phoronix.com/scan.php?page=article&item=gcc_47_optimizations&num=3 William
> If ./configure sets CFLAGS, it becomes tricky to override options on the command line. Why is CFLAGS special over CXXFLAGS? Does one really need to set CXXFLAGS and CPPFLAGS? Can you please put everything in multiple lines instead of having to = on the same line, makes it harder to read and saving 1 line in configure.ac is not something we need to worry about, it's too long already :D
Created attachment 117186 [details] [review] Updated patch to add configure --enable-build-type I think that you are correct about the flags. I made a new patch that does not touch CPPFLAGS, and I reformatted the text to put each assignment on its own line.
pushed
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.