Added 2 test cases that check that dll{ex,im}ported classes that are instantiated with classes with internal linkage as template arguments are not exported or imported.

In D145803#4219894, @probinson wrote:

This is pretty different from the "always desugar to the canonical type" habit that has always been in place. Sony has done some downstream things to try to work around that in the past. @wolfgangp will remember it better than I do, but I think we make some effort to preserve the type-as-written. This goes in completely the other direction.

Instead of suppressing the error message we drop the dllimport/dllexport attribute when we see that a class has UniqueExternal linkage. This will suppress exporting or importing any members, which could not be accessed outside the TU anyway.

In D146338#4206222, @hans wrote:

In D145271 it was suggested that we drop the attribute in such contexts, and this is effectively what happens. The compiler does not produce any exported definitions (or import any symbols) for such classes. The patch is simply to suppress the diagnostic for MSVC mode and Playstation.

In D145970#4192679, @hans wrote:

LGTM

I've always found this to be an interesting behavior, and I'm guessing you found some code where it does matter :)

It still seems like the export/import there is an accident since Base<f::Local> can't really be referenced from outside the file anyway.

Perhaps rather than giving Base<f::Local> external linkage to allow it to be imported/exported, the better fix would be to drop its dllimport/export attribute when instantiated with a local type?

In D145271#4171267, @hans wrote:

Interesting! Do you have an example where this (local dllexport/import classes) comes up in practice?

A customer complained about the following code (I'm obscuring the class names) compiling with MSVC but
rejected by clang:

In D140123#4124512, @efriedma wrote:

no separate section for module flags

https://llvm.org/docs/LangRef.html#module-flags-metadata ?

In D140123#4123625, @efriedma wrote:

In D140123#4113536, @efriedma wrote:

Missing LangRef change?

Ping?

In D140123#4094007, @probinson wrote:

I don't know if this can happen, but what about an LTO scenario with different values for the module flag? I seem to recall module flags needing to provide a merge policy? I might be remembering something else though.
If the result is "fail the compilation" that seems fine.

ping...

Ping...

Addressed a review comment regarding the clang test (tightening up the module flag check).

Instead of using the data layout string, we're using a module flag instead to describe the maximum alignment of a TLS variable. This works for LTO as well. As a test case, we're using the instcombine pass to demonstrate that we don't align beyond the maximum.

In D140123#4000196, @arsenm wrote:

I feel this is too specific and doesn't belong in the datalayout. I think a general maximum global alignment would make a bit more sense? I know very little about object formats

In D140123#4000068, @arsenm wrote:

What is the consequence of having excess alignment?

covered.cpp and uar.cpp seem to fail on ubuntu bots here.

ping ...

Sorry about the delay. Thank you for your test case, I have used it instead of the original one and added a warning.
Please check if the wording describes the issue reasonably correctly and succinctly, as the circumstances are somewhat arcane.

ping ...

Ping ...

LGTM.

In D127988#3593143, @tschuett wrote:

If I have lot of functions where the stack size is 1 byte below the threshold and do a lot of inlining, then the aggregated stack size will explode?

In D125998#3565739, @nikic wrote:

FYI it looks like this change has some negative effect on compile-time (http://llvm-compile-time-tracker.com/compare.php?from=d572fc3b49bd53b7871223b3054b235cf99f6b61&to=e3f6eda8c6ebc567755377911746d4ca2367e649&stat=instructions) and in -g LTO builds on memory usage as well (http://llvm-compile-time-tracker.com/compare.php?from=d572fc3b49bd53b7871223b3054b235cf99f6b61&to=e3f6eda8c6ebc567755377911746d4ca2367e649&stat=max-rss). Is that expected?

Addressed review comments.

Added a unit test to make sure the move constructor and the move assignment op of MDOperand adjust tracking info.

Use a better way to create a distinct node from an existing uniqued one and simplify dealing with AppendUnique module flags.

Use the default constructor for LargeStorageVector followed by a resize instead of the constructor using a numOps parameter. Doing the latter requires the copy constructor for MDOperand.

Only define the move constructor and move assignment operator. Use retrack() instead of relying on the destructor to untrack the source op.

This patch depends on D125998. When appending module flags to the module flags of another module (probably during module linking) we want to append the operands to the destination module's MDTuple instead of creating new ones. This should fix issue 51893.

You mentioned that phabricator reviews can be linked. I'm having trouble finding out how.

NM, found it.

Seems to be fine now. Thanks.

Hi, there seems to be a unit test failure, for example here.

This is a breakdown of MDNodes by number of operands using a bootstrap and a couple of other internal builds (-g -O2). The second column gives the percentage of MDnodes with the number of operands in the first column.
99.9 percent of all nodes have 15 operands or fewer. Almost half have 2 operands.

I'm finally getting around to take a stab at this. One thing in particular is giving me trouble:

Thanks for looking into this.

Hi. This patch is causing a couple of test failures on one of our internal bots which is still running Ubuntu 18.04. Do you have any idea what could be causing it? I'm posting the test output:

ping ...

ping ...

In D111185#3046921, @MaskRay wrote:

I'd prefer we don't work around such usage which is unsupported. This is an opportunity for users to migrate to -DLLVM_ENABLE_RUNTIMES=compiler-rt

In D111185#3046902, @hjl.tools wrote:

In D111185#3046891, @wolfgangp wrote:

This is causing problems with gcc versions that don't have cet.h yet. I think cet.h wasn't added until gcc 8.

Please try this:

0001-sanitizer-Include-cet.h-only-if-it-exists.patch3 KBDownload

This is causing problems with gcc versions that don't have cet.h yet. I think cet.h wasn't added until gcc 8.

In D110783#3035158, @dyung wrote:

Hi, the test you modified Driver/crash-report.cpp is failing on the PS4 bot after your change. Can you take a look?

https://lab.llvm.org/buildbot/#/builders/139/builds/10939