Opened 9 years ago

Closed 9 years ago

#542 closed enhancement (complete)

catching exceptions in run_unittests

Reported by: jinmei Owned by: stephen
Priority: low Milestone: Sprint-20110531
Component: build system Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

See the bind10-dev thread
https://lists.isc.org/pipermail/bind10-dev/2011-January/001867.html

Since catching an exception has a side effect of stack rewinding,
and some people rather prefer keeping the ability to get a full stack
trace (which may or not be possible for uncaught exceptions: it seems
possible with g++ and SunStudio?; not for clang++), this feature should
be able to be disabled (and probably should be disabled by default
except for clang++).

Subtickets

Attachments (1)

catch-exception.diff (3.6 KB) - added by jinmei 9 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 9 years ago by jinmei

  • Owner changed from jreed to UnAssigned
  • Status changed from new to assigned

comment:2 Changed 9 years ago by stephen

  • Milestone A-Team-Task-Backlog deleted

Milestone A-Team-Task-Backlog deleted

comment:3 Changed 9 years ago by jinmei

  • Milestone set to Year 3 Task Backlog

comment:4 Changed 9 years ago by stephen

  • Milestone changed from Year 3 Task Backlog to Sprint-20110503

comment:5 Changed 9 years ago by stephen

  • Defect Severity set to N/A
  • Estimated Difficulty changed from 0.0 to 3
  • Sub-Project set to DNS

comment:6 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:7 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to UnAssigned
  • Status changed from assigned to reviewing

The list of files for this change seems a bit intimidating, but it is basically the same change repeated for all copies of run_unittests. The main elements of the change are:

  • The code implementing the exception catching is in src/lib/util/unittests/run_all.{h,cc}
  • The util/io/tests directory has been renamed util/io_tests. There was a dependency loop in the build which has been removed by building all the utilities code first, followed by the unit tests in the directories tests and io_tests. (As part of this, the include paths in io_tests/fd*.cc had to be altered.)
  • The run_unittests code have been changed to call the new run_all() function, and the associated Makefile.am altered to link against the unit tests library.

comment:8 in reply to: ↑ 7 Changed 9 years ago by jinmei

Replying to stephen:

I'm willing to review this, but it failed to compile with clang++
on my MacOS X (10.6.7) in tests/tools/badpacket:

/bin/sh ../../../../libtool --tag=CXX   --mode=link clang++ -Wall -Wextra -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare -Werror -fPIC -g -O2  -L/opt/local/lib -Wl,--as-needed ../../../../src/lib/exceptions/libexceptions.la ../../../../src/lib/util/libutil.la -R/opt/local/lib       -o run_unittests run_unittests-run_unittests.o run_unittests-command_options_unittest.o run_unittests-option_info_unittest.o run_unittests-header_flags_unittest.o run_unittests-command_options.o run_unittests-option_info.o -lgtest -D_THREAD_SAFE ../../../../src/lib/util/unittests/libutil_unittests.la 
libtool: link: clang++ -Wall -Wextra -Wwrite-strings -Woverloaded-virtual -Wno-sign-compare -Werror -fPIC -g -O2 -Wl,--as-needed -o .libs/run_unittests run_unittests-run_unittests.o run_unittests-command_options_unittest.o run_unittests-option_info_unittest.o run_unittests-header_flags_unittest.o run_unittests-command_options.o run_unittests-option_info.o -D_THREAD_SAFE -Wl,-bind_at_load  -L/opt/local/lib ../../../../src/lib/exceptions/.libs/libexceptions.dylib ../../../../src/lib/util/.libs/libutil.dylib /opt/local/lib/libgtest.dylib ../../../../src/lib/util/unittests/.libs/libutil_unittests.dylib /Users/jinmei/src/isc/git/bind10-542/src/lib/util/.libs/libutil.dylib /Users/jinmei/src/isc/git/bind10-542/src/lib/util/io/.libs/libutil_io.dylib /Users/jinmei/src/isc/git/bind10-542/src/lib/exceptions/.libs/libexceptions.dylib
ld: unknown option: --as-needed
clang: error: linker command failed with exit code 1 (use -v to see invocation)

I've seen this error with clang++ on a FreeBSD machine before. In
that case a long test case (somehow) triggered it, so as a workaround
we divided it into two smaller tests (it's
CCSessionTest.checkCommand[,2] btw).

Assuming you use MacOS X, I suggest you try to reproduce it and fix it
first.

comment:9 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to stephen

comment:10 Changed 9 years ago by stephen

  • Owner changed from stephen to jinmei

Corrected. I'm not sure why the --as-needed flags was added - I think I was experimenting with ways to get round the problem I detailed in my bind10-dev posting and that change must have slipped through into a commit. At any rate, I've removed it from the Makefile.am and the test builds on Clang under OS X.

comment:11 Changed 9 years ago by jinmei

Overall it looks good. I've noticed some points that could be further
improved, made these changes in the repository and pushed them. I
believe these are trivial, so please directly check the diff. Of
course, if you don't agree with any of them please feel free to
complain.

There are a couple of more points we might want to discuss:

  • maybe a matter of taste, but it seems we can now merge tests and io_tests. wouldn't it be simpler?
  • Also the ordering is still a bit tricky, but with your change I'd not consider it a "hack". I'd rephrase the comment like this:
    SUBDIRS = . io unittests tests io_tests
    # Note the position of unittests: It uses io and tests and io_tests use
    # unittest.
    
    (if we merge tests and io_tests the comment should be adjusted accordingly)

If you agree these make sense, please change it. If not, I'm okay
with that, too.

And, there's another thing that may be controversial: as mentioned in
the ticket, I'd like to enable it by default for clang++. In fact,
there doesn't seem to be a reason for not enabling it for clang++
because it doesn't print exceptions in a fancy format or keep deeper
stack frames. I'm attaching a proposed patch for that.

One more thing: I'd also suggest introducing a wrapper for
EXPECT_NO_THROW as mentioned in:
https://lists.isc.org/pipermail/bind10-dev/2011-January/001891.html
but the amount of diff would be even bigger, so we should probably
defer it to a separate ticket (I'm completely fine with that).

Finally, you may or may not want to update ChangeLog? for this (I'm
okay with either way). If you do, please provide proposed text.

Changed 9 years ago by jinmei

comment:12 Changed 9 years ago by jinmei

  • Owner changed from jinmei to stephen

comment:13 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to jinmei

maybe a matter of taste, but it seems we can now merge tests and io_tests. wouldn't it be simpler?

Yes it would. Done.

I'd like to enable it by default for clang++.

Done. I've chosen a simpler solution than making it a configure option, simply choosing the default as to whether the compiler defines the symbol __clang__. (The reason being that the test will terminate regardless, so whether or not the unexpected exception is caught is only really of interest to someone debugging a test - who could be expected to set B10TEST_CATCH_EXCEPTION appropriately.)

One more thing: I'd also suggest introducing a wrapper for EXPECT_NO_THROW as mentioned in: https://lists.isc.org/pipermail/bind10-dev/2011-January/001891.html but the amount of diff would be even bigger, so we should probably defer it to a separate ticket (I'm completely fine with that).

I've created ticket #958 for it

Suggested ChangeLog entry:

XXX.	[func] 		stephen
        In unit tests, allow the choice of whether unhandled exceptions are
        caught in the unit test program (and details printed) or allowed to
        propagate to the default exception handler.  See the bind10-dev thread
        https://lists.isc.org/pipermail/bind10-dev/2011-January/001867.html
	for more details.
	(Trac #542, git xxxx)

comment:14 in reply to: ↑ 13 Changed 9 years ago by jinmei

Replying to stephen:
I've corrected one trivial typo in a comment line and pushed it to the
repo.

Otherwise it looks okay. Please merge.

comment:15 Changed 9 years ago by jinmei

  • Owner changed from jinmei to stephen

comment:16 Changed 9 years ago by stephen

  • Resolution set to complete
  • Status changed from reviewing to closed

Committed to master (6ea285d4f7cf6a9fd18bd9a3811944fc02d0e34c)

Note: See TracTickets for help on using tickets.