Skip to content

Windows compatibility changes for C++ code. #1967

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

stellaraccident
Copy link
Contributor

@stellaraccident stellaraccident commented Apr 22, 2025

Includes findings/fixes from #1864.

Fixes all portability issues that resulted in compilation errors in C++ code. There are still warnings and some possible functional issues which need more work, but this is a first step we should commit. This patch was written/tested in a combined tree that is capable of building on Windows, and we need to verify that any rewritten paths do not regress Posix. With this and new CMake patches, the project compiles on Windows. It has some linkage issues on the tests which need further investigation (we have another branch with some of these findings/workarounds).

Several additional changes from the above:

  • Rewrote shared library finding code to be based on resolving a library by address vs scanning for a named library. While the code for both is not identical on Windows/Posix, the same semantics for such a helper are directly available.
  • Rewrote a number of things for clarity and to use std::filesystem vs use of low level path and string munging.
  • Removed several conditional #ifdef _WIN32 when the branches could be expressed in a portable manner.

Explanations for some changes:

  • s/ABC/ABC_dims/: ABC is used as a bareword in Windows headers.
  • s/WIN32/_WIN32/: _WIN32 is the modern/correct way to detect if compiling on Windows. It is defined in all cases whereas WIN32 is only defined in some situations.
  • ssize_t is non-standard. In the place it is used, did the usual typedef from the SSIZE_T type that windows provides in its headers.
  • portable_uniform_int_distribution template specializations instead of direct use of std::uniform_int_distribution enables compilation on standards compliant C++ standard libs, which do not include char specializations (includes MSVC but also some others).
  • Remove non-standard DL_LIB ways to include -ldl on Posix. That is what CMAKE_DL_LIBS is for.
  • Remove HIPBLASLT_EXPORT from all deleted members. This is illegal and was not being caught in Posix builds.
  • Replace libgen.h with std::filesystem equivalents. We should not allow libgen.h in new code.
  • u_int16_t is not a real/standard type. Use uint16_t.
  • Rewrite some unreadably nested ifdefs in initialization code to use normal if statements and compile both paths vs leaving some compilation paths only for certain build configurations. We should enforce this as a coding convention.

@stellaraccident stellaraccident changed the title WIP: Windows compatibility changes for C++ code. Windows compatibility changes for C++ code. Apr 24, 2025
@stellaraccident stellaraccident marked this pull request as ready for review April 24, 2025 01:58
@stellaraccident
Copy link
Contributor Author

Reviewers: please provide a detailed review, especially focusing on code paths that were not just forked between Posix/Windows but were rewritten to be portable. The intent was that these should be NFC, but the original code had numerous bugs which I could spot (and expect were corner cases that were not being exercised), and I made judgment calls to fix them vs preserve broken behavior.

Rename incorrect find/replace ABC_dims back to abc.
@stellaraccident
Copy link
Contributor Author

PTAL - Rebased and fixed a couple of minor issues on the Posix side (dladdr call needed a cast, and some missed abc rename fixes). gfx1100 tests pass locally for me. Will see what the CI says.

}
return pathstr;
#else
return std::string(realpath("/proc/self/exe", 0));
Copy link
Contributor

@bstefanuk bstefanuk Apr 25, 2025

Choose a reason for hiding this comment

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

If realpath cannot resolve /proc/self/exe then a nullptr is returned, which would cause undefined behaviour here when constructing the std::string. However, I understand this is an edge case and I'm not too familiar with /proc/self/exe. Is this guaranteed to be defined on all Posix-compliant systems?

@ellosel
Copy link
Contributor

ellosel commented Apr 26, 2025

@stellaraccident I am rebooting tests because there was a spurious docker failure. I am going to approve now because tests are otherwise passing.

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.

4 participants