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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG33b6b674620d: [clang] Fix crash in __builtin_strncmp and other related builtin functions (authored by shafik). Herald added a project: clang.

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

erichkeane accepted this revision. erichkeane added a comment. Still LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

shafik updated this revision to Diff 553601. shafik added a comment. - Extended constexpr-string.cpp test - Added release notes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557 Files: clang/docs/ReleaseNotes.rst

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

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I see aaron mentioned the release note and is ok with it happening on commit, so I'll undo my 'needs revision'. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/

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

erichkeane requested changes to this revision. erichkeane added a comment. This revision now requires changes to proceed. Ah, wait, still no release note! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557

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

erichkeane added a comment. I suggest doing the -511LL value from the original bug report in the tests as well, else, fine to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557 ___ cfe-commits

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM but the changes need to be landed with a release note. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557

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

shafik updated this revision to Diff 553270. shafik marked 2 inline comments as done. shafik added a comment. - Add codegen test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557 Files: clang/lib/AST/ExprConstant.cpp

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

aaron.ballman added inline comments. Comment at: clang/test/SemaCXX/constexpr-string.cpp:681 +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wconstant-conversion" +namespace GH64876 { shafik wrote: > erichkeane wrote: > > rather than suppress

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

shafik added inline comments. Comment at: clang/test/SemaCXX/constexpr-string.cpp:681 +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wconstant-conversion" +namespace GH64876 { erichkeane wrote: > rather than suppress these, it makes sense to

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

erichkeane added a comment. Still think a codegen test for this example is VERY valuable here. Comment at: clang/test/SemaCXX/constexpr-string.cpp:681 +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wconstant-conversion" +namespace GH64876 {

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

shafik updated this revision to Diff 553195. shafik added a comment. - Silence -Wconstant-conversion warning in test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557 Files: clang/lib/AST/ExprConstant.cpp

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

erichkeane added a comment. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/

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

aaron.ballman added a comment. Precommit CI found issues that should be addressed, but otherwise the changes LGTM. You should add a release note for the fix, though! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557

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

shafik added a comment. 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

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

shafik updated this revision to Diff 552518. shafik added a comment. -Updated values used in test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557 Files: clang/lib/AST/ExprConstant.cpp clang/test/SemaCXX/constexpr-string.cpp Index:

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

shafik updated this revision to Diff 552516. shafik added a comment. - Diff w/ context CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557 Files: clang/lib/AST/ExprConstant.cpp clang/test/SemaCXX/constexpr-string.cpp Index:

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

erichkeane added a comment. 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. Comment at: clang/lib/AST/ExprConstant.cpp:9357

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

shafik created this revision. shafik added reviewers: aaron.ballman, erichkeane, davide, rsmith. Herald added a project: All. shafik requested review of this revision. The implementation of `__builtin_strncmp` and other related builtins function use `getExtValue()` to evaluate the size argument.