[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

This revision was automatically updated to reflect the committed changes. Closed by commit rG1deccd05ba8a: [analyzer] Retrieve a character from StringLiteral as an initializer for… (authored by ASDenysPetrov). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

martong accepted this revision. martong added a comment. Nice work! Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107339/new/ https://reviews.llvm.org/D107339 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added a comment. In D107339#3093207 , @steakhal wrote: > Next time I would rather have two patches, one of which adds tests to > document the existing behavior and a follow-up changing the behavior. It makes sense. I will take this into

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov updated this revision to Diff 382999. ASDenysPetrov edited the summary of this revision. ASDenysPetrov added a comment. Updated comments briefly explaining the root of the existing issue. Thanks, @steakhal and @martong. I appreciate your efforts! CHANGES SINCE LAST ACTION

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. There is no need to do anything with this. You can commit as-is. I've done it myself, and it seems like your patch only turns `TRUE` for the cases where it was `UNKNOWN` or `FALSE`

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1757-1760 + // FIXME: Nevertheless, we can't do the same for cases, like: + // const char *str = "123"; // literal length is 4 + // char c = str[41];// offset is 41 +

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1757-1760 + // FIXME: Nevertheless, we can't do the same for cases, like: + // const char *str = "123"; // literal length is 4 + // char c = str[41];// offset is 41 + // It

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1757-1760 + // FIXME: Nevertheless, we can't do the same for cases, like: + // const char *str = "123"; // literal length is 4 + // char c = str[41];// offset is 41 + // It

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

steakhal added a comment. I'm still worried about regressions. Please split the patch into two by separating the tests into an NFC patch, on which you would apply the behavioral change. That way it would be clear what and why changed. It would also help us to see what previously had defects you

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov updated this revision to Diff 382823. ASDenysPetrov added a comment. Added doc comment to `getSValFromStringLiteral` function. Fixed minor code defects. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107339/new/ https://reviews.llvm.org/D107339 Files:

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added a comment. I'm gonna add docs to `getSValFromStringLiteral` and update the patch. And of course will fix your nits. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1757-1760 + // FIXME: Nevertheless, we can't do the same for cases, like: + //

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

steakhal added a comment. It looks good to me. I don't mind that FIXME. That being said, I'm not even sure it's a FIXME. Check my comment inline about this. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1757-1760 + // FIXME: Nevertheless, we can't do the same for

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1757-1760 + // FIXME: Nevertheless, we can't do the same for cases, like: + // const char *str = "123"; // literal length is 4 + // char c = str[41];// offset is 41 + // It

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added a comment. @martong (inline) Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1757-1760 + // FIXME: Nevertheless, we can't do the same for cases, like: + // const char *str = "123"; // literal length is 4 + // char c = str[41];//

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1757-1760 + // FIXME: Nevertheless, we can't do the same for cases, like: + // const char *str = "123"; // literal length is 4 + // char c = str[41];// offset is 41 +

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1757-1760 + // FIXME: Nevertheless, we can't do the same for cases, like: + // const char *str = "123"; // literal length is 4 + // char c = str[41];// offset is 41 + // It

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov updated this revision to Diff 381533. ASDenysPetrov added a comment. Fixed test unpassing in `array-struct-region.c`. Added examples in comments according to suggestions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107339/new/ https://reviews.llvm.org/D107339 Files:

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1708-1710 + // Handle StringLiteral. + if (const auto *SL = dyn_cast(Init)) +return getSValFromStringLiteral(SL, Offset, R->getElementType()); ASDenysPetrov wrote: >

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1708-1710 + // Handle StringLiteral. + if (const auto *SL = dyn_cast(Init)) +return getSValFromStringLiteral(SL, Offset, R->getElementType()); martong wrote: > I

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1708-1710 + // Handle StringLiteral. + if (const auto *SL = dyn_cast(Init)) +return getSValFromStringLiteral(SL, Offset, R->getElementType()); I am wondering why this

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added a comment. @martong > I am close to accepting this. However, D111542 > should be its parent patch. I think > similar issues could arise here as well, so, redecl chain tests would be > really beneficial. I think I'm done. You can proceed

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added a comment. In D107339#3075306 , @steakhal wrote: > Why does `glob_invalid_index7()` and `glob_invalid_index8()` differ in > behavior? > I would expect that the analyzer produces the same `Loc` symbolic value for > both cases thus,

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov updated this revision to Diff 381274. ASDenysPetrov added a comment. Rebased. Improved behavior to make `glob_invalid_index8` case passed and some other cases. Added more tests. Added tests for universal characters. CHANGES SINCE LAST ACTION

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

steakhal added a comment. Why does `glob_invalid_index7()` and `glob_invalid_index8()` differ in behavior? I would expect that the analyzer produces the same `Loc` symbolic value for both cases thus, the array access should result in the same behavior regardless if `glob_arr6` is used, or

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added a comment. In D107339#3060812 , @martong wrote: > I am close to accepting this. However, D111542 > should be its parent patch. I think > similar issues could arise here as well, so, redecl chain

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

martong added a comment. I am close to accepting this. However, D111542 should be its parent patch. I think similar issues could arise here as well, so, redecl chain tests would be really beneficial. CHANGES SINCE LAST ACTION

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov updated this revision to Diff 376896. ASDenysPetrov added a comment. Updated according to suggestions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107339/new/ https://reviews.llvm.org/D107339 Files: clang/lib/StaticAnalyzer/Core/RegionStore.cpp

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added inline comments. Comment at: clang/test/Analysis/initialization.cpp:132 + +char const glob_arr6[5] = "123"; +void glob_array_index4() { steakhal wrote: > Ah, it's somewhat confusing. > At first, when I looked at it, I assumed that this array

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1641 + // FIXME: Take array dimension into account to prevent exceeding its size. + const int64_t I = Idx.getExtValue(); + uint32_t Code = ASDenysPetrov wrote: >

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1641 + // FIXME: Take array dimension into account to prevent exceeding its size. + const int64_t I = Idx.getExtValue(); + uint32_t Code = martong wrote: > steakhal

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

martong added a comment. @ASDenysPetrov This looks promising! Please address the nits which @steakhal found, than I think it is good to land. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1641 + // FIXME: Take array dimension into account to prevent exceeding its

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov updated this revision to Diff 373908. ASDenysPetrov added a comment. Rebased. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107339/new/ https://reviews.llvm.org/D107339 Files: clang/lib/StaticAnalyzer/Core/RegionStore.cpp clang/test/Analysis/initialization.cpp

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

steakhal added a comment. Looks great. Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1636-1640 + // Technically, only i == length is guaranteed to be null. + // However, such overflows should be caught before reaching this point; + // the only time such an

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov updated this revision to Diff 370611. ASDenysPetrov edited the summary of this revision. ASDenysPetrov added a comment. Adjusted the patch according to changes in the parent revision D104285 . CHANGES SINCE LAST ACTION

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov updated this revision to Diff 366083. ASDenysPetrov added a comment. Rebased. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107339/new/ https://reviews.llvm.org/D107339 Files: clang/lib/StaticAnalyzer/Core/RegionStore.cpp Index:

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov updated this revision to Diff 363713. ASDenysPetrov added a comment. Added tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107339/new/ https://reviews.llvm.org/D107339 Files: clang/lib/StaticAnalyzer/Core/RegionStore.cpp clang/test/Analysis/initialization.cpp

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added a comment. In D107339#2921896 , @vsavchenko wrote: > That looks interesting! > Can you please add tests, though? Yes, I'll add them soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

vsavchenko added a comment. That looks interesting! Can you please add tests, though? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107339/new/ https://reviews.llvm.org/D107339 ___ cfe-commits mailing

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov added a comment. TODO: add regression tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107339/new/ https://reviews.llvm.org/D107339 ___ cfe-commits mailing list

[PATCH] D107339: [analyzer] Retrieve a character from StringLiteral as an initializer for constant arrays.

ASDenysPetrov created this revision. ASDenysPetrov added reviewers: NoQ, rsmith, martong, vsavchenko. ASDenysPetrov added a project: clang. Herald added subscribers: manas, steakhal, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun.