Page MenuHomePhabricator

[analyzer] Put llvm.Conventions back in alpha
ClosedPublic

Authored by Szelethus on Oct 29 2018, 6:30 PM.

Details

Summary

Interestingly, this many year old (when I last looked I remember 2010ish) checker was committed without any tests, so I thought I'd implement them, but I was shocked to see how I barely managed to get it working. The code is severely outdated, I'm not even sure it has ever been used, so I'd propose to move it back into alpha, and possibly even remove it.

Diff Detail

Repository
rC Clang

Event Timeline

Szelethus created this revision.Oct 29 2018, 6:30 PM
Szelethus updated this revision to Diff 171623.Oct 29 2018, 6:36 PM

Added an entry to the website.

Szelethus added inline comments.Oct 29 2018, 6:51 PM
lib/StaticAnalyzer/Checkers/LLVMConventionsChecker.cpp
193–195

The StringRefpart of this checker doesn't do a thing for me at all (even after a good amount of trying to make it work), but this part of the checker suffers a lot too.

I'm sure we can some up with better ways for this function (and every single one in this file to be honest) now that almost a decade has passed.

NoQ accepted this revision.Oct 29 2018, 7:15 PM

I've actually never noticed this checker until you started updating the website :)

This might be also covered by @rnkovacs's string buffer escape checker - either already or eventually, it'll become just yet another string view API that the checker supports.

test/Analysis/llvm-conventions.cpp
9

I think we should try to re-use (and possibly extend) test/Analysis/inputs/system-header-simulator-cxx.h.

This revision is now accepted and ready to land.Oct 29 2018, 7:15 PM
MTC added a comment.Oct 29 2018, 7:46 PM

In addition, clang/lib/StaticAnalyzer/README.txt and other related docs in clang/lib/docs/analyzer are also out of date.

www/analyzer/alpha_checks.html
599

Like StringRef, ArrayRef may have similar issues. After this patch landed, maybe we can enhance this checker step by step. By that time, we may need the ideas of the heavy users of the LLVM library.

In D53856#1279887, @NoQ wrote:

This might be also covered by @rnkovacs's string buffer escape checker - either already or eventually, it'll become just yet another string view API that the checker supports.

I thought about that too, adding some StringRef specific information to that checker makes more sense then essentially duplicating the logic. However, what @MTC mentioned about ArrayRef<T> would be a neat addition too, and maybe it isn't worth making InnerPointerChecker that general.

@rnkovacs, any thoughts?

Szelethus updated this revision to Diff 172253.Nov 1 2018, 3:32 PM
  • Moved std::string implementation from InnerPointerChecker's testfile to the system header simulator header file.
In D53856#1279887, @NoQ wrote:

This might be also covered by @rnkovacs's string buffer escape checker - either already or eventually, it'll become just yet another string view API that the checker supports.

I thought about that too, adding some StringRef specific information to that checker makes more sense then essentially duplicating the logic. However, what @MTC mentioned about ArrayRef<T> would be a neat addition too, and maybe it isn't worth making InnerPointerChecker that general.

@rnkovacs, any thoughts?

I agree, adding StringRef support to the buffer escape checker around the time we add string_view will be great. Also, we named it InnerPointerChecker to allow for covering more general structures in the future, so I guess ArrayRefs may also be added eventually. Unfortunately, I have some other tasks to finish before getting to these, so I think this patch is fine now as-is.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.
NoQ added a comment.EditedNov 2 2018, 3:30 PM
In D53856#1279887, @NoQ wrote:

This might be also covered by @rnkovacs's string buffer escape checker - either already or eventually, it'll become just yet another string view API that the checker supports.

I thought about that too, adding some StringRef specific information to that checker makes more sense then essentially duplicating the logic. However, what @MTC mentioned about ArrayRef<T> would be a neat addition too, and maybe it isn't worth making InnerPointerChecker that general.

@rnkovacs, any thoughts?

I agree, adding StringRef support to the buffer escape checker around the time we add string_view will be great. Also, we named it InnerPointerChecker to allow for covering more general structures in the future, so I guess ArrayRefs may also be added eventually. Unfortunately, I have some other tasks to finish before getting to these, so I think this patch is fine now as-is.

I also agree that we should generally try harder to determine common logic behind checkers. Eg., MallocChecker and StreamChecker should be the same checker, or at least re-use this fairly large chunk of common code that implements the overall idea of "one function produces a symbol, then another function consumes it, and the consuming function always needs to be called on that symbol, while the symbol should not be used in certain (almost all) manners after it is consumed".