Patch review archives from 2012

Hi!

Where can I find design discussions and patch reviews similar to reviews.llvm.org, but from 2012? There are a variety of commits to some files I’m working with in the Static Analyzer that could use more explanation. I didn’t have any luck reading through the cfe-dev and cfe-commits archives.

Cheers,
Kristóf Umann

Back then Static Analyzer was developed almost exclusively by the first generation of Analyzer developers which was a small team at Apple, none of which are still active Analyzer developers. I guess they didn't really need that much open documentation when they could simply look at each other's monitor and discuss things on a whiteboard. These days the community is much bigger (and i'm much more of a masochist who does not feel happy to just sit down and write code, and actively prevents others from doing the same, so that everybody knew the taste of code review pain), so Phabricator turned out to be a spot-on tool.

But you should ask around for specific things. It might be that people have already managed to re-discover the motivation behind certain solutions.

Okay, I’ll ask specifics.

I’ve spent the better part of last week familiarizing myself with MallocChecker’s code in order to eventually split it up (which has been a surprisingly pleasant experience!). In the process of learning how it works, I’m adding doxygen comments to every non-trivial method and non-trivial hackery, from what I can gather from old cfe-dev threads and bug reports.
To me it isn’t clear what ReallocPair[1] (added by this[2] commit) does. I can come up with some ideas, but I don’t have a grasp on it at all, sadly. The very cryptic boolean out-parameter ReleasedAllocated[3], related to [1], doesn’t help this case either. It was added by this[4] commit, but I’m still struggleing to understand what’s the happening here.

[1]https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/MallocChecker.cpp#L141
[2]https://github.com/llvm-mirror/clang/commit/c8bb3befcad8cd8fc9556bc265289b07dc3c94c8
[2]https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/MallocChecker.cpp#L341
[3]https://github.com/llvm-mirror/clang/commit/55dd956d521d4d650dfd929d67f4b98ede61c0ea

Artem Dergachev <noqnoqneo@gmail.com> ezt írta (időpont: 2018. nov. 20., K, 1:43):

Just looked at some of these. The canonical example here is the following leak:

1 void foo() {
2 void *x = malloc(1); // note: Memory is allocated
3 void *y = realloc(x, 2); // note: Attempt to reallocate memory
4 if (y) { // note: Assuming 'y' is null
5 // note: Reallocation failed
6 free(y);
7 } else {
8 // warning: Potential leak of memory pointed to by 'x'
9 }
10 }

The extra state trait is necessary to track the connection between 'x' and 'y' that appears on line 3 and dissolves on line 4. This is, essentially, yet another "Schrodinger" state split: realloc has both succeeded and failed until we check the return value. An eager state split would make null dereference checker unusable in any sort of code that doesn't check for failed reallocs.

On the other hand, using 'x' after realloc should probably be disallowed unless a prior branch in the code suggested that realloc has failed. And using 'y' would mean that the realloc is assumed to have succeeded.

Thank you so much! I was stuck on this for a good couple days. I’ll definitely document it in the actual code.