[clang-tidy] Avoid overflow when dumping unsigned integer values #85060

ealcdan · 2024-03-13T10:45:58Z

Some options take the maximum unsigned integer value as default, but they are being dumped to a string as integers. This makes -dump-config write invalid '-1' values for these options. This change fixes this issue by using utostr if the option is unsigned.

Fixes #60217

github-actions · 2024-03-13T10:46:16Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

ealcdan · 2024-03-13T10:46:41Z

This PR attempts to fix #60217

llvmbot · 2024-03-13T10:46:43Z

@llvm/pr-subscribers-clang-tidy

Author: None (ealcdan)

Changes

Some options take the maximum unsigned integer value as default, but they are being dumped to a string as integers. This makes -dump-config write invalid '-1' values for these options. This change fixes this issue by using utostr if the option is unsigned.

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

2 Files Affected:

(modified) clang-tools-extra/clang-tidy/ClangTidyCheck.cpp (+6)
(modified) clang-tools-extra/clang-tidy/ClangTidyCheck.h (+25-2)

diff --git a/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
index 3e926236adb451..710b361e16c0a7 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
@@ -139,6 +139,12 @@ void ClangTidyCheck::OptionsView::storeInt(ClangTidyOptions::OptionMap &Options,
   store(Options, LocalName, llvm::itostr(Value));
 }
 
+void ClangTidyCheck::OptionsView::storeUnsigned(
+    ClangTidyOptions::OptionMap &Options, StringRef LocalName,
+    uint64_t Value) const {
+  store(Options, LocalName, llvm::utostr(Value));
+}
+
 template <>
 void ClangTidyCheck::OptionsView::store<bool>(
     ClangTidyOptions::OptionMap &Options, StringRef LocalName,
diff --git a/clang-tools-extra/clang-tidy/ClangTidyCheck.h b/clang-tools-extra/clang-tidy/ClangTidyCheck.h
index 656a2f008f6e0e..5ccd91d5d891ff 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyCheck.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyCheck.h
@@ -408,17 +408,26 @@ class ClangTidyCheck : public ast_matchers::MatchFinder::MatchCallback {
     /// Stores an option with the check-local name \p LocalName with
     /// integer value \p Value to \p Options.
     template <typename T>
-    std::enable_if_t<std::is_integral_v<T>>
+    std::enable_if_t<std::is_integral_v<T> && std::is_signed<T>::value>
     store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
           T Value) const {
       storeInt(Options, LocalName, Value);
     }
 
+    /// Stores an option with the check-local name \p LocalName with
+    /// unsigned integer value \p Value to \p Options.
+    template <typename T>
+    std::enable_if_t<std::is_unsigned<T>::value>
+    store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
+          T Value) const {
+      storeUnsigned(Options, LocalName, Value);
+    }
+
     /// Stores an option with the check-local name \p LocalName with
     /// integer value \p Value to \p Options. If the value is empty
     /// stores ``
     template <typename T>
-    std::enable_if_t<std::is_integral_v<T>>
+    std::enable_if_t<std::is_integral_v<T> && std::is_signed<T>::value>
     store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
           std::optional<T> Value) const {
       if (Value)
@@ -427,6 +436,18 @@ class ClangTidyCheck : public ast_matchers::MatchFinder::MatchCallback {
         store(Options, LocalName, "none");
     }
 
+    /// Stores an option with the check-local name \p LocalName with
+    /// unsigned integer value \p Value to \p Options.
+    template <typename T>
+    std::enable_if_t<std::is_unsigned<T>::value>
+    store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
+          std::optional<T> Value) const {
+      if (Value)
+        storeUnsigned(Options, LocalName, *Value);
+      else
+        store(Options, LocalName, "none");
+    }
+
     /// Stores an option with the check-local name \p LocalName as the string
     /// representation of the Enum \p Value to \p Options.
     ///
@@ -470,6 +491,8 @@ class ClangTidyCheck : public ast_matchers::MatchFinder::MatchCallback {
     void storeInt(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
                   int64_t Value) const;
 
+    void storeUnsigned(ClangTidyOptions::OptionMap &Options,
+                       StringRef LocalName, int64_t Value) const;
 
     std::string NamePrefix;
     const ClangTidyOptions::OptionMap &CheckOptions;

llvmbot · 2024-03-13T10:46:44Z

@llvm/pr-subscribers-clang-tools-extra

Author: None (ealcdan)

Changes

Some options take the maximum unsigned integer value as default, but they are being dumped to a string as integers. This makes -dump-config write invalid '-1' values for these options. This change fixes this issue by using utostr if the option is unsigned.

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

2 Files Affected:

(modified) clang-tools-extra/clang-tidy/ClangTidyCheck.cpp (+6)
(modified) clang-tools-extra/clang-tidy/ClangTidyCheck.h (+25-2)

diff --git a/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
index 3e926236adb451..710b361e16c0a7 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
@@ -139,6 +139,12 @@ void ClangTidyCheck::OptionsView::storeInt(ClangTidyOptions::OptionMap &Options,
   store(Options, LocalName, llvm::itostr(Value));
 }
 
+void ClangTidyCheck::OptionsView::storeUnsigned(
+    ClangTidyOptions::OptionMap &Options, StringRef LocalName,
+    uint64_t Value) const {
+  store(Options, LocalName, llvm::utostr(Value));
+}
+
 template <>
 void ClangTidyCheck::OptionsView::store<bool>(
     ClangTidyOptions::OptionMap &Options, StringRef LocalName,
diff --git a/clang-tools-extra/clang-tidy/ClangTidyCheck.h b/clang-tools-extra/clang-tidy/ClangTidyCheck.h
index 656a2f008f6e0e..5ccd91d5d891ff 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyCheck.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyCheck.h
@@ -408,17 +408,26 @@ class ClangTidyCheck : public ast_matchers::MatchFinder::MatchCallback {
     /// Stores an option with the check-local name \p LocalName with
     /// integer value \p Value to \p Options.
     template <typename T>
-    std::enable_if_t<std::is_integral_v<T>>
+    std::enable_if_t<std::is_integral_v<T> && std::is_signed<T>::value>
     store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
           T Value) const {
       storeInt(Options, LocalName, Value);
     }
 
+    /// Stores an option with the check-local name \p LocalName with
+    /// unsigned integer value \p Value to \p Options.
+    template <typename T>
+    std::enable_if_t<std::is_unsigned<T>::value>
+    store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
+          T Value) const {
+      storeUnsigned(Options, LocalName, Value);
+    }
+
     /// Stores an option with the check-local name \p LocalName with
     /// integer value \p Value to \p Options. If the value is empty
     /// stores ``
     template <typename T>
-    std::enable_if_t<std::is_integral_v<T>>
+    std::enable_if_t<std::is_integral_v<T> && std::is_signed<T>::value>
     store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
           std::optional<T> Value) const {
       if (Value)
@@ -427,6 +436,18 @@ class ClangTidyCheck : public ast_matchers::MatchFinder::MatchCallback {
         store(Options, LocalName, "none");
     }
 
+    /// Stores an option with the check-local name \p LocalName with
+    /// unsigned integer value \p Value to \p Options.
+    template <typename T>
+    std::enable_if_t<std::is_unsigned<T>::value>
+    store(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
+          std::optional<T> Value) const {
+      if (Value)
+        storeUnsigned(Options, LocalName, *Value);
+      else
+        store(Options, LocalName, "none");
+    }
+
     /// Stores an option with the check-local name \p LocalName as the string
     /// representation of the Enum \p Value to \p Options.
     ///
@@ -470,6 +491,8 @@ class ClangTidyCheck : public ast_matchers::MatchFinder::MatchCallback {
     void storeInt(ClangTidyOptions::OptionMap &Options, StringRef LocalName,
                   int64_t Value) const;
 
+    void storeUnsigned(ClangTidyOptions::OptionMap &Options,
+                       StringRef LocalName, int64_t Value) const;
 
     std::string NamePrefix;
     const ClangTidyOptions::OptionMap &CheckOptions;

PiotrZSL

Missing test (based on some check).

clang-tools-extra/clang-tidy/ClangTidyCheck.h

PiotrZSL · 2024-03-13T17:17:33Z

And just one comment, when this check uses max value as "limit", this behavior is also a reason why support for optional values were added. For me in this case check & description should be updated to handle this config as an optional, instead of mentioning max u64 in documentation. As for support for unsigned values, that's welcome too regardless.

ealcdan · 2024-04-08T15:35:15Z

Thanks for the review. I'm adding a test that checks that no '-1' is returned for misc-throw-by-value-catch-by-reference.MaxSize, that should pass even when no value is dumped for the option by default. I also added a test that sets 2⁶⁰ as a value for that option, and checks that the dumped value is 2⁶⁰ too.

ealcdan · 2024-04-22T07:48:47Z

ping :)

PiotrZSL

One code duplication that could be avoided.
Add entry in release notes, about fixing issue with big unsigned config option values printed as negative numbers in --dump-config

Overall, fine.
As for misc-throw-by-value-catch-by-reference check, we may need to use optional in future for this and fix documentation.

clang-tools-extra/clang-tidy/ClangTidyCheck.h

ealcdan · 2024-04-22T10:12:53Z

Great, both points have been addressed in the new patches, hope the release notes entry is clear enough.

clang-tools-extra/docs/ReleaseNotes.rst

Some options take the maximum unsigned integer value as default, but they are being dumped to a string as integers. This makes -dump-config write invalid '-1' values for these options. This change fixes this issue by using utostr if the option is unsigned. Change-Id: I551e6bc616071cf7aa10a8c0220d2076ed3d40e6

PiotrZSL · 2024-04-22T18:54:06Z

Unless you want this to be merged as 115098653+ealcdan@users.noreply.github.com, change your email privacy settings.

ealcdan · 2024-04-23T10:22:45Z

Sure, I just updated my settings, thanks for the suggestion.

github-actions · 2024-04-23T11:03:26Z

@ealcdan Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvmbot added clang-tools-extra clang-tidy labels Mar 13, 2024

EugeneZelenko requested a review from PiotrZSL March 13, 2024 13:44

PiotrZSL reviewed Mar 13, 2024

View reviewed changes

clang-tools-extra/clang-tidy/ClangTidyCheck.h Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/ClangTidyCheck.h Outdated Show resolved Hide resolved

clang-tools-extra/clang-tidy/ClangTidyCheck.h Outdated Show resolved Hide resolved

ealcdan force-pushed the main branch 3 times, most recently from 278a9a6 to 6ca7a38 Compare April 8, 2024 15:34

ealcdan force-pushed the main branch 2 times, most recently from f6196cf to 681f04d Compare April 9, 2024 09:34

ealcdan requested a review from PiotrZSL April 9, 2024 10:41

PiotrZSL approved these changes Apr 22, 2024

View reviewed changes

clang-tools-extra/clang-tidy/ClangTidyCheck.h Outdated Show resolved Hide resolved

ealcdan force-pushed the main branch 2 times, most recently from 20f7b31 to afe6658 Compare April 22, 2024 10:11

PiotrZSL reviewed Apr 22, 2024

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

PiotrZSL approved these changes Apr 22, 2024

View reviewed changes

ealcdan force-pushed the main branch from afe6658 to 0724887 Compare April 22, 2024 10:59

Merge branch 'main' into main

5a0304d

PiotrZSL merged commit c52b18d into llvm:main Apr 23, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang-tidy] Avoid overflow when dumping unsigned integer values #85060

[clang-tidy] Avoid overflow when dumping unsigned integer values #85060

ealcdan commented Mar 13, 2024 •

edited by PiotrZSL

github-actions bot commented Mar 13, 2024

ealcdan commented Mar 13, 2024

llvmbot commented Mar 13, 2024

llvmbot commented Mar 13, 2024

PiotrZSL left a comment

PiotrZSL commented Mar 13, 2024

ealcdan commented Apr 8, 2024

ealcdan commented Apr 22, 2024

PiotrZSL left a comment

ealcdan commented Apr 22, 2024

PiotrZSL commented Apr 22, 2024

ealcdan commented Apr 23, 2024

github-actions bot commented Apr 23, 2024

[clang-tidy] Avoid overflow when dumping unsigned integer values #85060

[clang-tidy] Avoid overflow when dumping unsigned integer values #85060

Conversation

ealcdan commented Mar 13, 2024 • edited by PiotrZSL

github-actions bot commented Mar 13, 2024

ealcdan commented Mar 13, 2024

llvmbot commented Mar 13, 2024

llvmbot commented Mar 13, 2024

PiotrZSL left a comment

Choose a reason for hiding this comment

PiotrZSL commented Mar 13, 2024

ealcdan commented Apr 8, 2024

ealcdan commented Apr 22, 2024

PiotrZSL left a comment

Choose a reason for hiding this comment

ealcdan commented Apr 22, 2024

PiotrZSL commented Apr 22, 2024

ealcdan commented Apr 23, 2024

github-actions bot commented Apr 23, 2024

ealcdan commented Mar 13, 2024 •

edited by PiotrZSL