[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2024-01-29 Thread Karl-Johan Karlsson via cfe-commits 

karka228 wrote:

Ping.

https://github.com/llvm/llvm-project/pull/74440
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2024-01-11 Thread Karl-Johan Karlsson via cfe-commits 

karka228 wrote:

Ping. Lets pickup the discussion from before Christmas. Is the solution with 
the dummy warning (that @hazohelet wrote about in a previous comment) an 
acceptable solution?


https://github.com/llvm/llvm-project/pull/74440
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-18 Thread Karl-Johan Karlsson via cfe-commits 

https://github.com/karka228 updated 
https://github.com/llvm/llvm-project/pull/74440

>From a80bf9d03f19d48c0aca4af7758dc49516da8825 Mon Sep 17 00:00:00 2001
From: Karl-Johan Karlsson 
Date: Tue, 5 Dec 2023 10:03:00 +0100
Subject: [PATCH 1/7] [Sema] Implement support for -Wformat-signedness

In gcc there exist a modifier option -Wformat-signedness that turns on
additional signedness warnings in the already existing -Wformat warning.

This patch implements that support in clang.
---
 clang/include/clang/AST/FormatString.h|   2 +
 .../include/clang/Basic/DiagnosticOptions.def |   1 +
 clang/include/clang/Driver/Options.td |   3 +
 clang/lib/AST/FormatString.cpp|  29 +++--
 clang/lib/Driver/ToolChains/Clang.cpp |   2 +
 clang/lib/Sema/SemaChecking.cpp   |  26 -
 format-strings-signedness.c   | 107 ++
 7 files changed, 158 insertions(+), 12 deletions(-)
 create mode 100644 format-strings-signedness.c

diff --git a/clang/include/clang/AST/FormatString.h 
b/clang/include/clang/AST/FormatString.h
index 5c4ad9baaef608..c267a32be4d6f4 100644
--- a/clang/include/clang/AST/FormatString.h
+++ b/clang/include/clang/AST/FormatString.h
@@ -264,6 +264,8 @@ class ArgType {
 /// The conversion specifier and the argument type are compatible. For
 /// instance, "%d" and int.
 Match = 1,
+/// The conversion specifier and the argument type have different sign
+MatchSignedness,
 /// The conversion specifier and the argument type are compatible because 
of
 /// default argument promotions. For instance, "%hhd" and int.
 MatchPromotion,
diff --git a/clang/include/clang/Basic/DiagnosticOptions.def 
b/clang/include/clang/Basic/DiagnosticOptions.def
index 6d0c1b14acc120..a9562e7d8dcb0d 100644
--- a/clang/include/clang/Basic/DiagnosticOptions.def
+++ b/clang/include/clang/Basic/DiagnosticOptions.def
@@ -44,6 +44,7 @@ DIAGOPT(Name, Bits, Default)
 #endif
 
 SEMANTIC_DIAGOPT(IgnoreWarnings, 1, 0)   /// -w
+DIAGOPT(FormatSignedness, 1, 0) /// -Wformat-signedness
 DIAGOPT(NoRewriteMacros, 1, 0)  /// -Wno-rewrite-macros
 DIAGOPT(Pedantic, 1, 0) /// -pedantic
 DIAGOPT(PedanticErrors, 1, 0)   /// -pedantic-errors
diff --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index 1d04e4f6e7e6d9..04fdf9a7eb92d6 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -918,6 +918,9 @@ def Wdeprecated : Flag<["-"], "Wdeprecated">, 
Group,
   HelpText<"Enable warnings for deprecated constructs and define 
__DEPRECATED">;
 def Wno_deprecated : Flag<["-"], "Wno-deprecated">, Group,
   Visibility<[ClangOption, CC1Option]>;
+def Wformat_signedness : Flag<["-"], "Wformat-signedness">,
+  Flags<[HelpHidden]>, Visibility<[ClangOption, CC1Option]>,
+  MarshallingInfoFlag>;
 def Wl_COMMA : CommaJoined<["-"], "Wl,">, Visibility<[ClangOption, 
FlangOption]>,
   Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass the comma separated arguments in  to the linker">,
diff --git a/clang/lib/AST/FormatString.cpp b/clang/lib/AST/FormatString.cpp
index e0c9e18cfe3a24..670cde017d3ac2 100644
--- a/clang/lib/AST/FormatString.cpp
+++ b/clang/lib/AST/FormatString.cpp
@@ -409,7 +409,7 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
 return Match;
   if (const auto *BT = argTy->getAs()) {
 // Check if the only difference between them is signed vs unsigned
-// if true, we consider they are compatible.
+// if true, return match signedness.
 switch (BT->getKind()) {
   default:
 break;
@@ -419,44 +419,53 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const 
{
 [[fallthrough]];
   case BuiltinType::Char_S:
   case BuiltinType::SChar:
+if (T == C.UnsignedShortTy || T == C.ShortTy)
+  return NoMatchTypeConfusion;
+if (T == C.UnsignedCharTy)
+  return MatchSignedness;
+if (T == C.SignedCharTy)
+  return Match;
+break;
   case BuiltinType::Char_U:
   case BuiltinType::UChar:
 if (T == C.UnsignedShortTy || T == C.ShortTy)
   return NoMatchTypeConfusion;
-if (T == C.UnsignedCharTy || T == C.SignedCharTy)
+if (T == C.UnsignedCharTy)
   return Match;
+if (T == C.SignedCharTy)
+  return MatchSignedness;
 break;
   case BuiltinType::Short:
 if (T == C.UnsignedShortTy)
-  return Match;
+  return MatchSignedness;
 break;
   case BuiltinType::UShort:
 if (T == C.ShortTy)
-  return Match;
+  return MatchSignedness;
 break;
   case BuiltinType::Int:
 if (T == C.UnsignedIntTy)
-  return Match;
+  return MatchSignedness;
 b

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-11 Thread Takuya Shimizu via cfe-commits 

hazohelet wrote:

> I need to extend the tablegen backend ClangDiagnosticsEmitter with some kind 
> of new option to handle this, right?

Alternatively, you could probably use `DiagnosticsEngine::isIgnored` to check 
if the `-Wformat-signedness` is enabled or not, and control whether 
`MatchSignedNess` is `NoMatch` or `Match`, as your first implementation did.
This way we can achieve GCC compatibility for 1, 2, 3, 4, 6, 7, while 
benefitting from a real warning flag.
However, in this implementation `-Wformat-signedness` would be an _dummy 
warning_ which never gets emitted. As far as I know, there isn't anything 
similar in the codebase, so it might not be the ideal way.

https://github.com/llvm/llvm-project/pull/74440
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-08 Thread Aaron Ballman via cfe-commits 

AaronBallman wrote:

> I have started to implement defining the -Wformat-signedness option config in 
> clang/include/clang/Basic/DiagnosticSemaKinds.td as suggested originally by 
> @hazohelet and I agree that the implementation is a lot cleaner that way. 
> However before finishing implementing that I think we need to conclude what 
> level of gcc compatibility that we aim for in clang.
> 
> Below I list the characteristics of -Wformat-signedness that I have seen by 
> testing it out in gcc and reading the gcc documentation (I have not inspected 
> the gcc source code):
> 
> 1. The option -Wformat-signedness is default off.
> 
> 2. The -Wformat-signedness warnings are not enabled alone by the -Wformat 
> option.
> 
> 3. The -Wformat-signedness warnings are not enabled alone by the 
> -Wformat-signedness option.
> 
> 4. The -Wformat-signedness warnings are enabled by the option -Wformat 
> together with the option -Wformat-signedness.
> 
> 5. Parts of the -Wformat-signedness warnings (regarding scanf) are 
> enabled by the options -Wformat together with the option -pedantic.
> 
> 6. Warnings produced by -Wformat-signedness is reported as a -Wformat 
> warnings (e.g "warning: format ‘%u’ expects argument of type ‘unsigned int’, 
> but argument 2 has type ‘int’ [-Wformat=]"
> 
> 7. Warnings produced by -Wformat-signedness can be suppressed by "#pragma 
> GCC diagnostic ignored -Wformat"
> 
> 
> Which of the above points are important for us to follow in clang to get a 
> reasonable gcc compatibility? To me I think point 1, 2 and maybe 7 is the 
> most important to follow.

I agree, 1, 2, 4 (same idea as 2, basically), and 7 are the critical things 
we'd need for compatibility. I personally find 3, 5, and 6 to be kind of 
strange behavior  though I can see how it would arise.

https://github.com/llvm/llvm-project/pull/74440
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-08 Thread Karl-Johan Karlsson via cfe-commits 

https://github.com/karka228 updated 
https://github.com/llvm/llvm-project/pull/74440

>From a80bf9d03f19d48c0aca4af7758dc49516da8825 Mon Sep 17 00:00:00 2001
From: Karl-Johan Karlsson 
Date: Tue, 5 Dec 2023 10:03:00 +0100
Subject: [PATCH 1/5] [Sema] Implement support for -Wformat-signedness

In gcc there exist a modifier option -Wformat-signedness that turns on
additional signedness warnings in the already existing -Wformat warning.

This patch implements that support in clang.
---
 clang/include/clang/AST/FormatString.h|   2 +
 .../include/clang/Basic/DiagnosticOptions.def |   1 +
 clang/include/clang/Driver/Options.td |   3 +
 clang/lib/AST/FormatString.cpp|  29 +++--
 clang/lib/Driver/ToolChains/Clang.cpp |   2 +
 clang/lib/Sema/SemaChecking.cpp   |  26 -
 format-strings-signedness.c   | 107 ++
 7 files changed, 158 insertions(+), 12 deletions(-)
 create mode 100644 format-strings-signedness.c

diff --git a/clang/include/clang/AST/FormatString.h 
b/clang/include/clang/AST/FormatString.h
index 5c4ad9baaef608..c267a32be4d6f4 100644
--- a/clang/include/clang/AST/FormatString.h
+++ b/clang/include/clang/AST/FormatString.h
@@ -264,6 +264,8 @@ class ArgType {
 /// The conversion specifier and the argument type are compatible. For
 /// instance, "%d" and int.
 Match = 1,
+/// The conversion specifier and the argument type have different sign
+MatchSignedness,
 /// The conversion specifier and the argument type are compatible because 
of
 /// default argument promotions. For instance, "%hhd" and int.
 MatchPromotion,
diff --git a/clang/include/clang/Basic/DiagnosticOptions.def 
b/clang/include/clang/Basic/DiagnosticOptions.def
index 6d0c1b14acc120..a9562e7d8dcb0d 100644
--- a/clang/include/clang/Basic/DiagnosticOptions.def
+++ b/clang/include/clang/Basic/DiagnosticOptions.def
@@ -44,6 +44,7 @@ DIAGOPT(Name, Bits, Default)
 #endif
 
 SEMANTIC_DIAGOPT(IgnoreWarnings, 1, 0)   /// -w
+DIAGOPT(FormatSignedness, 1, 0) /// -Wformat-signedness
 DIAGOPT(NoRewriteMacros, 1, 0)  /// -Wno-rewrite-macros
 DIAGOPT(Pedantic, 1, 0) /// -pedantic
 DIAGOPT(PedanticErrors, 1, 0)   /// -pedantic-errors
diff --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index 1d04e4f6e7e6d9..04fdf9a7eb92d6 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -918,6 +918,9 @@ def Wdeprecated : Flag<["-"], "Wdeprecated">, 
Group,
   HelpText<"Enable warnings for deprecated constructs and define 
__DEPRECATED">;
 def Wno_deprecated : Flag<["-"], "Wno-deprecated">, Group,
   Visibility<[ClangOption, CC1Option]>;
+def Wformat_signedness : Flag<["-"], "Wformat-signedness">,
+  Flags<[HelpHidden]>, Visibility<[ClangOption, CC1Option]>,
+  MarshallingInfoFlag>;
 def Wl_COMMA : CommaJoined<["-"], "Wl,">, Visibility<[ClangOption, 
FlangOption]>,
   Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass the comma separated arguments in  to the linker">,
diff --git a/clang/lib/AST/FormatString.cpp b/clang/lib/AST/FormatString.cpp
index e0c9e18cfe3a24..670cde017d3ac2 100644
--- a/clang/lib/AST/FormatString.cpp
+++ b/clang/lib/AST/FormatString.cpp
@@ -409,7 +409,7 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
 return Match;
   if (const auto *BT = argTy->getAs()) {
 // Check if the only difference between them is signed vs unsigned
-// if true, we consider they are compatible.
+// if true, return match signedness.
 switch (BT->getKind()) {
   default:
 break;
@@ -419,44 +419,53 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const 
{
 [[fallthrough]];
   case BuiltinType::Char_S:
   case BuiltinType::SChar:
+if (T == C.UnsignedShortTy || T == C.ShortTy)
+  return NoMatchTypeConfusion;
+if (T == C.UnsignedCharTy)
+  return MatchSignedness;
+if (T == C.SignedCharTy)
+  return Match;
+break;
   case BuiltinType::Char_U:
   case BuiltinType::UChar:
 if (T == C.UnsignedShortTy || T == C.ShortTy)
   return NoMatchTypeConfusion;
-if (T == C.UnsignedCharTy || T == C.SignedCharTy)
+if (T == C.UnsignedCharTy)
   return Match;
+if (T == C.SignedCharTy)
+  return MatchSignedness;
 break;
   case BuiltinType::Short:
 if (T == C.UnsignedShortTy)
-  return Match;
+  return MatchSignedness;
 break;
   case BuiltinType::UShort:
 if (T == C.ShortTy)
-  return Match;
+  return MatchSignedness;
 break;
   case BuiltinType::Int:
 if (T == C.UnsignedIntTy)
-  return Match;
+  return MatchSignedness;
 b

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-07 Thread Karl-Johan Karlsson via cfe-commits 

https://github.com/karka228 updated 
https://github.com/llvm/llvm-project/pull/74440

>From a80bf9d03f19d48c0aca4af7758dc49516da8825 Mon Sep 17 00:00:00 2001
From: Karl-Johan Karlsson 
Date: Tue, 5 Dec 2023 10:03:00 +0100
Subject: [PATCH 1/3] [Sema] Implement support for -Wformat-signedness

In gcc there exist a modifier option -Wformat-signedness that turns on
additional signedness warnings in the already existing -Wformat warning.

This patch implements that support in clang.
---
 clang/include/clang/AST/FormatString.h|   2 +
 .../include/clang/Basic/DiagnosticOptions.def |   1 +
 clang/include/clang/Driver/Options.td |   3 +
 clang/lib/AST/FormatString.cpp|  29 +++--
 clang/lib/Driver/ToolChains/Clang.cpp |   2 +
 clang/lib/Sema/SemaChecking.cpp   |  26 -
 format-strings-signedness.c   | 107 ++
 7 files changed, 158 insertions(+), 12 deletions(-)
 create mode 100644 format-strings-signedness.c

diff --git a/clang/include/clang/AST/FormatString.h 
b/clang/include/clang/AST/FormatString.h
index 5c4ad9baaef608..c267a32be4d6f4 100644
--- a/clang/include/clang/AST/FormatString.h
+++ b/clang/include/clang/AST/FormatString.h
@@ -264,6 +264,8 @@ class ArgType {
 /// The conversion specifier and the argument type are compatible. For
 /// instance, "%d" and int.
 Match = 1,
+/// The conversion specifier and the argument type have different sign
+MatchSignedness,
 /// The conversion specifier and the argument type are compatible because 
of
 /// default argument promotions. For instance, "%hhd" and int.
 MatchPromotion,
diff --git a/clang/include/clang/Basic/DiagnosticOptions.def 
b/clang/include/clang/Basic/DiagnosticOptions.def
index 6d0c1b14acc120..a9562e7d8dcb0d 100644
--- a/clang/include/clang/Basic/DiagnosticOptions.def
+++ b/clang/include/clang/Basic/DiagnosticOptions.def
@@ -44,6 +44,7 @@ DIAGOPT(Name, Bits, Default)
 #endif
 
 SEMANTIC_DIAGOPT(IgnoreWarnings, 1, 0)   /// -w
+DIAGOPT(FormatSignedness, 1, 0) /// -Wformat-signedness
 DIAGOPT(NoRewriteMacros, 1, 0)  /// -Wno-rewrite-macros
 DIAGOPT(Pedantic, 1, 0) /// -pedantic
 DIAGOPT(PedanticErrors, 1, 0)   /// -pedantic-errors
diff --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index 1d04e4f6e7e6d9..04fdf9a7eb92d6 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -918,6 +918,9 @@ def Wdeprecated : Flag<["-"], "Wdeprecated">, 
Group,
   HelpText<"Enable warnings for deprecated constructs and define 
__DEPRECATED">;
 def Wno_deprecated : Flag<["-"], "Wno-deprecated">, Group,
   Visibility<[ClangOption, CC1Option]>;
+def Wformat_signedness : Flag<["-"], "Wformat-signedness">,
+  Flags<[HelpHidden]>, Visibility<[ClangOption, CC1Option]>,
+  MarshallingInfoFlag>;
 def Wl_COMMA : CommaJoined<["-"], "Wl,">, Visibility<[ClangOption, 
FlangOption]>,
   Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass the comma separated arguments in  to the linker">,
diff --git a/clang/lib/AST/FormatString.cpp b/clang/lib/AST/FormatString.cpp
index e0c9e18cfe3a24..670cde017d3ac2 100644
--- a/clang/lib/AST/FormatString.cpp
+++ b/clang/lib/AST/FormatString.cpp
@@ -409,7 +409,7 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
 return Match;
   if (const auto *BT = argTy->getAs()) {
 // Check if the only difference between them is signed vs unsigned
-// if true, we consider they are compatible.
+// if true, return match signedness.
 switch (BT->getKind()) {
   default:
 break;
@@ -419,44 +419,53 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const 
{
 [[fallthrough]];
   case BuiltinType::Char_S:
   case BuiltinType::SChar:
+if (T == C.UnsignedShortTy || T == C.ShortTy)
+  return NoMatchTypeConfusion;
+if (T == C.UnsignedCharTy)
+  return MatchSignedness;
+if (T == C.SignedCharTy)
+  return Match;
+break;
   case BuiltinType::Char_U:
   case BuiltinType::UChar:
 if (T == C.UnsignedShortTy || T == C.ShortTy)
   return NoMatchTypeConfusion;
-if (T == C.UnsignedCharTy || T == C.SignedCharTy)
+if (T == C.UnsignedCharTy)
   return Match;
+if (T == C.SignedCharTy)
+  return MatchSignedness;
 break;
   case BuiltinType::Short:
 if (T == C.UnsignedShortTy)
-  return Match;
+  return MatchSignedness;
 break;
   case BuiltinType::UShort:
 if (T == C.ShortTy)
-  return Match;
+  return MatchSignedness;
 break;
   case BuiltinType::Int:
 if (T == C.UnsignedIntTy)
-  return Match;
+  return MatchSignedness;
 b

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-06 Thread Karl-Johan Karlsson via cfe-commits 

karka228 wrote:

I have started to implement defining the -Wformat-signedness option config in 
clang/include/clang/Basic/DiagnosticSemaKinds.td as suggested originally by 
@hazohelet and I agree that the implementation is a lot cleaner that way.
However before finishing implementing that I think we need to conclude what 
level of gcc compatibility that we aim for in clang.

Below I list the characteristics of -Wformat-signedness that I have seen by 
testing it out in gcc and reading the gcc documentation (I have not inspected 
the gcc source code):

1. The option -Wformat-signedness is default off.
2. The -Wformat-signedness warnings are not enabled alone by the -Wformat 
option.
3. The -Wformat-signedness warnings are not enabled alone by the 
-Wformat-signedness option.
4. The -Wformat-signedness warnings are enabled by the option -Wformat together 
with the option -Wformat-signedness.
5. Parts of the -Wformat-signedness warnings (regarding scanf) are enabled by 
the options -Wformat together with the option -pedantic.
6. Warnings produced by -Wformat-signedness is reported as a -Wformat warnings 
(e.g "warning: format ‘%u’ expects argument of type ‘unsigned int’, but 
argument 2 has type ‘int’ [-Wformat=]"
7. Warnings produced by -Wformat-signedness can be suppressed by "#pragma GCC 
diagnostic ignored -Wformat"

Which of the above points are important for us to follow in clang to get a 
reasonable gcc compatibility?
To me I think point 1, 2 and maybe 7 is the most important to follow.

https://github.com/llvm/llvm-project/pull/74440
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-06 Thread Karl-Johan Karlsson via cfe-commits 

karka228 wrote:

> I'd like to understand the need for this diagnostic a bit more. I realize GCC 
> has it, but it's off-by-default in GCC and `-Wformat` does not enable it 
> either: https://godbolt.org/z/Gxczsjdj5
> 
> We believe that users don't enable off by default warnings often enough to 
> warrant supporting them, and this feels like a very pedantic warning in many 
> cases. e.g., `printf("%u", 0);`

I work on an out of tree backend and our customers requested the warning 
-Wformat-signedness to be implemented in clang (as they found it in gcc). I 
agree that the warning is pedantic. Note that gcc will with the options 
-Wformat and -pedantic enable at least the warnings related to scanf (see 
https://godbolt.org/z/WjzWKj89G). 

If you feel that the clang community don't really want -Wformat-signedness I 
can simply implement this in our downstream llvm-project repo and abandon this 
pull request. We already have a large number of diffs compared to 
https://github.com/llvm/llvm-project and a few more probably don't make any 
difference.


https://github.com/llvm/llvm-project/pull/74440
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-06 Thread Aaron Ballman via cfe-commits 


@@ -918,6 +918,9 @@ def Wdeprecated : Flag<["-"], "Wdeprecated">, 
Group,
   HelpText<"Enable warnings for deprecated constructs and define 
__DEPRECATED">;
 def Wno_deprecated : Flag<["-"], "Wno-deprecated">, Group,
   Visibility<[ClangOption, CC1Option]>;
+def Wformat_signedness : Flag<["-"], "Wformat-signedness">,

AaronBallman wrote:

Yup, this will have to change to adding the diagnostic to 
DiagnosticSemaKinds.td. I think we will want to control this separately from 
`-Wformat` but have it grouped under that flag.

https://github.com/llvm/llvm-project/pull/74440
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-06 Thread Aaron Ballman via cfe-commits 


@@ -264,6 +264,8 @@ class ArgType {
 /// The conversion specifier and the argument type are compatible. For
 /// instance, "%d" and int.
 Match = 1,
+/// The conversion specifier and the argument type have different sign

AaronBallman wrote:

```suggestion
/// The conversion specifier and the argument type have different sign.
```

https://github.com/llvm/llvm-project/pull/74440
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-06 Thread Aaron Ballman via cfe-commits 

https://github.com/AaronBallman edited 
https://github.com/llvm/llvm-project/pull/74440
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-05 Thread Karl-Johan Karlsson via cfe-commits 


@@ -918,6 +918,9 @@ def Wdeprecated : Flag<["-"], "Wdeprecated">, 
Group,
   HelpText<"Enable warnings for deprecated constructs and define 
__DEPRECATED">;
 def Wno_deprecated : Flag<["-"], "Wno-deprecated">, Group,
   Visibility<[ClangOption, CC1Option]>;
+def Wformat_signedness : Flag<["-"], "Wformat-signedness">,

karka228 wrote:

I have tried to mimic the gcc behavior when implementing this and in gcc it 
seems like -Wformat-signedness don't introduce a new warning but only modify 
the behavior of the already existing -Wformat warning.
If I place the warning option config in 
clang/include/clang/Basic/DiagnosticSemaKinds.td as you suggest don't I then 
also create a new warning with a new warning text? ... that is not what gcc do 
(as far as I understand).

Well, with that said we don't have to follow exactly how gcc implemented this 
if we think there is a better way to implement this in clang.

I agree that support for -Wno-format-signednes is missing from my patch and 
that should be added.


https://github.com/llvm/llvm-project/pull/74440
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-05 Thread via cfe-commits 

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff 383e35048e16c85ab26bc46822d3ceb11fd4d3f2 
a80bf9d03f19d48c0aca4af7758dc49516da8825 -- format-strings-signedness.c 
clang/include/clang/AST/FormatString.h clang/lib/AST/FormatString.cpp 
clang/lib/Driver/ToolChains/Clang.cpp clang/lib/Sema/SemaChecking.cpp
``





View the diff from clang-format here.


``diff
diff --git a/format-strings-signedness.c b/format-strings-signedness.c
index 02964def10..876094ea4f 100644
--- a/format-strings-signedness.c
+++ b/format-strings-signedness.c
@@ -1,65 +1,68 @@
-// RUN: %clang_cc1 -std=c11 -fsyntax-only -verify -Wformat -Wformat-signedness 
%s
+// RUN: %clang_cc1 -std=c11 -fsyntax-only -verify -Wformat -Wformat-signedness
+// %s
 
 int printf(const char *restrict format, ...);
-int scanf(const char * restrict, ...);
+int scanf(const char *restrict, ...);
 
-void test_printf_bool(_Bool x)
-{
-printf("%d", x); // no-warning
-printf("%u", x); // expected-warning{{format specifies type 'unsigned int' 
but the argument has type '_Bool'}}
-printf("%x", x); // expected-warning{{format specifies type 'unsigned int' 
but the argument has type '_Bool'}}
+void test_printf_bool(_Bool x) {
+  printf("%d", x); // no-warning
+  printf("%u", x); // expected-warning{{format specifies type 'unsigned int' 
but
+   // the argument has type '_Bool'}}
+  printf("%x", x); // expected-warning{{format specifies type 'unsigned int' 
but
+   // the argument has type '_Bool'}}
 }
 
-void test_printf_char(char x)
-{
-printf("%c", x); // no-warning
+void test_printf_char(char x) {
+  printf("%c", x); // no-warning
 }
 
-void test_printf_unsigned_char(unsigned char x)
-{
-printf("%c", x); // no-warning
+void test_printf_unsigned_char(unsigned char x) {
+  printf("%c", x); // no-warning
 }
 
-void test_printf_int(int x)
-{
-printf("%d", x); // no-warning
-printf("%u", x); // expected-warning{{format specifies type 'unsigned int' 
but the argument has type 'int'}}
-printf("%x", x); // expected-warning{{format specifies type 'unsigned int' 
but the argument has type 'int'}}
+void test_printf_int(int x) {
+  printf("%d", x); // no-warning
+  printf("%u", x); // expected-warning{{format specifies type 'unsigned int' 
but
+   // the argument has type 'int'}}
+  printf("%x", x); // expected-warning{{format specifies type 'unsigned int' 
but
+   // the argument has type 'int'}}
 }
 
-void test_printf_unsigned(unsigned x)
-{
-printf("%d", x); // expected-warning{{format specifies type 'int' but the 
argument has type 'unsigned int'}}
-printf("%u", x); // no-warning
-printf("%x", x); // no-warning
+void test_printf_unsigned(unsigned x) {
+  printf("%d", x); // expected-warning{{format specifies type 'int' but the
+   // argument has type 'unsigned int'}}
+  printf("%u", x); // no-warning
+  printf("%x", x); // no-warning
 }
 
-void test_printf_long(long x)
-{
-printf("%ld", x); // no-warning
-printf("%lu", x); // expected-warning{{format specifies type 'unsigned 
long' but the argument has type 'long'}}
-printf("%lx", x); // expected-warning{{format specifies type 'unsigned 
long' but the argument has type 'long'}}
+void test_printf_long(long x) {
+  printf("%ld", x); // no-warning
+  printf("%lu", x); // expected-warning{{format specifies type 'unsigned long'
+// but the argument has type 'long'}}
+  printf("%lx", x); // expected-warning{{format specifies type 'unsigned long'
+// but the argument has type 'long'}}
 }
 
-void test_printf_unsigned_long(unsigned long x)
-{
-printf("%ld", x); // expected-warning{{format specifies type 'long' but 
the argument has type 'unsigned long'}}
-printf("%lu", x); // no-warning
-printf("%lx", x); // no-warning
+void test_printf_unsigned_long(unsigned long x) {
+  printf("%ld", x); // expected-warning{{format specifies type 'long' but the
+// argument has type 'unsigned long'}}
+  printf("%lu", x); // no-warning
+  printf("%lx", x); // no-warning
 }
 
-void test_printf_long_long(long long x)
-{
-printf("%lld", x); // no-warning
-printf("%llu", x); // expected-warning{{format specifies type 'unsigned 
long long' but the argument has type 'long long'}}
-printf("%llx", x); // expected-warning{{format specifies type 'unsigned 
long long' but the argument has type 'long long'}}
+void test_printf_long_long(long long x) {
+  printf("%lld", x); // no-warning
+  printf("%llu", x); // expected-warning{{format specifies type 'unsigned long
+ // long' but the argument has type 'long long'}}
+  printf("%llx", x); // expected-warning{{format specifies type 'unsigned long
+ // long' but the argument has type

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-05 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Karl-Johan Karlsson (karka228)


Changes

In gcc there exist a modifier option -Wformat-signedness that turns on 
additional signedness warnings in the already existing -Wformat warning.

This patch implements that support in clang.

---
Full diff: https://github.com/llvm/llvm-project/pull/74440.diff


7 Files Affected:

- (modified) clang/include/clang/AST/FormatString.h (+2) 
- (modified) clang/include/clang/Basic/DiagnosticOptions.def (+1) 
- (modified) clang/include/clang/Driver/Options.td (+3) 
- (modified) clang/lib/AST/FormatString.cpp (+19-10) 
- (modified) clang/lib/Driver/ToolChains/Clang.cpp (+2) 
- (modified) clang/lib/Sema/SemaChecking.cpp (+24-2) 
- (added) format-strings-signedness.c (+107) 


``diff
diff --git a/clang/include/clang/AST/FormatString.h 
b/clang/include/clang/AST/FormatString.h
index 5c4ad9baaef60..c267a32be4d6f 100644
--- a/clang/include/clang/AST/FormatString.h
+++ b/clang/include/clang/AST/FormatString.h
@@ -264,6 +264,8 @@ class ArgType {
 /// The conversion specifier and the argument type are compatible. For
 /// instance, "%d" and int.
 Match = 1,
+/// The conversion specifier and the argument type have different sign
+MatchSignedness,
 /// The conversion specifier and the argument type are compatible because 
of
 /// default argument promotions. For instance, "%hhd" and int.
 MatchPromotion,
diff --git a/clang/include/clang/Basic/DiagnosticOptions.def 
b/clang/include/clang/Basic/DiagnosticOptions.def
index 6d0c1b14acc12..a9562e7d8dcb0 100644
--- a/clang/include/clang/Basic/DiagnosticOptions.def
+++ b/clang/include/clang/Basic/DiagnosticOptions.def
@@ -44,6 +44,7 @@ DIAGOPT(Name, Bits, Default)
 #endif
 
 SEMANTIC_DIAGOPT(IgnoreWarnings, 1, 0)   /// -w
+DIAGOPT(FormatSignedness, 1, 0) /// -Wformat-signedness
 DIAGOPT(NoRewriteMacros, 1, 0)  /// -Wno-rewrite-macros
 DIAGOPT(Pedantic, 1, 0) /// -pedantic
 DIAGOPT(PedanticErrors, 1, 0)   /// -pedantic-errors
diff --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index 1d04e4f6e7e6d..04fdf9a7eb92d 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -918,6 +918,9 @@ def Wdeprecated : Flag<["-"], "Wdeprecated">, 
Group,
   HelpText<"Enable warnings for deprecated constructs and define 
__DEPRECATED">;
 def Wno_deprecated : Flag<["-"], "Wno-deprecated">, Group,
   Visibility<[ClangOption, CC1Option]>;
+def Wformat_signedness : Flag<["-"], "Wformat-signedness">,
+  Flags<[HelpHidden]>, Visibility<[ClangOption, CC1Option]>,
+  MarshallingInfoFlag>;
 def Wl_COMMA : CommaJoined<["-"], "Wl,">, Visibility<[ClangOption, 
FlangOption]>,
   Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass the comma separated arguments in  to the linker">,
diff --git a/clang/lib/AST/FormatString.cpp b/clang/lib/AST/FormatString.cpp
index e0c9e18cfe3a2..670cde017d3ac 100644
--- a/clang/lib/AST/FormatString.cpp
+++ b/clang/lib/AST/FormatString.cpp
@@ -409,7 +409,7 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
 return Match;
   if (const auto *BT = argTy->getAs()) {
 // Check if the only difference between them is signed vs unsigned
-// if true, we consider they are compatible.
+// if true, return match signedness.
 switch (BT->getKind()) {
   default:
 break;
@@ -419,44 +419,53 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const 
{
 [[fallthrough]];
   case BuiltinType::Char_S:
   case BuiltinType::SChar:
+if (T == C.UnsignedShortTy || T == C.ShortTy)
+  return NoMatchTypeConfusion;
+if (T == C.UnsignedCharTy)
+  return MatchSignedness;
+if (T == C.SignedCharTy)
+  return Match;
+break;
   case BuiltinType::Char_U:
   case BuiltinType::UChar:
 if (T == C.UnsignedShortTy || T == C.ShortTy)
   return NoMatchTypeConfusion;
-if (T == C.UnsignedCharTy || T == C.SignedCharTy)
+if (T == C.UnsignedCharTy)
   return Match;
+if (T == C.SignedCharTy)
+  return MatchSignedness;
 break;
   case BuiltinType::Short:
 if (T == C.UnsignedShortTy)
-  return Match;
+  return MatchSignedness;
 break;
   case BuiltinType::UShort:
 if (T == C.ShortTy)
-  return Match;
+  return MatchSignedness;
 break;
   case BuiltinType::Int:
 if (T == C.UnsignedIntTy)
-  return Match;
+  return MatchSignedness;
 break;
   case BuiltinType::UInt:
 if (T == C.IntTy)
-  return Match;
+  return MatchSignedness;
 break;
   case BuiltinType::Long:
 if (T == C.

[llvm] [clang] [Sema] Implement support for -Wformat-signedness (PR #74440)

2023-12-05 Thread Karl-Johan Karlsson via cfe-commits 

https://github.com/karka228 created 
https://github.com/llvm/llvm-project/pull/74440

In gcc there exist a modifier option -Wformat-signedness that turns on 
additional signedness warnings in the already existing -Wformat warning.

This patch implements that support in clang.

>From a80bf9d03f19d48c0aca4af7758dc49516da8825 Mon Sep 17 00:00:00 2001
From: Karl-Johan Karlsson 
Date: Tue, 5 Dec 2023 10:03:00 +0100
Subject: [PATCH] [Sema] Implement support for -Wformat-signedness

In gcc there exist a modifier option -Wformat-signedness that turns on
additional signedness warnings in the already existing -Wformat warning.

This patch implements that support in clang.
---
 clang/include/clang/AST/FormatString.h|   2 +
 .../include/clang/Basic/DiagnosticOptions.def |   1 +
 clang/include/clang/Driver/Options.td |   3 +
 clang/lib/AST/FormatString.cpp|  29 +++--
 clang/lib/Driver/ToolChains/Clang.cpp |   2 +
 clang/lib/Sema/SemaChecking.cpp   |  26 -
 format-strings-signedness.c   | 107 ++
 7 files changed, 158 insertions(+), 12 deletions(-)
 create mode 100644 format-strings-signedness.c

diff --git a/clang/include/clang/AST/FormatString.h 
b/clang/include/clang/AST/FormatString.h
index 5c4ad9baaef60..c267a32be4d6f 100644
--- a/clang/include/clang/AST/FormatString.h
+++ b/clang/include/clang/AST/FormatString.h
@@ -264,6 +264,8 @@ class ArgType {
 /// The conversion specifier and the argument type are compatible. For
 /// instance, "%d" and int.
 Match = 1,
+/// The conversion specifier and the argument type have different sign
+MatchSignedness,
 /// The conversion specifier and the argument type are compatible because 
of
 /// default argument promotions. For instance, "%hhd" and int.
 MatchPromotion,
diff --git a/clang/include/clang/Basic/DiagnosticOptions.def 
b/clang/include/clang/Basic/DiagnosticOptions.def
index 6d0c1b14acc12..a9562e7d8dcb0 100644
--- a/clang/include/clang/Basic/DiagnosticOptions.def
+++ b/clang/include/clang/Basic/DiagnosticOptions.def
@@ -44,6 +44,7 @@ DIAGOPT(Name, Bits, Default)
 #endif
 
 SEMANTIC_DIAGOPT(IgnoreWarnings, 1, 0)   /// -w
+DIAGOPT(FormatSignedness, 1, 0) /// -Wformat-signedness
 DIAGOPT(NoRewriteMacros, 1, 0)  /// -Wno-rewrite-macros
 DIAGOPT(Pedantic, 1, 0) /// -pedantic
 DIAGOPT(PedanticErrors, 1, 0)   /// -pedantic-errors
diff --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index 1d04e4f6e7e6d..04fdf9a7eb92d 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -918,6 +918,9 @@ def Wdeprecated : Flag<["-"], "Wdeprecated">, 
Group,
   HelpText<"Enable warnings for deprecated constructs and define 
__DEPRECATED">;
 def Wno_deprecated : Flag<["-"], "Wno-deprecated">, Group,
   Visibility<[ClangOption, CC1Option]>;
+def Wformat_signedness : Flag<["-"], "Wformat-signedness">,
+  Flags<[HelpHidden]>, Visibility<[ClangOption, CC1Option]>,
+  MarshallingInfoFlag>;
 def Wl_COMMA : CommaJoined<["-"], "Wl,">, Visibility<[ClangOption, 
FlangOption]>,
   Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass the comma separated arguments in  to the linker">,
diff --git a/clang/lib/AST/FormatString.cpp b/clang/lib/AST/FormatString.cpp
index e0c9e18cfe3a2..670cde017d3ac 100644
--- a/clang/lib/AST/FormatString.cpp
+++ b/clang/lib/AST/FormatString.cpp
@@ -409,7 +409,7 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
 return Match;
   if (const auto *BT = argTy->getAs()) {
 // Check if the only difference between them is signed vs unsigned
-// if true, we consider they are compatible.
+// if true, return match signedness.
 switch (BT->getKind()) {
   default:
 break;
@@ -419,44 +419,53 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const 
{
 [[fallthrough]];
   case BuiltinType::Char_S:
   case BuiltinType::SChar:
+if (T == C.UnsignedShortTy || T == C.ShortTy)
+  return NoMatchTypeConfusion;
+if (T == C.UnsignedCharTy)
+  return MatchSignedness;
+if (T == C.SignedCharTy)
+  return Match;
+break;
   case BuiltinType::Char_U:
   case BuiltinType::UChar:
 if (T == C.UnsignedShortTy || T == C.ShortTy)
   return NoMatchTypeConfusion;
-if (T == C.UnsignedCharTy || T == C.SignedCharTy)
+if (T == C.UnsignedCharTy)
   return Match;
+if (T == C.SignedCharTy)
+  return MatchSignedness;
 break;
   case BuiltinType::Short:
 if (T == C.UnsignedShortTy)
-  return Match;
+  return MatchSignedness;
 break;
   case BuiltinType::UShort:
 if (T == C.ShortTy)
-  return Match;
+  return MatchSignedn