atetubou wrote: Let me send generated comment from AI, but what the AI saying still makes sense to me for this issue especially when there are many variant in attributes combination for `__builtin_*` functions between targets.
-------------------- Let me clarify the mechanics of how `GlobalDecl D` affects IR emission here, the division of responsibilities between `__builtin_...` and system headers, and why relying on the AST declaration (rather than compiler-side automatic `dllimport` attachment under `/MD`) is important. ### 1. How existing AST declarations affect `GetOrCreateLLVMFunction` In [`CodeGenModule::getBuiltinLibFunction`](https://github.com/atetubou/llvm-project/blob/12f333aab8ee65be253aae0013a1cb3761c516ad/clang/lib/CodeGen/CGBuiltin.cpp#L250-L272), after [looking up the library function in the AST translation unit](https://github.com/atetubou/llvm-project/blob/12f333aab8ee65be253aae0013a1cb3761c516ad/clang/lib/CodeGen/CGBuiltin.cpp#L255-L266), we call: ```cpp return GetOrCreateLLVMFunction(Name, Ty, D, /*ForVTable=*/false); ``` (at [CGBuiltin.cpp#L271](https://github.com/atetubou/llvm-project/blob/12f333aab8ee65be253aae0013a1cb3761c516ad/clang/lib/CodeGen/CGBuiltin.cpp#L271)). When `GetOrCreateLLVMFunction` creates a new `llvm::Function* F` for `Name` (`"hypotf"`), it invokes [`SetFunctionAttributes(GD, F, IsIncompleteFunction, IsThunk)`](https://github.com/atetubou/llvm-project/blob/12f333aab8ee65be253aae0013a1cb3761c516ad/clang/lib/CodeGen/CodeGenModule.cpp#L5611) passing `GD = D`. Inside `SetFunctionAttributes`, [`setGVProperties(F, GD)`](https://github.com/atetubou/llvm-project/blob/12f333aab8ee65be253aae0013a1cb3761c516ad/clang/lib/CodeGen/CodeGenModule.cpp#L3546) (via `setDLLImportDLLExport`) checks [`GD.getDecl()->hasAttr<DLLImportAttr>()`](https://github.com/atetubou/llvm-project/blob/12f333aab8ee65be253aae0013a1cb3761c516ad/clang/lib/CodeGen/CodeGenModule.cpp#L2194). - Previously, `D` was constructed directly from the builtin `FunctionDecl` (`FD = __builtin_hypotf`). Because compiler builtins lack `dllimport`, `F` was created without `dllimport`. - With this patch, if `hypotf` was declared in an imported header or module (with `__declspec(dllimport)`), `D` is updated to `GlobalDecl(LibFD)` at [CGBuiltin.cpp#L261](https://github.com/atetubou/llvm-project/blob/12f333aab8ee65be253aae0013a1cb3761c516ad/clang/lib/CodeGen/CGBuiltin.cpp#L261). Thus `SetFunctionAttributes` attaches `dllimport` to `F` (`declare dllimport float @hypotf`). ### 2. Separation of Concerns: `__builtin_...` vs System Headers Regarding the question of why `__builtin_hypotf` should depend on whether `<math.h>` was included or imported: There is a fundamental separation of concerns in compiler architecture: 1. **Compiler builtins (`__builtin_hypotf`)** provide semantic understanding for AST-level optimization (constant folding, CPU instruction substitution, `constexpr` evaluation, and pure-function analysis). 2. **Platform SDK headers (`<math.h>`)** define platform-specific ABI properties (symbol mangling, calling conventions, and DLL storage classes). Across different OS platforms (Windows UCRT, glibc, macOS libSystem, newlib), standard library symbols have completely different storage classes (`dllimport` vs static) and symbol aliases (e.g., `_hypotf` on 32-bit MSVC vs `hypotf` on 64-bit MSVC). When `getBuiltinLibFunction` emits the runtime fallback symbol, it borrows the target platform's linkage specification from the AST declaration. ### 3. What happens when calling `__builtin_` without including `<math.h>` If a user calls `__builtin_hypotf` without including `<math.h>` or importing a math module, `TU->lookup` at [CGBuiltin.cpp#L257](https://github.com/atetubou/llvm-project/blob/12f333aab8ee65be253aae0013a1cb3761c516ad/clang/lib/CodeGen/CGBuiltin.cpp#L257) returns empty. The lookup falls back to `D = GlobalDecl(FD)` (the builtin Decl), emitting `@hypotf` without `dllimport`. The builtin continues to work validly as before. (Demonstrated in [test case 3 of `builtin-hypotf-dllimport.cpp`](https://github.com/atetubou/llvm-project/blob/12f333aab8ee65be253aae0013a1cb3761c516ad/clang/test/Modules/builtin-hypotf-dllimport.cpp#L22)). ### 4. Why Clang shouldn't automatically attach `dllimport` based on `/MD` While automatically attaching `dllimport` (and symbol aliases like `_hypotf`) to lib builtins under `/MD` without headers seems attractive, it introduces several major issues: 1. **User redeclarations & freestanding / static math libraries:** If a user defines their own `hypotf` in the same TU, or statically links a custom library/CRT helper (where `hypotf` is defined locally), automatically forcing `dllimport` on the IR symbol will cause compilation/linking failures (e.g., definition of dllimport function not allowed). 2. **SDK granularity (Single Source of Truth):** Even under `/MD` (Universal CRT / MSVC CRT), not all standard C library symbols are imported from DLLs. Some functions are inline or provided only in static runtime libs (`libvcruntime.lib` etc.). Which symbols require `dllimport` is strictly defined by the target platform's SDK headers (e.g., `_CRTIMP` in `<math.h>`). Hardcoding dllimport rules in the compiler would duplicate and potentially conflict with the SDK headers. 3. **Solving real-world C++ Modules non-determinism:** In real-world C++ code (e.g., Chromium CI using libc++ `std::hypot`), `<cmath>` / `<math.h>` IS imported in the AST. However, in Clang Modules (`-fmodules`), `ASTReader` lazily deserializes PCM declarations on name lookup. On Linux (ext4 hash order), PCM static initialization chains happened to deserialize `"hypotf"` before CodeGen; on Windows (NTFS alphabetical order), it did not. This patch guarantees deterministic behavior across platforms by performing the AST lookup at [CGBuiltin.cpp#L255-L266](https://github.com/atetubou/llvm-project/blob/12f333aab8ee65be253aae0013a1cb3761c516ad/clang/lib/CodeGen/CGBuiltin.cpp#L255-L266). https://github.com/llvm/llvm-project/pull/204792 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
