[clang-tools-extra] [clang-tidy] Fix broken fix-its with `bugprone-not-null-terminated-result` on Windows (PR #162874)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tidy

Author: Victor Chernyakin (localspook)

<details>
<summary>Changes</summary>

(See the test changes for a description of the problem.)

This is the problematic code:

https://github.com/llvm/llvm-project/blob/424fa833352177da4ea39238c040703e1dc3a0ab/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp#L310-L313

The key thing to know is that, on Windows, `unsigned long` is `uint32_t`.

Consider the `(isInjectUL(Result) ? 1UL : 1)` bit. The common type of `1UL` (`uint32_t`) and `1` (`int`) is `uint32_t`, so the type of the overall expression is `uint32_t`. Now consider the bigger `(... ? (isInjectUL(Result) ? 1UL : 1) : -1)`. We just saw that the type of the first expression is `uint32_t`. The type of `-1` is `int`, so once again, the type of the overall expression is `uint32_t`. That means the `-1` becomes `UINT32_MAX`. `getZExtValue()`, however, returns `uint64_t`, so when we go to add this `UINT32_MAX` to it, it's zero-extended, and boom, instead of subtracting 1, we get a very big `uint64_t` value.

Implicit conversions strike again!

(While we're at it: this 64-bit result is stored in a `size_t`. That's only 32 bits on a 32-bit system, so we have a narrowing conversion. That's rather ugly, so this PR also fixes that.)

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


4 Files Affected:

- (modified) clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp (+3-4) 
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) 
- (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c (-5) 
- (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp (-5) 


``````````diff
diff --git a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
index d4676842a97ff..39e3a542aaf6b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
@@ -307,10 +307,9 @@ static void lengthExprHandle(const Expr *LengthExpr,
   // Try to obtain an 'IntegerLiteral' and adjust it.
   if (!IsMacroDefinition) {
     if (const auto *LengthIL = dyn_cast<IntegerLiteral>(LengthExpr)) {
-      size_t NewLength = LengthIL->getValue().getZExtValue() +
-                         (LengthHandle == LengthHandleKind::Increase
-                              ? (isInjectUL(Result) ? 1UL : 1)
-                              : -1);
+      uint64_t NewLength =
+          LengthIL->getValue().getZExtValue() +
+          (LengthHandle == LengthHandleKind::Increase ? 1 : -1);
 
       const auto NewLengthFix = FixItHint::CreateReplacement(
           LengthIL->getSourceRange(),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index d2d79dcc92ec2..f0b21b88dd998 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -274,6 +274,10 @@ Changes in existing checks
   <clang-tidy/checks/bugprone/narrowing-conversions>` check by fixing
   false positive from analysis of a conditional expression in C.
 
+- Improved :doc:`bugprone-not-null-terminated-result
+  <clang-tidy/checks/bugprone/not-null-terminated-result>` check by fixing
+  bogus fix-its for `strncmp` and `wcsncmp` on Windows.
+
 - Improved :doc:`bugprone-reserved-identifier
   <clang-tidy/checks/bugprone/reserved-identifier>` check by ignoring
   declarations and macros in system headers.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c
index 366c1698e4f2d..795ab3d652f9f 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-strlen.c
@@ -1,11 +1,6 @@
 // RUN: %check_clang_tidy --match-partial-fixes %s bugprone-not-null-terminated-result %t -- \
 // RUN: -- -I %S/Inputs/not-null-terminated-result
 
-// FIXME: Something wrong with the APInt un/signed conversion on Windows:
-// in 'strncmp(str6, "string", 7);' it tries to inject '4294967302' as length.
-
-// UNSUPPORTED: system-windows
-
 #include "not-null-terminated-result-c.h"
 
 #define __STDC_LIB_EXT1__ 1
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp
index 06e2db9d6e0d6..288c5faabb0ec 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/not-null-terminated-result-wcslen.cpp
@@ -1,11 +1,6 @@
 // RUN: %check_clang_tidy --match-partial-fixes %s bugprone-not-null-terminated-result %t -- \
 // RUN: -- -std=c++11 -I %S/Inputs/not-null-terminated-result
 
-// FIXME: Something wrong with the APInt un/signed conversion on Windows:
-// in 'wcsncmp(wcs6, L"string", 7);' it tries to inject '4294967302' as length.
-
-// UNSUPPORTED: system-windows
-
 #include "not-null-terminated-result-cxx.h"
 
 #define __STDC_LIB_EXT1__ 1

``````````

</details>


https://github.com/llvm/llvm-project/pull/162874