search

Ericsson / clang

Cross Translation Unit analysis capability for Clang Static Analyzer. (Fork of official clang at http://llvm.org/git/clang)
http://clang.llvm.org/
Other
15 stars 10 forks source link

Correctly range check structs/unions when they are coming from a macro #436

Closed gerazo closed 6 years ago

gerazo commented 6 years ago

Fixes: https://github.com/Ericsson/clang/issues/416 On tmux, there errors are fixed and tmux is checked without a problem. On redis, the errors are all turned into "not a total ordering" problems. This change definitely allows more things to be imported, so there is a chance that more problems will come, still, it raises coverage.

gerazo commented 6 years ago

Well, the point is to make sure that a declaration is in the body of a function. I don't know how to do this without comparing ranges, but maybe there is a way. My other problem is that how robust is this solution. The record can come from a macro, the function can also come from that... It seems that getFileLoc is used for such thing at many places and inside it, it follows the expansion until it finds an isFileID loc. Still there may be better get***Loc functions for this.

martong commented 6 years ago

There is one other think which is not entirely clean. How was this connected to "total ordering"?

gerazo commented 6 years ago

The total ordering problem is a different bug currently here: https://github.com/Ericsson/clang/issues/428 probably needs rephrasing. There is no connection. In fact, the total ordering problem comes only late when all the checkers found problems and those are about to be reported. So if you have any crash earlier, than the crash supresses the total ordering problem.

gerazo commented 6 years ago

Check this out now @balazske and @martong . Tests are running perfectly, tmux and redis gives the exact same result. I would say, it is identical to the previous solution.

martong commented 6 years ago

LGTM, just could you please squash the first two commits into one? (I think it is still worth to preserve the source range version in a commit).

gerazo commented 6 years ago

@martong There are 4 commits, the first 2 should be squashed? The history will have the newer state anyway, no? Probably I don't get the purpose.

martong commented 6 years ago

@gerazo, Doesn't really matter, just would be good to preserve the SourceRange based version somehow in the history.

martong commented 6 years ago

Actually, the first commit out of the four commits keep the SourceLocation based version. So, let's just squash the last 3 commit into one. Does it make sense?

gerazo commented 6 years ago

Ok, I did it.

martong commented 6 years ago

Thanks Zozo, nice work! :)

gerazo commented 6 years ago

Thanks. Can I start the upstreaming?

martong commented 6 years ago

Of course :)

gerazo commented 6 years ago

Review is open: https://reviews.llvm.org/D49792