Bug#584172: Disabling GetIDMapper inlining for building VISU
Hello Denis, On Wed, Jun 02, 2010 at 02:13:51PM +0200, Denis Barbier wrote: On 2010/6/2 Andre Espaze wrote: [...] Your solution is however better because the exported symbols are in control while in my case I remove every GetIDMapper function inlining. [...] It seems that there is some disagreement between us, I believe that the sentence above is the root cause. You say that my solution gives a better control on symbols which are exported, but my opinion is that both solutions provide the exact same API. Can you please explain the differences induced by those patches? (For instance by running objdump, readelf,... on generated libraries and comparing output) Thanks. Sure, I am going to show the problem on real librairies. I had first imitated it by the test.cpp code provided in bug 457075, see: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=457075#400 From the beginning, the problem is specific to g++ optimizations that's why I wanted to reflect that fact in my patch. Am I wrong on that point? The disagreement is maybe just here. I assume that you run git-buildpackage on the revision f57b74db488c91754eadbca263c065ff2022ac71, thus the build has failed with the status message given by Adam in that bug entry. The relevant code is in the CONVERTOR directory of the VISU module: $ cd VISU_SRC_5.1.3/src/CONVERTOR The VISUConvertor can not be built because GetIDMapper symbols do not exist. You can check the problem in the corresponding module: $ readelf -s .libs/libVisuConvertor_la-VISU_MergeFilterUtilities.o | \ grep GetIDMapper However if you remove the O2 optimizations in the CXXFLAGS of the Makefile (by first making a backup): $ cp Makefile Makefile.orig $ sed -i s/^CXXFLAGS = .*/CXXFLAGS = -D_OCC64 -g -D_DEBUG_ -pthread/ \ Makefile and then force the module to be built again: $ rm libVisuConvertor_la-VISU_MergeFilterUtilities.lo $ make it will work. Now if you check the exported symbols: $ readelf -s .libs/libVisuConvertor_la-VISU_MergeFilterUtilities.o | \ grep GetIDMapper 759: 113 FUNCWEAK DEFAULT 395 _ZN4VISU11GetIDMapperINS_ 769: 113 FUNCWEAK DEFAULT 411 _ZN4VISU11GetIDMapperINS_ 879: 259 FUNCWEAK DEFAULT 569 _ZN4VISU11GetIDMapperINS_ 880: 259 FUNCWEAK DEFAULT 571 _ZN4VISU11GetIDMapperINS_ you should have 4 entries. At that step, I understood that the upstream code is valid C++ (but not necessarily robust) and thus I wanted to provide a patch controlling g++ optimizations while respecting their coding style. That's why you can find the 'if' statements since the beginning. My patch no-template-function-inline will remove every template function inlining of GetIDMapper when compiling with O2 optimizations: $ cd ../../.. $ patch -p1 no-template-function-inline.patch $ patch -p1 debian/patches/visu-no-template-inline.patch $ cd VISU_SRC_5.1.3/src/CONVERTOR $ mv Makefile.orig Makefile $ make $ readelf -s .libs/libVisuConvertor_la-VISU_MergeFilterUtilities.o | \ grep GetIDMapper 89: 113 FUNCWEAK DEFAULT 48 _ZN4VISU11GetIDMapperINS_ 95: 54 FUNCWEAK DEFAULT 50 _ZN4VISU11GetIDMapperINS_ 96: 54 FUNCWEAK DEFAULT 52 _ZN4VISU11GetIDMapperINS_ 97: 113 FUNCWEAK DEFAULT 54 _ZN4VISU11GetIDMapperINS_ Finally your solution will only export the needed GetIDMapper symbol: $ git checkout HEAD VISU_MergeFilterUtilities.hxx $ cd ../../.. $ patch -p1 visu-template-export.patch $ cd VISU_SRC_5.1.3/src/CONVERTOR $ make $ readelf -s .libs/libVisuConvertor_la-VISU_MergeFilterUtilities.o | \ grep GetIDMapper 68: 113 FUNCWEAK DEFAULT 26 _ZN4VISU11GetIDMapperINS_ I agree with you that your solution is valid and robust C++ code so the if statements could be removed no matter if the code is compiled with or without optimizations or even with others compilers. But I think that such a decision should be made by upstream because we discovered the problem only when working with g++ O2 optimizations. I am open to further explanations if needed. Best regards, André -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#584172: Disabling GetIDMapper inlining for building VISU
On 2010/6/7 Andre Espaze wrote: [...] $ patch -p1 no-template-function-inline.patch $ patch -p1 debian/patches/visu-no-template-inline.patch Thanks André for these detailed explanations, but why do you apply both patches? I thought that visu-no-template-inline.patch was superseding no-template-function-inline.patch. (Adam too since he committed only the former) To make it clear, my point is that #if macro can be dropped from visu-no-template-inline.patch, the result will be unchanged: * When compiled in debug mode, functions are not inlined and 4 symbols are exported. * When compiled with optimization, only one symbol is exported. Since those macros do nothing, it is better to strip them off and follow what is described in the C++ faq. If you want no-template-function-inline.patch to be applied, then visu-no-template-inline.patch should be dropped. $ cd VISU_SRC_5.1.3/src/CONVERTOR $ mv Makefile.orig Makefile $ make $ readelf -s .libs/libVisuConvertor_la-VISU_MergeFilterUtilities.o | \ grep GetIDMapper 89: 113 FUNC WEAK DEFAULT 48 _ZN4VISU11GetIDMapperINS_ 95: 54 FUNC WEAK DEFAULT 50 _ZN4VISU11GetIDMapperINS_ 96: 54 FUNC WEAK DEFAULT 52 _ZN4VISU11GetIDMapperINS_ 97: 113 FUNC WEAK DEFAULT 54 _ZN4VISU11GetIDMapperINS_ [...] These symbols are exported because of no-template-function-inline.patch, not visu-no-template-inline.patch. Denis -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#584172: Disabling GetIDMapper inlining for building VISU
On Mon, Jun 07, 2010 at 06:06:58PM +0200, Denis Barbier wrote: On 2010/6/7 Andre Espaze wrote: [...] $ patch -p1 no-template-function-inline.patch $ patch -p1 debian/patches/visu-no-template-inline.patch Thanks André for these detailed explanations, but why do you apply both patches? If you check out revision f57b74db488c, visu-no-template-inline.patch does not exist yet, so the first patch adds it. I thought that visu-no-template-inline.patch was superseding no-template-function-inline.patch. (Adam too since he committed only the former) Yes, no-template-function-inline.patch was only a way to apply my changes to Adam's repository (we decided to work like that since the beginning so Adam can review every patch and I do not need write access). To make it clear, my point is that #if macro can be dropped from visu-no-template-inline.patch, the result will be unchanged: * When compiled in debug mode, functions are not inlined and 4 symbols are exported. * When compiled with optimization, only one symbol is exported. Since those macros do nothing, it is better to strip them off and follow what is described in the C++ faq. Ok but what about respecting the upstream coding style? Moreover the macros do nothing only when using g++. I am fine to remove the if macros but I think that we will need to provide upstream with more explanations. If you want no-template-function-inline.patch to be applied, then visu-no-template-inline.patch should be dropped. No, no-template-function-inline.patch is only a communication tool. I am sorry if it has created confusion. The only patch that matters for us is visu-no-template-inline.patch. However I did not want to add one more patch to the bug and for describing reproductible steps I needed to introduce no-template-function-inline.patch. $ cd VISU_SRC_5.1.3/src/CONVERTOR $ mv Makefile.orig Makefile $ make $ readelf -s .libs/libVisuConvertor_la-VISU_MergeFilterUtilities.o | \ grep GetIDMapper 89: 113 FUNC WEAK DEFAULT 48 _ZN4VISU11GetIDMapperINS_ 95: 54 FUNC WEAK DEFAULT 50 _ZN4VISU11GetIDMapperINS_ 96: 54 FUNC WEAK DEFAULT 52 _ZN4VISU11GetIDMapperINS_ 97: 113 FUNC WEAK DEFAULT 54 _ZN4VISU11GetIDMapperINS_ [...] These symbols are exported because of no-template-function-inline.patch, not visu-no-template-inline.patch. Isn't is the opposite? no-template-function-inline.patch modifies the Debian repo while visu-no-template-inline.patch modifies the Salome sources. André -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#584172: Disabling GetIDMapper inlining for building VISU
On 2010/6/7 Andre Espaze wrote: On Mon, Jun 07, 2010 at 06:06:58PM +0200, Denis Barbier wrote: On 2010/6/7 Andre Espaze wrote: [...] $ patch -p1 no-template-function-inline.patch $ patch -p1 debian/patches/visu-no-template-inline.patch Thanks André for these detailed explanations, but why do you apply both patches? If you check out revision f57b74db488c, visu-no-template-inline.patch does not exist yet, so the first patch adds it. [...] Okay, I obviously did not carefully read your message and thought that we were discussing about visu-template-export.patch, sorry for the trouble. Actually Adam did apply your visu-template-export.patch, and not visu-no-template-inline.patch, hence my confusion. Denis -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#584172: Disabling GetIDMapper inlining for building VISU
It is finally not necessary to build the module with the -g option, I have enclosed a patch that disable the GetIDMapper function inlining when built with g++ and optimizations. commit 73f793cb0076b6847fc17750a5e1511af502e06c Author: André Espaze andre.esp...@logilab.fr Date: Tue Jun 1 16:53:53 2010 +0200 No GetIDMapper inlining when building VISU The template function GetIDMapper should not be inlined by g++ when compiling with optimizations (the Debian build system used -O2) because the VISUConvertor command needs to link with the resulting symbol contained in libVisuConvertor.so. The patch is provided in: debian/patches/visu-no-template-inline.patch and will be applied by quilt thanks to the list: debian/patches/series diff --git a/debian/patches/series b/debian/patches/series index 524b45d..d280ec5 100644 --- a/debian/patches/series +++ b/debian/patches/series @@ -26,6 +26,7 @@ med-fix-clean.patch visu-hdf5-needs-mpi.patch visu-build-in-tree.patch visu-flags-typo.patch +visu-no-template-inline.patch visu-fix-clean.patch smesh-hdf5-needs-mpi.patch smesh-use-gui-check.patch diff --git a/debian/patches/visu-no-template-inline.patch b/debian/patches/visu-no-template-inline.patch new file mode 100644 index 000..ead1d55 --- /dev/null +++ b/debian/patches/visu-no-template-inline.patch @@ -0,0 +1,28 @@ +The template function GetIDMapper should not be inlined by g++ when +compiling with optimizations (the Debian build system used -O2) because +the VISUConvertor command needs to link with the resulting symbol +contained in libVisuConvertor.so. + +diff --git a/VISU_SRC_5.1.3/src/CONVERTOR/VISU_MergeFilterUtilities.hxx b/VISU_SRC_5.1.3/src/CONVERTOR/VISU_MergeFilterUtilities.hxx +index 52f7440..e4f63ae 100755 +--- a/VISU_SRC_5.1.3/src/CONVERTOR/VISU_MergeFilterUtilities.hxx b/VISU_SRC_5.1.3/src/CONVERTOR/VISU_MergeFilterUtilities.hxx +@@ -93,12 +93,18 @@ namespace VISU + TObjectId2TupleGaussIdMap theObjectId2TupleGaussIdMap); + + templateclass TGetFieldData ++#if (__GNUG__ __OPTIMIZE__) ++ __attribute__((noinline)) ++#endif + vtkIntArray* + GetIDMapper(VISU::TFieldList* theFieldList, + TGetFieldData theGetFieldData, + const char* theFieldName); + + templateclass TGetFieldData ++#if (__GNUG__ __OPTIMIZE__) ++ __attribute__((noinline)) ++#endif + vtkIntArray* + GetIDMapper(vtkDataSet* theIDMapperDataSet, + TGetFieldData theGetFieldData,
Bug#584172: Disabling GetIDMapper inlining for building VISU
On 2010/6/2 Andre Espaze wrote: It is finally not necessary to build the module with the -g option, I have enclosed a patch that disable the GetIDMapper function inlining when built with g++ and optimizations. Hello, I am no C++ expert, but this looks very similar to http://www.parashift.com/c++-faq-lite/templates.html#faq-35.13 and it is thus not clear whether this is really a g++ bug. Here is a different fix, I tested this approach with Andre's test.cpp, but not with salome. Denis instantiation.patch Description: application/empty
Bug#584172: Disabling GetIDMapper inlining for building VISU
Hello Denis, On Wed, Jun 02, 2010 at 10:30:15AM +0200, Denis Barbier wrote: On 2010/6/2 Andre Espaze wrote: It is finally not necessary to build the module with the -g option, I have enclosed a patch that disable the GetIDMapper function inlining when built with g++ and optimizations. I am no C++ expert, but this looks very similar to http://www.parashift.com/c++-faq-lite/templates.html#faq-35.13 and it is thus not clear whether this is really a g++ bug. Here is a different fix, I tested this approach with Andre's test.cpp, but not with salome. Thank you very much for the suggestion, it works but it seems to me that you forgot a 'template' keyword before the symbol export. I have enclosed a patch tested on my machine, fell free to correct it if I was wrong. For the -9 release, I am anyway going to submit my change to Adam for getting a working version with VISU. A full build will again be required because other symbol exports may be needed in other modules. Your solution is however better because the exported symbols are in control while in my case I remove every GetIDMapper function inlining. I am neither a C++ expert but I found the FAQ entry different from our case because the symbol gets exported without -O2 optimizations. I would not say that it is a g++ bug because I think that you should control inlining when optimizing (as I have done in my first patch). However your solution is right and succeeds to export template function symbols with optimizations. Once again thank you because I would not have figured it out myself. Best regards, André diff --git a/VISU_SRC_5.1.3/src/CONVERTOR/VISU_MergeFilterUtilities.cxx b/VISU_SRC_5.1.3/src/CONVERTOR/VISU_MergeFilterUtilities.cxx index 126effb..89d1b50 100755 --- a/VISU_SRC_5.1.3/src/CONVERTOR/VISU_MergeFilterUtilities.cxx +++ b/VISU_SRC_5.1.3/src/CONVERTOR/VISU_MergeFilterUtilities.cxx @@ -718,7 +718,12 @@ namespace VISU } return NULL; } - + // Explicit symbol export when compiling with g++ and optimizations, + // needed by VISUConvertor during linking + #if (__GNUG__ __OPTIMIZE__) + template vtkIntArray* + GetIDMapperTGetPointData(TFieldList*, TGetPointData, const char* ); + #endif //--- templateclass TGetFieldData
Bug#584172: Disabling GetIDMapper inlining for building VISU
On 2010/6/2 Andre Espaze wrote: Hello Denis, On Wed, Jun 02, 2010 at 10:30:15AM +0200, Denis Barbier wrote: On 2010/6/2 Andre Espaze wrote: It is finally not necessary to build the module with the -g option, I have enclosed a patch that disable the GetIDMapper function inlining when built with g++ and optimizations. I am no C++ expert, but this looks very similar to http://www.parashift.com/c++-faq-lite/templates.html#faq-35.13 and it is thus not clear whether this is really a g++ bug. Here is a different fix, I tested this approach with Andre's test.cpp, but not with salome. Thank you very much for the suggestion, it works but it seems to me that you forgot a 'template' keyword before the symbol export. Oops, you are fully right, sorry. A new patch is attached for reference. I have enclosed a patch tested on my machine, fell free to correct it if I was wrong. [...] I do not understand why you added #if tests, just adding 'template' at the very beginning is enough. It was not explicit in my previous mail, but I tried to find an alternative solution because your's works only with g++, and you will need those #if tests if you want to forward it upstream. OTOH mine is portable, and it could be adopted more easily by upstream, I guess. Within Debian, which patch is checked in does not matter. Denis instantiation2.patch Description: application/empty
Bug#584172: Disabling GetIDMapper inlining for building VISU
I have enclosed a patch tested on my machine, fell free to correct it if I was wrong. [...] I do not understand why you added #if tests, just adding 'template' at the very beginning is enough. It was not explicit in my previous mail, but I tried to find an alternative solution because your's works only with g++, and you will need those #if tests if you want to forward it upstream. OTOH mine is portable, and it could be adopted more easily by upstream, I guess. Within Debian, which patch is checked in does not matter. I added the #if tests because the patch is a solution to a problem occuring only when compiling with g++ and -O2 optimizations. Would you like upstream to force the symbol export even if the code compiles without g++ optimizations? Did you check that the template problem occurs for others C++ compilers before saying that you have a portable solution? André -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#584172: Disabling GetIDMapper inlining for building VISU
On 2010/6/2 Andre Espaze wrote: [...] Your solution is however better because the exported symbols are in control while in my case I remove every GetIDMapper function inlining. [...] It seems that there is some disagreement between us, I believe that the sentence above is the root cause. You say that my solution gives a better control on symbols which are exported, but my opinion is that both solutions provide the exact same API. Can you please explain the differences induced by those patches? (For instance by running objdump, readelf,... on generated libraries and comparing output) Thanks. Denis -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Bug#584172: Disabling GetIDMapper inlining for building VISU
tags 584172 +fixed +pending thanks Hello, It looks like the patches from the two of you are identical except for the #if statements. I think upstream is more likely to accept André's patch with the #ifs because of portability, so I'm going with that one for now. Let me know if you find that the #ifs are not needed for portability. (Sorry, I'm not a C++ expert.) -Adam -- GPG fingerprint: D54D 1AEE B11C CE9B A02B C5DD 526F 01E8 564E E4B6 Engineering consulting with open source tools http://www.opennovation.com/ signature.asc Description: This is a digitally signed message part
