[Insight-developers] NrrdIO missing name magling

[Gordon L. Kindlmann]

Hi Brad,

Thanks for bringing this up - its something I had wanted to clarify before, and perhaps others can weigh in on this prior to the upcoming release.

The two functions you mention, _nrrdReadNrrdParse and _nrrdFormatNRRD_read, are declared in NrrdIO/parseNrrd.c and NrrdIO/formatNRRD.c, respectively, as :

static int
_nrrdReadNrrdParse_space_directions(FILE *file, Nrrd *nrrd,
                                    NrrdIoState *nio, int useBiff) {
...

static int
_nrrdFormatNRRD_read(FILE *file, Nrrd *nrrd, NrrdIoState *nio) {
…

So they are both static.  My understanding was that the point of name-mangling is to avoid ambiguity at link time, in which case static symbols are irrelevant, right?

I wanted to keep the name mangling header as short as possible, and as stable as possible, so that it wouldn't have to change with internal implementation changes (involving changing static functions).

You're suggesting that name mangling could also have a role in valgrind-style debugging.  Do you think its important to re-list all the static functions in the name mangling header?

Gordon


On Nov 20, 2012, at 9:08 AM, Matt McCormick wrote:

> Hi Brad,
> 
> Gordon found that unnecessary mangles were in itk_NrrdIO_mangle.h, and
> they were purposely removed.
> 
> Thanks,
> Matt
> 
> On Tue, Nov 20, 2012 at 2:47 PM, Bradley Lowekamp
> <blowekamp at mail.nih.gov> wrote:
>> Hello,
>> 
>> I was looking at suppressing some new Valgrind error on SimpleITK (then my
>> new ITK valgrind build), that sprung up after the NrrdIO update:
>> 
>> http://open.cdash.org/viewDynamicAnalysis.php?buildid=2673721
>> 
>> I had these suppressed before. But they came back... Looking at them closer
>> I think it reveals a problem with certain symbols what were mangled before
>> the update, but now are not. For example the following valgrind error:
>> 
>> b>UMC</b> ==27272== Conditional jump or move depends on uninitialised
>> value(s)
>> ==27272==    at 0x1E2C9D27: _nrrdReadNrrdParse_space_directions
>> (parseNrrd.c:550)
>> ==27272==    by 0x1E2D3881: _nrrdFormatNRRD_read (formatNRRD.c:469)
>> ==27272==    by 0x1E2CEB3C: itk__nrrdRead (read.c:441)
>> ==27272==    by 0x1E2CECED: itk_nrrdRead (read.c:482)
>> ==27272==    by 0x1E2CF03D: itk_nrrdLoad (read.c:628)
>> ==27272==    by 0x1E094C2B: itk::NrrdImageIO::ReadImageInformation()
>> (itkNrrdImageIO.cxx:250)
>> ==27272==    by 0x553AE8A:
>> itk::simple::ImageReaderBase::GetImageIOBase(std::string const&amp;)
>> (sitkImageReaderBase.cxx:55)
>> ==27272==    by 0x553AF13:
>> itk::simple::ImageReaderBase::GetPixelIDFromImageIO(std::string const&amp;,
>> int&amp;, unsigned int&amp;) (sitkImageReaderBase.cxx:67)
>> ==27272==    by 0x52C8A93: itk::simple::ImageFileReader::Execute()
>> (sitkImageFileReader.cxx:59)
>> ==27272==    by 0x6E17EF: BasicFilters_ConstantPad_more_Test::TestBody()
>> (sitkConstantPadImageFilterTest.cxx:171)
>> ==27272==    by 0xA518C3: void
>> testing::internal::HandleSehExceptionsInMethodIfSupported&lt;testing::Test,
>> void&gt;(testing::Test*, void (testing::Test::*)(), char const*)
>> (gtest-all.cc:3394)
>> ==27272==    by 0xA4CF0D: void
>> testing::internal::HandleExceptionsInMethodIfSupported&lt;testing::Test,
>> void&gt;(testing::Test*, void (testing::Test::*)(), char const*)
>> (gtest-all.cc:3430)
>> ==27272==    by 0xA3927D: testing::Test::Run() (gtest-all.cc:3466)
>> ==27272==    by 0xA39AB7: testing::TestInfo::Run() (gtest-all.cc:3642)
>> ==27272==    by 0xA3A0E3: testing::TestCase::Run() (gtest-all.cc:3749)
>> ==27272==    by 0xA3F466: testing::internal::UnitTestImpl::RunAllTests()
>> (gtest-all.cc:5541)
>> ==27272==    by 0xA52C38: bool
>> testing::internal::HandleSehExceptionsInMethodIfSupported&lt;testing::internal::UnitTestImpl,
>> bool&gt;(testing::internal::UnitTestImpl*, bool
>> (testing::internal::UnitTestImpl::*)(), char const*) (gtest-all.cc:3394)
>> ==27272==    by 0xA4DC84: bool
>> testing::internal::HandleExceptionsInMethodIfSupported&lt;testing::internal::UnitTestImpl,
>> bool&gt;(testing::internal::UnitTestImpl*, bool
>> (testing::internal::UnitTestImpl::*)(), char const*) (gtest-all.cc:3430)
>> ==27272==    by 0xA3DFF0: testing::UnitTest::Run() (gtest-all.cc:5178)
>> ==27272==    by 0x58C0D4: main (SimpleITKUnitTestDriver.cxx:50)
>> 
>> 
>> 
>> The first two symbols there use to be proceeded with itk. So I don't think
>> they are correctly mangled. The last commit to the file itk_NrrdIO_magle.h
>> reveal a suspiciously large number of symbols being removed:
>> 
>> http://itk.org/gitweb?p=ITK.git;a=commitdiff;h=a264fff5a637a223b54d4e1aaa425411a9cee617#patch36
>> 
>> I am guessing that the mangle perl script was run on OSX, and needs to be
>> also run on linux to be complete. If anyone else know more about this please
>> speak up.
>> 
>> Thanks,
>> Brad
>> 
>> _______________________________________________
>> Powered by www.kitware.com
>> 
>> Visit other Kitware open-source projects at
>> http://www.kitware.com/opensource/opensource.html
>> 
>> Kitware offers ITK Training Courses, for more information visit:
>> http://kitware.com/products/protraining.php
>> 
>> Please keep messages on-topic and check the ITK FAQ at:
>> http://www.itk.org/Wiki/ITK_FAQ
>> 
>> Follow this link to subscribe/unsubscribe:
>> http://www.itk.org/mailman/listinfo/insight-developers
>> 
>