[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-02-17 Thread Artem Dergachev via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rG5a11233a2fa5: [analyzer] VforkChecker: allow execve after vfork. (authored by dergachev.a). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73629/new/

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-02-17 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Thank you!! I'll commit. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73629/new/ https://reviews.llvm.org/D73629 ___ cfe-commits mailing

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-02-17 Thread Jan Včelák via Phabricator via cfe-commits
janvcelak updated this revision to Diff 245025. janvcelak added a comment. I'm attaching updated version with tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73629/new/ https://reviews.llvm.org/D73629 Files: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-02-12 Thread Kristóf Umann via Phabricator via cfe-commits
Szelethus added a comment. LGTM granted the test is supplied, nice catch! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73629/new/ https://reviews.llvm.org/D73629 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-02-11 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Let's still add the test. It'll make sure that you understand exactly what you're doing and that there aren't more aspects to this problem. More tests never hurt and it's a local culture to have at least some tests for every commit that changes the behavior. Our tests are

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-02-11 Thread Jan Včelák via Phabricator via cfe-commits
janvcelak added a comment. Herald added a subscriber: martong. Please, can the reviewers respond to my comments? I would like to know if this needs additional revision or if it can be accepted as is. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73629/new/

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-01-30 Thread Jan Včelák via Phabricator via cfe-commits
janvcelak added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp:109 "execvpe", + "execve", nullptr xazax.hun wrote: > Well, this is not the case now, but I wonder if it would also make sense to > sort this list

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-01-30 Thread Jan Včelák via Phabricator via cfe-commits
janvcelak added a comment. In D73629#1847750 , @NoQ wrote: > We should add a test for this (cf. `test/Analysis/vfork.c`). There is already a test to check if the whitelist works by making sure that call to `execl` doesn't generate the warning. I think

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-01-29 Thread Gábor Horváth via Phabricator via cfe-commits
xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp:109 "execvpe", + "execve", nullptr Well, this is not the case now, but I wonder if it would also make sense to sort this list alphabetically.

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-01-29 Thread Jan Včelák via Phabricator via cfe-commits
janvcelak updated this revision to Diff 241245. janvcelak added a comment. Sorry. I missed that step in contribution guidelines. I'm attaching updated version with larger context. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73629/new/ https://reviews.llvm.org/D73629 Files:

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-01-29 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ added a comment. Yup, thanks, nice catch! We should add a test for this (cf. `test/Analysis/vfork.c`). Also do you have any immediate opinions on https://bugs.llvm.org/show_bug.cgi?id=43871? 'Cause i'm super confused. Like, it's trivial to fix but i'm not sure what the correct behavior

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-01-29 Thread Csaba Dabis via Phabricator via cfe-commits
Charusso added a reviewer: NoQ. Charusso added a comment. Hey, thanks! The patch looks great, but please note that we do the reviews with context using `git diff -U99` or uploading with `arc` (https://secure.phabricator.com/book/phabricator/article/arcanist/). Repository: rG LLVM Github

[PATCH] D73629: [analyzer] vfork checker: allow execve after vfork

2020-01-29 Thread Jan Včelák via Phabricator via cfe-commits
janvcelak created this revision. janvcelak added a reviewer: dcoughlin. Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. Herald added a project: clang. `execve` is missing in the list of functions