lgtm Thanks for the investigation. I RDPd to the bot and then got distracted before debugging it.

I'm unhappy with the need for instrumentation passes to "color" their inserted call instructions. It's a real drag. What we really want here are single-entry, multi-exit regions. If you ignore unreachable, then funclets are already single-entry, single-exit regions. Unfortunately, unreachable is very very common, because it comes after throw, which always calls a noreturn function. So this is common:

try { may_throw(); } catch (...) { do_something(); throw; /*unreachable*/ }
try { may_throw(); } catch (...) { do_something(); throw; /*unreachable*/ }

We needed to prevent simplifycfg from tail merging the two otherwise identical catch funclets that don't end in catchret.

lgtm

+@pcc

@zturner: We need to add logic to this section merging code to make S_SECTION / S_COFFGROUP records.

because of the previous
permission merging semantics, the .CRT sections were causing the entire
.rdata section to become writable, which caused the SEH runtime to
crash because it apparently requires .xdata to be read-only. This
change also implements the same special case.

• fix boolean logic thinko

• fix hello32.test: default to marking image as "no seh", then remove it if we record handlers

• move logic to createSEHTable

lgtm

• remove tab

In D45778#1071211, @mstorsjo wrote:

What effect does it have if a DLL is marked as not using safe SEH?

• Set the flag if there are handlers but no load config

@mstorsjo will this be a problem for mingw? Does the mingw CRT provide a load config for each image? This could be an undesirable behavior change if mingw does not provide a load config, and the user links in a 32-bit object file that has exception handlers that they don't actually need for program correctness, but now the DLL is marked as not using safe SEH.

Sorry, I skipped over the message and looked at the code, which seems pretty straightforward.

lgtm

lgtm

In D45536#1069392, @espindola wrote:

Could you please put a --reproduce package of those libraries in a bug report? Thanks!

This is causing LLD to drop __cxa_finalize from the symbol tables of some Chromium binaries, and that causes them to crash on shutdown. See the test failures here:
https://ci.chromium.org/buildbot/chromium.clang/ToTLinux/2152

Mostly looks good.

The overall goal makes sense to me, but I feel like this patch adds many CMake variables to accomplish it, which are only set from the Fuchsia CMake cache. I feel like it would be simpler to have one CMake flag that clang and the runtime libraries look at to decide if they want to use the old libclang_rt.$arch.$lib paths, or the new clang/$version/$triple/lib/$lib paths.

Yikes. Thanks for the fix, looks good.

lgtm, thanks!

OK, here are the two competing use cases that I am imagining:

1. (yours) Full cross-compilation from Unix, with linker diagnostics referring to files that exist locally on Unix.
2. Distributed cross-compilation on Unix, local linking on Windows with the VC linker.

In D45164#1064411, @niravd wrote:

I have not found any documentation giving guarantees about the correspondence between output types, but it seem natural to me that direct object generation may be more permissive than compilation through assembly hence this patch. There already appear to be additional restrictions in the AsmStreamer (e.g. dwarf CUID) over the ObjectStreamer so this isn't a new thing.

In D45122#1063889, @asmith wrote:

Only considering DIFlagPassByReference for CxxReturnUdt will not cover all
cases. There are times when DIFlagPassByValue is also CxxReturnUdt.

You're right, this patch does get the constructor member LF right, but it would be easier and more direct if the constructor itself were marked trivial or just omitted by clang.

In D45473#1063531, @pcc wrote:

In D45473#1063321, @rnk wrote:

Can you elaborate on the use case? What tool do you expect will consume these Unix style paths?

We can use the paths to provide better linker error messages. See D45467.

I think we actually want the triviality marker on the DISubprogram, not the DICompositeType. If I understand, this patch will still emit LF_METHOD records for trivial, but user-defaulted constructors, like:

struct Foo { Foo() = default; };

lgtm

In D44406, I was imagining instead that we'd mark DISubprograms as trivial or non-trivial, not the type itself, and then CodeViewDebug.cpp in LLVM could skip trivial constructors when looking for classes.

lgtm

Looks good, feel free to address the is64() check in a follow-up.

Can you elaborate on the use case? What tool do you expect will consume these Unix style paths? What aspect of the path is invalidated by changing the slash style?

lgtm

lgtm

lgtm

I don't think this is enough, this issue generalizes to more than just lambdas. The more general problem is this:

• C++ classes with methods create a cycle in the type hierarchy (class refers to method, method refers to class)
• Some C++ classes without names may not be externally visibible
• CodeView requires class identifiers to enable this circular reference.

lgtm I'm so happy that fixing this doesn't require a buildmaster restart.

Looks good to me, but let's ask @zturner too.

Thousands of lines of C++ to answer one of life's most simple questions: where are the libraries and where are the headers. =p

The only think that links against libxml2 is c-index-test. Surely nobody cares about installing c-index-test on some other machine that lacks the local version of libxml2. What's the use case for this to justify the complexity of interacting with LLVM's configuration options?

Thanks for getting this and grinding through and updating the test cases!

Looks good to me, what do you think, Peter?

lgtm

In D45116#1056216, @pcc wrote:

Are you sure that would work? The pass runs after RA.

Good point. You might want X86ISelLowering to copy these registers into and out of the ICALL_BRANCH_FUNNEL node, though.

In D45116#1055897, @pcc wrote:

Maybe I forgot to check intrinsics, or the arguments that are part of the variadic pack in the verifier.

I had to disable the verifier check for intrinsics because the check is not appropriate for all intrinsics, specifically this one.

Have you considered passing these extra parameters as an operand bundle, or do those not support variable numbers of operands?

Something like that might work, but even if we were to represent the target list like that, that wouldn't be enough, because the backend would also need to know how to emit the other arguments as if they were being passed as part of a regular call.

That is, not being able to inline arguments into an intrinsic is effectively a backend limitation that the inliner ought to respect. If that changes, we can change the inliner as well.

In D45152#1056122, @mstorsjo wrote:

In D45152#1055871, @rnk wrote:

Seems reasonable, looks good.

Any opinion on the wording of the option name?

Can you split this into a patch that adds the basic YAML serialization and dumping functionality to LLVM, and a patch that adds the type merging capabilities across LLVM and LLD? The first one should take care of a lot of the boilerplate and we can stamp it quickly and then move on to review the complex type merging code.

Seems reasonable, looks good.

Thanks, I looked, and now I understand better what's going on. These are thunks, and we do in fact want the argument forwarding behavior of a variadic musttail call.

Why is it invalid to inline the call to the branch funnel?

lgtm, thanks!

• rebase over committed changes

• this isn't about thunks

In D43165#1054919, @asmith wrote:

I committed a one line fix before noticing you reverted the change.
Did you happen to check if the fix works for you?

In D45174#1054820, @rsmith wrote:

+rnk This might also affect the MS ABI, but it does not result in any test case failures at least (and MSVC's type trait matches our state after this patch rather than before).