My only nit is to add more description to commit message about why we are removing this and perhaps a link to the review that this was introduced. https://reviews.llvm.org/D114294

Small insignificant note from me: When this lands, please be sure to add me as co-author.
https://github.blog/2018-01-29-commit-together-with-co-authors/

Yes, it's here: https://ci.chromium.org/ui/p/chromium/builders/try/mac_upload_clang/3593/overview

I haven't confirmed this patch is the cause, but it looks very likely. I'm trying to setup the environment for build llvm on my mac to confirm it.

Yes. it's very likey, but seems strange, no other OSX bots complained.
Not sure why linker is unhappy. Could this be an issue with incremental build, e.g. it's linking agains older library?
I will disabled darwin for now.

In D147725#4256168, @delcypher wrote:

@thetruestblue It looks like the new test in this change fails https://green.lab.llvm.org/green/job/clang-stage1-RA/33833/

In D147725#4249850, @yln wrote:

One small nit that I just thought of:
If it's easy to do, then let's move the test under sanitizer_common/TestCases/Darwin, since sanitizer_common is where the code change is as well.

In D147725#4249794, @yln wrote:

However, in new simulators there is an early call that atos does not return any symbol information for

1. Do we know why this is / what changed?

2. Previously we completely stopped trying to symbolicate after the first failure. Now we will always try for new requests. Can you think of any scenarios where this might be a problem, i.e., some interaction with a retry where we would now spend much more time, or maybe even hang on report generation?

Can you just do

git cherry-pick -n

aka no commit? shouldn't require an email?

In D146745#4235114, @rsundahl wrote:

Ping - Thank you.

This is fine... feels a little off.
Is there a script to run the docker container? Couldn't we bring it in there or when we build it?

fixed trailing white space issues.

This looks good to me. I'm not sure the policy on what should be added to the release notes. But this was a temporary flag only added to slowly deprecate this, and was noted when it was added that it would be removed.

Currently & ideally asabi_shim.h is unnecessary -- but we hope to use only headers from ../asan/

You could probably use FILTER to remove all arm7* variants in one command, but there's no real need to do that.

This seems reasonable and fine to me.
Though I'd feel better if someone familiar with MSVC/Windows gave approval.

Nit: When you land this will you update commit message to reflect the new change?

Thanks @delcypher for reviewing. Thanks @thieta for the work -- I'll keep an eye out for the updated diff, @delcypher doesn't work on Sanitizers as much anymore.

LGTM, but also confused regarding @yln's point.

This seems reasonable to me.

update revision

Rather than skip the test, lower expected ratio. Per @rsundahl 's suggestion.

incorporate @yln's suggestion.

In D141171#4040524, @yln wrote:

If we are not doing the optional cleanup, then I think I would prefer this:

#  if (SANITIZER_ANDROID && defined(__aarch64__)) || SANITIZER_FUCHSIA
#    define SANITIZER_CAN_USE_ALLOCATOR64 1
# elif defined(__mips64) || defined(__arm__) || defined(__i386__) || \
      SANITIZER_RISCV64 || defined(__hexagon__) || SANITIZER_IOS
#    define SANITIZER_CAN_USE_ALLOCATOR64 0
#  else

That is, adding SANITIZER_IOS to the branch that disable the 64 bit allocator.

Please also adjust the outdated comment:

// But in some cases (e.g. AArch64's 39-bit address space) SizeClassAllocator64
-->
// But in some cases (e.g. iOS/AArch64's 39-bit address space) SizeClassAllocator64

Removed check for apple in second conditional.

In D141171#4038389, @thetruestblue wrote:

In D141171#4038142, @yln wrote:

I think we can drop && !defined(SANITIZER_APPLE) altogether and let the "default" (last else) case handle things. It definitely doesn't make sense in it's current form: && takes precedence so the code is currently saying: xxx || (hexagon && !apple)

We need !SANITIZER_APPLE [we don't need !defined(SANITIZER_APPLE) tho, the way written here is incorrect]
Because otherwise MacOS takes this path because arm is defined. But you're right. I moved it to the end thinking it would make readability easier and spaced on the precedence.

Updated the diff for now with a fix for the spacing and that logic. But need to think on your optional suggestion, cause I agree it needs a refactor. Did you intentionally skip i386?

In D141171#4038142, @yln wrote:

I think we can drop && !defined(SANITIZER_APPLE) altogether and let the "default" (last else) case handle things. It definitely doesn't make sense in it's current form: && takes precedence so the code is currently saying: xxx || (hexagon && !apple)

In D141171#4036669, @wrotki wrote:

Before you land this - I just noticed that your #elifs spacing is slightly off.

fixed long line

This fixes the case where map_end is greater than end because of page size and alignment difference.
However, I believe this function could use further refinement, I don't think it is handling "alignment" versus "page-aligned" adequately.
alignment is being treated as a size, rather than a page aligned location, which seems to be what we should be expecting here.
Particularly with the calls to UnmaporDie, which expect a page-aligned value.

LGTM

This looks fine to me.

Thanks for fixing this quick, @rsundahl.

In D137024#3931497, @tstellar wrote:

@thetruestblue What paths besides LLVM_TOOLS_BINARY_DIR do you need?

While I sympathize with you, I don't think this is valid reason to maintain full compatibility with llvm-config.

This breaks our use case. We run sanitizer tests built in a standalone build using a toolchain from an artifact built upstream-- the toolchain contains binaries only.
Compiler-rt tests rely on the llvm tools binary being in the PATH.

Is there a radar tracking this that we can add to these updates?
LGTM.

LGTM -- fixes the arm64 hang I've been running into.

Updating commit message

removed conditionalw

C++ standard can be manually overwritten and manually defined. As the codebase stands, without this overwrite, no this is never not true. And in the case of someone running Apple tests, I'm not sure *why* this would happen. But because the the mechanism exists, I added a conditional. I agree that guarding tests against every potential case might be unnecessary.

Abandoning for a more lightweight change.

Keeping the null guard from the original commit.

In D133445#3775419, @yln wrote:

Because of the change in the expected behavior

What is that change? I am a bit confused about this revert.

If I compile the test file without ASan, it still doesn't crash so ASan should allow execution without a crash as well.

Made the format consistent.

Additional weak def