Turn on -Wl,--no-undefined by default in development mode

[nschloe]

The checkin-test script by default currently runs one MPI_DEBUG and one SERIAL_RELEASE build. I suppose the MPI/SERIAL DEBUG/RELEASE crossover this to cover as much of the code as possible.

One more way to extend bug checking here would be to build shared libraries one of the two builds, and use -Wl,--no-undefined to catch undefined symbols. This way, we would avoid the weekly load of underlinking bugs, see for example Trilinos commit 6670c7364c082062d286fa25dd531eb417518b29.

Trilinos is virtually free of underlinking right now, except for Trilinos STK bug 5929 which didn't get fixed in over a year. I'm not sure what to do about STK anymore.

[bartlettroscoe]

Really this should be added to all builds in development mode so that developers see the problems right way, like we do with strong warning options. Basically, you do something like:

IF (${PROJECT_NAME}_ENABLE_DEVELOPMENT_MODE)
    SET(CMAKE_SHARED_LINKER_FLAGS
      "-Wl,--no-undefined ${CMAKE_SHARED_LINKER_FLAGS} " )
ENDIF()

I would likely add a special option to allow a developer to skip this if desired so I would not add the exact cmake code as is to TriBITS.

I would really like to switch between shared and static builds but there are some machines where this will be problematic for some reason.

[bartlettroscoe]

Nico,

When you can verify that all of Trilinos builds with this flag set, I will add support to TriBITS to turn this on automatically for Trilinos.

Actually, I need to refactor the handling of strong checking options like this so that the can be defined at the repository level and package level better. Developers need to be able to turn off options that just don't work for their package.

[nschloe]

There are three bugs about unresolved symbols that I'm aware of, and they are listed as dependencies of Trilinos bug #6147. All of them are in STKClassic, so there's nothing I can do about those. That isn't a blocker, though, since STKClassic changes are principally never checked by the checkin-test script either, owing to policy that prevents changes on the Trilinos side.

Actually, I need to refactor the handling of strong checking options like this so that the can be defined > at the repository level and package level better.

Hm, really? I would think that linking options (just like compiler settings) are something for which there is one global switch.

[bartlettroscoe]

Hm, really? I would think that linking options (just like compiler settings) are something for which there is one global switch.

There are compiler options that are safe to turn on and off on a package-by-package basis. For example, the types of warnings the compiler emits can be specialized for each package with TriBITS. For example see the options CLEANED, ENABLE_SHADOWING_WARNINGS and DISABLE_STRONG_WARNINGS in TRIBITS_PACKAGE_DECL().

What is different about Wl,--no-undefined is that it has to be applied to every package upstream from it. It is just that some sloppy downstream packages might just as well turn this off until they can be fixed. For example, we could have Wl,--no-undefined enabled for all of Trilinos but then turn off for STK, for example, until STK can be cleaned up.

Therefore, to avoid breaking backward compatibility and breaking current packages like STK, I think we should consider adding an option LIB_NO_UNDEFINED to TRIBITS_PACKAGE_DECL() that will add Wl,--no-undefined to the package's CMAKE_SHARED_LINKER_FLAGS var. Or, we could add option LIB_ALLOW_UNDEFINED to skip adding Wl,--no-undefined to the link flags. I would guess the latter would be preferable since all of Trilinos is currently clean except for STK. We don't want to loose the clean links of the other packages in the mean time.

So the proposal I would have is to define the global option ${PROJECT_NAME}_ENABLE_NO_UNDEFINED_CHECK that would be ON by default (for Trilions but not other TriBITS projects) but then add the option LIB_ALLOW_UNDEFINED to TRIBITS_PACKGE_DECL() to have this check turned off on a package-by-package basis (starting with STK). This would be easy to implement. I just need to add the automated testing for this. That is what takes the time.

Sound reasonable?

[nschloe]

What is different about Wl,--no-undefined is that it has to be applied to every package upstream from it.

Really? How so?

So the proposal I would have is to define the global option ${PROJECT_NAME}_ENABLE_NO_UNDEFINED_CHECK that would be ON by default (for Trilions but not other TriBITS projects) [...] Sound reasonable?

It does. I'm a little concerned about working around the STK bug: This is adding code to TriBITS to work around something that should be fixed on the STK side. More than that, it also adds an additional option that needs to be documented. (TriBITS/checkin-test is already quite complex as it is.)

[bartlettroscoe]

What is different about -Wl,--no-undefined is that it has to be applied to every package upstream from it.

Really? How so?

I am just assuming that if your have yourlib and you link it to lib1, lib2, etc. that all of the libraries in the link chain need to have all of their symbols defined. The is another option link option -Wl--no-allow-shlib-undefined that seems to do more checking, perhaps recusing down the shared libs?

I'm a little concerned about working around the STK bug: This is adding code to TriBITS to work around something that should be fixed on the STK side.

We should not give up on fixing STK, it us just that I don't want to loose what is now in place with the other packages. We can't let prefect be the enemy of better.

More than that, it also adds an additional option that needs to be documented. (TriBITS/checkin-test is already quite complex as it is.)

The checkin-test.py script is independent and it not complicated by this at all. I wish it was as simple as typing checkin-test.py --do-all --push but Trilinos is too big, too complex, and has too many optional TPLs for that to be all that anyone ever needs to do to push to Trilinos for any changes.

I would be curious to hear what your primary complaints are about using the checkin-test.py script. It it the tool itself of issues with Trilinos (e.g. PT vs. ST packages and code)?

[nschloe]

I am just assuming that if your have yourlib and you link it to lib1, lib2, etc. that all of the libraries in the link chain need to have all of their symbols defined.

Hm, I wouldn't think so. -Wl,--no-undefined only checks that all symbols are present for the link call in which it is contained. If there are downstream libraries that link without -Wl,--no-undefined, well, then they won't have that sanity check. Bad for them, but no harm done anywhere else.

The checkin-test.py script is independent and it not complicated by this at all.

Ah okay! I misunderstood then. The link option is going into TriBITS directly?

[bartlettroscoe]

Ah okay! I misunderstood then. The link option is going into TriBITS directly?

Yes, it will just be on by default for every configuration of Trilinos. That way, developers will get instant and automatic feedback, just like they down now with the strong warning options.

[nschloe]

Yes, it will just be on by default for every configuration of Trilinos.

For everyone who builds shared libraries, of course.

I'm not sure if I the biggest fan of baking this intro TriBITS. It makes things somewhat less explicit than the standard

  -DCMAKE_SHARED_LINKER_FLAGS:STRING="-Wl,--no-undefined" \

Plus, we're buying in to compiler checks of the kind

IF ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
   SET(CMAKE_SHARED_LINKER_FLAGS ${CMAKE_SHARED_LINKER_FLAGS} "-Wl,--no-undefined")
ENDIF()

[bartlettroscoe]

Plus, we're buying in to compiler checks of the kind

IF ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
  SET(CMAKE_SHARED_LINKER_FLAGS ${CMAKE_SHARED_LINKER_FLAGS} "-Wl,--no-undefined")
ENDIF()

NOTE: Checks for GNU already exist for all of the strong compiler warning flags in TriBITS. See TribitsSetupStrongCompileWarnings.cmake.

I will try to get to this soon. We will just need to to exclude SEACAS until they can fix it.

[bartlettroscoe]

Note that before we can turn this on for all of Trilinos, someone needs to address all of the link problems blocking Trilinos bug #6147.

[nschloe]

All of them are in STK, so only member of the inner circle can touch those files. Again, not sure how to proceed.

[bartlettroscoe]

All of them are in STK, so only member of the inner circle can touch those files. Again, not sure how to proceed.

When I get around to implementing this (in the next 2 weeks hopefully), if the issue are not fixed yet by Brent or the STK developers, then you can just fix this on a branch and I will just warn them and push this to master myself. Anyone with any git skills should be able to adjust the sync process to accommodate this. I have no more patience for this messed up STK and SEACAS integration model.

[bartlettroscoe]

TriBITS needs to get out of the business of setting compiler options. This should be a decision for a given CMake proejct using TriBITS.