|
|
Created:
13 years, 1 month ago by kcc Modified:
13 years ago CC:
daniel.dunbar, daniel_zuster.org Base URL:
https://llvm.org/svn/llvm-project/cfe/trunk/ Visibility:
Public. |
Patch Set 1 #Patch Set 2 : comments #Patch Set 3 : comments #
Total comments: 17
Patch Set 4 : simplify the patch #Patch Set 5 : simplify the patch #Patch Set 6 : just one file is here #
Total comments: 1
Patch Set 7 : comments #
Total comments: 1
MessagesTotal messages: 8
http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp File lib/Driver/Tools.cpp (right): http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode1094 lib/Driver/Tools.cpp:1094: static bool AsanIsEnabled(const ArgList &Args) { I don't really see the benefit of this helper given that ArgList already provides hasFlag? http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode1101 lib/Driver/Tools.cpp:1101: std::string res = TC.getDriver().Dir + "/../lib/clang/linux/"; Is this really the right path? I would assume you want the 'lib' subdirectory of the ResourceDir in clang. The ResourceDir is similar to what you have, but with a version number added. Also, you should use the PathV2 API to manipulate these paths rather than just string munging. http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode1113 lib/Driver/Tools.cpp:1113: res += "arch_unsupported_by_address_sanitizer"; Rather than this, why not emit a diagnostic? The Driver can emit diagnostics for invalid options. http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode2500 lib/Driver/Tools.cpp:2500: addAsanLinkerFlagsForLinux(getToolChain(), Args, CmdArgs); Why do we need this in addition to the Linux link job modification below? http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode4462 lib/Driver/Tools.cpp:4462: addAsanLinkerFlagsForLinux(getToolChain(), Args, CmdArgs); I would put this down below with addProfileRT. I would also sink the option checking into the helper.
Sign in to reply to this message.
http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp File lib/Driver/Tools.cpp (right): http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode2500 lib/Driver/Tools.cpp:2500: addAsanLinkerFlagsForLinux(getToolChain(), Args, CmdArgs); On 2011/11/22 00:36:09, chandlerc wrote: > Why do we need this in addition to the Linux link job modification below? Clang will follow this codepath, instead of linuxtools::Link, if there is no mention of "linux" in the target triple. We don't really need this mode anymore, but may be a good idea to keep it for completeness and feature parity between generic_gcc and linuxtools.
Sign in to reply to this message.
http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp File lib/Driver/Tools.cpp (right): http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode1094 lib/Driver/Tools.cpp:1094: static bool AsanIsEnabled(const ArgList &Args) { On 2011/11/22 00:36:09, chandlerc wrote: > I don't really see the benefit of this helper given that ArgList already > provides hasFlag? Done. http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode1101 lib/Driver/Tools.cpp:1101: std::string res = TC.getDriver().Dir + "/../lib/clang/linux/"; On 2011/11/22 00:36:09, chandlerc wrote: > Is this really the right path? I would assume you want the 'lib' subdirectory of > the ResourceDir in clang. The ResourceDir is similar to what you have, but with > a version number added. The path was suggested by Daniel Dunbar. Note, no one creates such path yet, we only *plan* to have it like this. > > Also, you should use the PathV2 API to manipulate these paths rather than just > string munging. Nice. Fixed. http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode1113 lib/Driver/Tools.cpp:1113: res += "arch_unsupported_by_address_sanitizer"; On 2011/11/22 00:36:09, chandlerc wrote: > Rather than this, why not emit a diagnostic? The Driver can emit diagnostics for > invalid options. Removed this. http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode2500 lib/Driver/Tools.cpp:2500: addAsanLinkerFlagsForLinux(getToolChain(), Args, CmdArgs); removed http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode4462 lib/Driver/Tools.cpp:4462: addAsanLinkerFlagsForLinux(getToolChain(), Args, CmdArgs); On 2011/11/22 00:36:09, chandlerc wrote: > I would put this down below with addProfileRT. This does not work. We need the asan run-time to be passed before libc, otherwise libc's malloc will get linked > > I would also sink the option checking into the helper. done
Sign in to reply to this message.
Few more comments... http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp File lib/Driver/Tools.cpp (right): http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode1101 lib/Driver/Tools.cpp:1101: std::string res = TC.getDriver().Dir + "/../lib/clang/linux/"; On 2011/11/22 19:10:41, kcc wrote: > On 2011/11/22 00:36:09, chandlerc wrote: > > Is this really the right path? I would assume you want the 'lib' subdirectory > of > > the ResourceDir in clang. The ResourceDir is similar to what you have, but > with > > a version number added. > > The path was suggested by Daniel Dunbar. > Note, no one creates such path yet, we only *plan* to have it like this. I think it would be very bad to diverge from the existing ResourceDir here. It is already set up to handle this. If Daniel has a reason to diverge, I'd like to see that spelled out. Also, rather than using 'linux' and 'arch', why not use the target triple as the single subdirectory? We already have a canonical string representation for the triple. If the goal is to hide differences such as i386 vs i686; we should directly add support to the LLVM triple class for expressing this. It would simplify other bits of code here. For ARM, I suspect that we'll specifically want to retain a bit more than just "linux" and "arm" as my understanding is there are multiple ARM ABIs in use on top of Linux. > > > > > > Also, you should use the PathV2 API to manipulate these paths rather than just > > string munging. > > Nice. Fixed. > > http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode2500 lib/Driver/Tools.cpp:2500: addAsanLinkerFlagsForLinux(getToolChain(), Args, CmdArgs); On 2011/11/22 08:36:59, Evgeniy Stepanov wrote: > On 2011/11/22 00:36:09, chandlerc wrote: > > Why do we need this in addition to the Linux link job modification below? > > Clang will follow this codepath, instead of linuxtools::Link, if there is no > mention of "linux" in the target triple. > > We don't really need this mode anymore, but may be a good idea to keep it for > completeness and feature parity between generic_gcc and linuxtools. I think we should be hesitant to add it as it seems unlikely that the generic GCC targets will actually be able to work with the runtime (at least at first). I would prefer to slowly grow the support in various platforms as we gain confidence (and testers) using it on those platforms. http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode4462 lib/Driver/Tools.cpp:4462: addAsanLinkerFlagsForLinux(getToolChain(), Args, CmdArgs); On 2011/11/22 19:10:41, kcc wrote: > On 2011/11/22 00:36:09, chandlerc wrote: > > I would put this down below with addProfileRT. > This does not work. We need the asan run-time to be passed before libc, > otherwise libc's malloc will get linked Cool, can you add a comment clarifying this? Otherwise a hapless future maintainer may mess it up. > > > > I would also sink the option checking into the helper. > > done http://codereview.appspot.com/5416061/diff/7004/lib/Driver/Tools.cpp File lib/Driver/Tools.cpp (right): http://codereview.appspot.com/5416061/diff/7004/lib/Driver/Tools.cpp#newcode1109 lib/Driver/Tools.cpp:1109: CmdArgs.push_back("-export-dynamic"); Are these three flags going to be correct for every system? I suspect not, and that they should be hoisted into each target platform's code. For example, Solaris will want '-pthread' iirc,..
Sign in to reply to this message.
http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp File lib/Driver/Tools.cpp (right): http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode1101 lib/Driver/Tools.cpp:1101: std::string res = TC.getDriver().Dir + "/../lib/clang/linux/"; On 2011/11/22 23:14:35, chandlerc wrote: > On 2011/11/22 19:10:41, kcc wrote: > > On 2011/11/22 00:36:09, chandlerc wrote: > > > Is this really the right path? I would assume you want the 'lib' > subdirectory > > of > > > the ResourceDir in clang. The ResourceDir is similar to what you have, but > > with > > > a version number added. > > > > The path was suggested by Daniel Dunbar. > > Note, no one creates such path yet, we only *plan* to have it like this. > > I think it would be very bad to diverge from the existing ResourceDir here. I have no opinion here. Adding Daniel Dunbar (he is in CC on the original message, but not in CC in this codereview thread. oh) > It > is already set up to handle this. If Daniel has a reason to diverge, I'd like to > see that spelled out. > > Also, rather than using 'linux' and 'arch', why not use the target triple as the > single subdirectory? We already have a canonical string representation for the > triple. > > If the goal is to hide differences such as i386 vs i686; we should directly add > support to the LLVM triple class for expressing this. It would simplify other > bits of code here. > > For ARM, I suspect that we'll specifically want to retain a bit more than just > "linux" and "arm" as my understanding is there are multiple ARM ABIs in use on > top of Linux. > > > > > > > > > > > Also, you should use the PathV2 API to manipulate these paths rather than > just > > > string munging. > > > > Nice. Fixed. > > > > > http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode4462 lib/Driver/Tools.cpp:4462: addAsanLinkerFlagsForLinux(getToolChain(), Args, CmdArgs); On 2011/11/22 23:14:35, chandlerc wrote: > On 2011/11/22 19:10:41, kcc wrote: > > On 2011/11/22 00:36:09, chandlerc wrote: > > > I would put this down below with addProfileRT. > > This does not work. We need the asan run-time to be passed before libc, > > otherwise libc's malloc will get linked > > Cool, can you add a comment clarifying this? Otherwise a hapless future > maintainer may mess it up. > > > > > > > I would also sink the option checking into the helper. > > > > done > Done.
Sign in to reply to this message.
http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp File lib/Driver/Tools.cpp (right): http://codereview.appspot.com/5416061/diff/3001/lib/Driver/Tools.cpp#newcode1101 lib/Driver/Tools.cpp:1101: std::string res = TC.getDriver().Dir + "/../lib/clang/linux/"; On 2011/11/22 23:14:35, chandlerc wrote: > On 2011/11/22 19:10:41, kcc wrote: > > On 2011/11/22 00:36:09, chandlerc wrote: > > > Is this really the right path? I would assume you want the 'lib' > subdirectory > > of > > > the ResourceDir in clang. The ResourceDir is similar to what you have, but > > with > > > a version number added. > > > > The path was suggested by Daniel Dunbar. > > Note, no one creates such path yet, we only *plan* to have it like this. > > I think it would be very bad to diverge from the existing ResourceDir here. It > is already set up to handle this. If Daniel has a reason to diverge, I'd like to > see that spelled out. > > Also, rather than using 'linux' and 'arch', why not use the target triple as the > single subdirectory? We already have a canonical string representation for the > triple. > > If the goal is to hide differences such as i386 vs i686; we should directly add > support to the LLVM triple class for expressing this. It would simplify other > bits of code here. > > For ARM, I suspect that we'll specifically want to retain a bit more than just > "linux" and "arm" as my understanding is there are multiple ARM ABIs in use on > top of Linux. Yes, there are lots of them. In fact, I plan to add a variant of asan runtime for arm-elf-linux-androideabi soon. Calling it libasan_android seems really messy.
Sign in to reply to this message.
Only one real question... feel free to submit whenever, this can be addressed post-commit. http://codereview.appspot.com/5416061/diff/8003/lib/Driver/Tools.cpp File lib/Driver/Tools.cpp (right): http://codereview.appspot.com/5416061/diff/8003/lib/Driver/Tools.cpp#newcode1111 lib/Driver/Tools.cpp:1111: TC.AddCXXStdlibLibArgs(Args, CmdArgs); I don't understand this bit... Why do we need ta add the CXX standard library here? Is this safe to do when building C code?
Sign in to reply to this message.
On 2011/11/30 00:26:16, chandlerc wrote: > Only one real question... feel free to submit whenever, this can be addressed > post-commit. > > http://codereview.appspot.com/5416061/diff/8003/lib/Driver/Tools.cpp > File lib/Driver/Tools.cpp (right): > > http://codereview.appspot.com/5416061/diff/8003/lib/Driver/Tools.cpp#newcode1111 > lib/Driver/Tools.cpp:1111: TC.AddCXXStdlibLibArgs(Args, CmdArgs); > I don't understand this bit... Why do we need ta add the CXX standard library > here? asan rt uses some of the C++ run-time. basically, a bit of STL. I think we can get rid of it with some effort, but now we do need it. > Is this safe to do when building C code? I have not seen problems with C programs, but the only large C programs I've built with asan were those from SPEC cpu2006. (someone else had success with ffmpeg and perl)
Sign in to reply to this message.
|