- User Since
- Aug 21 2015, 4:29 PM (304 w, 2 d)
Wed, Jun 9
Thanks for looking at this. Ordering the CMake options makes a lot of sense. You probably should clean up the commit description to only be about the change you're making here and handle "Frequently used options" to a separate patch. Other than that LGTM.
Thu, Jun 3
@kda This change has broken our internal builds on macOS. I'm not sure why it hasn't broken the public ones. If you add a new weak symbol it needs to be added to lib/asan/weak_symbols.txt so that the linker on macOS knowns it's okay for the symbol to not be defined. We'll put up a patch to fix this shortly.
May 20 2021
The patch seems reasonable. I have a minor nit about the use of the % operator but other than that it seems fine.
May 15 2021
I've landed b7f60d861ad7a8d03c09c0b72277eabaa53731fb to try to unbreak the likely misconfigured https://lab.llvm.org/buildbot/#/builders/119/builds/3801 bot.
May 14 2021
Rename option name and lit variables to address @yln 's feedback.
Address @aralisza's feedback.
May 13 2021
I tried out this patch on Linux with an in-tree build (tests run without failing the new check added) and an out of tree build (testing refuses to run as expected).
May 11 2021
Simplify patch because https://reviews.llvm.org/D101682 has landed.
May 8 2021
May 6 2021
May 4 2021
Use isOSDarwin() instead of explicitly checking Darwin OS enum values.
May 3 2021
Use -print-runtime-dir to support non Darwin platforms.
May 1 2021
- Support other Apple target triples.
Apr 30 2021
I'm abandoning this change. It is superseded by the much more general https://reviews.llvm.org/D101681
Apr 29 2021
@jansvoboda11 I'm going to land this patch as is (with your nit fixed). If you would like me to add an alias please follow up with me and I can put up a patch to do that.
Rename def too.
Apr 28 2021
@jansvoboda11 Should I use the the alias feature so that the old -fsanitize-address-destructor-kind argument is still parsed? I'm not sure if it's worth it but if doing so is very simple and has a low maintenance burden we should probably do it.
@yln Your comments have convinced me that I should try doing this a different way. I'll try the approach I outlined and I'll see how far I get.
Apr 27 2021
Apr 26 2021
Apr 23 2021
LGTM other than my nits.
Apr 22 2021
@vitalybuka Damn looks like you landed the patch while I was in the middle of writing a review. Could you make the fix I requested as a follow up patch?
The approach looks good but I'd like to see us type check the new run_under parameter.
@vitalybuka Thanks for tackling this. I've always wanted to have our unit tests be runnable on targets that aren't the host. This looks like the first step towards that.
Apr 21 2021
LGTM apart from the description. The description probably should mention this is the runtime counterpart to the previous patch that added static checks. We have to do this check at runtime because we get the VM size from the kernel at runtime.
Apr 20 2021
Apr 19 2021
LGTM apart from the static keyword which "I think" shouldn't be necessary here.
Apr 16 2021
Apr 13 2021
Apr 12 2021
LGTM. Do we have a test that fails without this, but succeeds with it (on AS)?
Apr 9 2021
Just floating this as on option. We could call the new define SANITIZER_MACOS.
@aralisza You can ignore the lint checks here.
Apr 8 2021
I'm not a big fan of the name but the sensible name (SANTIZER_MAC) is already taken unfortunately.
Apr 7 2021
@aralisza Sorry I missed this patch. LGTM.
Apr 6 2021
@bruno Thanks for the patch. TSan's runtime isn't my specialty so I've added other reviewers.
@phosek Thanks for the review.
- Renamed CRT_SRC_ROOT_PATH to COMPILER_RT_ROOT_SRC_PATH
Apr 4 2021
The code in compiler-rt/cmake/builtin-config-ix.cmake is reasonable. Although we (Apple) should probably upstream our change because it's slightly different. I'm not sure about the change in compiler-rt/lib/builtins/CMakeLists.txt we don't have the change you made. I'll put a patch up to upstream our version and we'll see how it goes.