Enable and propagate /permissive- on MSVC

[Gold856]

Previously in #2256:

For reasons I do not understand, dynamic_pointer_cast/dynamic_cast fails when building in Debug mode...

I gave it some more thought, and now I do understand why it fails! Well, at least how to fix it. For whatever reason, /permissive-, which enables standards compliance, fixes this issue. Adding it as a build flag makes the ifdef unnecessary. This also happens to fix some other issues I encountered when attempting to build gtsam statically, namely unresolved symbol errors for SO<2> SO<3>. The lack of this flag may actually be the root cause for several of the open Windows issues.

Enabling this flag does have some consequences however. As demonstrated, this flag subtly changes the behavior of the code in potentially unknown and mysterious ways. And all downstream consumers must also compile with /permissive- (made easier by the fact that I put it in the public compile flags), which could also be relying on MSVC's default /permissive behavior. However, given that Linux and Mac are already compiling with standards compliance flags, and that some gtsam code is already broken by non-standard behavior in MSVC, I think it's acceptable to enable this flag for gtsam and all downstream consumers.

[dellaert]

Ok, that’s cool I think we need to mention that somewhere, though. Could you add it to the INSTALL.md file in the windows section?

BTW that section seems very old - I think most people use VS code now - so are those instructions still valid?

[Gold856]

It looks mostly up-to-date, just needs to be Project -> Configure Cache instead of Project -> Generate Cache. Although, yeah, most people use VS Code, including me :). I personally use the Ninja generator instead of the VS generator because Ninja builds projects much faster, and even Visual Studio allows you to use Ninja. The instructions can essentially be universal at this point, since you can just have people do cmake --build build --target install and that works on all platforms (it's how I build gtsam on Windows.)

I put this right under the steps for Windows:
Important: gtsam requires compiling with /permissive- and for all projects to also compile with /permissive- (due to lots of code being in headers) and sets the list of public compiler flags accordingly. If your project does not current build with /permissive-, make sure it does and fix whatever is needed to make it work. Failure to compile with /permissive- can cause various runtime or build errors.

[ProfFan]

Thank you for the PR! The other problems are probably related to the visibility of type_info under Windows.

I think it's still a bit unclear whether this is the right approach though.

some other issues I encountered when attempting to build gtsam statically, namely unresolved symbol errors for SO<2>

Have you checked if the symbol SO<2> has correct visibility? It's unlikely that a static build will have unresolved symbol error unless the template is not instantiated in an object file.

[Gold856]

Whoops, I made a mistake. It was actually SO<3>, not SO<2>. My bad. And it looks like it does have the correct visibility, since GTSAM_EXPORT is applied to all the relevant functions. One of the errors I got was while building check.nonlinear_unstable:

libgtsamDebug.lib(Rot3.cpp.obj) : error LNK2019: unresolved external symbol "public: class Eigen::Matrix<double,9,1,0,9,1> __cdecl gtsam::SO<3>::vec(class gtsam::OptionalJacobian<9,3>)const " (?vec@?$SO@$02@gtsam@@QEBA?AV?$Matrix@N$08$00$0A@$08$00@Eigen@@V?$OptionalJacobian@$08$02@2@@Z) referenced in function "public: class Eigen::Matrix<double,3,1,0,3,1> __cdecl gtsam::Rot3::xyz(class gtsam::OptionalJacobian<3,3>)const " (?xyz@Rot3@gtsam@@QEBA?AV?$Matrix@N$02$00$0A@$02$00@Eigen@@V?$OptionalJacobian@$02$02@2@@Z)

Dumping the object file, that symbol just didn't exist when I built statically. It did when I built shared libraries, and it was successfully statically linked only under /permissive-. Notably, moving the definition of SO<3>::vec to the header instead of the source file would get it to work. I didn't PR that because it felt kinda wrong, and I was looking to also remove the pointer casting hack I introduced earlier.

The other problems are probably related to the visibility of type_info under Windows

Given the fact that I had dynamic_pointer_cast fail when it should've succeeded in a similar way to some of those issues, I think the visibility of type_info might be affected by /permissive-, and that permissive mode being on is the true root cause of those issues.

[talregev]

I think to let permissive flags is to avoid the problems you have in gtsam. Usually it connected to improper use of const. But here as I understand you try to avoid other problems.
Dealing with unresolved symbol in static usually connected to wrong order in the compilation flags.
I can try to do this on vcpkg and try to resolve the problems.
Just let me know. I can also add a CI build that compile windows in static.

I also myself encounter bugs when I compile gtsam on debug on windows.
talregev#42 (comment)
I didn't post it, because I didn't sure if you have the time to deal with that.

[Gold856]

Yeah, a static library build would be good. It definitely fails on my machine (with Boost disabled) with unresolved symbol errors unless I add /permissive-.

I also myself encounter bugs when I compile gtsam on debug on windows.

The vector iterator failure is fixed by #2256, and the nonlinear_unstable test failure can definitely be fixed by adding /permissive- without also applying #2256.

[Gold856]

Here's a GitHub Actions run where I use develop and just build statically, no other changes except to enable CI and a gitignore I accidently committed (look at the debug run): https://github.com/Gold856/gtsam/actions/runs/18263446940
Note that it can not find gtsam::SO<3>::vec.

Here's a GitHub Actions run where I do that, but also enable /permissive- (look at the debug run; release build seems to have tripped a separate warning): https://github.com/Gold856/gtsam/actions/runs/18265376760
I did have to make a few other changes in cephes to get it to link, but if you look through the commits, you'll note that there's no other changes to gtsam itself. Note that it builds successfully.

I have no idea why this is the case. I take permissive mode to essentially mean that MSVC can do whatever it wants. All I know is enabling /permissive- fixes this. This is definitely the right approach in my opinion, since it seems like the lack of /permissive- is causing a bunch of issues related to RTTI on Windows.

[talregev]

After we speak on this PR to compile static windows, I tried it myself.
I create a CI that I recreate the unresolved symbol errors and other more compilation error and try to deal with that.
Ended create a PR that demonstrate how to compile static windows without the permissive flag.
#2272

In this process I seen the problems that gtsam have that need to be fixed.
Add the flag will avoid dealing with that problems, and dev will create code that will work for linux and osx but can have a problems in the windows compilation.
This is was the same that gtsam didn't have a CI that compile windows, and when I add the CI we start fixing these problems, one by one. It wasn't easy task and it take long process and now windows back to good shape that people can use it. Using the permissive flag can bring us to the same state for windows was before. Also there is a good reason why msvc don't put this flag as a default and let you compile in the "hard" way.

[Gold856]

I don't really understand what you're saying. Are you disagreeing with adding /permissive-?

In this process I seen the problems that gtsam have that need to be fixed.
Add the flag will avoid dealing with that problems, and dev will create code that will work for linux and osx but can have a problems in the windows compilation.

You have it reversed, I think. I'm having trouble parsing this sentence. I assume you meant that adding /permissive- will make it such that devs write code that works on Linux/Mac, but doesn't work on Windows. But adding /permissive- actually causes the opposite effect. Currently, because you're compiling without /permissive-, gtsam as-is works on Linux/Mac, but has issues on Windows. Case in point, right now, static builds work perfectly with GCC, but not MSVC. Adding /permissive- is not avoiding problems that exist, it's disabling non-standard behavior in MSVC, bringing it to the same level as GCC/Clang. Standards compliance is good, and this project has gone as far to disable compiler extensions. GCC/Clang are forced to be standards-compliance, why exempt Windows? Adding /permissive- means that gtsam is always being compiled as standard C++, no more, no less.

Also there is a good reason why msvc don't put this flag as a default and let you compile in the "hard" way.

MSVC is older than the first C++ standard. The reason non-standards compliance is disabled by default is because Microsoft really tries hard to keep things backwards-compatible. If they forced standards compliance on everyone, many codebases would stop compiling until they fixed it to be standards compliant. So Microsoft just leaves standards compliance off by default to not make them do work. Well, until you turn on C++20 or beyond. Then /permissive- becomes the default. This is not a project from the Windows 95 era. This is a project that uses C++17 and is trying to move away from Boost to more standardized things (a very welcomed move!) The direction this project is moving in is to use modern, standard C++. /permissive- is simply another step in that direction.

The only reason I note that adding /permissive- could cause issues, is people might be compiling be compiling with permissive mode, and may be using features only allowed in permissive mode. The solution is for them to fix their code to be standards compliant, and I think warning that you need standards compliance is a fine solution to that.

Also, sorry if I come off as argumentative. I just want to be sure we do things the right way :)

[dellaert]

OK, @Gold856 and @talregev and @ProfFan . I love this effort on making our windows life smoother and more compatible :-) I'd love to merge - if this is the right thing to do, but have two requests:

• Which of the windows issues specifically would this close, and can we do that automatically when merging this PR by mentioning them here?
• could you come to an agreement on whether this is the right approach and this PR should be merged? I tend to agree with @Gold856 last comment, just a bit unsure about downstream project consequences. But @talregev and @ProfFan , please
  • comment with a yes-vote
  • OR keep the discussion going until we resolved all possible objections

[talregev]

I am also waiting for @ProfFan respond.
I did some digging about /permissive-.
It will bring msvc to strictly standards-conforming, pushing toward portable, standard C++. plus that it will be the default for c++20.
So I also want to intend to say yes, but
There is set of problems that it will hide:
/permissive- doesn’t warn about many export/import mistakes (e.g., exporting inline things, inconsistent dllimport/dllexport, exporting namespace-scope const without extern).

I was thinking maybe to export all the symbols in gtsam. I am not sure what the effect will be, but it will not need to deal with export / import dll and all the weird problems include linkage problem that it get gtsam project.
Do similar to gtsam like @Gold856 did for cephes:
d18d368

Then if we succeeded in this direction, Then it will be sure yes for /permissive-.

[ProfFan]

I agree we should have the flag, it disables msvc hacks.

[Gold856]

There is set of problems that it will hide:
/permissive- doesn’t warn about many export/import mistakes

I don't think it's the job of /permissive- to warn about that, and I don't think it's "hiding" them either, but we can definitely look for fixes anyways.

exporting inline things

Could you clarify what you mean by this? Based on some articles I've found, it seems like exporting inline things is fine? Would /Zc:inline do what you want? That flag is actually not turned on by default by /permissive- despite it being a conformance option. Maybe we should add that as well.

inconsistent dllimport/dllexport

I'm assuming this means using dllimport/dllexport when you should've used the other, like how cephes currently incorrectly does dllimport for both shared and static libraries? This can easily be caught by CI by building static as well as shared.

exporting namespace-scope const without extern

Could you clarify this as well? The closest thing I found was /Zc:externConstexpr, but that only applies to constexpr variables, not const variables.

I was thinking maybe to export all the symbols in gtsam. I am not sure what the effect will be, but it will not need to deal with export / import dll and all the weird problems include linkage problem that it get gtsam project.

This is actually more complicated than you might think. It's also related to exporting const variables without extern. Exporting data is tricky, mostly since it requires dllimport on the consumer end. A cursory search indicates that DefaultKeyFormatter, MultiRobotKeyFormatter, and VariableSlots::Empty seem to be impacts. For exporting all symbols to work, any exported data would need to either become functions, or have the current export macros applied to them. There's also another key issue: if you export everything, you can hit the library object limited, which I did hit when I tried exporting all symbols for gtsam.dll: LINK : fatal error LNK1189: library limit of 65535 objects exceeded. So that likely means no exporting everything.

check_slam_program error

``` 13/23 Testing: check_slam_program 13/23 Test: check_slam_program Command: "D:/gtsam/build/bin/check_slam_program.exe" Directory: D:/gtsam/build/gtsam/slam/tests "check_slam_program" start time: Oct 11 04:48 Eastern Daylight Time Output: ---------------------------------------------------------- Not equal: expected: [ 3.26795e-07, -1, 0; 1, 3.26795e-07, 0; 0, 0, 1 ] actual: [ -2.97202e-05, -1, -4.11641e-06; 1, -2.97202e-05, -1.08352e-06; 1.0834e-06, -4.11644e-06, 1 ] D:\gtsam\gtsam\slam\tests\testInitializePose3.cpp:118: Failure: "assert_equal(simple::R1, initial.at(x1), 1e-6)" Not equal: expected: [ -1, 3.4641e-07, 0; -3.4641e-07, -1, 0; 0, 0, 1 ] actual: [ -1, 6.04471e-05, -8.23338e-06; -6.04471e-05, -1, -2.16733e-06; -8.23351e-06, -2.16684e-06, 1 ] D:\gtsam\gtsam\slam\tests\testInitializePose3.cpp:119: Failure: "assert_equal(simple::R2, initial.at(x2), 1e-6)" Not equal: expected: [ 1.96153e-08, 1, 0; -1, 1.96153e-08, 0; 0, 0, 1 ] actual: [ 8.70269e-05, 1, -1.25086e-05; -1, 8.70268e-05, -3.25862e-06; -3.25753e-06, 1.25089e-05, 1 ] D:\gtsam\gtsam\slam\tests\testInitializePose3.cpp:120: Failure: "assert_equal(simple::R3, initial.at(x3), 1e-6)" There were 3 failures Test time = 0.16 sec ---------------------------------------------------------- Test Failed. "check_slam_program" end time: Oct 11 04:48 Eastern Daylight Time "check_slam_program" time elapsed: 00:00:00 ---------------------------------------------------------- ```

Which of the windows issues specifically would this close, and can we do that automatically when merging this PR by mentioning them here?

So I might've gotten a bit confused here. Most of the Windows issues that show up seem to have already been resolved? But they're not closed either. They look kinda similar, but further analysis suggests to me that only #496 seems to fit the profile of something that would be fixed by this PR (unresolved symbol on SO, not unresolved by another PR) but I can't test it without MATLAB installed. I can try using the closing keywords to link the PR if you want.

a bit unsure about downstream project consequences

Yeah, I can't say I know how this will impact downstream projects either. But if 4.3 is gonna be a fresh release with breaking changes, I think this can reasonably be noted in the release notes as a heads-up to anyone compiling with MSVC.

[talregev]

I cannot give you an example for each problem it will hide, I can show the ones I encounter when I tried to compile gtsam in static. These problem will be hidden for /permissive- flag.
#2272 (comment)

I don't say no to /permissive-, I am concern about that it will hide problems.
as I say, I intend to say yes to /permissive- and suggested to solve the dll import / export differently and give an example how it can done. aka let the build system to deal with the export.
You can merge this PR, and try to think / discuss / have a PR for solve the dll import / export differently.

[Gold856]

Okay, that's fair. I think we should just stick to our current export system, given that exporting all symbols isn't possible and has some issues with the way gtsam is currently built. As for the other issues, they certainly aren't hidden by /permissive-. The incorrect cephes export (where it always uses dllimport) still throws a linker error under /permissive-, and the other two unresolved symbols, I talked about already:

Dumping the object file, that symbol just didn't exist when I built statically. It did when I built shared libraries, and it was successfully statically linked only under /permissive-.

Dumping the SO3.cpp.obj object file with dumpbin, what happened is the object file that was supposed to contain the SO<3>::vec symbol actually marked as UNDEF, meaning it wasn't defined in the object file. Turning on /permissive- changed the symbol to be marked as SECT8AB like so:

1294 00000000 SECT8AB notype ()    External    | ?vec@?$SO@$02@gtsam@@QEBA?AV?$Matrix@N$08$00$0A@$08$00@Eigen@@V?$OptionalJacobian@$08$02@2@@Z (public: class Eigen::Matrix<double,9,1,0,9,1> __cdecl gtsam::SO<3>::vec(class gtsam::OptionalJacobian<9,3>)const )

indicating that this symbol is, in fact, in this object file. It's my understanding that without /permissive-, MSVC can do all sorts of weird things. But I think its interactions with exporting goes only as far as exports not working when they should. Keep in mind that GCC/Clang can handle that symbol just fine (and MSVC in standards-compliance mode can as well).

[dellaert]

The CI ran except for vcpkg, which got killed. Re-running to make sure it's not a systematic OOM error.

[dellaert]

@Gold856 @talregev
Sorry guys - very busy in GTSFM, deadline coming up.
Seems that me re-running again ran out of memory. I'll try once more but maybe there is a systematic issue with Ubuntu vcpkg that this PR introduced.

[Gold856]

Hm, wasn’t there a commit that fixed this by increasing the swap space? I think if I update my branch to be up-to-date with develop, that should pull in that change and fix it.

[dellaert]

Oh, this time it passed. But I'll wait to see if you are pulling in that swap change here. Might be good to pull in develop regardless. I'll await word.

[talregev]

please rebase to the latest dev

[dellaert]

OK, great ! Let's do it :-)

[Gold856]

Thank you so much!

[dellaert]

Thank you @Gold856 and all who gave feedback esp @talregev and @ProfFan