While adding the documentation I realized that a better name for this option would be -fmodules-strict-context-hash to make it clear which hash it's referring to.

Fixed spelling and updated comment.

Added .i, .ii, .mi, and .mmi as files to minimize.

lgtm. Jan may want to take a look as I believe he was looking at a related issue.

Addressed review comments.

lgtm, but wait for Alex or Duncan to also take a look.

In D68193#1701426, @kousikk wrote:

@arphaman I don't mind changing this if there are race conditions as you say, but isn't the assumption of the tool that the filesystem remains unchanged for a single run of the tool? If so, should we actually throw error conditions instead of crashing in those cases?

Add diagnostics.

Do we really want to output \r\n on Windows? There is all of one program (notepad.exe) that doesn't support \n on Windows.

lgtm

Also, please use -U999999 for patches so there's full context.

Looking more into this I'm not sure you need the PPCallbacks. The Preprocessor should just own the PreprocessorSkippedMappings data structure and the dependency scanner can update it live.

lgtm

This fix works, but we could also use openat to get around max path length issues. Windows also has an API that can be used similarly.

Fixed and committed as r368381.

This looks like a good simplification, but I think call_once could be simplified more.

looks good. @ldionne are you happy with the changes?

In D64191#1577566, @thakis wrote:

FYIe, you can also prevent this symbol to be linked in by putting a cxa_demangle function in an obj file that gets linked first, like so: https://cs.chromium.org/chromium/src/third_party/android_crazy_linker/src/src/crazy_linker_wrappers.cpp?type=cs&q=cxa_demangle+file:android&sq=package:chromium&g=0&l=54 – but I agree that since cxa_demangle is so large, it'd be nice if the upstream version was behind some kind of define. Then we could set that and remove that hack in chromium too. I don't have an opinion on if that should be a new define or if _LIBCXXABI_NO_EXCEPTIONS should be reused. Actually, I think we build libcxxabi with exceptions enabled because some of our test code can in theory throw exceptions, but we have an external dumper so we don't need the stack from terminate() – so it'd be somewhat more convenient for us if we could turn off the stack from terminate() (and the pulling in of __cxa_demangle) while still being able to build without setting _LIBCXXABI_NO_EXCEPTIONS.

_LIBCXXABI_NO_EXCEPTIONS removes the exceptions abi from libc++abi, so I think this change as fine as __cxa_throw isn't even defined if _LIBCXXABI_NO_EXCEPTIONS is defined.

In D64164#1569679, @rnk wrote:

Please check the commit message:

[clang][ARM] Fix msvc arm{64} builtins to use int on LLP64 systems.

I think you mean "use int on LP64 systems", since long is 32-bits on LLP64, right?

Otherwise, sounds good.

In D64164#1569442, @efriedma wrote:

Do the changes to BuiltinsARM.def have any practical effect? long should be 32 bits on all 32-bit ARM targets.

LGTM

Where do you want to keep these open? The default OS limit on open files for OSX is 256.

Also adding the llvm-commits list.

Thanks a lot for working on this. There's a lot of room to improve the performance of exceptions that nobody has done for C++ yet. I've added some reviewers that I'm aware are familiar with the area or that I've talked with about this before.

LGTM

It would be nice to get tests for these cases, although I realize that can be difficult as it requires malformed inputs.

Thanks for finding this!

In D62627#1522081, @zhizhouy wrote:

IIUC, user will always call clang to pass integrated as flag. Actually I also used to consider checking UseIntegratedAS-ish in PassBuilder,
but looking for a codegen options in PassBuilder seems going to break the integrity. Solutions for this is appreciated.

I find it a bit odd to have clang care about this, but I suppose it is the driver that knows which features tools support. I also have an issue with the name as CGProfilePass doesn't actually do the sorting. I would just keep the option as -enable-call-graph-profile-emission or something like that.

LGTM.

I have a bit more review to do, but this is what I've found so far. The naming comments are just suggestions, but the digit separators' are actually an issue.

LGTM

I agree with Zach, this should be little_t and big_t. You should also add aligned_big_t and aligned_little_t.

Ah, I see. This seems fine.

What's the use case?

LGTM.

Thanks for the update. This still looks incorrect in that SANITIZER_NON_UNIQUE_TYPEINFO should be 1 on defined(__arm64__) && SANITIZER_IOS and 0 otherwise on SANITIZER_MAC.

Then I'm fine with the patch as is.

Should the +0 be +0x0 so that it's always a hex digit? Or is the +0 what the binutils already output?

In D58028#1402149, @marxin wrote:

I can if you want. Have you tested that on MacOS or iOS with:

-stdlib=libstdc++

?

I do not believe this change is correct for MacOS or iOS. The original bug does not reproduce on MacOS, and on arm64 iOS you always need to compare strings. Could you preserve the existing behavior for those platforms?

lgtm

Ah, the bugs canceled each other out. Adding a unit test is fine. Just add a new file in llvm/unittests/Object/.

Overall this looks fine, but can you add a test of something that this fixes? I'm rather surprised that no tests fail now.

In D43157#1283535, @PkmX wrote:

Ping?

I have a patch available that handles PCREL_LO12_S and is rebased on trunk, if it is okay I will create a separate patch for reviewing.

lgtm

I'm all in favor of removing the warnings. I was against adding them to begin with.

Pretty sure I understand the algorithm now, but I still want to see what performance impact it has to see if it's worth the (minor) complexity. I'll benchmark it on some large links and see what happens.

I really like adding the option to disable it, but I'm not sure about disabling it by default. Is there any situation where the profile is present where a user wouldn't want to perform layout? The algorithm is very fast so unexpected link time increase isn't really an issue, and the exact order of sections already isn't guaranteed by ELF and differs between linkers so I don't expect there to be correctness issues.

• Added test for unorderable symbols from object files
• Removed unneeded temporary

ping

Address review comments.

lgtm

lgtm.

This just needs the recommonmark python package added on whatever server builds the docs.

In D48859#1151697, @zturner wrote:

In D48859#1151696, @Bigcheese wrote:

Is there a good reason the use cases you want this for can't be added to libSupport? I'm a bit worried about continuing to add platform specific code outside of libSupport.

Mostly that it's quite large. See my recent post on llvm-dev about RFC: libtrace. I don't want to develop an entire library inside of libSupport, and something like a libtrace will unavoidably have a lot of platform specific code. As another example, consider lldb. There's no way to avoid a bunch of platform specific code, and it would be nice if LLVM had a solution that allowed LLDB to re-use some of LLVM's own platform specific code.

Is there a good reason the use cases you want this for can't be added to libSupport? I'm a bit worried about continuing to add platform specific code outside of libSupport.

lgtm

Remove duplication on updating Counts.

Add support for indirect calls using value profiling data.

• Move to new PM
• Use CallSite
• Bail early if no profile counts
• Use TargetTransformInfo::isLoweredToCall

I mean performance of actual cases. I'm interested in if the cache effects of list vs vector matter.