This is an archive of the discontinued LLVM Phabricator instance.

-Wformat-nonliteral should not trigger for format strings passed to blocks with __attribute__((format))
ClosedPublic

Authored by fcloutier on Oct 26 2021, 12:14 PM.

Details

Summary

The checker that implements -Wformat-nonliteral does not understand __attribute__((format)) on blocks in the same way that it understands it on functions. This works just fine (assuming #define __printflike(A, B) __attribute__((format(printf, A, B)))):

void printfblock(const char *fmt, ...) __printflike(1, 2) {
	va_list ap;
	va_start(ap, fmt);
	vprintf(fmt, ap);
	va_end(ap);
}

void foo(void) {
	printfblock("%s %i\n", "hello", 1);
}

However, this incorrectly triggers -Wformat-nonliteral:

void foo(void) {
	void (^printfblock)(const char *fmt, ...) __printflike(1, 2) = ^(const char *fmt, ...) __printflike(1, 2) {
		va_list ap;
		va_start(ap, fmt);
		vprintf(fmt, ap); // warning: format string is not a string literal [-Wformat-nonliteral]
		va_end(ap);
	};
	
	printfblock("%s %i\n", "hello", 1);
}

This patch updates checkFormatStringExpr so that it can look through BlockDecls and find out which parameter is identified as a format string.

rdar://84603673

Diff Detail

Event Timeline

fcloutier requested review of this revision.Oct 26 2021, 12:14 PM
fcloutier created this revision.
NoQ added a subscriber: NoQ.Oct 26 2021, 12:24 PM
NoQ added inline comments.
clang/lib/Sema/SemaChecking.cpp
7787

This code now says that all block parameters have index 1 because blocks aren't named(?) I don't think this is correct. I also don't see why this cast is necessary in the first place. The only place where ND is used is the cast into MD which might as well be performed on D directly.

fcloutier updated this revision to Diff 382461.Oct 26 2021, 2:16 PM

Thanks Artem for pointing out that I was completely misusing getFunctionScopeIndex. This should be better. I added a test that you can pick a non-1 value for the format parameter in blocks.

ahatanak added inline comments.
clang/lib/Sema/SemaChecking.cpp
7792

Isn't changing dyn_cast<NamedDecl to dyn_cast<Decl> enough to avoid the warning? Just wondering why it is also necessary to move the above code out of the for loop. Actually, it's not clear to me why PVIndex had to be incremented in the loop in the first place.

fcloutier marked an inline comment as done.Oct 26 2021, 10:11 PM
fcloutier updated this revision to Diff 382788.Oct 27 2021, 2:13 PM
fcloutier added a reviewer: ahatanak.
This revision is now accepted and ready to land.Oct 27 2021, 2:35 PM