Skip to content

Add type-aware allocator functions to the default allocator#87

Open
QuietMisdreavus wants to merge 8 commits intogfmfrom
vgm/typed-allocators
Open

Add type-aware allocator functions to the default allocator#87
QuietMisdreavus wants to merge 8 commits intogfmfrom
vgm/typed-allocators

Conversation

@QuietMisdreavus
Copy link
Copy Markdown

Resolves rdar://170007028

This PR adapts swift-cmark to use type-aware memory allocators in its default allocator set. This required a significant divergence from upstream, due to the requirements for the automatic typed-memory allocator annotations: They can't be applied to function pointers. This means that all the calls to cmark_mem methods needed to be reworked into standalone functions so that the swap for the type-aware version could occur.

Implementation

This PR adds two new fields to cmark_mem: calloc_typed and realloc_typed, that take the same arguments as their untyped counterparts, plus a new cmark_malloc_type_id that denotes a type ID. When typed memory operations are enabled, this is typedef'd to malloc_type_id_t for compatibility with the system type-aware allocators; otherwise it is typedef'd to unsigned long long, which is the current underlying type on macOS and is selected to ensure they are the same size.

In addition, there is a new mem.h header that defines five functions. These are freestanding functions that do nothing but call their associated method in the given cmark_mem instance, but the calloc and realloc functions have an annotation to allow for the typed-memory allocator substitution to occur so that the type-aware allocators can be called when available. (They also introduce a handful of convenience macros to make calling them easier, and ensures that type information is available since they always use the functions with a sizeof and a cast.)

To facilitate the use of these methods and to increase the security of swift-cmark, i've updated the CMake configuration and the Swift package spec to include the -ftyped-memory-operations flag when available, as well as the -Wallocator-wrappers flag (which still triggers on these allocator wrappers due to a bug, heh). I limited the SwiftPM updates to a new package spec that requires Swift 6.3, since my testing with 6.2 indicated that its associated Clang didn't have both flags available. I was able to build it with the Xcode 26.4 beta and it seems like everything works, so unless there's something wrong on non-Apple platforms we should be able to keep it.

Finally, the biggest thing this PR does is replace all the calls to cmark_mem methods to use the mem.h freestanding functions, so that the type-aware memory allocators can be used throughout the library. (I added a freestanding function for free even though we don't have a type-aware implementation of that - there is a malloc_type_free in the headers on macOS but i'm not sure how to wire that one up. 😅)

in order for the typed memory operations use of typed allocators to
work, calls to allocator functions need to be decorated with the
_MALLOC_TYPED attribute. since cmark accesses its allocator wrappers
through function pointers and the cmark_mem struct, this commit adds the
groundwork necessary to allow the typed allocator wrappers to be called
correctly.
Copy link
Copy Markdown

@ojhunt ojhunt left a comment

Choose a reason for hiding this comment

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

This appears to be set up to work correctly with the typed allocators, but I can't comment on the style/design, etc for swift/cmark itself

Comment thread src/cmark.c Outdated
}

static void *xrealloc(void *ptr, size_t size) {
static void *xrealloc(void *ptr, size_t size) CMARK_MALLOC_TYPED(xrealloc_typed, 2) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These methods are all internal to this TU, so the inference attributes aren't needed, as any inference here is going to just produce opaque type anyway.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I was hoping that by adding the inference attribute it would silence the allocator-wrappers warning, but it seems like the Clang that's in the Xcode 26.4 beta is firing on them anyway. I agree that they're not useful here, so i can take them out.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Where is it firing?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

All of the allocator wrapper functions are still generating a warning, with or without the annotation:

$ swift build 
[1/1] Planning build
Building for debugging...
/Users/misdreavus/git/gh-docc/swift-cmark/src/cmark.c:26:14: warning: function xcalloc_typed might be an allocator wrapper [-Wallocator-wrappers]
   26 | static void *xcalloc_typed(size_t nmem, size_t size, cmark_malloc_type_id type_id) {
      |              ^
/Users/misdreavus/git/gh-docc/swift-cmark/src/cmark.c:41:3: note: value obtained from allocator call is returned here
   41 |   return ptr;
      |   ^
/Users/misdreavus/git/gh-docc/swift-cmark/src/cmark.c:47:14: warning: function xrealloc_typed might be an allocator wrapper [-Wallocator-wrappers]
   47 | static void *xrealloc_typed(void *ptr, size_t size, cmark_malloc_type_id type_id) {
      |              ^
/Users/misdreavus/git/gh-docc/swift-cmark/src/cmark.c:62:3: note: value obtained from allocator call is returned here
   62 |   return new_ptr;
      |   ^
/Users/misdreavus/git/gh-docc/swift-cmark/src/cmark.c:82:14: warning: function xcalloc might be an allocator wrapper [-Wallocator-wrappers]
   82 | static void *xcalloc(size_t nmem, size_t size) {
      |              ^
/Users/misdreavus/git/gh-docc/swift-cmark/src/cmark.c:88:3: note: value obtained from allocator call is returned here
   88 |   return ptr;
      |   ^
/Users/misdreavus/git/gh-docc/swift-cmark/src/cmark.c:91:14: warning: function xrealloc might be an allocator wrapper [-Wallocator-wrappers]
   91 | static void *xrealloc(void *ptr, size_t size) {
      |              ^
/Users/misdreavus/git/gh-docc/swift-cmark/src/cmark.c:97:3: note: value obtained from allocator call is returned here
   97 |   return new_ptr;
      |   ^
4 warnings generated.
[45/45] Applying cmark-gfm-bin
Build complete! (1.67s)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ah, this is a case where you'll want to suppress the warnings, eg. something like

#define START_TYPE_ALLOCATOR_IMPL \
  _Pragma(clang diagnostic push) \
  _Pragma(clang diagnostic ignored "-Wallocator-wrappers")
#define END_TYPE_ALLOCATOR_IMPL
  _Pragma(clang diagnostic pop)

Syntax subject coding in GitHub comments

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Cool, if i can get away with just suppressing the warning right there, then that works for me. I've added it, thanks!

Comment thread src/arena.c Outdated
}

cmark_mem CMARK_ARENA_MEM_ALLOCATOR = {arena_calloc, arena_realloc, arena_free};
cmark_mem CMARK_ARENA_MEM_ALLOCATOR = {arena_calloc, arena_realloc, arena_free, 0, 0};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

rather than zero initializing, it might be worth considering:

static void *arena_calloc_typed(...) {
  return arena_calloc(...);
}
...
cmark_mem CMARK_ARENA_MEM_ALLOCATOR = {arena_calloc, arena_realloc, arena_free, &arena_calloc_typed, ...};

Comment thread src/include/mem.h Outdated
Comment on lines +30 to +31
#define CMARK_CALLOC(MEM, TYPE, COUNT) (TYPE *)cmark_mem_calloc(MEM, COUNT, sizeof(TYPE))
#define CMARK_MALLOC(MEM, TYPE) CMARK_CALLOC(MEM, TYPE, 1)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm unsure I understand the reason for these macros? where is inference currently failing? (it's always a possibility, so we like to accumulate failure cases so we can try to support them)

e.g. cmark_mem_calloc(...) should automatically be achieving the same behavior as this macro

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh, the only reason for these macros is pure laziness on my part. 😅 I wanted to use the type name both for a cast and for a sizeof call, and i felt like that repetition was too annoying to deal with. Also it helped me fix an issue in a couple places where i had the arguments to calloc backwards.

Copy link
Copy Markdown

@snprajwal snprajwal left a comment

Choose a reason for hiding this comment

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

Mostly looks good, some minor concerns. Thanks for working on this!

Comment thread src/include/mem.h Outdated
void *cmark_mem_calloc(cmark_mem *mem, size_t count, size_t size) CMARK_MALLOC_TYPED(cmark_mem_calloc_typed, 3);

#define CMARK_CALLOC(MEM, TYPE, COUNT) (TYPE *)cmark_mem_calloc(MEM, COUNT, sizeof(TYPE))
#define CMARK_MALLOC(MEM, TYPE) CMARK_CALLOC(MEM, TYPE, 1)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This looks misleading to me, I wouldn't expect a *_MALLOC macro to expand into a calloc call. Something like CMARK_CALLOC_ONE might be better.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I had considered them to be functionally equivalent, but i guess i'm not aware of the finer differences between the allocator functions. I've renamed it.

Comment thread src/include/mem.h
Comment on lines +17 to +25
CMARK_GFM_EXPORT
void *cmark_mem_calloc_typed(cmark_mem *mem, size_t count, size_t size, cmark_malloc_type_id type_id);

#define cmark_mem_calloc_typed_backdeploy cmark_mem_calloc_typed

CMARK_GFM_EXPORT
void *cmark_mem_realloc_typed(cmark_mem *mem, void *ptr, size_t size, cmark_malloc_type_id type_id);

#define cmark_mem_realloc_typed_backdeploy cmark_mem_realloc_typed
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Both these functions are gated behind _MALLOC_TYPE_ENABLED in mem.c, they would be declared but undefined symbols on platforms that don't define the macro.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Oh right, i didn't account for when the feature isn't present. 🤦‍♀️ I can write up fallbacks so that these symbols resolve correctly.

Comment thread src/include/cmark-gfm.h
void *(*realloc)(void *, size_t);
void (*free)(void *);
void *(*calloc_typed)(size_t, size_t, cmark_malloc_type_id);
void *(*realloc_typed)(void *, size_t, cmark_malloc_type_id);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

General note: this breaks ABI

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Right, i didn't know a good way to handle this without either breaking ABI here or breaking the API massively somewhere else. I think this was the least-bad option to take due to how swift-cmark is itself used (either as a Swift package as part of Swift-Markdown or built live into another tool, rather than being distributed as a dylib), but if there's a way to preserve ABI without also messing up the cmark_get_*_allocator functions and the types that take the result, i'd love to hear it.

Now that the wrappers are added to the type-aware allocator machinery,
we can silence the allocator wrapper warning (which still fires here) to
prevent future false-positives.
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.

3 participants