Skip to content

[clang] "modular_format" attribute for functions using format strings #147431

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.

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

Draft
wants to merge 3 commits into
base: users/mysterymath/modular-printf/ir
Choose a base branch
from

Conversation

mysterymath
Copy link
Contributor

@mysterymath mysterymath commented Jul 8, 2025

This provides a C language modular_format attribute. This combines with information from the existing format to set the new IR modular-format attribute.

The purpose of these attributes is to enable "modular printf". A statically linked libc can provide a modular variant of printf that only weakly references implementation routines. Regular printf would strongly reference those routines, and the compiler would transform calls with constant format strings to calls to the modular printf, along with strong references to aspect symbols that bring in those aspects.

See issue #146159 for context.

@mysterymath mysterymath requested a review from AaronBallman July 8, 2025 00:02
@mysterymath
Copy link
Contributor Author

mysterymath commented Jul 8, 2025

Sending this out as a draft to obtain some early feedback about the direction of the implemenation of the Modular Printf RFC.

Prev PR: #147429
Next PR: #147426

This provides a C language version of the new IR modular-format
attribute. This, in concert with the format attribute, allows a library
function to declare that a modular version of its implementation is
available.

See issue #146159 for context.
@mysterymath mysterymath force-pushed the users/mysterymath/modular-printf/clang branch from 619dfb7 to e77a856 Compare July 8, 2025 22:16
@AaronBallman AaronBallman requested a review from erichkeane July 16, 2025 12:20
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.

One thing that would help me is if the PR came with tests so we could see examples of its usage. (The docs could use examples as well.) I'm having a bit of a hard time understanding the attribute and its effects.


The second argument is a implementation name, and the remaining arguments are
aspects of the format string for the compiler to report. If the compiler does
not understand a aspect, it must summarily report that the format string has
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
not understand a aspect, it must summarily report that the format string has
not understand an aspect, it must summarily report that the format string has

not understand a aspect, it must summarily report that the format string has
that aspect.

The compiler reports an aspect by issing a relocation for the symbol
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The compiler reports an aspect by issing a relocation for the symbol
The compiler reports an aspect by issuing a relocation for the symbol

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

No real comments here. I still don't really understand what this is for, which tells me we probably need some additional work on the commit message and documentation.

@@ -2569,6 +2569,18 @@ void CodeGenModule::ConstructAttributeList(StringRef Name,

if (TargetDecl->hasAttr<ArmLocallyStreamingAttr>())
FuncAttrs.addAttribute("aarch64_pstate_sm_body");

if (auto *ModularFormat = TargetDecl->getAttr<ModularFormatAttr>()) {
// TODO: Error checking
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a heck of a TODO :) Though, I'd expect us to do diagnostics during our normal checking of the format string, so we shouldn't really require anything here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, fair; this is very much a Draft PR. My intent was to get this in front of a bunch of eyes sooner rather than later, as this PR set touches everything every layer from the compiler through to libc (skipping the linker).

@mysterymath
Copy link
Contributor Author

One thing that would help me is if the PR came with tests so we could see examples of its usage. (The docs could use examples as well.) I'm having a bit of a hard time understanding the attribute and its effects.

Very fair; I was relying a lot on the tracking issue and RFC discussion for context. I've added some meat to the PR description, and I've added a brief example to the attribute docs. I'll add a test once I get the chance.

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