[clang] Fix crash in __builtin_strncmp and other related builtin functions

[shafik]

Summary

The implementation of __builtin_strncmp and other related builtins function use getExtValue() to evaluate the size argument. This can cause a crash when the value does not fit into an int64_t value, which is can be expected since the type of the argument is size_t.

The fix is to switch to using getZExtValue().

This fixes:

https://github.com/llvm/llvm-project/issues/64876

[erichkeane]

Are there any Sema tests we can add to show that we warn/diagnose/SOMETHING on these? If someone passes a negative size, we should probably at least do the warning that it was converted/truncated.

clang/lib/AST/ExprConstant.cpp

9357

Context missing throughout

[shafik]

• Diff w/ context

[shafik]

-Updated values used in test

[shafik]

In D158557#4608262, @erichkeane wrote:

Are there any Sema tests we can add to show that we warn/diagnose/SOMETHING on these? If someone passes a negative size, we should probably at least do the warning that it was converted/truncated.

The arguments have type size_t so using -Wsign-conversion would warn for the previous value but that is not really the point of the fix, which is why I updated the value to clarify.

[aaron.ballman]

Precommit CI found issues that should be addressed, but otherwise the changes LGTM. You should add a release note for the fix, though!

[erichkeane]

I think there is value to ensuring we diagnose the negative-to-size_t conversion here, but I also think there is value to a CodeGen test to ensure we emit the correct value.

[shafik]

• Silence -Wconstant-conversion warning in test

[erichkeane]

Still think a codegen test for this example is VERY valuable here.

clang/test/SemaCXX/constexpr-string.cpp

681

rather than suppress these, it makes sense to me to just mark them expected-warning.

[shafik]

clang/test/SemaCXX/constexpr-string.cpp

681

It depends on the size of size_t whether I get this diagnostic or not.

[aaron.ballman]

clang/test/SemaCXX/constexpr-string.cpp

681

Given that the point to this test isn't to test constant conversion behavior but is testing boundary cases where you'll get those diagnostics, I think suppressing the warnings is fine.

[shafik]

• Add codegen test

[aaron.ballman]

LGTM but the changes need to be landed with a release note.

[erichkeane]

I suggest doing the -511LL value from the original bug report in the tests as well, else, fine to me.

[erichkeane]

Ah, wait, still no release note!

[erichkeane]

I see aaron mentioned the release note and is ok with it happening on commit, so I'll undo my 'needs revision'.

[shafik]

• Extended constexpr-string.cpp test
• Added release notes

[erichkeane]

Still LGTM.

clang/lib/AST/ExprConstant.cpp

       APSInt N;
       if (!EvaluateInteger(E->getArg(2), N, Info))
         return false;
-      MaxLength = N.getExtValue();
+      MaxLength = N.getZExtValue();
     }
     // We cannot find the value if there are no candidates to match against.
     if (MaxLength == 0u)

       APSInt N;
       if (!EvaluateInteger(E->getArg(2), N, Info))
         return false;
-      MaxLength = N.getExtValue();
+      MaxLength = N.getZExtValue();
     }
 
     // Empty substrings compare equal by definition.

clang/test/SemaCXX/constexpr-string.cpp

   }
   static_assert(test_address_of_incomplete_struct_type()); // expected-error {{constant}} expected-note {{in call}}
 }
+
+namespace GH64876 {
+void f() {
+  __builtin_strncmp(0, 0, -511LL);
+  __builtin_memcmp(0, 0, -511LL);
+  __builtin_bcmp(0, 0, -511LL);
+  __builtin_wmemcmp(0, 0, -511LL);
+  __builtin_memchr((const void*)0, 1, -511LL);
+  __builtin_wmemchr((const wchar_t*)0, 1, -511LL);
+}
+}