Skip to content

Conversation

@compnerd
Copy link
Member

This allows us to target a lower level library (corecrt) rather than the higher level library (ucrt) which can be helpful in mordularising the SDK.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Mar 17, 2025
@compnerd compnerd added platform:windows and removed clang Clang issues not falling into any other category backend:X86 labels Mar 17, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 17, 2025

@llvm/pr-subscribers-platform-windows
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-x86

Author: Saleem Abdulrasool (compnerd)

Changes

This allows us to target a lower level library (corecrt) rather than the higher level library (ucrt) which can be helpful in mordularising the SDK.


Full diff: https://github.com/llvm/llvm-project/pull/131668.diff

1 Files Affected:

  • (modified) clang/lib/Headers/mm_malloc.h (+1-1)
diff --git a/clang/lib/Headers/mm_malloc.h b/clang/lib/Headers/mm_malloc.h
index d32fe59416277..4052995fff5cd 100644
--- a/clang/lib/Headers/mm_malloc.h
+++ b/clang/lib/Headers/mm_malloc.h
@@ -13,7 +13,7 @@
 #include <stdlib.h>
 
 #ifdef _WIN32
-#include <malloc.h>
+#include <corecrt_malloc.h>
 #else
 #ifndef __cplusplus
 extern int posix_memalign(void **__memptr, size_t __alignment, size_t __size);

@compnerd compnerd added the clang Clang issues not falling into any other category label Mar 17, 2025
@compnerd compnerd requested review from AaronBallman and rnk March 17, 2025 20:33
@github-actions
Copy link

github-actions bot commented Mar 17, 2025

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff HEAD~1 HEAD --extensions h -- clang/lib/Headers/mm_malloc.h
View the diff from clang-format here.
diff --git a/clang/lib/Headers/mm_malloc.h b/clang/lib/Headers/mm_malloc.h
index addb598f6..93c98bae2 100644
--- a/clang/lib/Headers/mm_malloc.h
+++ b/clang/lib/Headers/mm_malloc.h
@@ -13,11 +13,11 @@
 #include <stdlib.h>
 
 #ifdef _WIN32
-# if defined(__MINGW32__)
-#   include <malloc.h>
-# else
-#   include <corecrt_malloc.h>
-# endif
+#if defined(__MINGW32__)
+#include <malloc.h>
+#else
+#include <corecrt_malloc.h>
+#endif
 #else
 #ifndef __cplusplus
 extern int posix_memalign(void **__memptr, size_t __alignment, size_t __size);

@compnerd compnerd requested a review from mstorsjo March 17, 2025 20:52
Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

Looks ok to me.

I don't know about whether this really works as expected with corecrt_malloc.h or not, but I would expect you to have tested it.

We could look into adding corecrt_malloc.h in mingw-w64 as well, and then we could simplify this after a couple of years maybe.

@compnerd
Copy link
Member Author

Looks ok to me.

I don't know about whether this really works as expected with corecrt_malloc.h or not, but I would expect you to have tested it.

We could look into adding corecrt_malloc.h in mingw-w64 as well, and then we could simplify this after a couple of years maybe.

Right. I'm not 100% certain if it fully solves the issue that I am encountering yet, however, this seems like a good thing to do nonetheless.

The header inclusion is present to avoid forward declaring _aligned_malloc and _aligned_free. Both of these are declared in the header thusly:

_ACRTIMP
void __cdecl _aligned_free(
    _Pre_maybenull_ _Post_invalid_ void* _Block
    );

_Check_return_ _Ret_maybenull_ _Post_writable_byte_size_(_Size)
_ACRTIMP _CRTALLOCATOR _CRTRESTRICT
void* __cdecl _aligned_malloc(
    _In_ _CRT_GUARDOVERFLOW size_t _Size,
    _In_                    size_t _Alignment
    );

It is easy for us to miss some of the annotations as corecrt evolves which is why the header inclusion is preferable to retain if we can manage.

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

The CI failure on Windows looks potentially related. Also, can you validate that this change actually solves the issue for you regarding modules?

@compnerd
Copy link
Member Author

compnerd commented Apr 3, 2025

I don't think that this fully fixes the issue as it doesn't completely break the cycle.

This allows us to target a lower level library (corecrt) rather than the
higher level library (ucrt) which can be helpful in modularising the
SDK.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category platform:windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants