This is an archive of the discontinued LLVM Phabricator instance.

[Analyzer] Produce SymbolCast for pointer to integer cast
Needs ReviewPublic

Authored by martong on Jan 13 2022, 8:18 AM.

Details

Summary

Produce SymbolCast for a pointer-to-integer cast if the
support-symbolic-integer-casts flag is enabled.

Diff Detail

Event Timeline

martong created this revision.Jan 13 2022, 8:18 AM
martong requested review of this revision.Jan 13 2022, 8:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2022, 8:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ASDenysPetrov added inline comments.Jan 13 2022, 8:39 AM
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
1044

And it'd be great if you bring the original comment here, as the logic is here.

martong updated this revision to Diff 401274.Jan 19 2022, 9:00 AM
martong marked an inline comment as done.
  • Move the comment hunk
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
1044

Ok, I've moved it.

ASDenysPetrov added inline comments.Jan 20 2022, 9:14 AM
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
1110

Notify the user about the contract.

1117–1119

Explain please, why do you need to create a new SymbolVal here, but not just returning V as long as you know that the type is the same?
And assert also seems weird for me. If there's smth obscure that you're trying to handle, please add some explaination comment.

ASDenysPetrov added inline comments.Jan 20 2022, 9:15 AM
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
1110

Mistake. CastTy, not SafeTy

martong marked 3 inline comments as done.May 6 2022, 6:49 AM

Thanks for the review Denys, and sorry for the long delay with the update. I hope that this patch is going to complement nicely the rest of the cast patches.

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
1110

Okay, I've added an assertion for this check.

1117–1119

You're right, we can just simply return V, I've updated like so. (I just wanted to make sure that the makeSymbolVal is consistent, but that has nothing to do with this patch.)

Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2022, 6:49 AM
martong updated this revision to Diff 427621.May 6 2022, 6:49 AM
martong marked 2 inline comments as done.
  • add new assert for canonical type
  • remove superflous assert about makeSymbolVal
martong updated this revision to Diff 435152.Jun 8 2022, 7:20 AM
  • Rebase to llvm/main

Looks correct to me.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
110–113

You don't need to have gap between these two. They belong in the same overload set.

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
798–807
clang/test/Analysis/produce-ptr-to-integer-symbolcast.cpp
25–28

Is this the only obstacle to eradicate LocAsInteger?

clang/test/Analysis/symbol-simplification-mem-region-to-int-cast.cpp
2–7

I think you need to pin the target. You assume a specific pointer bitwidth in the test.

24

Why is this `UNKNOWN? I would expect TRUE here as well.

steakhal added inline comments.Jun 18 2022, 11:22 AM
clang/test/Analysis/symbol-simplification-mem-region-to-int-cast.cpp
24

Hmm, I meant it for line 39 :D