User Details
- User Since
- Aug 21 2015, 4:29 PM (283 w, 23 h)
Today
Yesterday
Address second round of feedback.
Address first round of feedback.
Address first round of feedback.
Thu, Jan 21
Tweak condition to make it easier to read.
@vitalybuka This patch implements the "update the frontier" approach you outlined in the https://reviews.llvm.org/D94210 . The patch depends on https://reviews.llvm.org/D95183 .
This is superseded by https://reviews.llvm.org/D95183 and https://reviews.llvm.org/D95184 .
This change isn't needed anymore.
This change isn't needed anymore.
Thu, Jan 7
Wed, Jan 6
Dec 7 2020
Thanks for addressing my feedback. LGTM
Dec 6 2020
This seems reasonable but I do have a few nits.
Dec 3 2020
LGTM. @ostannard Thanks for investigating!
Oct 15 2020
I'm not entirely satisfied with this patch -- if folks think we should implement not as an in-process builtin, let me know and I can look into that.
Oct 14 2020
Oct 13 2020
Oct 12 2020
LGTM.
Oct 9 2020
Ah I see. That seems pretty reasonable. I had forgotten that lit_config.params is set from the command line. I'd suggest adding the comment I've suggested to make this clearer.
It would also be helpful to update the commit message to expand on the reason why you are doing this with the reason you just gave me in this review.
Oct 8 2020
@abidh Can you explain why this is needed? The emulator is set at CMake configure time and its value depends on the target being tested. Why do you want to override this with a value from the lit_config object?
Oct 7 2020
Fix typo.
Address first round of feedback.
This patch is superseded by https://reviews.llvm.org/D89020.
Oct 6 2020
Oct 5 2020
Oct 4 2020
Aug 31 2020
LGTM provided that you explain in the commit message why this change is being made.
Side note: This get_test_cc_for_arch function is bad in multiple ways.
Could you use get_test_cflags_for_apple_platform platform instead at all the call site?
Aug 20 2020
You should probably refer to https://reviews.llvm.org/D85803 or the commit that removed -mmacosx-version-min=10.12 in the commit message. Other than that LGTM. Thanks for cleaning this up.
Aug 17 2020
Aug 13 2020
@yln Are you sure this actually works for all platforms, in particular the watchOS simulator? The code you link (https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/tsan/CMakeLists.txt#L122) is not the minimum deploy target. That code just checks the SDK version used to build which is different from the minimum deploy target used.
LGTM. Other than the nit.
Aug 6 2020
@vitalybuka Do I need to worry about overflow here, i.e. kHighMemEnd is ULONG_MAX on some platform in which case we'd overflow and therefore ask for a shadow size of 0?
@tejohnson Side comment about the refactor. The comments on MapDynamicShadow(...) say that is function is actually supposed to mmap the found memory as NO_ACCESS. The current implementation doesn't do this. It just finds a space but doesn't mmap it. Is this a problem?
@yln Note there are other bugs here but I plan to fix this in future commits (e.g. if (size < gap_size) should be if (size <= gap_size)).
Aug 5 2020
@tejohnson Looks like this bug is present on Linux too
Jul 29 2020
LGTM
Jul 27 2020
LGTM except for the lint checks failing. Please fix those before landing.
Jul 24 2020
Looks really good. I just have a few nits.
Jul 22 2020
Jul 21 2020
Fix failing test due to LSan.