- User Since
- Aug 21 2015, 4:29 PM (265 w, 1 d)
Mon, Aug 31
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
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.
Jul 17 2020
Jul 16 2020
Jul 14 2020
Jul 9 2020
Jul 8 2020
Other than minor nit LGTM.
Jul 7 2020
Jun 26 2020
Jun 9 2020
Jun 3 2020
LGTM but please to make sure the linter is working correctly on your system before you land this.
I have an optional suggestion to improve this patch. Other than that LGTM.
Jun 2 2020
Jun 1 2020
LGTM other than the use of uint32_t.
May 30 2020
Looks good apart from the missing check for underflow. This patch is of course broken for the simulators but it was before this patch and I know in future patches that you plan to fix this.
May 28 2020
May 27 2020
May 20 2020
I don't think calling os_system_version_get_current_version is safe.
LGTM, minus the problems in previous patches.
Suggestion: You could also write a small Darwin only unit test that tests DarwinKernelVersion operators.
LGTM. Nice clean up!
Suggestion: You could write a Darwin only unit test that calls GetDarwinKernelVersion() and checks the result matches the kernel reported by the host.
The overall idea looks good but I think the current patch has a few bugs.
@yln Any opinions on this? You touch lit much more than I do these days.
I've done another pass.
May 19 2020
Tweak comment per feedback.
May 8 2020
@yln I did a quick investigation to see if we could do this without macros. Unfortunately this turns out to be really tricky. Below is a rough sketch that almost works. Due to the way the ptrauth calls are implemented Clang enforces that the key and string passed to ptrauth_string_discriminator be literals.
For the key we can use templates to make this work but the string literal proves to be difficult because string literals aren't allowed as template arguments.