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

common/BackTrace: add operator<< #9028

Merged
merged 1 commit into from Jan 17, 2017

Conversation

Projects
None yet
4 participants
@tchaikov
Copy link
Contributor

tchaikov commented May 10, 2016

could be handy when using along with dout.

Signed-off-by: Kefu Chai tchaikov@gmail.com

@tchaikov tchaikov force-pushed the tchaikov:wip-bt-dump branch from 11b0626 to b72d3b7 May 10, 2016

@ghost

This comment has been minimized.

Copy link

ghost commented Nov 22, 2016

jenkins test this please (no logs)

@tchaikov tchaikov force-pushed the tchaikov:wip-bt-dump branch from b72d3b7 to 75c0aa8 Jan 11, 2017

@tchaikov tchaikov added cleanup and removed feature labels Jan 11, 2017

@tchaikov tchaikov force-pushed the tchaikov:wip-bt-dump branch 3 times, most recently from 716a6fc to b4bffe3 Jan 11, 2017

@tchaikov tchaikov requested a review from badone Jan 11, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor Author

tchaikov commented Jan 11, 2017

@badone could you help take a look, thanks!

@badone
Copy link
Contributor

badone left a comment

@tchaikov would this benefit from a test?

@tchaikov tchaikov force-pushed the tchaikov:wip-bt-dump branch from b4bffe3 to 4f91e20 Jan 12, 2017

@badone

badone approved these changes Jan 12, 2017

Copy link
Contributor

badone left a comment

Looks good.

@tchaikov tchaikov added the needs-qa label Jan 12, 2017

@tchaikov tchaikov self-assigned this Jan 12, 2017

@yuriw

This comment has been minimized.

Copy link
Contributor

yuriw commented Jan 13, 2017

test this please

1 similar comment
@yuriw

This comment has been minimized.

Copy link
Contributor

yuriw commented Jan 13, 2017

test this please

@tchaikov tchaikov changed the title common/BackTrace: add operator<< [DNM] common/BackTrace: add operator<< Jan 14, 2017

@tchaikov tchaikov removed the needs-qa label Jan 14, 2017

std::regex e("^ 1: "
"\\(foo.*\\)\\s"
"\\[0x[[:xdigit:]]+\\]$");
EXPECT_TRUE(std::regex_match(lines[lineno], e));

This comment has been minimized.

Copy link
@badone

badone Jan 14, 2017

Contributor

At runtime there appears to be an issue with this code as it is throwing an exception, "regex_error". Need to wrap it in a try/catch and output std::regex_error::what to see what it's complaining about.

This comment has been minimized.

Copy link
@badone

badone Jan 15, 2017

Contributor

Tried to catch this with #12933 but all test passed :(

This comment has been minimized.

Copy link
@badone

badone Jan 15, 2017

Contributor

Oops, forgot to return an error condition. Trying again.

@badone

This comment has been minimized.

Copy link
Contributor

badone commented Jan 15, 2017

Test this please

@tchaikov tchaikov force-pushed the tchaikov:wip-bt-dump branch from 4f91e20 to 0ca779d Jan 15, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor Author

tchaikov commented Jan 15, 2017

@badone turns out the regex implemented in libstdc++ prior to 4.9 was buggy, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53631. so let use boost::regex at this moment. thanks for looking at this failure!

@tchaikov tchaikov force-pushed the tchaikov:wip-bt-dump branch from 0ca779d to 30e84ad Jan 15, 2017

@tchaikov tchaikov changed the title [DNM] common/BackTrace: add operator<< common/BackTrace: add operator<< Jan 15, 2017

@tchaikov tchaikov added the needs-qa label Jan 15, 2017

@badone

This comment has been minimized.

Copy link
Contributor

badone commented Jan 15, 2017

The test is still failing with boost::regex

@tchaikov tchaikov force-pushed the tchaikov:wip-bt-dump branch 3 times, most recently from b666779 to ff00612 Jan 15, 2017

common/BackTrace: add operator<<
replace BackTrace::print() with the operator<< where the former is used.

Signed-off-by: Kefu Chai <kchai@redhat.com>

@tchaikov tchaikov force-pushed the tchaikov:wip-bt-dump branch from ff00612 to 74025ef Jan 15, 2017

@badone

This comment has been minimized.

Copy link
Contributor

badone commented Jan 15, 2017

Test this please

@badone

This comment has been minimized.

Copy link
Contributor

badone commented Jan 16, 2017

Looks good now :)

@tchaikov

This comment has been minimized.

Copy link
Contributor Author

tchaikov commented Jan 16, 2017

yeah, i figured out that we should add -fno-inline last night.

@yuriw

This comment has been minimized.

Copy link
Contributor

yuriw commented Jan 16, 2017

@tchaikov

this was tested along with others:

http://pulpito.ceph.com/yuriw-2017-01-13_21:05:46-rados-wip-yuri-testing2_2017_1_13-distro-basic-smithi/

http://pulpito.ceph.com/yuriw-2017-01-14_16:29:03-rados-wip-yuri-testing2_2017_1_13---basic-smithi/

(see http://tracker.ceph.com/issues/18478)

centos run
http://pulpito.front.sepia.ceph.com/yuriw-2017-01-14_16:34:35-rados-wip-yuri-testing2_2017_1_13-distro-basic-vps/

One unit test failed the following tests FAILED: 82 - unittest_back_trace (Failed) and @liewegas requested @tchaikov to see if this is related and can be fixed

Here is the backtrace:

`yuriw@smithi098:~/wip2/build$ ctest -R unittest_back_trace -V
UpdateCTestConfiguration from :/home/yuriw/wip2/build/DartConfiguration.tcl
UpdateCTestConfiguration from :/home/yuriw/wip2/build/DartConfiguration.tcl
Test project /home/yuriw/wip2/build
Constructing a list of tests
Done constructing a list of tests
Checking test dependency graph...
Checking test dependency graph end
test 82
Start 82: unittest_back_trace

82: Test command: /home/yuriw/wip2/build/bin/unittest_back_trace
82: Environment variables:
82: CEPH_ROOT=/home/yuriw/wip2
82: CEPH_BIN=/home/yuriw/wip2/build/bin
82: CEPH_LIB=/home/yuriw/wip2/build/lib
82: CEPH_BUILD_DIR=/home/yuriw/wip2/build
82: LD_LIBRARY_PATH=/home/yuriw/wip2/build/lib
82: PATH=/home/yuriw/wip2/build/bin:/home/yuriw/wip2/src:/home/yuriw/bin:/home/yuriw/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
82: PYTHONPATH=/home/yuriw/wip2/build/lib/cython_modules/lib.2:/home/yuriw/wip2/src/pybind
82: CEPH_BUILD_VIRTUALENV=/tmp
82: Test timeout computed to be: 3600
82: Running main() from gmock_main.cc
82: [==========] Running 1 test from 1 test case.
82: [----------] Global test environment set-up.
82: [----------] 1 test from BackTrace
82: [ RUN ] BackTrace.Basic
82: /home/yuriw/wip2/src/test/common/test_back_trace.cc:36: Failure
82: Value of: std::regex_match(lines[lineno], e)
82: Actual: false
82: Expected: true
82: [ FAILED ] BackTrace.Basic (1 ms)
82: [----------] 1 test from BackTrace (1 ms total)
82:
82: [----------] Global test environment tear-down
82: [==========] 1 test from 1 test case ran. (1 ms total)
82: [ PASSED ] 0 tests.
82: [ FAILED ] 1 test, listed below:
82: [ FAILED ] BackTrace.Basic
82:
82: 1 FAILED TEST
1/1 Test #82: unittest_back_trace ..............***Failed 0.02 sec

0% tests passed, 1 tests failed out of 1

Total Test time (real) = 0.04 sec

The following tests FAILED:
82 - unittest_back_trace (Failed)
Errors while running CTest
`

@yuriw

This comment has been minimized.

Copy link
Contributor

yuriw commented Jan 16, 2017

@tchaikov pls merge or assign back to me if needed, meanwhile un-tagged

@tchaikov

This comment has been minimized.

Copy link
Contributor Author

tchaikov commented Jan 17, 2017

@yuriw the unit test was fixed. and the code have been tested in the qa run was not changed. merging.

@tchaikov tchaikov merged commit c3aed5c into ceph:master Jan 17, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@tchaikov tchaikov deleted the tchaikov:wip-bt-dump branch Jan 17, 2017

@wjwithagen

This comment has been minimized.

Copy link
Contributor

wjwithagen commented Jan 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.