windows support #31
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| [target.x86_64-pc-windows-msvc] | ||
| rustflags = ["-C", "link-arg=/STACK:8388608"] | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,8 @@ type Result<T> = std::result::Result<T, InvalidEphemeris>; | |
|
|
||
| /// Various statuses that an ephemeris can be in | ||
| #[derive(Copy, Clone, Debug)] | ||
| #[cfg_attr(windows, repr(i32))] | ||
| #[cfg_attr(not(windows), repr(u32))] | ||
|
Comment on lines
+39
to
+40
There was a problem hiding this comment. Maybe we should instead switch to 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. There was a problem hiding this comment. Using |
||
| pub enum Status { | ||
| Null = c_bindings::ephemeris_status_t_EPH_NULL, | ||
| Invalid = c_bindings::ephemeris_status_t_EPH_INVALID, | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 stackerror 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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