Skip to content

Add ITKCommon_EXPORT macro in the declaration of inlined Zero in NumericTraits#13

Closed
grizonnetm wants to merge 2 commits intoInsightSoftwareConsortium:masterfrom
grizonnetm:fix_inlined_Zero
Closed

Add ITKCommon_EXPORT macro in the declaration of inlined Zero in NumericTraits#13
grizonnetm wants to merge 2 commits intoInsightSoftwareConsortium:masterfrom
grizonnetm:fix_inlined_Zero

Conversation

@grizonnetm
Copy link
Contributor

commit 3ed8e8a in itk NumericTraits leads to compilation error in orfeo toolbox.

I was able to solve them adding ITKCommon_EXPORT macro in the declaration. Not sure to understand why it is necessary as the related commit in ITK seems to handle linkage resolution.

The link error related to new declaration of inlined Zero for integers type can be seen on otb dashboard:

http://dash.orfeo-toolbox.org/viewBuildError.php?buildid=225288

Configurations for ITK and OTB which leads to the linking issue can also be found on otb git repositories:

itk configuration:

https://git.orfeo-toolbox.org/otb-devutils.git/blob/HEAD:/Config/pc-christophe/pc-christophe-ITK-trunk.cmake

otb configuration:

https://git.orfeo-toolbox.org/otb-devutils.git/blob/HEAD:/Config/pc-christophe/pc-christophe-OTB-Nightly-clang-ThirdPartyTrunk.cmake

commit 3ed8e8a  in itk NumericTraits leads to compilation error in orfeo toolbox.

I was able to solve them adding ITKCommon_EXPORT macro in the declaration. Not sure to understand why it is necessary as the related commit in ITK seems to handle linkage resolution.

The link error related to new declaration of inlined Zero for integers type can be seen on otb dashboard:

http://dash.orfeo-toolbox.org/viewBuildError.php?buildid=225288

Configurations for ITK and OTB which leads to the linking issue can also be found on otb git repositories:

itk configuration:

https://git.orfeo-toolbox.org/otb-devutils.git/blob/HEAD:/Config/pc-christophe/pc-christophe-ITK-trunk.cmake

otb configuration:

https://git.orfeo-toolbox.org/otb-devutils.git/blob/HEAD:/Config/pc-christophe/pc-christophe-OTB-Nightly-clang-ThirdPartyTrunk.cmake

Change-Id: I3b528d2ccdf61c061b84f3ed5417a6eeb771f3dc
Change-Id: I0fe477009d2a209a0893347989d3d912b71c9ce6
@grizonnetm grizonnetm changed the title Add ITKCommon_EXPORT macro in the declaration of inlined Zero in NUmericTraits Add ITKCommon_EXPORT macro in the declaration of inlined Zero in NumericTraits Apr 20, 2016
@blowekamp
Copy link
Member

Thank you for including that configuration file.

You appear to be using clang on Fedora 22. What version of clang? Are you building shared libraries?

There are a lot of OTB flags. Can you please provide an ITK configuration to reproduce the error.

With C++11 and constexpr the need for export should not be there. Do you still get the error with "-std=c++11" flag? Does you change work OK with this flag?

@grizonnetm
Copy link
Contributor Author

Thanks for reviewing the patch.

ITK trunk is compiled on Fedora 22 with GCC (5.3.1, system package). Flags used for the configuration:

CMAKE_CXX_FLAGS:STRING=-fPIC -Wall -Wextra -Wunused-local-typedefs
CMAKE_C_FLAGS:STRING=-fPIC -Wall -Wextra

The ITK configuration can be found in the commit message and is available in otb scripts repo:
https://git.orfeo-toolbox.org/otb-devutils.git/blob/HEAD:/Config/pc-christophe/pc-christophe-ITK-trunk.cmake

OTB is compiled with clang 3.5.0 (system package on fedora 22):

CMAKE_C_COMPILER=/usr/bin/clang
CMAKE_CXX_COMPILER=/usr/bin/clang++
CMAKE_C_FLAGS:STRING=-Wall
CMAKE_CXX_FLAGS:STRING=-Wall -Wno-gnu-static-float-init -Wno-\#warnings -Wno-unknown-attributes

I suspect that the error will not appear with "-std=c++11" flag but I did not check my patch with it. Indeed something to do...

Note that what I've found investigating the issue with 'nm' command:

  • without my patch: Zero symbols are in a read only data section local marked 'r' (lowercase)
    • with the patch : still in a read only data section but global, marked 'R' (uppercase)

I confirm that the patch solve the issue on otb dashboard:

http://dash.orfeo-toolbox.org/buildSummary.php?buildid=225558

Side question, is it on the ITK roadmap to only support compiler with c++11 activated?

@blowekamp
Copy link
Member

I am surprised that this error is not showing up on the ITK dashboard. Is there a minimal code example that is required to reproduce this error?

I have a patch up for review that extends the testing for these variables, that may be what triggered this error in your code.

http://review.source.kitware.com/#/c/21050/

@grizonnetm
Copy link
Contributor Author

Actually I'm not able to reproduce the issue with a minimal code example yet :(

There must be something related to the way that OTB link with ITK. ITK is added to OTB as a module (following itk modularity architecture):

https://git.orfeo-toolbox.org/otb.git/tree/HEAD:/Modules/ThirdParty/ITK

A bit stuck now with this...

@blowekamp
Copy link
Member

blowekamp commented Apr 25, 2016

With that test patch I added, I think we are now getting the error on the dashboard. Only one machine which runs valgrind:
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-4)
Debug
Shared Libraries.

@grizonnetm
Copy link
Contributor Author

Ouf!

I've got a simple example only based on ITK which allows to reproduce the problem above.

`#include "itkImage.h"

int main()
{
const unsigned int Dimension = 2;
typedef unsigned char PixelType;

const PixelType val = itk::NumericTraits::Zero;

typedef itk::Image<PixelType, Dimension> ImageType;

ImageType::Pointer image = ImageType::New();

image->FillBuffer(itk::NumericTraits::Zero);

return EXIT_SUCCESS;
}
`

Note that only declaring a const PyxelType val = itk::NumericTraits<PixelType>::Zero will not triggered the link error.

I can reproduce the error only with clang (not with gcc).

@blowekamp
Copy link
Member

I presume it needs to be shared. Does Debug vs Release matter?

@grizonnetm
Copy link
Contributor Author

grizonnetm commented Apr 25, 2016

Debug vs Release does not matter. What's really matter in my test case seems to be the compiler.

What needs to be shared? the ITK build?

@thewtex
Copy link
Member

thewtex commented May 3, 2016

Thanks for patch! This has been pushed to Gerrit and merged:

http://review.source.kitware.com/#/c/21085/

@thewtex thewtex closed this May 3, 2016
kwrobot pushed a commit to Kitware/ITK that referenced this pull request Feb 24, 2017
Matt McCormick (7):
      BUG: Skip NumPy tests when numpy package is not available
      Merge pull request InsightSoftwareConsortium#13 from thewtex/TestsWithoutNumPy
      ENH: Add support for Fortran contiguous in GetImageFromArray
      STYLE: Remove unnecessary obj variable
      BUG: Avoid shapeseq memory leak
      DOC: Improve axes descriptions
      Merge pull request InsightSoftwareConsortium#14 from thewtex/FortranOrder

Change-Id: I5bf4ce66dad0fd4a4c373e0a2e7980b655775a54
HastingsGreer pushed a commit to HastingsGreer/ITK that referenced this pull request Feb 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants