Skip to content

Add __attribute__((noreturn,cold)) for _mi_assert_fail #134586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
picnixz opened this issue May 23, 2025 · 1 comment
Closed

Add __attribute__((noreturn,cold)) for _mi_assert_fail #134586

picnixz opened this issue May 23, 2025 · 1 comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@picnixz
Copy link
Member

picnixz commented May 23, 2025 •
edited by bedevere-app bot
Loading

Feature or enhancement

Note

I already have a PR ready in case this is accepted

Proposal:

Because _mi_assert_fail is not marked as a noreturn function, the assertion is not understood by CLion and I have some false positives telling me "this ptr is not NULL" while just above there is mi_assert(ptr != NULL);. To improve IDE's analysis, I suggest to mark _mi_assert_fail with __attribute__((noreturn)) and __THROW for GCC, the same attributes that are put on __assert_fail:

extern void __assert_fail (const char *__assertion, const char *__file,
			   unsigned int __line, const char *__function)
     __THROW __attribute__ ((__noreturn__));

cc @colesbury

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere.

Linked PRs
@picnixz picnixz added type-feature A feature request or enhancement interpreter-core (Objects, Python, Grammar, and Parser dirs) labels May 23, 2025
@thesamesam
Copy link
Contributor

You could mark it cold as well while at it. The equivalent in glibc has that with latest versions (f6f904745942540c7ea99c403b75b695ee560035, e3b0b3484cac61f9eae373751ac6eaf816a2c9c3).

@picnixz picnixz changed the title Add __attribute__((noreturn)) for _mi_assert_fail Add __attribute__((noreturn,cold)) for _mi_assert_fail May 24, 2025
@bedevere-app bedevere-app bot mentioned this issue May 24, 2025
Merged
picnixz added a commit that referenced this issue May 30, 2025
@picnixz
gh-134586: mark _mi_assert_fail as noreturn, cold and throw (#…
c600310 
…134624)

We add the following attributes on `_mi_assert_fail` to help IDE introspection:

* `__attribute__((__noreturn__))`
* `__attribute__((cold))`
* `__THROW` (GCC only)
@picnixz picnixz closed this as completed May 30, 2025
@picnixz picnixz mentioned this issue May 30, 2025
Closed
daanx added a commit to daanx/mimalloc that referenced this issue May 30, 2025
@daanx
mark assert_fail as cold and noreturn; move assert to internal.h (see…
715acc0 
… issue microsoft#1091, and python/cpython#134586)
clrpackages pushed a commit to clearlinux-pkgs/mimalloc that referenced this issue Jun 11, 2025
@fenrus75
mimalloc: Autospec creation for update from version 3.0.3 to version …
31a2ac7 
…3.1.4

Daan (53):
      bump version to 1.9.4 for further development
      update readme
      fix release build warning (unused mi_stat_total_print)
      fix dynamic override test on non-windows platforms
      fix invalid pointer detection in release mode (issue #1051 and #1053)
      fix dynamic override test on non-windows platforms
      make dynamic override test verbose
      nicer cmake logic for windows override test
      add comment
      fix alpine compilation with prctl.h (issue #1059)
      typo
      fix prctl.h includes for alpine linux/musl (hopefully fixes #1065, #1066, #1067)
      validate pointer before assertion in mi_free_size (issue #754)
      fix assertion in mi_free_size (issue #754)
      fix cast on msvc
      fix mi_ctz/clz/bsf/bsr to avoid msvc compilation bug (issue #1071, pr #1081)
      revert use of selectany for msvc (issue #1078)
      add fixed TLS slot test to pipeline on Windows
      fix debug assertion for windows TLS
      make windows fixed TLS opt-in
      fix syntax error
      fix stats for pages and page_bins
      merge 3301ba0 to fix page_bin and pages statistics
      ensure page stats are done on the subproc stats in case the tld is NULL
      merge 3b567b1 to track pages statistics correctly
      fix merge conflict
      fix output for api test
      fix api test for 32-bits
      fix format specifier for numa nodes
      fix format specifier in stat output
      fix missing csize assignment in _mi_os_free_ex
      check all os_commit calls and return NULL on failure
      fix base address if commit fails on aligned overallocation
      fix missing csize assignment in _mi_os_free_ex
      check all os_commit calls and return NULL on failure
      fix base address if commit fails on aligned overallocation
      fix windows 32-bit compilation
      fix merge error
      check return value of mi_arena_commit, potential fix for #1098
      use main instead of master in readme
      add initial support for _mi_prim_reuse and MADV_FREE_REUSABLE on macOS (issue #1097)
      add MI_UNUSED for unix _mi_prim_reuse
      fall back to MADV_DONTNEED if MADV_FREE_REUSABLE fails on macOS; disable use of MADV_FREE_REUSE on a reset (issue #1097)
      add _mi_os_zalloc
      fix assertion in mi_os_ensure_zero
      fix assertion
      improve handling of commit failure in arena allocation
      make macOS interposes compile for older macOS versions (by @Noxybot, PR #1028)
      fix link error without static library build (by @fd00, PR #1082)
      fix build for TSAN tests
      fix armv7 detection
      update vcpkg config to 1.9.4
      update readme for upcoming release

Daan Leijen (3):
      change default page_reclaim_max and change reclamation test to potentially address perf regresion in mimalloc v3 with respect to v2 -- see also leanprover/lean4#7786
      add mi_process_done to the api
      do not automatically call mi_process_done if mi_option_destroy_on_exit > 1

Eduard Voronkin (1):
      fix recursion in TLS init on Android

daanx (41):
      nicer show arenas
      fix build error on msvc/clang-cl in C mode (issue #1060)
      refactor numa_node_count
      add atomic_cas_ptr_strong_acq_rel
      fix signed compare warning
      use C++ compilation with clang-cl (as well as msvc) on Windows
      fix typo
      allow size==0 for mi_prim_free (issue #1041)
      extend override test on windows
      Use second user TLS slot to avoid using reserved fields in the TEB (issue #1078)
      fix use of mi_bsr
      add unix large page size constant and adjust aligment to the large page size for large allocations
      improve TLS access on Windows with msvc (by Frank Richter, issue #1078)
      add more decl_hidden specifiers on extern variables to improve access on arm64
      improve TLS access on Windows with msvc (by Frank Richter, issue #1078)
      add more decl_hidden specifiers on extern variables to improve access on arm64
      use TlsAlloc with a dynamic offset for MI_WIN_USE_FIXED_TLS by default (issue #1078)
      fix MI_WIN_USE_FIXED_TLS conditions
      fix TLS initialization for MI_WIN_USE_FIXED_TLS with redirection
      destroy the page map as well if MIMALLOC_DESTROY_ON_EXIT is set; see issue #1041
      add page_cross_thread_max_reclaim option
      potential fix for low address allocations, issue #1087
      add alpine x86 docker file
      fix include of prctl.h on alpine linux x86
      fix format specifier (for alpine linux x86, issue #1086)
      fix guarded sample rate of 1 (issue #1085)
      add guarded TLS test for Windows fixed TLS
      refine test for allowing page reclaim on free to limit reclaim if the originating heap is part of a threadpool
      use proper atomics for the page_map
      fix valid pointer check for low addresses (#issue 1087)
      always use non-flat map in secure mode and validate pointers passed to free
      don't reset stats at process start so page_map commit is included
      get Windows version dynamically
      add _mi_stats_init to initialize the process start time
      fix OS allocation size tracking in the memid
      fix page map resolving for low addresses (issue #1087)
      mark assert_fail as cold and noreturn; move assert to internal.h (see issue #1091, and python/cpython#134586)
      define mi_decl_align separate from mi_decl_cache_align
      ensure use of subproc_main during unsafe destroy to avoid derefercing the heap
      use subproc_main for final statistics output to avoid dereferencing the heap
      enable building for xbox, based on pr #1084 by @maxbachmann
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@picnixz @thesamesam