Looks good, but please run git-clang-format before committing. Thanks!

extern "C" void *memcpy(void *, const void *, unsigned long);
template <typename a> using b = a attribute((ext_vector_type(4)));
using c = b<char>;
template <typename b, typename a> b d(a e, unsigned) {

b f;
f[2] = 2;
memcpy(&f, e, 2);
return f;

}
void g(c);
char h;
long i;
void j() { g(d<c>(&h, i)); }

In D64971#1593801, @probinson wrote:

(I though Reid was going to invent dbg.addr and solve all this nonsense for us...)

lgtm

lgtm

In D64799#1591732, @ilya-biryukov wrote:

@rsmith, I'll look into emitting the typos when we pop expression evaluation context, but do we expect this to cover all cases where TypoExprs are produced?
(conservatively assuming that the answer is "no") should we land this patch and also emit at the end of TU in addition to expression evaluation context?

I suggested to @akhuang offline that we should discuss this on llvm-dev. I'll add some other X86 reviewers.

In D64067#1592201, @troyj wrote:

Hi, we just inherited this commit at Cray when we did our latest upstream merge and there are a few problems with it that I'd like to point out. Sorry that I was not part of the initial discussion here, but I didn't know that this work was being done and I had already done it for x86 in our downstream compiler a while ago.

1. This patch adds the -m options to f_Group, which is weird. They should be in m_Group since they are target-specific. I actually implemented them as part of a subgroup of m_Group so that I can take the last option from that group that appears on the command line.

lgtm with a suggestion

I would place this macro setting with the other code that sets this macro, in cmake/modules/HandleLLVMOptions.cmake.

lgtm, sorry this fell out of my inbox.

I'm in favor of this as is. We should loop in and get confirmation from @rsmith though.

lgtm

Some suggestions, you don't have to take all of them.

To avoid a similar inconsistence with host compilers that don't predefine _FILE_OFFSET_BITS=64
(e.g. clang < 9, gcc < 9), this needs a compantion patch to be submitted shortly.

In D64812#1588055, @rnk wrote:

You broke my build. =/ I got this output:

CMake Error at C:/src/llvm-project/lldb/cmake/modules/LLDBConfig.cmake:201 (message):
  Found incompatible Python interpreter (3.7.3) and Python libraries ()

I'll mess with it a bit I guess.

You broke my build. =/ I got this output:

CMake Error at C:/src/llvm-project/lldb/cmake/modules/LLDBConfig.cmake:201 (message):
  Found incompatible Python interpreter (3.7.3) and Python libraries ()

I'll mess with it a bit I guess.

• rebase

In D64461#1587532, @tejohnson wrote:

In D64461#1586582, @rnk wrote:

Personally I believe we should prioritize fixing that. It's trivial to fix for thin archives, since those are practically equivalent to the command line object groups.

Someone asked about this on the llvm-dev mailing list recently, pasting my response below:

Somehow, the module identifier for each constituent object would need to be both unique (we currently generate this from the name of the archive plus the offset in the archive plus the name of the source file IIRC), but also correctly identify the extracted bitcode object used in the post-thinlink backend invocation. This is so it can write out the distributed index file with a filename that gets consumed by the associated backend invocation (passed to -fthinlto-index=), and so that the module paths emitted in those index files correctly identify where we can import functions from. Since the bitcode objects need to be extracted for the corresponding backend clang invocations, a couple possibilities come to mind:

1. Do it outside the compiler/linker: wrap the whole thing in a script that does the extraction, invokes the link with extracted constituents surrounded by --start-lib/-end-lib pairs, invokes each backend through some parallel or distributed mechanism, and then invokes the final link; or
2. Add support to pass some kind of mapping file into LTO that maps from each archive constituent to the extracted filename including path that the corresponding ThinLTO backend clang invocation will use, and have LTO set the module identifiers accordingly so that everything "just works" (in theory). We already support some munging of these names (see the thinlto_object_suffix_replace plugin option in either gold-plugin.cpp or in lld), but what you need here is a bit more complicated than a simple suffix change. But since there is already support for adjusting the name, it might not be too bad to add this support.

lgtm, up to you if you want to wait for @zturner. I like the new error handling approach. :)

• change default
• simplify

lgtm with the explicit marker removed. Thanks!

I'd like to get feedback on the high-level design here, since I'm not familiar with how things are supposed to be done in this code. In particular, does the split between NativeEnumInjectedSources and InjectedSourceStream make sense? Are these names consistent with the rest of the code?

In D64461#1586526, @smeenai wrote:

Distributed ThinLTO doesn't support archives: http://lists.llvm.org/pipermail/llvm-dev/2019-June/133145.html. @tejohnson suggested -start-lib -end-lib as a workaround, but that's not a thing for COFF. Do you have plans for archive support, or is that not something you need to deal with for your use cases?

lgtm too, since now this is just whitelisting a clang flag for clang-cl.

Looks good to me too. :)

In D64062#1584219, @saugustine wrote:

This revision breaks python 2.7.16 builds, which are still supported by upstream python for a few more months. I'm preparing a revert.

The file is getcompiler.c:

lgtm

lgtm

lgtm

In D64640#1583291, @thakis wrote:

In D64640#1583265, @rnk wrote:

The traits object is only used by a few methods. Deserializing a hash
table and walking it is possible without the traits object, so it
shouldn't be required to build a dummy object for that use case.

The traits are needed to do any actual hashing.

I believe that's not really true: The has key is always an uint32_t, and assuming that lookupKeyToStorageKey() and storageKeyToLookupKey() are inverses of each other (which currently is true for StringTableHashTraits; for NamedStreamMapTraits I'm not yet sure what the semantics for two streams with the same name are. I believe it's not allowed – passing two identical /natvis: params to lld seems to confuse everyone at least -- and then it'd be true there too). So I currently think (but could be wrong, I'm not sure yet) that the hashtable hash function is overly complicated and should always work just on the uint32_t key and there should be a convenience function to convert a string key to an uint32_t. But that's for another change.

I reverted @thakis's change in rC365956, but I think in retrospect I should've just stamped your change. I was glazing over the "Integrated" part of "CollectArgsForIntegratedAssembler". Anyway, I think we should leave it alone and let him decide how to handle integrated and non-integrated assembly in a uniform way.

I don't think this is the right fix, because now I think we get this behavior that we don't want:

$ clang -E t.c -o t.i -Wa,-mbig-obj
# no warning
$ clang -E t.c -o t.i -Wa,-mbig-obj -fno-integrated-as
clang: warning: argument unused during compilation: '-Wa,-mbig-obj' [-Wunused-command-line-argument]

lgtm, minor changes

The traits object is only used by a few methods. Deserializing a hash
table and walking it is possible without the traits object, so it
shouldn't be required to build a dummy object for that use case.

In D64630#1583174, @probinson wrote:

With ASAN, packs of up to 800 DBG_VALUEs in a row appear (for that file)

That seems excessive; in the past when I've seen this explosion of DBG instructions, the vast majority were redundant. Have you looked at them to see if this is the case? We might rather eliminate duplicates than write code to make it cheaper to have lots of unnecessary instructions.

lgtm

I think the intention of this change was to ignore assembler flags in pre-processing actions without warning about them. It implements that behavior by running the code that gathers and validates assembler flags. The validation is whats emitting the problematic errors. I don't think that's desirable, so I'd recommend reverting for now.

lgtm with suggestion

In D64062#1581662, @sylvestre.ledru wrote:

In D64062#1581290, @rnk wrote:

Perhaps we should just remove __VERSION__ and keep -dumpversion.

As you wish. clang --version seems an alternative and dumpversion never worked for clang. so, I don't expect software to use it for clang.

I still have this open concern:

In D64505#1580928, @jfb wrote:

Let me rephrase: does it ever make sense to call InitLLVM if there's already a new handler installed? Or is it always a sign of trouble if that's done?

In D64538#1579632, @smeenai wrote:

I'm not fully understanding this. If the .sh file was generated on Windows, it'll contain backslashes in any arguments that are paths, right? Before this change, those backslashes would have been doubled up, but the backslash still wouldn't work as a path separator on Linux, so those arguments would need manual adjustment. I guess the other case that matters is if you have a backslash in an argument which isn't a path, in which case the old behavior was definitely correct and the new one definitely isn't.

lgtm

lgtm

In D64538#1579573, @smeenai wrote:

In D64538#1579561, @rnk wrote:

An extremely convenient feature of the current escaping pattern is that it magically works for both the cmd shell and various bash implementations. You can simply copy paste the commands and run them. This matters, for example, for the .sh crash reproducer script. On Windows today, you can just run it with bash, and it works. So, I'd prefer it if we didn't do this.

If you want -### output to be more FileCheck friendly, then I would recommend adding a new driver flag for testing that dumps the commands without doing any escaping. This came up before:
https://reviews.llvm.org/D62493

As long as the backslashes aren't followed by a few special characters (as detailed in my comment), the backslash should still continue to work. I've been testing this with git bash and I'm still able to copy-paste the ouptut command.

In D64505#1579577, @jfb wrote:

So you're saying: maybe it's a bug if we install one but there was already one installed?

An extremely convenient feature of the current escaping pattern is that it magically works for both the cmd shell and various bash implementations. You can simply copy paste the commands and run them. This matters, for example, for the .sh crash reproducer script. On Windows today, you can just run it with bash, and it works. So, I'd prefer it if we didn't do this.

Hit send without saying things...

In D64505#1578699, @jfb wrote:

Does it make sense to reset any previous handler in llvm_shutdown?

Does clang-cl -O0 without a / still work, though? It's used:

lgtm

lgtm, and I think Rui approves as well.

In D64458#1577435, @pcc wrote:

Do we really need to support clang-cl syntax here? Can't the user of -fthinlto-index= use the regular clang driver?

I got back to work and found some time, so now I have easy access to Linux and Windows, and should be able to finish this.

Reverted in rC365581.