LGTM

David, could you please take a look to this patch. It fixes potential issue and should have no side effects. If you have no objections, I'll commit it for Serge.

LGTM

I think we need to fix this regression.

LGTM, added Richard in case he has something to add.

Committed as rL282059

Richard, please take another look.

Committed as rL275970

Hi Hans, it seems that you've just created release branch for 3.9 and this patch should go directly to the branch, right? If so could you please commit this patch for me because I'm working with git-svn and there is no instruction how to work with release LLVM branches from git so I'm worry that my setup could break things. I'm absolutely fine with moving abi_tag above --build-id. Thanks!

• defer var generation if their type is class with dllimport.

In D22034#482925, @majnemer wrote:

A flag on CXXRecordDecl which is sensitive to the most recent expression evaluation might not be the best way to go.
Perhaps we should be able to use the VFTableBuilder to build imported and local vftables for the same vftable? Not entirely sure though...

Sorry for delay with patch rework, PTAL!

• added local vftable for constexpr
• added use of imported vftable

Friendly ping, PTAL!

In D22034#475603, @majnemer wrote:

Thinking about this some more, it is possible for clang to emit code that will make everybody happy:

If a class is being constructed in a constexpr context and all the vftable entries it references are marked import, emit local vftables and reference them in the object. If a vftable entry it references is not marked import, report an error.

If a class is constructed via operator new and all the vftable entries it references are marked import, emit local vftables and store it in the object after the constructors run. If a vftable entry it references is not marked import, report an error.

If a class is constructed via a local or global variable and all the vftable entries it references are marked import, create a dllimport available_externally vftable. Otherwise create a normal dllimport external vftable.

I believe this behavior captures the best behavior across the spectrum of functionality we all care about: it supports devirtualization, constexpr and importing classes which don't have all their vftable methods exported.

In D22034#475551, @majnemer wrote:

In D22034#475540, @DmitryPolukhin wrote:

Here is B::foo is not exported but required to build vftable for D.

What happens if D also had a virtual destructor?

In D22034#475331, @majnemer wrote:

Wait, can you give an example of MSVC exporting a vftable but not all the virtual methods (other than the deleting destructor)? I don't believe I've ever come across an example of this.

In D22034#474985, @majnemer wrote:

In D22034#474937, @DmitryPolukhin wrote:

David, do you know real programs that relay on constexpr and dllexport semantic that doesn't work on MSVC?

Yes, Chrome relied on these semantics.

I found the patch that prevents using imported vftable, http://llvm.org/viewvc/llvm-project?view=revision&revision=260548 but the patch has no examples of constexpr + dllimport.

David, do you know real programs that relay on constexpr and dllexport semantic that doesn't work on MSVC? If not, I think we might want to report error message instead of miss-compile it as MSVC does. Anyway current implementation is not compatible with MSVC in much more common case without constexp. At the moment my patch works with your example just because it only changes mangling but don't use imported vftable that seems to be wrong. Is it you changes somewhere that prevent using imported vftable?

Richard, thank you for the review!

Use SmallVector instead of SmallSetVector and sort the vector when needed.

Fixed issue with substitution from function name used in bare type encoding and added test. With this patch Clang passes self build with GCC 5.3.1 headers and C++ libraries so it gives some level of confidence that it works on real apps.

• removed wrong asserts that I used for an experiment

This patch set contains serious refactoring to make it closer to what Richard suggested. Unfortunately there is no way to fully implement Richard's idea because we have to mangle twice name of functions and variables. But this patch reuse mangled bare function type generated during tags calculation.

In D18035#450258, @rsmith wrote:

Yes, I definitely want us to implement this for GCC compatibility. And now that we have a specification for this feature, we can evaluate whether this is doing the right thing. On that basis:

I still want this refactored so that the normally-mangled part of a function name is only mangled once, rather than being mangled twice (once to find the implicit tag set and once to actually produce the mangled name). As suggested, you can achieve this by first computing the set of tags from the return type, then mangling the encoding to a separate buffer (collecting tags as you go) if the set is non-empty, and finally writing any remaining tags and the buffer contents.

Please factor out a function to mangle the source name and ABI tags for a NamedDecl rather than duplicating that pair of calls throughout the patch.

Current discussion on the ABI list suggests that it is not correct to mangle the return type / variable type to get the implicit tag set. The set of available attributes should be determined by a recursive walk of the original type (prior to any substitution), not by mangling it and seeing what it references. It's not yet clear whether that's the actual design intent or just the emergent behavior of the GCC implementation, however.

In D18035#447473, @hfinkel wrote:

FYI: There is now a specification, see: http://sourcerytools.com/pipermail/cxx-abi-dev/2016-June/002919.html

It seems that there is no interest in this patch, abandon.

PTAL

• drop dllexport for trivial defaulted x-tors for compatibility with MSVC2015
• move checks to CheckCompletedCXXClass because completed class definition is required to detect if x-tor is trivial

Fix test/PCH/attrs.c failure due to warning about unsupported abi_tag attribute (committed in rL269869).
This patch implements abi_tag so the warning is not expected with this patch.

Comments resolved, PTAL.

One more friendly ping...... :(

Committed as rL269400

Richard, friendly ping. It seems that your counterexample cause no issue with this patch. I added tests to prove it. Could you please take another look?

• fixed tests for i686 when function may have hidden parameter

Thank you for reporting this issue, I'll take a look.
I'm mostly testing on x86_64 so I may not notice the problem.

+ rebase
+ added testcase with Richard's example

Richard, could you please share your counterexample so I could test it on my patch and GCC?

In D18641#413277, @rsmith wrote:

I'm a little concerned about the possibility of this breaking uses of this feature on platforms where Clang is the system compiler. For instance, this pattern would be broken by your change:

// stddef.h
#include "stddef-helper.h"
 
// stddef-helper.h
#include_next <stddef.h>

Conversely, I don't think any important library is likely to be relying on the GCC behavior, because compilations with gcc -I- would effectively get the current Clang behavior (because relative-looking paths would be found in the relevant include search path rather than as relative paths).

Richard, friendly ping, PTAL!

Thank you for the review!

Weekly friendly ping, PTAL!

Richard, this is just weekly friendly ping about abi_tag support.

I added few more negative tests. Thank you for the review!

The patch itself looks good to me but the main question do we want to support VLA in C++ in GNU extension mode.

Sorry, I was out for couple days.

Committed as rL265770

Richard, Reid and David,

• rebase after committing llvm patch

• fixed comments

Committed as rL265530

• fixed all comments
• rebase

Alexey, please review OpenMP specific things.

Rebased on top of D18754

• implemented ad hoc solution for printing
• added documentation for the attrbute
• reabse

• rebease on top of GlobalIndirectSymbol committed to LLVM
• removed C interface for ifunc, it seems that it is not required for now

Could you please take a look to OMPDeclareTargetDecl attribute implementation and printPrettyEnd approach in general?
For post print mechanism alternative approach is to use ad hoc solution in DeclPrinter.

• Added test for templates in declare target region

Friendly ping, please take a look! This patch is relatively short and has no functional changes, just class hierarchy preparation.

Friendly ping, please take a look!

Extracted GlobalIndirectSymbol creation to separate patch http://reviews.llvm.org/D18433

Eric, that you for looking to the code! I'll split the patch and send first part for review ASAP.

Fixed comments.

All comments resolved.

Hi Pavel,

Re-base to resolve merge conflicts with r263109.

Thank you for the comments!

• fixed comments
• rebase with resolving conflicts

Rebase

Aaron, friendly ping, please take a look!