Patch review archives from 2012

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Patch review archives from 2012

Oleg Smolsky via cfe-dev
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

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Patch review archives from 2012

Oleg Smolsky via cfe-dev
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.

On 11/19/18 3:57 PM, Kristóf Umann via cfe-dev wrote:

> Hi!
>
> Where can I find design discussions and patch reviews similar to
> reviews.llvm.org <http://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
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Patch review archives from 2012

Oleg Smolsky via cfe-dev
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.



Artem Dergachev <[hidden email]> ezt írta (időpont: 2018. nov. 20., K, 1:43):
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.

On 11/19/18 3:57 PM, Kristóf Umann via cfe-dev wrote:
> Hi!
>
> Where can I find design discussions and patch reviews similar to
> reviews.llvm.org <http://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
>
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Patch review archives from 2012

Oleg Smolsky via cfe-dev
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.

On 11/19/18 5:00 PM, Kristóf Umann wrote:

> 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 <[hidden email] <mailto:[hidden email]>> ezt
> írta (időpont: 2018. nov. 20., K, 1:43):
>
>     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.
>
>     On 11/19/18 3:57 PM, Kristóf Umann via cfe-dev wrote:
>     > Hi!
>     >
>     > Where can I find design discussions and patch reviews similar to
>     > reviews.llvm.org <http://reviews.llvm.org>
>     <http://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
>     >
>     >
>     > _______________________________________________
>     > cfe-dev mailing list
>     > [hidden email] <mailto:[hidden email]>
>     > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Patch review archives from 2012

Oleg Smolsky via cfe-dev
Thank you so much! I was stuck on this for a good couple days. I'll definitely document it in the actual code.

On 20 Nov 2018 09:12, "Artem Dergachev" <[hidden email]> wrote:
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.


On 11/19/18 5:00 PM, Kristóf Umann wrote:
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 <[hidden email] <mailto:[hidden email]>> ezt írta (időpont: 2018. nov. 20., K, 1:43):

    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.

    On 11/19/18 3:57 PM, Kristóf Umann via cfe-dev wrote:
    > Hi!
    >
    > Where can I find design discussions and patch reviews similar to
    > reviews.llvm.org <http://reviews.llvm.org>
    <http://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
    >
    >
    > _______________________________________________
    > cfe-dev mailing list
    > [hidden email] <mailto:[hidden email]>
    > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev