Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve compatibility with other Compilers like MinGW #8387

Merged
merged 7 commits into from
Sep 4, 2024

Conversation

Mellich
Copy link
Collaborator

@Mellich Mellich commented Sep 2, 2024

Problem solved by the commit

I am working on an integration of XRT into Julia. However, the current Windows build does only support MSVC and the packages for Julia are build using MinGW, so I XRT to improve the compatibility with MinGW. I ported back some of the patches because they should not affect the currently supported toolchains and could be useful to support further toolchains in the future. No features are added or removed to XRT with this PR - it only affects the build process.

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

Currently, some preprocessor defines are used in non-standard ways, increasing the difficulty to compile the code on Windows with GCC. Also, some library dependencies seem to be missing for some of the Windows binaries.

How problem was solved, alternative solutions (if any) and why they were rejected

I changed some occurrences of __GNU__ to __linux__ because the contained codes rather depend on the used OS than on the compiler.
The missing libraries were added as link dependency.

Risks (if any) associated the changes in the commit

I was not able to test the build on a native Windows machine with MSVC, because I have no access to one.
The proposed changes should only affect the build and linking of binaries, and not the functionality.

What has been tested and how, request additional testing if necessary

Build on Red Head Enterprise Linux and Alpine Linux using GCC 9.1 as well as MinGW on Alpine Linux with GCC 9.1 and some additional minor patches.

Documentation impact (if any)

Should not affect documentation, because this PR only applies changes to the build process.

@gbuildx
Copy link
Collaborator

gbuildx commented Sep 2, 2024

Mellich - is not a collaborator
Can XRT admins please validate PR

@gbuildx
Copy link
Collaborator

gbuildx commented Sep 2, 2024

Can one of the admins verify this patch?

Copy link
Collaborator

@stsoe stsoe left a comment

Choose a reason for hiding this comment

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

Thanks much. In general this looks okay.Few question around the added link libraries.
Are we certain that __attribute__ ((...)) converted from __GNUC__ to __linux__ are in fact not GNU specific?

src/runtime_src/core/tools/xbutil2/CMakeLists.txt Outdated Show resolved Hide resolved
src/runtime_src/core/pcie/windows/alveo/CMakeLists.txt Outdated Show resolved Hide resolved
@gbuildx
Copy link
Collaborator

gbuildx commented Sep 2, 2024

Mellich - is not a collaborator
Can XRT admins please validate PR

@Mellich
Copy link
Collaborator Author

Mellich commented Sep 2, 2024

Are we certain that __attribute__ ((...)) converted from __GNUC__ to __linux__ are in fact not GNU specific?

At least for Clang, this seems also to be available: https://clang.llvm.org/docs/LTOVisibility.html, so it is not GNU specific.
Regarding the additional libraries: I made them depend on the compiler ID to prevent issues with MSVC. As said in another comment, these are required because linking via pragmas is not supported by the GNU compilers and the libraries should be provided via link time instead.

@Mellich Mellich requested a review from stsoe September 2, 2024 11:14
Copy link
Collaborator

@stsoe stsoe left a comment

Choose a reason for hiding this comment

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

This looks good. @Mellich please signoff your commits.

@manikandan-xilinx, we will need to get CI to kick in and validate this PR, what's the trick to run CI for outside contributors?

@gbuildx
Copy link
Collaborator

gbuildx commented Sep 2, 2024

Mellich - is not a collaborator
Can XRT admins please validate PR

@gbuildx
Copy link
Collaborator

gbuildx commented Sep 2, 2024

Mellich - is not a collaborator
Can XRT admins please validate PR

Marius Meyer added 7 commits September 2, 2024 21:12
The __GNU__ flag is used in situations where an OS
dependent flag should be used instead because the
changes in the source files are not compiler
dependent

Signed-off-by: Marius Meyer <mariusme@mail.upb.de>
Signed-off-by: Marius Meyer <mariusme@mail.upb.de>
Signed-off-by: Marius Meyer <mariusme@mail.upb.de>
Signed-off-by: Marius Meyer <mariusme@mail.upb.de>
Signed-off-by: Marius Meyer <mariusme@mail.upb.de>
Signed-off-by: Marius Meyer <mariusme@mail.upb.de>
Signed-off-by: Marius Meyer <mariusme@mail.upb.de>
@stsoe stsoe merged commit 2c97fe5 into Xilinx:master Sep 4, 2024
17 checks passed
@stsoe
Copy link
Collaborator

stsoe commented Sep 4, 2024

@Mellich . Thank you very much for this effort.

vipangul pushed a commit to vipangul/XRT that referenced this pull request Sep 11, 2024
* Make preprocessor conditions depend on os

The __GNU__ flag is used in situations where an OS
dependent flag should be used instead because the
changes in the source files are not compiler
dependent

Signed-off-by: Marius Meyer <mariusme@mail.upb.de>

* Fix upper case in include

Signed-off-by: Marius Meyer <mariusme@mail.upb.de>

* Fix libxrt_core windows link dependencies

Signed-off-by: Marius Meyer <mariusme@mail.upb.de>

* Fix xbutil2 windows link dependencies

Signed-off-by: Marius Meyer <mariusme@mail.upb.de>

* Fix xclbinutil windows link dependencies

Signed-off-by: Marius Meyer <mariusme@mail.upb.de>

* Only define types if GCC is not used

Signed-off-by: Marius Meyer <mariusme@mail.upb.de>

* Remove DLL exports and imports from header-defined methods

Signed-off-by: Marius Meyer <mariusme@mail.upb.de>

---------

Signed-off-by: Marius Meyer <mariusme@mail.upb.de>
Co-authored-by: Marius Meyer <mariusme@mail.upb.de>
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