All submissions to this site are governed by the Second Life Viewer Contribution Agreement. By submitting patches and other information using this site, you acknowledge that you have read, understood, and agreed to those terms.

Review Board 1.6.11

Welcome to the Second Life Viewer Code Review tool.
See the documentation on our wiki for how to use this site.

CMAKE_EXE_LINKER_FLAGS not honored when linking the viewer binary if -DLL_TESTS:BOOL=ON

Review Request #147 - Created Feb. 9, 2011 and submitted

Merov Linden Reviewers
https://bitbucket.org/merov_linden/viewer-development-storm-981 viewer
STORM-981
None viewer-development 
From Aleric's patch:

Setting CMAKE_EXE_LINKER_FLAGS to "" because the tests "need" that is pretty hard measure.  Not only is it not necessary to do so, it also changes how the viewer is linked depending on a whether or not the tests are compiled and that is not good.

The reason that this was needed is that libgmock is underlinked (see http://wiki.mandriva.com/en/Underlinking), which is not compatible with -Wl,--as-needed that is being used on linux. libgmock.so.0 needs a symbol that is defined in libgtest.so.o, but -lgtest was not passed to the linker when creating libgmock.so.0:

Underlinked (no libgtest.so.o):
$ objdump -p /usr/lib/libgmock.so.0 | grep NEEDED
  NEEDED               libstdc++.so.6
  NEEDED               libm.so.6
  NEEDED               libc.so.6
  NEEDED               libgcc_s.so.1

The solution is to wrap between -Wl,--no-as-needed -lgtest -Wl,--as-needed causing it to be added again. This is only needed on linux, since that the only platform that we use -Wl,--as-needed on. Moreover, we can just set GOOGLEMOCK_LIBRARIES to "gmock -Wl,--no-as-needed gtest -Wl,--as-needed" since that is only passed to TARGET_LINK_LIBRARIES which only adds -l in front of 'things' that don't start with '-', to allow you do pass special flags like this.


 

 










  

 


















 

  




 

 

  
  

  
Boroondas Gupte

  
Posted  (Feb. 10, 2011, 2:02 a.m.)

 

 

   
A good change to avoid heisenbugs.

   

  

   

   
    
    
   


  






   







    

     
     

      

       
        

         
          indra/cmake/GoogleMock.cmake
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


        
 


       
      

     

    

    

     
Don't mix spaces and tabs. (Use spaces here, like in the rest of the file.)


     

  

   

   
    
    
   


  





    

    

    

     
     

      

       
        

         
          indra/cmake/GoogleMock.cmake
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


        
 


       
      

     

    

    

     
I'd put the
	-Wl,--no-as-needed
down on gtest's line, so that the "wrapping" is more obvious. Or maybe even use some creative indentation like:
# ...
if (LINUX)
    set(GOOGLEMOCK_LIBRARIES 
        gmock
        -Wl,--no-as-needed # VWR-24366: gmock is underlinked, it needs gtest.
            gtest
        -Wl,--as-needed) # continue to use "as-needed" for other libs
elseif(WINDOWS)
# ...


     

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    I don't think you can add comments like that in the middle of a SET( ... # ... ) ? Never tried it though.
If GOOGLEMOCK_LIBRARIES would end up like "gmock -Wl,--no-as-needed # VWR-24366: gmock is underlinked, it needs gtest. gtest -Wl,--as-needed" then that definitely will break things ;)

@merov: https://codereview.secondlife.com/r/95/ is still open and present on this reviewboard. It's about this same patch?

    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Also, I think that what merov is looking for is someone who builds on linux (standalone or not) to actually test this patch... since building is the only thing that is influenced. Since a TC build doesn't set CMAKE_EXE_LINKER_FLAGS, you probably should be someone who builds standalone and sets CMAKE_EXE_LINKER_FLAGS. Alternatively, you can fake setting it (just add "-Lhello_world" for example) and confirm that survives with patch. A positive test of the patch would need you to install any needed libs in a non-default path that actually need that -Lpath and check that the viewer doesn't link anymore without the patch, but does with. In all cases makes sure you have LL_TESTS on. I'll add a full test plan to http://jira.secondlife.com/browse/STORM-981

    
    
    
   
    3.