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

librbd: do not instantiate templates while building tests #14891

Merged
merged 2 commits into from May 4, 2017

Conversation

Projects
None yet
2 participants
@tchaikov
Contributor

tchaikov commented May 1, 2017

No description provided.

@tchaikov tchaikov requested a review from dillaman May 1, 2017

@dillaman

@tchaikov I don't like this -- not one bit. The correct way to solve this is to use anonymous MockImageCtx classes per-test like the vast majority of librbd test cases already utilize.

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 1, 2017

Contributor

@dillaman i wish i knew the right way or the common practice of librados tests as you guys. but it's not obvious to me how we can resolve the issue of duplicated classes definition such as librbd::AsyncObjectThrottle<librbd::ImageCtx> using the MockImageCtx in anonymous namespace? each inclusion of librbd/AsyncObjectThrottle.cc instantiates a new librbd::AsyncObjectThrottle<librbd::ImageCtx> instantiation in the including compilation unit.

Contributor

tchaikov commented May 1, 2017

@dillaman i wish i knew the right way or the common practice of librados tests as you guys. but it's not obvious to me how we can resolve the issue of duplicated classes definition such as librbd::AsyncObjectThrottle<librbd::ImageCtx> using the MockImageCtx in anonymous namespace? each inclusion of librbd/AsyncObjectThrottle.cc instantiates a new librbd::AsyncObjectThrottle<librbd::ImageCtx> instantiation in the including compilation unit.

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman May 1, 2017

Contributor

@tchaikov Ah -- I misinterpreted the issue you were trying to fix. It sounds like the armhf GCC is not tagging the explicitly instantiated template symbols as weak like the other GCC compilers. I think I'd rather just #ifdef SOMETHING around the explicit instantiation such that they are only activated when building librbd -- and not when pulled in via a test.

Contributor

dillaman commented May 1, 2017

@tchaikov Ah -- I misinterpreted the issue you were trying to fix. It sounds like the armhf GCC is not tagging the explicitly instantiated template symbols as weak like the other GCC compilers. I think I'd rather just #ifdef SOMETHING around the explicit instantiation such that they are only activated when building librbd -- and not when pulled in via a test.

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 3, 2017

Contributor

@dillaman updated and repushed.

Contributor

tchaikov commented May 3, 2017

@dillaman updated and repushed.

@tchaikov tchaikov changed the title from test/librbd: move explicit template specializations out into a dedicated file to librbd: do not instantiate templates while building tests May 3, 2017

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 3, 2017

Contributor

test failure is addressed by #14929

Contributor

tchaikov commented May 3, 2017

test failure is addressed by #14929

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman May 3, 2017

Contributor

@tchaikov Has this been tested? Just wondering why only a few classes need this workaround instead of all the classes that have mock tests.

Contributor

dillaman commented May 3, 2017

@tchaikov Has this been tested? Just wondering why only a few classes need this workaround instead of all the classes that have mock tests.

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 3, 2017

Contributor

@dillaman tested by compiling unittest_librbd using gcc-armhf.

Contributor

tchaikov commented May 3, 2017

@dillaman tested by compiling unittest_librbd using gcc-armhf.

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman May 3, 2017

Contributor

@tchaikov Do you have any idea why the DisableFeaturesRequest needed the #ifndef? Is there a way for me to run a cross-compile on my local machine to look into this?

Contributor

dillaman commented May 3, 2017

@tchaikov Do you have any idea why the DisableFeaturesRequest needed the #ifndef? Is there a way for me to run a cross-compile on my local machine to look into this?

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 3, 2017

Contributor

@dillaman because "src/test/librbd/operation/test_mock_DisableFeaturesRequest.cc" includes librbd/operation/DisableFeaturesRequest.cc. yes. you can create a toolchain.cmake file like #14881 (comment), and launch cmake like

cmake -DCMAKE_TOOLCHAIN_FILE=~/toolchain.cmake

after installing all dev packages, on debian (or its derivatives), one can install them like

dpkg --add-architecture armhf
apt-get update
apt-get install crossbuild-essential-armhf
apt-get install libudev-dev:armhf
...

probably

mk-build-deps --host-arch armhf --build-arch armhf -a armhf --install debian/control

can also work...

Contributor

tchaikov commented May 3, 2017

@dillaman because "src/test/librbd/operation/test_mock_DisableFeaturesRequest.cc" includes librbd/operation/DisableFeaturesRequest.cc. yes. you can create a toolchain.cmake file like #14881 (comment), and launch cmake like

cmake -DCMAKE_TOOLCHAIN_FILE=~/toolchain.cmake

after installing all dev packages, on debian (or its derivatives), one can install them like

dpkg --add-architecture armhf
apt-get update
apt-get install crossbuild-essential-armhf
apt-get install libudev-dev:armhf
...

probably

mk-build-deps --host-arch armhf --build-arch armhf -a armhf --install debian/control

can also work...

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman May 3, 2017

Contributor

@tchaikov Yes -- understand that the mock test includes the implementation, but so do all the other mock tests. I was wondering what was so special about just that one test:

$ git grep -E "#include.*.cc" | wc -l
48

Contributor

dillaman commented May 3, 2017

@tchaikov Yes -- understand that the mock test includes the implementation, but so do all the other mock tests. I was wondering what was so special about just that one test:

$ git grep -E "#include.*.cc" | wc -l
48

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 3, 2017

Contributor

@dillaman yeah, not sure why =(. i was just fixing anything linker was complaining about.

Contributor

tchaikov commented May 3, 2017

@dillaman yeah, not sure why =(. i was just fixing anything linker was complaining about.

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman May 3, 2017

Contributor

@tchaikov Ack -- thx, I'll try to look into it some more.

Contributor

dillaman commented May 3, 2017

@tchaikov Ack -- thx, I'll try to look into it some more.

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 3, 2017

Contributor

thanks, i am assigning this PR to you for future investigation.

Contributor

tchaikov commented May 3, 2017

thanks, i am assigning this PR to you for future investigation.

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman May 3, 2017

Contributor

@tchaikov Did you have to update your apt sources.list somehow? Neither "apt-get install libudev-dev:armhf" nor "mk-build-deps ..." works

Contributor

dillaman commented May 3, 2017

@tchaikov Did you have to update your apt sources.list somehow? Neither "apt-get install libudev-dev:armhf" nor "mk-build-deps ..." works

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 3, 2017

Contributor

@dillaman did you run "apt-get update" after "dpkg --add-architecture armhf"?

Contributor

tchaikov commented May 3, 2017

@dillaman did you run "apt-get update" after "dpkg --add-architecture armhf"?

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman May 3, 2017

Contributor

@tchaikov Yup -- status shows it fails to download the package lists for armhf. That's why I was wondering if you had to do something w/ your source list.

Contributor

dillaman commented May 3, 2017

@tchaikov Yup -- status shows it fails to download the package lists for armhf. That's why I was wondering if you had to do something w/ your source list.

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman May 3, 2017

Contributor

@tchaikov Are you cross compiling from an arm64 box? It looks like all the arm packages are in a totally different repo @ http://ports.ubuntu.com/ubuntu-ports/dists/xenial/main/

Contributor

dillaman commented May 3, 2017

@tchaikov Are you cross compiling from an arm64 box? It looks like all the arm packages are in a totally different repo @ http://ports.ubuntu.com/ubuntu-ports/dists/xenial/main/

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 3, 2017

Contributor

@dillaman i am cross-compiling from an amd64 box. btw, i am using debian sid. will post you my sources.list tomorrow. maybe ubuntu/xenial is different.

Contributor

tchaikov commented May 3, 2017

@dillaman i am cross-compiling from an amd64 box. btw, i am using debian sid. will post you my sources.list tomorrow. maybe ubuntu/xenial is different.

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman May 3, 2017

Contributor

@tchaikov I'll try debian -- thanks

Contributor

dillaman commented May 3, 2017

@tchaikov I'll try debian -- thanks

@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 3, 2017

Contributor

@dillaman thank you! i will also try to nm other other mock tests to understand why my fix works.

Contributor

tchaikov commented May 3, 2017

@dillaman thank you! i will also try to nm other other mock tests to understand why my fix works.

@dillaman

This comment has been minimized.

Show comment
Hide comment
@dillaman

dillaman May 3, 2017

Contributor

@tchaikov I still wasn't able to get cross-compiling working under Debian unstable due to all the package dependencies. I am currently just running a build under a qemu-arm-static chroot environment.

Contributor

dillaman commented May 3, 2017

@tchaikov I still wasn't able to get cross-compiling working under Debian unstable due to all the package dependencies. I am currently just running a build under a qemu-arm-static chroot environment.

test/librbd: remove duplicated explicit template specializations
to avoid violation of ODR

Fixes: http://tracker.ceph.com/issues/18938
Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov

This comment has been minimized.

Show comment
Hide comment
@tchaikov

tchaikov May 4, 2017

Contributor

@dillaman i updated http://tracker.ceph.com/issues/18938#note-46 to answer your question at #14891 (comment),

tl;dr: AsyncRequest defines key function. so does its derived classes. and gcc-armhf does not put type_info into COMDAT sections to comply the ARM's C++ ABI spec.

Contributor

tchaikov commented May 4, 2017

@dillaman i updated http://tracker.ceph.com/issues/18938#note-46 to answer your question at #14891 (comment),

tl;dr: AsyncRequest defines key function. so does its derived classes. and gcc-armhf does not put type_info into COMDAT sections to comply the ARM's C++ ABI spec.

Show outdated Hide outdated src/librbd/operation/DisableFeaturesRequest.cc
@@ -636,4 +636,6 @@ Context *DisableFeaturesRequest<I>::handle_finish(int r) {
} // namespace operation
} // namespace librbd
#ifndef TEST_F

This comment has been minimized.

@dillaman

dillaman May 4, 2017

Contributor

This doesn't appear to be necessary -- I successfully built and linked unittest_librbd w/o this change.

@dillaman

dillaman May 4, 2017

Contributor

This doesn't appear to be necessary -- I successfully built and linked unittest_librbd w/o this change.

This comment has been minimized.

@tchaikov

tchaikov May 4, 2017

Contributor

@dillaman no, it does not. updated and pushed.

@tchaikov

tchaikov May 4, 2017

Contributor

@dillaman no, it does not. updated and pushed.

librbd: do not instantiate templates while building tests
to avoid violation of ODR

Fixes: http://tracker.ceph.com/issues/18938
Signed-off-by: Kefu Chai <kchai@redhat.com>
@dillaman

lgtm

@tchaikov tchaikov merged commit cca1640 into ceph:master May 4, 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-librbd-test-odr branch May 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment