mgehre / llvm-project

The home of the clang-based implementation of lifetime safety warnings.
39 stars 4 forks source link

Type caching issues #82

Open Xazax-hun opened 4 years ago

Xazax-hun commented 4 years ago

Ironically, there is a big lifetime problem in our lifetime analysis.

Currently, we use to maps to cache Types (category and pointee) with static lifetime.

The problem is the following: if you use clang-tidy it will do multiple invocations in the same process and the maps will not be cleared between the invocations. The pointers in the cache are meaningless after the first invocation and this is basically a use after free error. A good question is where should we actually put the cache? It should have the same lifetime as ASTContext, because the types we reference live in ASTContext. Should we just dump it there? Will that pass the review?

A little side note, why do we have two caches in the first place? We only need pointee types for owners and pointers so we should be able to get away with only one.

@mgehre thoughts?

mgehre commented 4 years ago

As far as I know, clang-tidy uses a single clang instance per process. Did you witness an actual problem?

Which caches are you referring to specifically?

Xazax-hun commented 4 years ago

Yeah, I witnessed the issue and spent like ~6 hours figuring out what was wrong :D

Actually, this is not specific to clang tidy. Any tool relying on libTooling would have the same problem.

See https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Tooling.cpp#L546

Xazax-hun commented 4 years ago

And this is one of the caches: https://github.com/mgehre/llvm-project/blob/lifetime/clang/lib/Analysis/LifetimeTypeCategory.cpp#L277

The other one is very similar in the same file.

mgehre commented 4 years ago

I think that cache could live in the LifetimeContext, https://github.com/mgehre/llvm-project/blob/lifetime/clang/lib/Analysis/Lifetime.cpp#L36

Xazax-hun commented 4 years ago

It would do some caching, but not us much as before as LifetimeContext is recreated for each function. If we have a lot of small functions it might end up being a pessimization.

mgehre commented 4 years ago

True, I wonder if there is a way to have a GlobalLifetimeContext per AST.