Page MenuHomePhabricator

[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
1022

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
1022

Ok, I've moved it.

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

Notify the user about the contract.

1095–1097

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
1088

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
1088

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

1095–1097

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.Wed, Jun 8, 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
776–785
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.Sat, Jun 18, 11:22 AM
clang/test/Analysis/symbol-simplification-mem-region-to-int-cast.cpp
24

Hmm, I meant it for line 39 :D