[PATCH] D159138: [clang][Sema] Fix format size estimator's handling of %o, %x, %X with alternative form

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet marked 4 inline comments as done.
hazohelet added a comment.

Thanks for the review!



================
Comment at: clang/docs/ReleaseNotes.rst:125
 * ``-Woverriding-t-option`` is renamed to ``-Woverriding-option``.
 * ``-Winterrupt-service-routine`` is renamed to ``-Wexcessive-regsave`` as a generalization
 
----------------
serge-sans-paille wrote:
> ??
I'm not seeing any diff around here, so perhaps you were seeing some Phabricator bug?


================
Comment at: clang/test/Sema/warn-fortify-source.c:100-102
+  __builtin_snprintf(buf, 2, "%#x", n);
+  __builtin_snprintf(buf, 2, "%#X", n);
+  __builtin_snprintf(buf, 2, "%#o", n);
----------------
nickdesaulniers wrote:
> hazohelet wrote:
> > nickdesaulniers wrote:
> > > Note that GCC -Wformat-truncation can warn on some of these.
> > > 
> > > https://godbolt.org/z/jE3axWe1W
> > > 
> > > Looks like the diagnostic keeps an up and lower bounds on the estimated format string expansion.
> > > 
> > > Trunk for Clang also warns for these, so is this change a regression? Or are both GCC and Clang (trunk) incorrect?
> > Clang trunk is saying something factually incorrect because it says the output `will always be truncated`, when in fact `__builtin_snprintf(buf, 2, "%#x", n);` doesn't trigger truncation if `n` is zero.
> > 
> > GCC is correct but is more conservative than clang's `ALWAYS be truncated` diagnostics.
> > GCC's warning message is `... directive output MAY BE truncated` .
> > GCC doesn't warn on it when `n` is known to be zero. (https://godbolt.org/z/E51a3Pfhr)
> > 
> > GCC's behavior makes sense here because the truncation does happen whenever `n` is not zero. If the user knows `n` is zero then they have no reason to use `%#x` specifier.
> > So, I think it makes sense to assume `n` is not zero and emit diagnostics, but it definitely needs diagnostics rewording like `is likely to be truncated`.
> I see; thanks for the explanation. Sorry for the code review delay; I missed this comment in my inbox.
As a future plan, I think it's generally good to follow GCC's behavior regarding this format warning considering GCC's high smartness on it.
I want to emit some warning here like GCC does because users are highly likely to expect non-zero value to be printed, but I think it's reasonable to defer it for now because it's not ideal to add a new warning flag when we are considering changing this warning's flag from `Wfortify-source` to `Wformat-truncation`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159138/new/

https://reviews.llvm.org/D159138