Windows importing is now done by name, and new exports do not need to have an ordinal specified for them i.e. you can add a line with just the API name to dllexports.

Changing kmp_stats to use the new definitions would require moving them up into a header somewhere while still requiring <cmath> in kmp_stats. Do you think this would be worth it?

I have reworked the naming but am not seeing any other uses of std::abs in the runtime code. There are a couple of <cmath> references, but none of them appear to be reachable in the current build: one is apparently in a test case, the other - in the stats collection support which is off by default and where math use is heavier (e.g. sqrt) making its removal trickier.

removed <cmath> reference and changed the names of the local replacement implementations to convention.

closed by commit 7fd6d2babf4dcaa24f24da18cd0a858fa89579e8

Thanks, this has been tested with GCC 12 on Windows.

Could someone in the know comment whether the openmp removal was unrelated to the previous GCC build failures, or it was in which case what action may be required to restore it now that this is being fixed? Thanks

Implementation changes to remove the dependency on the C++ standard library

Folks, the same two bots that failed on the previous check-in that this one attempts to fix continue to fail, but with a different problem down the build line. Both are doing GCC builds that are now complaining about unresolved C++ runtime references that can apparently be fixed by adding -lstdc++ to the cmake linker set up. Not sure why this not being picked up by default in GCC configs - others (clang, MSVC, etc.) look OK.

Ok, thanks for the info. It did seem to pass the Phabricator review Linux build here.

Hmm, the couple of similar build breaks I fixed before pushing did make it in. These are something new that I am not seeing on our end. What Windows compiler are the problems showing up with?

Yes, somehow the typedef fixes didn't make it into the push. Let me fix it.
It would be good to add a Windows build to the initial PR check. The absence of such has bitten before.

looks good to me

removed a now redundant dependency

Updated with a better fix.

I will be good with this solution, but I think there could be a simpler one in the end. While CMAKE does wants to see a source, that does not have to be a cpp (which is what's causing this need to unnecessarily feed the first step's library to satisfy lib's attempt at def symbols resolution). There are other source types (e.g., a. txt) which will satisfy the source presence requirement but which CMAKE will filter out from the librarian generating the right command line for it.

Removed redundant comments.

The source of problem is the attempt to re-use the results of the first build in order to avoid rebuilding the sources the second time to just re-generate the import library. This is being done by feeding the main build's import library as a source to the second one (@301) with CMAKE ending up assuming the library is located where the sources are, hence the reference to projects/openmp/runtime/src/libomp.dll.lib instead of what should be lib/libomp.dll.lib in the LLVM based build. At this point, I do not see a good way to just repoint CMAKE to the first build's drop location, and reusing its library would likely require doing a custom target. I think a cleaner and better solution is to simply rebuild the second one from sources (it's a small build). I'll submit a patch.

Could you describe some more what needs fixing? The Windows build with a MSVC toolset should perhaps use lib.

merge check

addressed feedback

merge test

addressed remaining review comments

Fixed test and output file name logic.

Addressed review feedback.
Added a test for creating an import library from a def file.

white space

Updated the controversial comment to promote toolchain equity.

Yes, clang-cl style builds are a known open issue, we are looking at patching llvm-lib.

Looks good. I will include Martin in the subsequent reviews here.

The patch looks OK to fix the mingw build for now. We have work in flight to address a few issues brought up here, e.g. kmp_import, the need to build the second set of sources and .def support outside of the MSVC compatible toolsets (e.g. llvm-lib) that may further touch on this particular script.

Jonathan, if all looks good could you commit on my behalf?

kmp_import.cpp turned up as a random side-effect of this change as the latter piggy-backed on the existing build step that rebuilt the import library dragging kmp_import in (for unrelated reasons, apparently to enforce some rules for code built with a toolset mix) With this change, kmp_imports is no longer necessary because the intent looks to already be covered elsewhere in the code now being linked (kmp_import might be some workaround from days past). I did not include its removal here for two reasons a) it is not directly related to the intent of the ordinals change and b) we (MSVC and Intel) are discussing what the desired behavior here should going forward for LLVM OpenMP flavors. I will remove kmp_import (if it is indeed no longer required) with the check-in that effects what we want this to be.

I have removed the dead code as Joachim requested, and the builds have passed. Could you complete the check-in (I have no write access) If everything looks good?

Removed dead code in __kmpc_next_section.

clang-formatted

fixed a build problem

Addressed review comments

The patch looks to be ready to go in. Could someone with access please commit it? Thanks

clang-formatted the previous version