Avoid C/C++ math function ambiguity for explicit module builds on Windows #85609

hjyamauchi · 2025-11-20T01:45:04Z

This tweaks the existing logic that disables the import of the C++
math functions to avoid ambiguity with the C math functions by
handling the case where the module directory can be null in explicit
module builds.

For #85606

hjyamauchi · 2025-11-20T01:45:50Z

@swift-ci please test

hjyamauchi · 2025-11-20T21:27:08Z

@swift-ci please test

compnerd · 2025-11-20T21:44:35Z

stdlib/public/Platform/tgmath.swift.gyb

@@ -295,7 +295,11 @@ public func lgamma(_ x: ${T}) -> (${T}, Int) {
 @_transparent
 public func remquo(_ x: ${T}, _ y: ${T}) -> (${T}, Int) {
  var quo = Int32(0)
+#if !os(Windows)


This problem should occur for any platform that doesn't differentiate long double from double right?

I think that rather than differentiating this on os(Windows), we want to use something to differentiate this based on sizeof(long double) == sizeof(double).

Not exactly. This also requires another condition: the way the Windows headers declare it. This doesn't occur and the following reproducer test doesn't err on macos because the macos C++ header imports the C remquo into the std namespace for C++ rather than declaring another remquo for long double that would cause an overload with C remquo for long in Swift. I added text on it in the bug description here. So I think this is specific to Windows.

Looking at that description, I think that this is the wrong solution. The problem is that corecrt_math.h does define a module, and the header being transitively imported in the headers by CxxStdlib results in the incorrect definition.

My previous comment was not right. I think the headers are similar in macos. I updated the PR to explicitly call the C remquo. I think the conditions to detect long double == double case is captured by the % if T == 'Float80': 6 lines above. I think this should work.

hjyamauchi · 2025-11-24T17:16:21Z

@swift-ci please test

hjyamauchi · 2025-12-02T03:48:58Z

@swift-ci please test

hjyamauchi · 2025-12-02T16:49:22Z

@swift-ci please test

compnerd · 2025-12-03T04:02:26Z

lib/ClangImporter/ImportDecl.cpp

+            // owningModule->Directory can be null in an explicit module build
+            if ((!owningModule->Directory || file->getDir() == owningModule->Directory) &&


Suggested change

// owningModule->Directory can be null in an explicit module build

if ((!owningModule->Directory || file->getDir() == owningModule->Directory) &&

// If we are a non-umbrella module without a canonical location the directory member may

// not be populated.

if (!owningModule->Directory)

return nullptr;

if (file->getDir() == owningModule->Directory &&

Would the suggested change cause non-math functions to not be imported?

compnerd · 2025-12-03T04:07:17Z

I think that the check should be separated as it is unrelated to the check being modified. The Directory member may not be populated in some cases (but unrelated to explicit modules). This field represents the directory that the module is notionally built in. Since we inject the module maps via the VFS, the path may be unresolved, leaving the member not populated.

@ian-twilightcoder and @cachemeifyoucan might be able to shed more light on this.

ian-twilightcoder · 2025-12-03T21:11:53Z

I think that the check should be separated as it is unrelated to the check being modified. The Directory member may not be populated in some cases (but unrelated to explicit modules). This field represents the directory that the module is notionally built in. Since we inject the module maps via the VFS, the path may be unresolved, leaving the member not populated.

@ian-twilightcoder and @cachemeifyoucan might be able to shed more light on this.

I'm not all that familiar with the Directory member or how that gets put together. I'm not totally sure what the workaround is trying to work around either, neither cmath nor math.h are textual so I'm not sure what the issues are with those.

compnerd · 2025-12-04T05:42:48Z

I'm not all that familiar with the Directory member or how that gets put together. I'm not totally sure what the workaround is trying to work around either, neither cmath nor math.h are textual so I'm not sure what the issues are with those.

It seems that the field is optional and not always populated. Does it make sense to ignore the check for the directory? It seems that would open us up to cases where the header may be attributed to the wrong module.

Alternatively, should we bail early if the Directory member is not populated?

egorzhdan

LGTM, although I'm not too familiar with the testing approach here!

ian-twilightcoder · 2025-12-04T17:32:04Z

I'm not all that familiar with the Directory member or how that gets put together. I'm not totally sure what the workaround is trying to work around either, neither cmath nor math.h are textual so I'm not sure what the issues are with those.

It seems that the field is optional and not always populated. Does it make sense to ignore the check for the directory? It seems that would open us up to cases where the header may be attributed to the wrong module.

Alternatively, should we bail early if the Directory member is not populated?

I don't really know, @Bigcheese @vsapsai @jansvoboda11 @benlangmuir do any of you know?

benlangmuir · 2025-12-04T19:10:33Z

@ian-twilightcoder am I missing something or did you not add this Directory check in #71075? I don't see why we are checking it at all here. I don't really understand why we're trying to hide decls from these headers in this manner, but given that goal I think just checking the filename without looking at Directory should be enough.

The Directory will be nullptr when building cached modules.

hjyamauchi · 2025-12-08T21:53:18Z

In the non-explicit module build case, the Module is instantiated and its Directory is set in clang::ModuleMapLoader::handleModuleDecl. In the explicit module case, the Module is instantiated from clang::ASTReader::ReadSubmoduleBlock -> clang::ModuleMap::createModule (and Directory isn't set) when the module is loaded from a pcm file that was previously built. Perhaps it's not necessarily known/conveyed where it was built.

Encouraged by @benlangmuir 's comment, I removed the Directory part of the check. PTAL.

@swift-ci please test

hjyamauchi · 2025-12-08T22:34:03Z

@swift-ci please test macos platform

hjyamauchi · 2025-12-08T22:34:12Z