Skip to content

Add format attribute to printf-style wrappers and fix format string errors - #123920

Merged
jkoritzinsky merged 75 commits into
mainfrom
copilot/add-format-attribute-to-wrappers
Aug 14, 2026
Merged

jkoritzinsky merged 75 commits into
mainfrom
copilot/add-format-attribute-to-wrappers

Conversation

Copilot AI commented Feb 2, 2026 •

Copy link
Copy Markdown
Contributor

Description

Enabled compiler validation of printf-style format strings by adding __attribute__ ((format (printf, ...))) to wrapper functions. Fixed 85+ format string errors discovered by the compiler across CoreCLR, Mono, and Corehost. Merged with latest main branch to ensure compatibility with recent changes.

Changes

Centralized format attribute macro in src/native/minipal/types.h:

  • Added MINIPAL_ATTR_FORMAT_PRINTF(fmt_pos, arg_pos) macro for consistent usage across the codebase
  • Eliminates need for duplicated #ifdef __GNUC__ blocks throughout the codebase
  • Does not redefine standard C99 PRI macros - relies entirely on system inttypes.h

Added format attributes to 60+ wrapper functions across 13 headers using MINIPAL_ATTR_FORMAT_PRINTF:

  • src/native/minipal/types.h - Centralized MINIPAL_ATTR_FORMAT_PRINTF macro definition
  • src/native/minipal/log.h - minipal_log_print
  • src/native/corehost/hostmisc/trace.h - trace::verbose, info, warning, error, println
  • src/native/libs/Common/pal_compiler.h - do_abort_unless
  • src/native/libs/System.Native/pal_string.h - SystemNative_SNPrintF
  • src/native/libs/System.Security.Cryptography.Native/osslcompat_30.h - ERR_set_error (uses shared MINIPAL_ATTR_FORMAT_PRINTF macro)
  • src/coreclr/inc/log.h - LogSpew, LogSpew2, LogSpewAlways
  • src/coreclr/inc/stresslog.h - StressLog::LogMsg, ThreadStressLog::LogMsg
  • src/coreclr/inc/sstring.h - Printf, AppendPrintf
  • src/coreclr/jit/host.h - jitprintf, logf, flogf, gcDump_logf
  • src/coreclr/jit/compiler.h - printfAlloc, JitLogEE
  • src/coreclr/gc/gc.h - GCLog
  • src/coreclr/gc/gcpriv.h - GCLogConfig
  • src/mono/mono/eglib/glib.h - g_error_new, g_set_error, g_print, g_printerr, g_log, g_assertion_message, g_async_safe_*

Fixed 85+ format string errors across 24 source files:

Error Type Count Fix
Platform-specific format codes 16 %I64d/%Id → %zd/%zu
uint64_t formatting 19 Use standard PRIX64/PRIx64/PRIu64 from <inttypes.h>
Invalid %p flags 4 Removed # and 0 flags
Missing pointer casts 7 Added (void*) cast for pointers
Format-security warnings 24 printf(str) → printf("%s", str)
Member function attributes 1 Adjusted positions for implicit this
Miscellaneous type mismatches 14+ Fixed various format/argument type mismatches

Files with format errors fixed:

  • JIT: emit.cpp, emitwasm.cpp, emitxarch.cpp, emitarm.cpp, emitarm64.cpp, emitarm64sve.cpp, codegencommon.cpp, jitinterface.cpp, gentree.cpp, inlinepolicy.cpp
  • VM: perfmap.cpp, stubgen.cpp, crst.cpp, excep.cpp, dynamicmethod.cpp, stubmgr.cpp, gcheaputilities.cpp, threadsuspend.cpp, amd64/excepamd64.cpp
  • GC: gc.cpp, diagnostics.cpp
  • Corehost: sdk_resolver.cpp, hostpolicy_init.cpp, hostpolicy.cpp, hostpolicy_context.cpp, deps_format.cpp, nethost.cpp, bundle/reader.h
  • Mono: mono-threads-state-machine.c, mono-threads.c, mono-os-mutex.c
  • Tools: ildasm/dasm.cpp, metainfo/mdinfo.cpp
  • Debug: di/rsthread.cpp, debug/ee/arm/walker.cpp, debug/ee/controller.cpp

Added missing header includes:

  • src/coreclr/ildasm/ildasmpch.h - Added #include <inttypes.h> for PRI macros
  • src/coreclr/tools/metainfo/mdinfo.cpp - Added #include <inttypes.h> for PRI macros
  • src/coreclr/vm/jitinterface.cpp - Added #include <inttypes.h> for PRI macros
  • src/coreclr/jit/gentree.cpp - Added #include <inttypes.h> for PRI macros

Portability for PRI macros in C++: Added guarded __STDC_FORMAT_MACROS define before <inttypes.h> includes in C++ translation units that use PRI* format macros (emitwasm.cpp, jitinterface.cpp, stubgen.cpp, mdinfo.cpp, ildasmpch.h, arm64/loongarch64/riscv64 singlestepper.cpp). This keeps PRI macros visible on older C++ libc implementations (notably glibc) where they are gated behind __STDC_FORMAT_MACROS in C++ mode.

Merged from main:

  • Successfully merged latest changes from origin/main
  • Verified clean build with 0 errors and 0 warnings
  • All format attributes remain compatible with latest codebase changes

Review feedback addressed:

  • src/coreclr/vm/crst.cpp: Simplified format strings to avoid unnecessary line splits
  • src/coreclr/gc/diagnostics.cpp: Removed unnecessary size_t casts (type already size_t)
  • Spurious indentation changes: Restored original indentation in excep.cpp, emitxarch.cpp, dynamicmethod.cpp, and stubmgr.cpp to keep diff clean
  • Format/cast mismatch: Fixed emitxarch.cpp line 12477 to use %zu instead of %zd for unsigned size_t
  • Format error in jitinterface.cpp: Reverted incorrect %lld back to %d for INT32 type
  • Unnecessary blank lines: Removed two blank lines in CMakeLists.txt
  • 32-bit ARM build failure: Fixed gentree.cpp, compiler.cpp, error.cpp, morph.cpp, and codegencommon.cpp by replacing empty printf("") calls with fflush(stdout) to fix GCC format-zero-length errors on linux.armel.Checked
  • Latest CI/review-feedback follow-ups:
    • src/coreclr/vm/gcheaputilities.cpp: Log invalid GC module name as UTF-8 string (MAKE_UTF8PTR_FROMWIDE + %s) instead of pointer (%p) so the diagnostic shows the actual name
    • src/coreclr/jit/inlinepolicy.cpp: Print m_ModelCodeSizeEstimate for the size= label (the original code had a label/value mismatch where the per-call instruction estimate was being printed under the size label)
    • src/native/corehost/hostpolicy/hostpolicy_init.cpp: Changed %zd → %zu for size_t input->version_lo in two locations
    • src/mono/mono/utils/mono-os-mutex.c: Cast ts.tv_sec to long long for %lld and ts.tv_nsec to long for %ld in both pthread_cond_timedwait and pthread_cond_timedwait_relative_np branches
    • src/native/libs/System.Security.Cryptography.Native/osslcompat_30.h: Replaced inline #ifdef __GNUC__ block on ERR_set_error with the shared MINIPAL_ATTR_FORMAT_PRINTF macro and added #include <minipal/types.h>
    • src/native/corehost/bundle/reader.h: Cast int64_t m_offset_in_file to unsigned long long to match %llx (avoids varargs UB)
    • src/coreclr/vm/stubgen.cpp: Use 0x%zx/0x%08zx with (size_t) cast for UINT_PTR pInstruction->uArg to fix Win64 truncation (where unsigned long is 32-bit)
    • src/coreclr/vm/threadsuspend.cpp:4312 and src/coreclr/vm/amd64/excepamd64.cpp:197: Removed redundant 0x literal prefix before %p format specifier (which already includes 0x on most platforms, resulting in 0x0x... output)
    • src/coreclr/debug/di/rsthread.cpp:692: Added explicit (size_t) cast for UINT_PTR m_id used with %zx format specifier
    • src/tools/ilasm: Reverted accidental changes to auto-generated C# files that had introduced Windows-specific absolute paths into // Generated from comments

Example fixes:

// Before: Platform-specific, requires #ifdef blocks
#ifdef TARGET_64BIT
printf("%lu", (unsigned long)uint64_value);
#else
printf("%llu", (unsigned long long)uint64_value);
#endif

// After: Portable using standard inttypes.h macros
#include <inttypes.h>
printf("%" PRIX64, uint64_value);

// Before: Format-security warning
printf(sstr);

// After: Safe format string
printf("%s", sstr);

// Before: Platform-specific
printf("%I64d", ssize_value);

// After: Portable
printf("%zd", (size_t)ssize_value);

// Before: pthread_t with wrong format
printf("thread %d", pthread_id);

// After: Pointer type with correct format
printf("thread %p", (void*)pthread_id);

// Before: GCC format-zero-length error (linux.armel.Checked)
printf(""); // null string means flush

// After: Explicit flush
fflush(stdout); // flush

Impact

  • Security: Eliminated 24 format-security vulnerabilities where non-literal strings were used as format arguments
  • Code quality: Removed 170+ lines of duplicated #ifdef __GNUC__ blocks via centralized MINIPAL_ATTR_FORMAT_PRINTF macro (now also used by osslcompat_30.h); restored original indentation to keep git history clean; reverted accidental changes to auto-generated src/tools/ilasm files
  • Portability: Replaced platform-specific format codes with standard C99/POSIX specifiers; uses system inttypes.h macros without any custom redefinition; added guarded __STDC_FORMAT_MACROS before <inttypes.h> in C++ files for older libc compatibility; fixed UINT_PTR formatting in stubgen for Win64 (where unsigned long is 32-bit); fixed builds across x64, ARM64, and 32-bit ARM platforms
  • Correctness: Fixed type mismatches causing crashes/incorrect output on 64-bit platforms and macOS; corrected format/cast mismatches; restored a useful diagnostic in gcheaputilities.cpp (UTF-8 module name instead of pointer); fixed a pre-existing label/value mismatch in inlinepolicy.cpp so the size= log label now reports the model code size; removed redundant 0x prefixes before %p specifiers that were producing 0x0x... output
  • Maintainability: Format errors now caught at compile time with consistent macro usage across CoreCLR, Mono, and Corehost; verified compatibility with latest main branch
  • Compatibility: No macro redefinition conflicts with Windows MSVC, Android NDK, or other standard library implementations; all files using PRI macros include proper headers

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 5 commits February 2, 2026 23:48
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Comment thread src/coreclr/jit/emit.cpp Outdated
Copilot AI changed the title [WIP] Add format attribute to printf-style API wrappers Add format attribute to printf-style wrappers and fix format string errors Feb 3, 2026
Copilot AI requested a review from jkoritzinsky February 3, 2026 00:20
Comment thread src/coreclr/jit/emitxarch.cpp Outdated
Comment thread src/native/minipal/log.h Outdated
Comment thread src/native/minipal/log.h Outdated
@jkotas

jkotas commented Feb 3, 2026

Copy link
Copy Markdown
Member

@copilot

Comment thread src/coreclr/jit/emitxarch.cpp Outdated
Copilot AI and others added 2 commits February 3, 2026 00:55
…, str) pattern

Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 10 pipeline(s).
6 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 210 out of 210 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/coreclr/vm/peimagelayout.cpp:304

  • This LOG call uses %p but passes GetPreferredBase()/GetBase() without casting to a pointer type. In this file these values are treated as pointer-sized integers (e.g., preferredBase = (void*)GetPreferredBase()), so this will trip format checking (and is UB). Cast to void* at the call site.
    src/coreclr/debug/di/divalue.cpp:813
  • Same as the constructor: the explicit "0x" prefix with %p will commonly result in "0x0x..." output. Drop the literal prefix here as well.
    LOG((LF_CORDB,LL_EVERYTHING,"CRV::~CRV: this:0x%p\n",this));

src/coreclr/inc/stresslog.h:316

  • StressLog::LogMsg stores varargs based on cArgs. LogMsgOL currently passes cArgs=0 but also passes one vararg ("%s", format), so the argument is not stored and later formatting of "%s" will read a missing argument (undefined behavior / corrupted stress log output).
    src/coreclr/debug/di/divalue.cpp:744
  • Using the literal prefix "0x" with %p will typically produce duplicated prefixes ("0x0x...") on platforms where %p already includes 0x. This file only has these two occurrences (constructor and destructor); consider dropping the explicit "0x" in both places for consistent output.

This issue also appears on line 813 of the same file.

    LOG((LF_CORDB,LL_EVERYTHING,"CRV::CRV: this:0x%p\n",this));

Comment thread src/coreclr/dlls/mscorpe/pewriter.cpp Outdated
Comment thread src/coreclr/jit/compiler.cpp Outdated
Comment thread src/coreclr/jit/compiler.cpp Outdated
Comment thread src/coreclr/vm/i386/cgenx86.cpp

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 210 out of 210 changed files in this pull request and generated 1 comment.

Comment thread src/coreclr/jit/codegencommon.cpp Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 210 out of 210 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/coreclr/vm/generics.cpp:724

  • The new %p format specifiers now require void* arguments under MINIPAL_ATTR_FORMAT_PRINTF; passing PTR_Module/MethodTable* directly will trigger format warnings (and defeats the goal of enabling compiler validation). Cast the pointer arguments to void* (use static_cast<void*> in C++).
    src/coreclr/vm/methodtablebuilder.cpp:7096
  • With MINIPAL_ATTR_FORMAT_PRINTF enabled on logging helpers, %p expects a void*. Passing MethodDesc* directly will trigger format warnings. Cast the pointer to void* (prefer static_cast<void*> in C++).

Comment thread src/coreclr/jit/error.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 210 out of 210 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/coreclr/debug/di/divalue.cpp:813

  • Same as above: this:0x%p will commonly log 0x0x... because %p already includes the 0x prefix on most platforms. Dropping the extra 0x avoids noisy diagnostics.
    LOG((LF_CORDB,LL_EVERYTHING,"CRV::~CRV: this:0x%p\n",this));

src/coreclr/gc/interface.cpp:749

  • The dprintf format string uses %zd for min_gen0_new_allocation/max_gen0_new_allocation, but those fields are size_t (see gcpriv.h), so the correct specifier is %zu. With the new printf-format attributes this can produce warnings or UB on some platforms.
    src/coreclr/debug/di/divalue.cpp:744
  • %p typically already includes the 0x prefix. Keeping this:0x%p will often log 0x0x..., which is the exact issue this PR fixes elsewhere. Consider dropping the redundant 0x prefix for pointer formatting.

This issue also appears on line 813 of the same file.

    LOG((LF_CORDB,LL_EVERYTHING,"CRV::CRV: this:0x%p\n",this));

@jkotas

jkotas commented Aug 14, 2026 •

Copy link
Copy Markdown
Member

@copilot Replate all jitsdout typos in the PR changes with jitstdout to fix build breaks

Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>

Copilot AI commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Replate all jitsdout typos in the PR changes with jitstdout to fix build breaks

Fixed all five jitsdout typos in c4507d333. The CI build also exposed a missing <inttypes.h> include in valuenum.cpp; that is fixed in the same commit. ./build.sh clr+libs+host succeeds with 0 warnings and 0 errors.

Note

This reply was generated by GitHub Copilot.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 210 out of 210 changed files in this pull request and generated no new comments.

@jkoritzinsky

Copy link
Copy Markdown
Member

/ba-g wasm deadletter unrelated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants