llvm / llvm/llvm-project

[c23] incomplete support for N2680 (specific width length modifier) in format string checking

Open
#212,840 0 comments 0 reactions 0 assignees View on GitHub
c23 clang:diagnostics
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

Testcase:
```c
__attribute__((format(printf, 1, 2))) int printf(const char*, ...);

void thing(__INT64_TYPE__ n) {
printf("%w64d", n);
}
```
This should be portable: `__INT64_TYPE__` should be `int64_t`, and `int64_t` should be compatible with `%w64d`. But on 64-bit Darwin targets [we get a warning](https://godbolt.org/z/fEsrva9qc):

```console
:4:19: warning: format specifies type 'int64_t' (aka 'long') but the argument has type 'long long' [-Wformat]
4 | printf("%w64d", n);
| ~~~~~ ^
| %lld
```

Note that the warning is wrong: `int64_t` [is `long long` on this target](https://github.com/llvm/llvm-project/blob/8d404384dd842d43362732a11b233ee3d6ead023/clang/lib/Basic/Targets/AArch64.cpp#L1869).

The problem is that the target configuration appears to be broken: it sets `Int64Type` to be `SignedLongLong`, but `getIntTypeByWidth` is not overridden and [returns `SignedLong`](https://github.com/llvm/llvm-project/blob/7bed6591be3191446ec2cf92ef247487fff03a84/clang/lib/Basic/TargetInfo.cpp#L325) as the target's 64-bit signed integer type. The latter is [used by format string checking](https://github.com/llvm/llvm-project/blob/a42bed3f72c427dcd023aae169bec9305ec29ed6/clang/lib/AST/FormatString.cpp#L841) for the `w64` modifier.

It's not actually clear where the problem lies, though. The existence of `getLeastIntTypeByWidth` next to `getIntTypeByWidth` strongly implies that these are computing `intN_t` and `int_leastN_t`, but this is not explicitly documented. Some targets (for example, wasm) have comments in their overrides that imply that they interpreted these functions this way. I'm assuming the intent is for these functions to produce the `int*N_t` types rather than something that's not actually target-specific, such as the lowest-ranked type of the given size, and it's just a bug that the `Int64Type` set on the target doesn't match the type returned here. (Presumably an easy fix would be to look at `Int64Type` in `getIntTypeByWidth`.)

This issue seems to be new in C23: the Darwin system headers map (eg) `PRId64` to `lld`, so they allow `int64_t` using the old format string modifiers.

Looking through other uses of `getIntTypeByWidth`, the bug seems to be benign in most of them (either because they don't pass 64, or because they aren't reached on LP64 targets, or because the actual type doesn't matter). But interestingly our support for the GNU mode attribute seemingly has the opposite bug: https://github.com/llvm/llvm-project/blob/f86c81b2a896eb6fabda38f68eefb8ac982423cc/clang/lib/Sema/SemaDeclAttr.cpp#L4831 uses `getIntTypeForBitwidth`, but I think GCC's `__attribute__((mode(DI)))` always picks the lowest-rank type of the given width, which we don't even have a function for -- `getLeastIntTypeForBitwidth` produces `int_leastN_t`, which for 64-bit wasm is `long long` even though `long` is also 64 bits.

Contributor guide

Open the contributing guide

Research direction

Start with clang/lib/Basic/TargetInfo.cpp:getIntTypeByWidth, the AArch64 target's Int64Type definition, and clang/lib/AST/FormatString.cpp where the w64 modifier is checked. Compare the related getLeastIntTypeByWidth behavior and target overrides, including wasm, then inspect the mode(DI) use in clang/lib/Sema/SemaDeclAttr.cpp. Done means the Darwin C23 testcase accepts %w64d for __INT64_TYPE__ and regression coverage captures the intended type selection.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
compilers
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.