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

Exception/Terminate warnings when building on Debian 9 / gcc 6.3 #134

Closed
mundoEarth opened this issue Jul 28, 2017 · 14 comments
Closed

Exception/Terminate warnings when building on Debian 9 / gcc 6.3 #134

mundoEarth opened this issue Jul 28, 2017 · 14 comments

Comments

@mundoEarth
Copy link

Hi..

I need some help I got an error upon executing command "make" in debian 9..
below is the screenshot of the error :
gearman libtest error

@esabol
Copy link
Member

esabol commented Jul 28, 2017

What version of gcc are you compiling with, @mundoEarth ?

@esabol
Copy link
Member

esabol commented Jul 28, 2017

And what version of gearmand are you compiling?

@SpamapS SpamapS changed the title Error ./libtest/thread.hpp. Error ./libtest/thread.hpp. on Debian 9 Jul 28, 2017
@mundoEarth
Copy link
Author

@esabol, what do you mean "gcc"? Where can I found that?
With gearmand version I amm using the version 1.1.17

@SpamapS
Copy link
Member

SpamapS commented Jul 29, 2017

gcc is the compiler used when you run 'make' on gearmand on Debian. I went ahead and spun up a Debian 9 docker container and in fact I see those warnings too. FYI, you can just copy/paste the text next time, as a screen shot isn't actually helpful for search engines to help people find this bug (nor is it easy for us to read).

Those are just warnings though. They should definitely be addressed, but gearmand still builds. I used this Dockerfile to build from the 1.1.17 tarball:

FROM debian
RUN apt-get update
RUN DEBCONF_FRONTEND=noninteractive apt-get -y install build-essential uuid-dev libboost-dev libboost-program-options-dev gperf file libevent-dev
COPY . /src
WORKDIR /src
RUN ./configure
RUN make
RUN make install

The build completes fine and gearmand works fine. So I'm going to go ahead and change the bug title to "Exception warnings on Debian 9 (gcc 6.3.0)"

BTW, the simplest way to find out things like compiler version and build settings in general is to upload the 'config.log' file from your build tree.

@SpamapS SpamapS changed the title Error ./libtest/thread.hpp. on Debian 9 Exception/Terminate warnings when building on Debian 9 / gcc 6.3 Jul 29, 2017
@SpamapS
Copy link
Member

SpamapS commented Jul 29, 2017

btw a text example:

  CXX      tests/libgearman-1.0/t_client-protocol.o
In file included from ./libtest/test.hpp:102:0,
                 from tests/libgearman-1.0/gearman_execute_partition.cc:39:
./libtest/thread.hpp: In destructor 'libtest::thread::Mutex::~Mutex()':
./libtest/thread.hpp:59:93: warning: throw will always call terminate() [-Wterminate]
       throw libtest::fatal(LIBYATL_DEFAULT_PARAM, "pthread_cond_destroy: %s", strerror(_err));
                                                                                             ^
./libtest/thread.hpp:59:93: note: in C++11 destructors default to noexcept
./libtest/thread.hpp: In destructor 'libtest::thread::ScopedLock::~ScopedLock()':
./libtest/thread.hpp:92:92: warning: throw will always call terminate() [-Wterminate]
       throw libtest::fatal(LIBYATL_DEFAULT_PARAM, "pthread_mutex_unlock: %s", strerror(err));
                                                                                            ^
./libtest/thread.hpp:92:92: note: in C++11 destructors default to noexcept
./libtest/thread.hpp: In destructor 'libtest::thread::Condition::~Condition()':
./libtest/thread.hpp:132:92: warning: throw will always call terminate() [-Wterminate]
       throw libtest::fatal(LIBYATL_DEFAULT_PARAM, "pthread_cond_destroy: %s", strerror(err));
                                                                                            ^
./libtest/thread.hpp:132:92: note: in C++11 destructors default to noexcept

@esabol
Copy link
Member

esabol commented Jul 29, 2017

@SpamapS, your output shows them as warnings, but @mundoEarth's output shows them as errors. I think there's still some difference here that we haven't quite gotten to the bottom of. @mundoEarth, can you type gcc --version and tell us what it says and/or upload the config.log file from your build directory for us?

@p-alik
Copy link
Collaborator

p-alik commented Aug 2, 2017

It isn't a new issue. It's a part of #8 and is fixed in #114
See:

V509
The 'throw' operator inside the destructor should be placed within the try..catch block. Raising exception inside the destructor is illegal.

in report

@p-alik
Copy link
Collaborator

p-alik commented Aug 3, 2017

 index c67bc33..13ec630 100644
--- a/libtest/comparison.hpp
+++ b/libtest/comparison.hpp
@@ -93,17 +93,6 @@ bool _compare(const char *file, int line, const char *func, const T1_comparable&
 template <class T1_comparable, class T2_comparable>
 bool _compare_strcmp(const char *file, int line, const char *func, const T1_comparable& __expected, const T2_comparable& __actual)
 {
-  if (__expected == NULL)
-  {
-    FATAL("Expected value was NULL, programmer error");
-  }
-
-  if (__actual == NULL)
-  {
-    libtest::stream::make_cerr(file, line, func) << "Expected " << __expected << " but got NULL";
-    return false;
-  }
-
   if (strncmp(__expected, __actual, strlen(__expected)))
   {
     libtest::stream::make_cerr(file, line, func) << "Expected " << __expected << " passed \"" << __actual << "\"";

Build exception is the reason for the patch:

In file included from ./libtest/test.hpp:80:0,
                 from tests/libgearman-1.0/internals.cc:42:
./libtest/comparison.hpp: In instantiation of ‘bool libtest::_compare_strcmp(const char*, int, const char*, const T1_comparable&, const T2_comparable&) [with T1_comparable = char*; T2_comparable = char [6]]’:
tests/libgearman-1.0/internals.cc:372:3:   required from here
./libtest/comparison.hpp:101:16: error: the compiler can assume that the address of ‘__actual’ will never be NULL [-Werror=address]
   if (__actual == NULL)
                ^
./libtest/comparison.hpp: In function ‘bool libtest::_compare_strcmp(const char*, int, const char*, const T1_comparable&, const T2_comparable&) [with T1_comparable = char*; T2_comparable = char [6]]’:
./libtest/comparison.hpp:101:3: error: nonnull argument ‘__actual’ compared to NULL [-Werror=nonnull-compare]
   if (__actual == NULL)
   ^~
cc1plus: all warnings being treated as errors
  CXX      tests/libgearman-1.0/regression.o
Makefile:5553: recipe for target 'tests/libgearman-1.0/internals.o' failed
make[2]: *** [tests/libgearman-1.0/internals.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory '/vagrant/gearmand'
Makefile:8369: recipe for target 'all-recursive' failed
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory '/vagrant/gearmand'
Makefile:3088: recipe for
  • make test failed: unittest ; skipped: drizzle, memcached, postgres, mysql, tokyocabinet and redis
libtest/unittest.cc:663: in application_doesnotexist_BINARY() pid(24254) Assertion 'Application::SUCCESS' != 'true_app.run(args)'
unittest.application.doesnotexist --fubar					[ failed ]
  • Configuration summary
   * Installation prefix:       /usr/local
   * System type:               pc-linux-gnu
   * Host CPU:                  x86_64
   * C Compiler:                cc cc (Debian 6.3.0-18) 6.3.0 20170516
   * C Flags:                   -g -O2  -fstack-check -Wpragmas -Wunknown-pragmas -Wall -Wextra -Wunsuffixed-float-constants -Wjump-misses-init -Wno-attributes -Waddress -Wvarargs -Warray-bounds -Wbad-function-cast -Wchar-subscripts -Wcomment -Wfloat-equal -Wformat-security -Wformat=2 -Wformat-y2k -Wlogical-op -Wmaybe-uninitialized -Wmissing-field-initializers -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wnormalized=id -Woverride-init -Wpointer-arith -Wpointer-sign -Wredundant-decls -Wshadow -Wsign-compare -Wstrict-overflow=1 -Wswitch-enum -Wtrampolines -Wundef -funsafe-loop-optimizations -Wclobbered -Wunused -Wunused-result -Wunused-variable -Wunused-parameter -Wunused-local-typedefs -Wwrite-strings -fwrapv -pipe -fPIE -pie -Wsizeof-pointer-memaccess -Wpacked -Werror
   * C++ Compiler:              c++ c++ (Debian 6.3.0-18) 6.3.0 20170516
   * C++ Flags:                 -g -O2 -std=c++0x -fstack-check -Wpragmas -Wunknown-pragmas -Wall -Wextra -Wno-attributes -Wvarargs -Waddress -Warray-bounds -Wchar-subscripts -Wcomment -Wctor-dtor-privacy -Wfloat-equal -Wformat=2 -Wformat-y2k -Wmaybe-uninitialized -Wmissing-field-initializers -Wlogical-op -Wnon-virtual-dtor -Wnormalized=id -Woverloaded-virtual -Wpointer-arith -Wredundant-decls -Wshadow -Wsign-compare -Wstrict-overflow=1 -Wswitch-enum -Wtrampolines -Wundef -funsafe-loop-optimizations -Wc++11-compat -Wclobbered -Wunused -Wunused-result -Wunused-variable -Wunused-parameter -Wunused-local-typedefs -Wwrite-strings -Wformat-security -fwrapv -pipe -fPIE -pie -Wsizeof-pointer-memaccess -Wpacked -Werror
   * CPP Flags:                  -fvisibility=hidden
   * LIBS:                      
   * LDFLAGS Flags:              -Werror -rdynamic
   * Assertions enabled:        yes
   * Debug enabled:             no
   * Warnings as failure:       yes
   * Building with hiredis      no
   * Building with libsqlite3   no
   * Building with libdrizzle   yes
   * Building with libmemcached yes
   * Building with libpq        no
   * Building with tokyocabinet no
   * Building with libmysql     no
   * SSL enabled:               no
   * wolfssl found:             no
   * openssl found:             no
   * make -j:                   2
   * VCS checkout:              yes
   * sphinx-build:              /usr/bin/sphinx-build
  • make, make test with same code succeeded on Ubuintu 16.04

@BasBastian
Copy link

BasBastian commented Nov 2, 2017

I've reproduced that behavior. What is failing part is make test, which is (by default) part of make process.
It seems, that GCC compiler is being too strict about compiling tests.
I did not check which framework is currently used for unit tests, but throwing exception from destructor of class and all classes inheriting it causes compilation failure. It is due to -Werror flag, which forces compiler to exit on any warning (but warnings are not severe errors and I am not sure if that assumption is correct).

Partial and quick solution is to check all errors and comment them out.

The right solution would be to check if we need to throw error from object's destructor, being able to avoid that kind of errors.

On my way to solve those. Not yet successfully compiled on Debian 9.0.

@SpamapS, @p-alik, @BrianAker , see: https://akrzemi1.wordpress.com/2011/09/21/destructors-that-throw/
I think, that gcc compiler in version 6.3 is more C++11 compliant than we are in our source code. Autotools-generated Makefile might be correct about -Werror flag, but we are throwing exception from destructor ( thread tests ) which is implicitly defined as noexcept. Maybe that's an issue.

@p-alik
Copy link
Collaborator

p-alik commented Nov 3, 2017

These days raising exception inside the destructor is illegal.

a1e4dd9

@BasBastian
Copy link

BasBastian commented Nov 3, 2017

I found the solution. I successfully fired make.
Until tommorow evening (04.11 A.D.2017 - Gregorian calendar, 11 P.M. GMT+01) Pull Request will be ready to merge.

But that does not close a single issue which will bug us as Gearman is C/C++ project. It is compiler compabilities. Unless we state it clearly and explicitly, such issues may happen, or even worse - some folks may want to compile some version of gearman with older compiler version, having the newest source code version.

Being compliant to specific version is important (I don't think we should stay compliant to C99 standard), but user/admin/developer knowing it earlier may easily find the version that will suit his OS and compiler.

There are also some stuff in code that seems like legacy or trying to make C++ project stay compliant with C compiler. I think it could be compared to half naked man standing in the middle of Manhattan. Noone knows if he is sane. It causes lot of commotion. People are not sure how to behave.
If he was all naked (only C) people could think he is crazy, though he might be just efficient :P
If he was dressed (C++) people would not pay much attention and keep continuing their day-to-day activities.

@BasBastian
Copy link

#145
Solution - @ALL please check if that solution appeals to you/

BasBastian added a commit to BasBastian/gearmand that referenced this issue Nov 15, 2017
1. Fixes some NULL comparisons issue.
2. Fixes bad practice - throwing Exception from destructor.
3. It does not solve all issues with C++11 code compliance.
4. Tested on Virtual Machine with Debian 9.0 installed.
@illuusio
Copy link

illuusio commented Nov 5, 2018

Now there should be some of these are addressed in master. Things in 'libtest/comparison.hpp' should be handled.

@esabol
Copy link
Member

esabol commented Nov 28, 2019

I think this issue can be closed, right?

@p-alik p-alik closed this as completed Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants