Skip to content

Conversation

@albinahlback
Copy link
Collaborator

@albinahlback albinahlback commented Mar 18, 2023

The error message we currently get in AppVeyor is very cryptic. Here is an attempt to fix it.

@albinahlback
Copy link
Collaborator Author

@fredrik-johansson I have tried everything I can now (tried to emulate Arb's CMakeLists.txt), and it never shows any proper error messages. If you have a solution I am all for it, but now I'm out of Microsoft tolerance haha.

@albinahlback
Copy link
Collaborator Author

One important note, however, is that you probably want to keep the changes in qsieve/[factor/init].c. MSVC did not like including windows.h, which was the culprit for all those errors in qsieve.h.

Now it just doesn't like linking.

@fredrik-johansson
Copy link
Collaborator

@isuruf Want to have a look at this?

@isuruf
Copy link
Member

isuruf commented Mar 19, 2023

Forcing IPO in cmake at https://github.com/flintlib/flint2/blob/trunk/CMakeLists.txt#L309-L318 takes too much memory and gets killed.

@albinahlback
Copy link
Collaborator Author

Forcing IPO in cmake at https://github.com/flintlib/flint2/blob/trunk/CMakeLists.txt#L309-L318 takes too much memory and gets killed.

Is that the problem though?

@isuruf
Copy link
Member

isuruf commented Mar 19, 2023

Is that the problem though?

That's the first problem. Removing IPO, builds the flint2 library, but building tests fail.
The problem is the removal of FLINT_DLL and ARB_DLL macros. CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS only work for functions and does not work for global data symbols. Previously FLINT_DLL was used for functions and data whereas ARB_DLL was used for global data only and relied on CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS to export the functions.
With the removal of the _DLL macros, the global data is not exported anymore.

@albinahlback
Copy link
Collaborator Author

If it's just the data it should be an easy fix. Will probably get around to doing it tomorrow. Thanks for your input!

@isuruf
Copy link
Member

isuruf commented Mar 19, 2023

I've got a branch at https://github.com/isuruf/flint2/tree/fix_windows

@isuruf
Copy link
Member

isuruf commented Mar 19, 2023

Can you merge my branch to yours?

There are only a few errors after that

t-approx_dot.c.obj : error LNK2019: unresolved external symbol acb_dot_gauss_dot_cutoff referenced in function main
t-dot.c.obj : error LNK2019: unresolved external symbol acb_dot_gauss_dot_cutoff referenced in function main
t-mul_block.c.obj : error LNK2019: unresolved external symbol arb_mat_mul_block_min_block_size referenced in function main
t-exp_series.c.obj : error LNK2019: unresolved external symbol arb_poly_newton_exp_cutoff referenced in function main
t-exp_series.c.obj : error LNK2019: unresolved external symbol acb_poly_newton_exp_cutoff referenced in function main
t-adjugate.c.obj : error LNK2019: unresolved external symbol _ca_methods referenced in function main

@albinahlback
Copy link
Collaborator Author

Yes, that's fine! Do you wanna add a copyright claimer to CMakeLists.txt? Mainly so we know who got the knowledge.

@albinahlback
Copy link
Collaborator Author

Isuru, would you be fine by replacing AppVeyor with the MSVC CI? I could also add a 32-bit checker if you'd like.

@isuruf
Copy link
Member

isuruf commented Mar 19, 2023

Remove some FLINT_DLLs

Why? This would not work.

Isuru, would you be fine by replacing AppVeyor with the MSVC CI?

I'm not sure what you mean by MSVC CI.

Do you wanna add a copyright claimer to CMakeLists.txt? Mainly so we know who got the knowledge.

Sure.

@albinahlback
Copy link
Collaborator Author

Why? This would not work.

Oh, I thought FLINT_DLL was not supposed to be in the definition. How am I supposed to fix this?

I'm not sure what you mean by MSVC CI.

I mean the Github action.

@isuruf
Copy link
Member

isuruf commented Mar 19, 2023

Oh, I thought FLINT_DLL was not supposed to be in the definition. How am I supposed to fix this?

Sorry. I made that comment before 9cef372. 9cef372 should fix all the linking errors.

At 9cef372 we have only two errors

The following tests FAILED:
3076 - src-gr-test-t-fmpz_mpoly_evaluate (SEGFAULT)
3336 - src-ca_mat-test-t-exp (SEGFAULT)

@fredrik-johansson
Copy link
Collaborator

3076 - src-gr-test-t-fmpz_mpoly_evaluate (SEGFAULT)

Likely due to the test program allocating temporaries on the stack inside the main loop and overflowing the stack (a bunch of other gr test programs potentially suffer from this as well, and need to be fixed).

3336 - src-ca_mat-test-t-exp (SEGFAULT)

Don't really have a clue here.

@albinahlback
Copy link
Collaborator Author

I will put the responsibility on you, Fredrik! Do you want me to merge this for the time being? Will squash commits, add copyright to CMakeLists.txt and uncomment the rest in CI.yml.

@fredrik-johansson
Copy link
Collaborator

Sure, this can be merged.

albinahlback and others added 3 commits March 19, 2023 20:17
Previously, windows.h was included which resulted in errors for pure
Windows machines. Now Windows will emulate what Unix-type systems does.

Moreover, a MSVC CI was added in order to the need for AppVeyor.
Windows needs import and export symbols for data. Moreover, Windows
crashed because of interprocedural optimizations in CMake; this was
removed.
@albinahlback albinahlback merged commit 306a2f2 into flintlib:trunk Mar 19, 2023
@albinahlback albinahlback deleted the fix_windows branch March 19, 2023 23:01
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.

3 participants