Skip to content

Conversation

silverjam
Copy link
Contributor

@silverjam silverjam commented Jan 26, 2021

Fixes a compilation issue on Windows and adds a stack size linker flag which is necessary to run the tests (sadly, this flag is also needed if you are linking against swiftnav-rs on windows).

Comment on lines +39 to +40
#[cfg_attr(windows, repr(i32))]
#[cfg_attr(not(windows), repr(u32))]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should instead switch to repr(C)? Based on this in the reference: https://doc.rust-lang.org/reference/type-layout.html#reprc-field-less-enums

It sounds like we should maybe abandon this approach in the long term since it risks invoking UB and isn't scalable to particularly weird target/compiler configs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using repr(C) fails with the same issue unfortunately

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate. Oh well

@@ -0,0 +1,2 @@
[target.x86_64-pc-windows-msvc]
rustflags = ["-C", "link-arg=/STACK:8388608"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what to do about this either, it's required in a project that uses swiftnav-rs and here as well, apparently this is by design for how rust links with native libraries:

https://doc.rust-lang.org/beta/unstable-book/language-features/link-args.html

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is is something in our code that requires the 8 MB stack size specifically?

Copy link
Contributor Author

@silverjam silverjam Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to understand this change more, but during compilation things will crash with a thread 'main' has overflowed its stack error otherwise-- I think 4MiB worked, 8MiB was a "just in case" value.

LSN on windows uses the stack by default for allocation-- but this is a runtime thing-- I don't think it would causes issues until something was actually invoked. Nevertheless I tried to enable heap allocation with some compiler flags and it didn't seem to affect this issue.

This made me think it might be a huge table somewhere that gets initialize as part of setting up globals, like the geoid grid... I tried disabling the compilation of the geoid grid but that didn't help.

Didn't try anything else past this though.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're probably right. I'm cool if this is a workaround, maybe we can investigate it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this is definitely a workaround for now, at some point we can (hopefully) revisit and see if we can track down what's requiring such a large stack

@silverjam silverjam requested a review from jbangelo January 27, 2021 03:53
Copy link
Collaborator

@jbangelo jbangelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to your greater knowledge about getting rust code to compile for Windows.

I'll try and clean up our enums soon, I just found that page of the rust reference today and didn't realize that the current approach was slightly dangerous.

@silverjam silverjam merged commit 64ff337 into master Jan 27, 2021
@silverjam silverjam deleted the silverjam/msvc-stack-size branch January 27, 2021 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants