Hi Chris,
You said “Having functionality like this in mainline would be really interesting.”
Could you suggest us our next steps?
Btw, most our tests already work on Mac 10.6 (32- and 64- bit).
–kcc
Hi Kostya,
I haven't had a chance to look at your patch yet, I'm backed up on "big patches". Did you see my review of the safecode patch here?
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20110718/124515.html
I expect to have similar concerns and suggestions for your patch,
-Chris
Hi,
What would be our next steps in getting ASan into the LLVM trunk?
I’d like to do it in two steps, first for the LLVM part with minimal tests and then for the run-time library and all tests.
The current ASan’s source repository will probably stay the primary home for the run-time library and tests as we plan to use it in non-LLVM environments.Hi Kostya,
I haven’t had a chance to look at your patch yet, I’m backed up on “big patches”. Did you see my review of the safecode patch here?
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20110718/124515.htmlI expect to have similar concerns and suggestions for your patch,
Hi Chris,
Thanks for the reply.
Indeed, some of your comments to safecode patch apply here.
Codingstyle: I’ll try to cleanup as much as I can today.
Do you have any lint-like tool that checks for llvm coding style (similar to cpplint)?
Documentation: everything is in the wiki. The main pages are:
http://code.google.com/p/address-sanitizer/wiki/AddressSanitizer
http://code.google.com/p/address-sanitizer/wiki/AddressSanitizerAlgorithm
“who is going to maintain”: my team at Google (in particular, myself and Alexander, in CC) are highly motivated to keep this working.
“Do you have particular clients”: the Chromium project is a very active client.
“The work can be decomposed into small and incremental pieces”:the LLVM part is just 1000 LOC. If you still like it to be decomposed, we can do it in 3 parts: a) general instrumentation, b) redzones for stack, c) redzones for globals.
Thanks,
–kcc
Any updates on this?
In particular, I’d like to see concrete patches proposed for review and inclusion into LLVM. I think having actual patches on the table and under review will help a great deal. Kostya, let me know if I can help prepare them. A few general comments as well inline…
Hi,
What would be our next steps in getting ASan into the LLVM trunk?
I’d like to do it in two steps, first for the LLVM part with minimal tests and then for the run-time library and all tests.
The current ASan’s source repository will probably stay the primary home for the run-time library and tests as we plan to use it in non-LLVM environments.Hi Kostya,
I haven’t had a chance to look at your patch yet, I’m backed up on “big patches”. Did you see my review of the safecode patch here?
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20110718/124515.htmlI expect to have similar concerns and suggestions for your patch,
Hi Chris,
Thanks for the reply.
Indeed, some of your comments to safecode patch apply here.Codingstyle: I’ll try to cleanup as much as I can today.
Do you have any lint-like tool that checks for llvm coding style (similar to cpplint)?
I don’t know of any effective ones. Kostya, maybe you can send the code by myself and/or Nick to help walk through it for style issues?
Documentation: everything is in the wiki. The main pages are:
http://code.google.com/p/address-sanitizer/wiki/AddressSanitizer
http://code.google.com/p/address-sanitizer/wiki/AddressSanitizerAlgorithm
It would be good to have these written up in HTML for easy inclusion in the existing LLVM documentation tree. This tree is checked in along side the code itself. However, contributing snapshots of the documentation from the wiki page could likely occur in follow-up patches. Alternatively, maybe Chris would be OK linking to these wiki pages from the primary LLVM documentation.
“who is going to maintain”: my team at Google (in particular, myself and Alexander, in CC) are highly motivated to keep this working.
“Do you have particular clients”: the Chromium project is a very active client.
For reference, myself and others on my team at Google are working actively with Clang and LLVM and would be able to maintain, fix bugs, and update this. We also have a very large base of users that would be interested in the functionality.
“The work can be decomposed into small and incremental pieces”:
the LLVM part is just 1000 LOC. If you still like it to be decomposed, we can do it in 3 parts: a) general instrumentation, b) redzones for stack, c) redzones for globals.
I think breaking up the LLVM part would be very helpful. Can you break out the first part, and after a style cleanup mail the attached patch? Then the review for that can overlap with preparing the other 2 parts.
How do you plan to test this? It’s important that there are regression tests in the LLVM suite that exercise the functionality independent of any runtime so that other developers can catch regressions. Also, unittests in the LLVM unittest tree would be nice as well.
Have you written a Clang patch to turn this functionality on and off? Looking at the wiki documentation shows one thing that gives me pause: you’re using the -mllvm flag in Clang to directly pass options down to the LLVM layer. Also, it indicates the asan functionality defaults to on.
Ideally all of this functionality would default to off, and be enabled via ‘-fasan’ or even better ‘-faddress-sanitizer’ in Clang. That would match the behavior of ‘-fcatch-undefined-behavior’, ‘-fmudflap’, etc. If you want to expose the more fine grained flags to users that are mentioned on the wiki page, they could also have ‘-f…’ Clang flags, but it seems unlikely that those are important.
What’s your expected plan for the runtime library? Is that something you would be interested in contributing to the LLVM project proper? If so, I’d be interested in where Chris thinks that should go… in the LLVM runtimes tree? In a separate LLVM project?
Anyways, thanks for working on integrating this back into LLVM!
-Chandler
Any updates on this?
In particular, I’d like to see concrete patches proposed for review and inclusion into LLVM. I think having actual patches on the table and under review will help a great deal. Kostya, let me know if I can help prepare them.
Ok, I’ll send the first (small) patch shortly.
A few general comments as well inline…
Hi,
What would be our next steps in getting ASan into the LLVM trunk?
I’d like to do it in two steps, first for the LLVM part with minimal tests and then for the run-time library and all tests.
The current ASan’s source repository will probably stay the primary home for the run-time library and tests as we plan to use it in non-LLVM environments.Hi Kostya,
I haven’t had a chance to look at your patch yet, I’m backed up on “big patches”. Did you see my review of the safecode patch here?
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20110718/124515.htmlI expect to have similar concerns and suggestions for your patch,
Hi Chris,
Thanks for the reply.
Indeed, some of your comments to safecode patch apply here.Codingstyle: I’ll try to cleanup as much as I can today.
Do you have any lint-like tool that checks for llvm coding style (similar to cpplint)?I don’t know of any effective ones. Kostya, maybe you can send the code by myself and/or Nick to help walk through it for style issues?
Documentation: everything is in the wiki. The main pages are:
http://code.google.com/p/address-sanitizer/wiki/AddressSanitizer
http://code.google.com/p/address-sanitizer/wiki/AddressSanitizerAlgorithmIt would be good to have these written up in HTML for easy inclusion in the existing LLVM documentation tree. This tree is checked in along side the code itself. However, contributing snapshots of the documentation from the wiki page could likely occur in follow-up patches. Alternatively, maybe Chris would be OK linking to these wiki pages from the primary LLVM documentation.
“who is going to maintain”: my team at Google (in particular, myself and Alexander, in CC) are highly motivated to keep this working.
“Do you have particular clients”: the Chromium project is a very active client.
For reference, myself and others on my team at Google are working actively with Clang and LLVM and would be able to maintain, fix bugs, and update this. We also have a very large base of users that would be interested in the functionality.
“The work can be decomposed into small and incremental pieces”:
the LLVM part is just 1000 LOC. If you still like it to be decomposed, we can do it in 3 parts: a) general instrumentation, b) redzones for stack, c) redzones for globals.I think breaking up the LLVM part would be very helpful. Can you break out the first part, and after a style cleanup mail the attached patch? Then the review for that can overlap with preparing the other 2 parts.
How do you plan to test this? It’s important that there are regression tests in the LLVM suite that exercise the functionality independent of any runtime so that other developers can catch regressions. Also, unittests in the LLVM unittest tree would be nice as well.
Currently, I have tests that work only with the run-time library.
I will definitely need tests that don’t require run-time support.
Have you written a Clang patch to turn this functionality on and off? Looking at the wiki documentation shows one thing that gives me pause: you’re using the -mllvm flag in Clang to directly pass options down to the LLVM layer. Also, it indicates the asan functionality defaults to on.
Yes, I have clang patch that adds -fasan flag (off by default) to Linux and Mac (the run-time library doesn’t work on other OSes yet).
The patch changes tools/clang/lib/Driver/Tools.cpp and tools/clang/include/clang/Driver/Options.td
The instrumentation pass has a bunch of flags which one can use via “-mllvm -asan-flag”, but most of these flags should not be user-visible.
Ideally all of this functionality would default to off, and be enabled via ‘-fasan’ or even better ‘-faddress-sanitizer’ in Clang.
That’s what I have now (-fasan). I slightly prefer -fasan over -faddress-sanitizer because the former is shorter.
That would match the behavior of ‘-fcatch-undefined-behavior’, ‘-fmudflap’, etc. If you want to expose the more fine grained flags to users that are mentioned on the wiki page, they could also have ‘-f…’ Clang flags, but it seems unlikely that those are important.
I will probably need a few more user-visible flags: [don’t]instrument stack, [don’t]instrument globals, [don’t]instrument reads.
What’s your expected plan for the runtime library? Is that something you would be interested in contributing to the LLVM project proper? If so, I’d be interested in where Chris thinks that should go… in the LLVM runtimes tree? In a separate LLVM project?
I’d like to be able to keep the run-time completely separate from the rest of LLVM so that the same library could be used in other environments.
This of course does not prevent us from having the library sources in the LLVM tree.
–kcc
My first patch: http://codereview.appspot.com/4842041/ (the diff file is in attachment).
This patch adds the instrumentation pass only (no clang driver changes, no run-time, no tests).
The instrumentation pass is shorter than the original one (does not handle globals, stack and black list).
Please comment.
The next step would be to add instrumentation-only tests (something like an input llvm file and an expected output llvm file checked with FileCheck, right?).
Please suggest where to add such tests and how to run them.
Thanks,
–kcc
issue4842041_2001.diff (27 KB)